Common container: facilitate timing for gadget navigation

- timing information can be broadcast'ed to containers for analysis / custom reporting.
- Shindig will only (to start) synchronously broadcast metadata response time, mrt.
- at Google, we additionally broadcast in-gadget-render times, page render times (prt), dom load (dl) and ol (onload). This is done by an asychronous hook to our custom timing feature. &mid= is in render URL to uniquely identify gadget on page.
- will add tests if impl/design of this look sound.

[johnfargo, 2010-10-19 23:57:59]

We optionally could choose to list (ie. in some kind of "timing.consts" feature)
the various constants used @ Goog, as they're generally applicable anyway (ol,
prt, etc.)

http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
File features/src/main/javascript/features/container/container.js (right):

http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
features/src/main/javascript/features/container/container.js:122:
this.navigateCallback_, gadgetEl, bufferEl);
while we're modifying the GadgetSite ctor API anyway, perhaps we should consider
moving to an arguments-bag approach:
new s.c.GadgetSite({ service: "service", ... });

http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
File features/src/main/javascript/features/container/gadget_site.js (right):

http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
features/src/main/javascript/features/container/gadget_site.js:215: var xrt =
(!cached) ? (shindig.container.util.getCurrentTimeMs() - start) : 0
moving to an arg-bag approach could facilitate "injecting" an optional (rarely
used except for tests) getCurrentTimeMs() impl

[johnfargo, 2010-10-20 00:01:38]

Naming nit: I'd prefer to call the callback "timingCallback" rather than
"navigateCallback," since the latter to me implies requestNavigateTo(),
while we want to register timing for any load event. Correct me if I'm
mixing things up.

On Tue, Oct 19, 2010 at 4:57 PM, <johnfargo@gmail.com> wrote:

> We optionally could choose to list (ie. in some kind of "timing.consts"
> feature) the various constants used @ Goog, as they're generally
> applicable anyway (ol, prt, etc.)
>
>
>
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> File features/src/main/javascript/features/container/container.js
> (right):
>
>
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> features/src/main/javascript/features/container/container.js:122:
> this.navigateCallback_, gadgetEl, bufferEl);
> while we're modifying the GadgetSite ctor API anyway, perhaps we should
> consider moving to an arguments-bag approach:
> new s.c.GadgetSite({ service: "service", ... });
>
>
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> File features/src/main/javascript/features/container/gadget_site.js
> (right):
>
>
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> features/src/main/javascript/features/container/gadget_site.js:215: var
> xrt = (!cached) ? (shindig.container.util.getCurrentTimeMs() - start) :
> 0
> moving to an arg-bag approach could facilitate "injecting" an optional
> (rarely used except for tests) getCurrentTimeMs() impl
>
>
> http://codereview.appspot.com/2560041/
>

[mhermanto, 2010-10-21 04:39:14]

On Tue, Oct 19, 2010 at 5:01 PM, John Hjelmstad <johnfargo@gmail.com> wrote:

> Naming nit: I'd prefer to call the callback "timingCallback" rather than
> "navigateCallback," since the latter to me implies requestNavigateTo(),
> while we want to register timing for any load event. Correct me if I'm
> mixing things up.


What other load event examples do you have in mind that we'd care about
aside navigateTo() ?
The naming navigateCallback, NavigateTiming, etc is somewhat intentional on
my part to keep the scope narrow to the individual action that one wants to
time. If one wants to time other action X, it'll be XCallback, XTiming, etc.


On Tue, Oct 19, 2010 at 4:57 PM, <johnfargo@gmail.com> wrote:
>
>> We optionally could choose to list (ie. in some kind of "timing.consts"
>> feature) the various constants used @ Goog, as they're generally
>> applicable anyway (ol, prt, etc.)
>>
>>
>>
>>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
>> File features/src/main/javascript/features/container/container.js
>> (right):
>>
>>
>>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
>> features/src/main/javascript/features/container/container.js:122:
>> this.navigateCallback_, gadgetEl, bufferEl);
>> while we're modifying the GadgetSite ctor API anyway, perhaps we should
>> consider moving to an arguments-bag approach:
>> new s.c.GadgetSite({ service: "service", ... });
>>
>>
>>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
>> File features/src/main/javascript/features/container/gadget_site.js
>> (right):
>>
>>
>>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
>> features/src/main/javascript/features/container/gadget_site.js:215: var
>> xrt = (!cached) ? (shindig.container.util.getCurrentTimeMs() - start) :
>> 0
>> moving to an arg-bag approach could facilitate "injecting" an optional
>> (rarely used except for tests) getCurrentTimeMs() impl
>>
>>
>> http://codereview.appspot.com/2560041/
>>
>
>

