Cache charms viewed from inspector

Cache charms viewed from inspector

Viewing a charm from a deployed service in the inspector now caches the charm for future viewing.

https://code.launchpad.net/~makyo/juju-gui/charm-cache/+merge/175869

(do not edit description out of merge proposal)

[matthew.scott, 2013-07-19 15:44:01]

Please take a look.

[benjamin.saller, 2013-07-19 15:56:59]

I think this is very close, some suggestions below

https://codereview.appspot.com/11526044/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/11526044/diff/1/app/models/models.js#newcode703
app/models/models.js:703: this.cachedBrowserCharms = new
models.BrowserCharmList();
Could we just say browserCharms? I don't think we need cached, its not different
than .charms in that way.

https://codereview.appspot.com/11526044/diff/1/app/store/charm.js
File app/store/charm.js (right):

https://codereview.appspot.com/11526044/diff/1/app/store/charm.js#newcode237
app/store/charm.js:237: charmWithCache: function(charmID, callbacks, bindScope,
cache) {
looking for a name w/o cache here. 
(get/resolve)BrowserCharm

https://codereview.appspot.com/11526044/diff/1/app/store/charm.js#newcode245
app/store/charm.js:245: Y.later(10, this, callbacks.success, charm);
Will Y.soon work here?

https://codereview.appspot.com/11526044/diff/1/app/store/charm.js#newcode249
app/store/charm.js:249: this.charm(charmID, {
could this be _charm and this method charm? When wouldn't we want the loaded
version if we have it?

[matthew.scott, 2013-07-19 16:20:01]

Responded to some, questions on others

https://codereview.appspot.com/11526044/diff/1/app/models/models.js
File app/models/models.js (right):

https://codereview.appspot.com/11526044/diff/1/app/models/models.js#newcode703
app/models/models.js:703: this.cachedBrowserCharms = new
models.BrowserCharmList();
On 2013/07/19 15:57:00, benjamin.saller wrote:
> Could we just say browserCharms? I don't think we need cached, its not
different
> than .charms in that way.

Done.

https://codereview.appspot.com/11526044/diff/1/app/store/charm.js
File app/store/charm.js (right):

https://codereview.appspot.com/11526044/diff/1/app/store/charm.js#newcode237
app/store/charm.js:237: charmWithCache: function(charmID, callbacks, bindScope,
cache) {
On 2013/07/19 15:57:00, benjamin.saller wrote:
> looking for a name w/o cache here. 
> (get/resolve)BrowserCharm
> 

See comment on refactor below; in that case this would be charm, current charm
would be _charm (or getCharmFromStore or something)

https://codereview.appspot.com/11526044/diff/1/app/store/charm.js#newcode245
app/store/charm.js:245: Y.later(10, this, callbacks.success, charm);
On 2013/07/19 15:57:00, benjamin.saller wrote:
> Will Y.soon work here?

Oh, yeah, definitely.  Didn't know about that.  Thanks.

https://codereview.appspot.com/11526044/diff/1/app/store/charm.js#newcode249
app/store/charm.js:249: this.charm(charmID, {
On 2013/07/19 15:57:00, benjamin.saller wrote:
> could this be _charm and this method charm? When wouldn't we want the loaded
> version if we have it?

This would be a reasonable refactor, but touches a good portion of the browser
subapp and several tests.  I'm willing to do this, would it be alright in a
separate branch?

[matthew.scott, 2013-07-19 16:21:45]

Please take a look.

[benjamin.saller, 2013-07-19 16:23:54]

LGTM with the more extensive change coming later as talked about.

[rharding, 2013-07-19 18:34:08]

LGTM

[matthew.scott, 2013-07-19 18:59:11]

*** Submitted:

Cache charms viewed from inspector

Viewing a charm from a deployed service in the inspector now caches the charm
for future viewing.

R=benjamin.saller, rharding
CC=
https://codereview.appspot.com/11526044