Support preloading gadgets metadata as part of container config

Add support for in-lining of gadgets metadata as part of the container page in order to save another round-trip to server to get metadata separately.

[mhermanto, 2011-01-07 02:32:55]

On 2011/01/05 01:21:47, zhoresh wrote:
LGTM++.

Mostly nits, please fix those before submitting.

[mhermanto, 2011-01-07 02:33:09]

http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
File features/src/main/javascript/features/container/container.js (right):

http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:364:
shindig.container.ContainerConfig.PRELOAD_TOKENS = 'preloadTokens';
Major nit on plural, PRELOAD_TOKEN or PRELOAD_METADATAS. service has been using
either, but consistently.

http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:428:
shindig.container.Container.prototype.addPreloadGadgets_ =  function(response) {
extra space before function

http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:449:
shindig.container.Container.prototype.preloadFromConfig_ =  function(config) {
ditto

http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:458:
this.service_.addGadgetTokens(tokens, refTime);
Same major nit, rename method with either singular or plural.

http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
File features/src/main/javascript/features/container/service.js (right):

http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/service.js:115: var resp =
self.updateResponse_(response[id], id, null, currentTimeMs);
Just do --
var resp = response[id];
self.updateResponse(resp, ...); 
// remove return resp from updateResponse.

http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/service.js:130: * @param
{Number} data time to override responseTime (in order to support external
caching)
@param {Object} refTime ...
(Please fix the rest)

http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/service.js:151: * @param
{Object} the cache to update
add @private, should tell compiler that this is safe obfuscate.

http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/service.js:164: * @param
{Object} gadget metadata item
No "gadget" param.

http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/service.js:167: * @param
{Number} current time
Ditto.

http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/service.js:169:
shindig.container.Service.prototype.updateResponse_ = function(resp, id,
refTime, currentTimeMs) {
Change to resp, id, currentTimeMs, opt_refTime. From the caller side, just call
the first 3 params.

http://codereview.appspot.com/3842042/diff/1/features/src/test/javascript/fea...
File features/src/test/javascript/features/container/container_test.js (right):

http://codereview.appspot.com/3842042/diff/1/features/src/test/javascript/fea...
features/src/test/javascript/features/container/container_test.js:79:
//test[container.preloadedGadgetUrls_['preloaded1.xml']] = 6;
remove?

[zhoresh, 2011-01-07 18:17:08]

Thanks for the review, all comments were addressed and code is committed.


On Thu, Jan 6, 2011 at 6:33 PM, <mhermanto@gmail.com> wrote:

>
>
>
http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
> File features/src/main/javascript/features/container/container.js
> (right):
>
>
>
http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/container/container.js:364:
> shindig.container.ContainerConfig.PRELOAD_TOKENS = 'preloadTokens';
> Major nit on plural, PRELOAD_TOKEN or PRELOAD_METADATAS. service has
> been using either, but consistently.
>
>
>
http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/container/container.js:428:
> shindig.container.Container.prototype.addPreloadGadgets_ =
> function(response) {
> extra space before function
>
>
>
http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/container/container.js:449:
> shindig.container.Container.prototype.preloadFromConfig_ =
> function(config) {
> ditto
>
>
>
http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/container/container.js:458:
> this.service_.addGadgetTokens(tokens, refTime);
> Same major nit, rename method with either singular or plural.
>
>
>
http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
> File features/src/main/javascript/features/container/service.js (right):
>
>
>
http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/container/service.js:115: var resp
> = self.updateResponse_(response[id], id, null, currentTimeMs);
> Just do --
> var resp = response[id];
> self.updateResponse(resp, ...);
> // remove return resp from updateResponse.
>
>
>
http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/container/service.js:130: * @param
> {Number} data time to override responseTime (in order to support
> external caching)
> @param {Object} refTime ...
> (Please fix the rest)
>
>
>
http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/container/service.js:151: * @param
> {Object} the cache to update
> add @private, should tell compiler that this is safe obfuscate.
>
>
>
http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/container/service.js:164: * @param
> {Object} gadget metadata item
> No "gadget" param.
>
>
>
http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/container/service.js:167: * @param
> {Number} current time
> Ditto.
>
>
>
http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
> features/src/main/javascript/features/container/service.js:169:
> shindig.container.Service.prototype.updateResponse_ = function(resp, id,
> refTime, currentTimeMs) {
> Change to resp, id, currentTimeMs, opt_refTime. From the caller side,
> just call the first 3 params.
>
>
>
http://codereview.appspot.com/3842042/diff/1/features/src/test/javascript/fea...
> File features/src/test/javascript/features/container/container_test.js
> (right):
>
>
>
http://codereview.appspot.com/3842042/diff/1/features/src/test/javascript/fea...
> features/src/test/javascript/features/container/container_test.js:79:
> //test[container.preloadedGadgetUrls_['preloaded1.xml']] = 6;
> remove?
>
>
> http://codereview.appspot.com/3842042/
>

[johnfargo, 2011-01-07 22:51:53]

LGTM++

http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
File features/src/main/javascript/features/container/container.js (right):

http://codereview.appspot.com/3842042/diff/1/features/src/main/javascript/fea...
features/src/main/javascript/features/container/container.js:352:
shindig.container.ContainerConfig.PRELOAD_REF_TIME = 'preloadRefTime';
nit for a later time: I'd personally prefer to get rid of these long-form
"constants"