[mhermanto, 2010-10-21 04:39:36]

On Tue, Oct 19, 2010 at 4:57 PM, <johnfargo@gmail.com> wrote:

> We optionally could choose to list (ie. in some kind of "timing.consts"
> feature) the various constants used @ Goog, as they're generally
> applicable anyway (ol, prt, etc.)
>
>
Right. I'm introducing START and XRT for now (for XHR response time for
metadata/token). I'm holding back on OL, PRT, etc, because there's no
feature in Shindig ticking on these yet. But that's the plan. :)


>
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> File features/src/main/javascript/features/container/container.js
> (right):
>
>
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> features/src/main/javascript/features/container/container.js:122:
> this.navigateCallback_, gadgetEl, bufferEl);
> while we're modifying the GadgetSite ctor API anyway, perhaps we should
> consider moving to an arguments-bag approach:
> new s.c.GadgetSite({ service: "service", ... });
>

Sure. I sometimes dislike this because it'd not be clear to users of
GadgetSite ctor what is consumed from the bag, without looking into the
implementation. However, I'd be fine with this one, because it's only new'ed
by the container, ie: not an API that people will directly call.


>
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> File features/src/main/javascript/features/container/gadget_site.js
> (right):
>
>
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> features/src/main/javascript/features/container/gadget_site.js:215: var
> xrt = (!cached) ? (shindig.container.util.getCurrentTimeMs() - start) :
> 0
> moving to an arg-bag approach could facilitate "injecting" an optional
> (rarely used except for tests) getCurrentTimeMs() impl


I'm thinking of just re-declaring this in test --
shindig.container.util.getCurrentTimeMs = function() { return 123; }

(Writing unit tests, will let you know when it's ready)

http://codereview.appspot.com/2560041/
>

[mhermanto, 2010-11-05 01:30:19]

On 2010/10/21 04:39:36, mhermanto wrote:
> On Tue, Oct 19, 2010 at 4:57 PM, <mailto:johnfargo@gmail.com> wrote:
> 
> > We optionally could choose to list (ie. in some kind of "timing.consts"
> > feature) the various constants used @ Goog, as they're generally
> > applicable anyway (ol, prt, etc.)
> >
> >
> Right. I'm introducing START and XRT for now (for XHR response time for
> metadata/token). I'm holding back on OL, PRT, etc, because there's no
> feature in Shindig ticking on these yet. But that's the plan. :)
> 
> 
> >
> >
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> > File features/src/main/javascript/features/container/container.js
> > (right):
> >
> >
> >
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> > features/src/main/javascript/features/container/container.js:122:
> > this.navigateCallback_, gadgetEl, bufferEl);
> > while we're modifying the GadgetSite ctor API anyway, perhaps we should
> > consider moving to an arguments-bag approach:
> > new s.c.GadgetSite({ service: "service", ... });
> >
> 
> Sure. I sometimes dislike this because it'd not be clear to users of
> GadgetSite ctor what is consumed from the bag, without looking into the
> implementation. However, I'd be fine with this one, because it's only new'ed
> by the container, ie: not an API that people will directly call.
> 
> 
> >
> >
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> > File features/src/main/javascript/features/container/gadget_site.js
> > (right):
> >
> >
> >
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> > features/src/main/javascript/features/container/gadget_site.js:215: var
> > xrt = (!cached) ? (shindig.container.util.getCurrentTimeMs() - start) :
> > 0
> > moving to an arg-bag approach could facilitate "injecting" an optional
> > (rarely used except for tests) getCurrentTimeMs() impl
> 
> 
> I'm thinking of just re-declaring this in test --
> shindig.container.util.getCurrentTimeMs = function() { return 123; }
> 
> (Writing unit tests, will let you know when it's ready)
> 
> http://codereview.appspot.com/2560041/
> >

Unit tests added for 1) new changes in this CL, 2) placeholders for existing
code (merely to setup base).

[mhermanto, 2010-11-05 01:30:34]

On 2010/11/05 01:30:19, mhermanto wrote:
> On 2010/10/21 04:39:36, mhermanto wrote:
> > On Tue, Oct 19, 2010 at 4:57 PM, <mailto:johnfargo@gmail.com> wrote:
> > 
> > > We optionally could choose to list (ie. in some kind of "timing.consts"
> > > feature) the various constants used @ Goog, as they're generally
> > > applicable anyway (ol, prt, etc.)
> > >
> > >
> > Right. I'm introducing START and XRT for now (for XHR response time for
> > metadata/token). I'm holding back on OL, PRT, etc, because there's no
> > feature in Shindig ticking on these yet. But that's the plan. :)
> > 
> > 
> > >
> > >
> >
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> > > File features/src/main/javascript/features/container/container.js
> > > (right):
> > >
> > >
> > >
> >
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> > > features/src/main/javascript/features/container/container.js:122:
> > > this.navigateCallback_, gadgetEl, bufferEl);
> > > while we're modifying the GadgetSite ctor API anyway, perhaps we should
> > > consider moving to an arguments-bag approach:
> > > new s.c.GadgetSite({ service: "service", ... });
> > >
> > 
> > Sure. I sometimes dislike this because it'd not be clear to users of
> > GadgetSite ctor what is consumed from the bag, without looking into the
> > implementation. However, I'd be fine with this one, because it's only new'ed
> > by the container, ie: not an API that people will directly call.
> > 
> > 
> > >
> > >
> >
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> > > File features/src/main/javascript/features/container/gadget_site.js
> > > (right):
> > >
> > >
> > >
> >
>
http://codereview.appspot.com/2560041/diff/11001/features/src/main/javascript...
> > > features/src/main/javascript/features/container/gadget_site.js:215: var
> > > xrt = (!cached) ? (shindig.container.util.getCurrentTimeMs() - start) :
> > > 0
> > > moving to an arg-bag approach could facilitate "injecting" an optional
> > > (rarely used except for tests) getCurrentTimeMs() impl
> > 
> > 
> > I'm thinking of just re-declaring this in test --
> > shindig.container.util.getCurrentTimeMs = function() { return 123; }
> > 
> > (Writing unit tests, will let you know when it's ready)
> > 
> > http://codereview.appspot.com/2560041/
> > >
> 
> Unit tests added for 1) new changes in this CL, 2) placeholders for existing
> code (merely to setup base).

Changes re-uploaded. PTAL.

[johnfargo, 2010-11-05 20:31:30]

LGTM

Only comment is FMI.

http://codereview.appspot.com/2560041/diff/31001/features/src/test/javascript...
File features/src/test/javascript/features/container/gadget_site_test.js
(right):

http://codereview.appspot.com/2560041/diff/31001/features/src/test/javascript...
features/src/test/javascript/features/container/gadget_site_test.js:63:
this.assertEquals(1, site.getId());
are these asserts strict requirements or is it just a requirement that
site.getId() != site2.getId()?

[mhermanto, 2010-11-05 20:33:11]

On Fri, Nov 5, 2010 at 1:31 PM, <johnfargo@gmail.com> wrote:

> LGTM
>
> Only comment is FMI.
>
>
>
>
http://codereview.appspot.com/2560041/diff/31001/features/src/test/javascript...
> File features/src/test/javascript/features/container/gadget_site_test.js
> (right):
>
>
>
http://codereview.appspot.com/2560041/diff/31001/features/src/test/javascript...
> features/src/test/javascript/features/container/gadget_site_test.js:63:
> this.assertEquals(1, site.getId());
> are these asserts strict requirements or is it just a requirement that
> site.getId() != site2.getId()?


You're right, with !=.

http://codereview.appspot.com/2560041/
>