Last Comment Bug 822480 - Add in the Resource Timing API
: Add in the Resource Timing API
Status: RESOLVED FIXED
[timing][performance][standards][netp...
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
-- normal with 6 votes (vote)
: mozilla31
Assigned To: Valentin Gosu [:valentin]
:
: Andrew Overholt [:overholt]
Mentors:
https://dvcs.w3.org/hg/webperf/raw-fi...
Depends on: 1071527 CVE-2015-7207 CVE-2016-1967 CVE-2016-5250
Blocks: 845315 936813 936814 1002855 1089923
  Show dependency treegraph
 
Reported: 2012-12-17 15:31 PST by Andy McKay [:andym]
Modified: 2016-04-01 16:33 PDT (History)
32 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (40.46 KB, patch)
2013-02-12 05:41 PST, Andrea Marchesini [:baku]
bzbarsky: feedback-
Details | Diff | Splinter Review
WIP (37.75 KB, patch)
2013-03-07 05:31 PST, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
Resource Timing intrface (19.37 KB, patch)
2013-08-14 13:08 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
Resource Timing intrface (+ test) (21.88 KB, patch)
2013-08-14 13:14 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
Resource Timing intrface (spec links updated) (21.82 KB, patch)
2013-08-15 11:35 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
bug-822480-fix.patch (33.31 KB, patch)
2013-08-21 19:15 PDT, Adrian Lungu
bzbarsky: feedback+
Details | Diff | Splinter Review
Version 3 (add the entry after the resource is loaded) (59.45 KB, patch)
2013-08-26 23:23 PDT, Adrian Lungu
bzbarsky: feedback+
Details | Diff | Splinter Review
Version 4 (the nsPerformance object is retrived in the nsHttpChannel via nsDocShell) (57.59 KB, patch)
2013-08-27 18:25 PDT, Adrian Lungu
bzbarsky: feedback+
Details | Diff | Splinter Review
Version 5 (initiator type added) (79.09 KB, patch)
2013-09-05 17:33 PDT, Adrian Lungu
bzbarsky: feedback-
Details | Diff | Splinter Review
Resource Timing Interface (v1) (16.46 KB, patch)
2013-09-08 21:26 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
Build and add the performance entry (27.86 KB, patch)
2013-09-08 21:28 PDT, Adrian Lungu
bzbarsky: feedback+
Details | Diff | Splinter Review
Initiator type added (v1) (27.13 KB, patch)
2013-09-08 21:31 PDT, Adrian Lungu
bzbarsky: feedback+
Details | Diff | Splinter Review
part3_v1_to_v2.diff (13.20 KB, patch)
2013-09-10 11:40 PDT, Adrian Lungu
bzbarsky: feedback+
Details | Diff | Splinter Review
Build and add the performance entry (v2) (51.55 KB, patch)
2013-09-11 14:59 PDT, Adrian Lungu
honzab.moz: feedback-
Details | Diff | Splinter Review
part3_v1_to_v3.diff (9.27 KB, patch)
2013-09-12 11:00 PDT, Adrian Lungu
bzbarsky: feedback+
Details | Diff | Splinter Review
Initiator type added (v3) (30.64 KB, patch)
2013-09-12 11:30 PDT, Adrian Lungu
adrian.lungu89: feedback+
Details | Diff | Splinter Review
Entries management (v1) (11.32 KB, patch)
2013-09-12 17:19 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
Resource Timing Interface (v2) (18.97 KB, patch)
2013-09-12 17:55 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
part1_v2_to_v3.diff (5.72 KB, patch)
2013-09-23 10:46 PDT, Adrian Lungu
bzbarsky: feedback+
Details | Diff | Splinter Review
Build and add the performance entry (v3) (57.12 KB, patch)
2013-09-24 21:53 PDT, Adrian Lungu
bzbarsky: feedback-
Details | Diff | Splinter Review
Subdoc bugfix (13.02 KB, patch)
2013-09-26 16:41 PDT, Adrian Lungu
bzbarsky: feedback+
Details | Diff | Splinter Review
part2_fixIframe_interdiff (1.53 KB, patch)
2013-09-27 14:01 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
part0_preparePerformanceTiming_v1 (12.74 KB, patch)
2013-09-27 18:22 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
part0_preparePerformanceTiming_v1_to_v2.diff (7.99 KB, patch)
2013-09-30 22:52 PDT, Adrian Lungu
bzbarsky: feedback+
Details | Diff | Splinter Review
part0_adjustRedirects_v1.patch (24.53 KB, patch)
2013-10-01 01:31 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
part2_addEntries_v4.patch (27.47 KB, patch)
2013-10-02 13:40 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
part4_entriesManagement_v2.patch (6.66 KB, patch)
2013-10-02 17:51 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
part5_addPreference_v1.patch (5.32 KB, patch)
2013-10-02 17:53 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
part0_adjustRedirects_v1_to_v2.diff (16.94 KB, patch)
2013-10-08 16:20 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
part2_removeIsDocFlag.diff (5.96 KB, patch)
2013-10-08 18:08 PDT, Adrian Lungu
honzab.moz: feedback+
Details | Diff | Splinter Review
part0_adjustRedirects_v2.patch (24.62 KB, patch)
2013-10-09 10:35 PDT, Adrian Lungu
honzab.moz: feedback-
Details | Diff | Splinter Review
part6_testing_v1.patch (12.30 KB, patch)
2013-10-11 17:44 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
resourceTimingFull (v1) (105.40 KB, patch)
2013-10-15 18:39 PDT, Adrian Lungu
honzab.moz: feedback+
Details | Diff | Splinter Review
resourceTimingFull_v2.patch (113.81 KB, patch)
2013-10-16 23:26 PDT, Adrian Lungu
honzab.moz: review-
Details | Diff | Splinter Review
resourceTimingFull_v1_to_v2.diff (39.92 KB, patch)
2013-10-16 23:28 PDT, Adrian Lungu
honzab.moz: review-
Details | Diff | Splinter Review
resourceTimingFull_v3.patch (117.04 KB, patch)
2013-10-17 19:13 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
resourceTimingFull_v4.patch (116.87 KB, patch)
2013-10-17 23:56 PDT, Adrian Lungu
honzab.moz: review+
Details | Diff | Splinter Review
resourceTimingFull_v5.patch (118.43 KB, patch)
2013-10-18 18:29 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
resourceTimingFull_v5.patch (b2g fixed) (118.43 KB, patch)
2013-10-24 10:49 PDT, Adrian Lungu
no flags Details | Diff | Splinter Review
resourceTimingFull_v5.patch (proper patch attached) (118.42 KB, patch)
2013-10-24 10:55 PDT, Adrian Lungu
jst: review+
Details | Diff | Splinter Review
resourceTimingFull_v6.patch (117.68 KB, patch)
2013-11-09 11:27 PST, Adrian Lungu
no flags Details | Diff | Splinter Review
resourceTimingFull_v7.patch (117.67 KB, patch)
2013-11-09 11:45 PST, Adrian Lungu
no flags Details | Diff | Splinter Review
Add in the Resource Timing API (118.17 KB, patch)
2014-03-31 15:54 PDT, Valentin Gosu [:valentin]
no flags Details | Diff | Splinter Review
Add in the Resource Timing API (119.27 KB, patch)
2014-04-08 07:40 PDT, Valentin Gosu [:valentin]
no flags Details | Diff | Splinter Review
pref_resourceTiming.patch (1.12 KB, patch)
2014-04-08 07:49 PDT, Valentin Gosu [:valentin]
bzbarsky: review+
Details | Diff | Splinter Review
Add in the Resource Timing API (118.60 KB, patch)
2014-04-10 11:54 PDT, Valentin Gosu [:valentin]
no flags Details | Diff | Splinter Review
res_timing_e10s.patch (6.88 KB, patch)
2014-04-10 17:31 PDT, Valentin Gosu [:valentin]
no flags Details | Diff | Splinter Review
imported patch pref_resourceTiming.patch (1.27 KB, patch)
2014-04-16 18:49 PDT, Valentin Gosu [:valentin]
no flags Details | Diff | Splinter Review
Fix for e10s (34.69 KB, patch)
2014-04-16 18:50 PDT, Valentin Gosu [:valentin]
honzab.moz: review+
bzbarsky: review+
Details | Diff | Splinter Review
pref_resourceTiming.patch (1.24 KB, patch)
2014-04-18 15:27 PDT, Valentin Gosu [:valentin]
no flags Details | Diff | Splinter Review
res_timing_e10s.patch (34.24 KB, patch)
2014-04-18 15:28 PDT, Valentin Gosu [:valentin]
no flags Details | Diff | Splinter Review

Description User image Andy McKay [:andym] 2012-12-17 15:31:19 PST
In bug 576006 the Navigation Timing API was added. The Resource Timing API is similar, but reports the timings for each resource. Having this would be really helpful to developers and the developer tools.

Landed in IE10 and Chromium I believe.

Spec: http://www.w3.org/TR/resource-timing/
Comment 1 User image Rob Campbell [:rc] (:robcee) 2013-01-02 06:47:41 PST
Agreed, we should definitely present this.

It would be pretty easy to add to the Web Console. When we get a network panel, we could populate it there as well. I'm going to move this to Web Console for now.
Comment 2 User image Andrea Marchesini [:baku] 2013-02-12 05:41:10 PST
Created attachment 712873 [details] [diff] [review]
WIP

Boris, I need feedback about I'm approaching this patch.

What I'm going is:

There is a nsPerformanceManager object that takes care of nsPerformanceEntry.
This nsPerformanceManager is shared between window in the nsDocLoader.

The reason why I share the PerformanceManager is that the window that is handled by nsDocLoader is not the same that is "running" javascript. So I just share the object. The same approach is done for different elements related to nsPerformance.

nsPerformanceManager receives communication any time a new request is created/removed. For any request it creates a nsPerformanceEntry. The ownership of the nsPerformanceEntry is changed when getEntries() is called.

This is what I have done right now. Let me know if you see big big big errors. Thanks!
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2013-02-14 12:09:48 PST
Comment on attachment 712873 [details] [diff] [review]
WIP

You may want to use an XPCOM hashtable, not raw pldhash.

The one thing confusing me is that the docloader never updates its mPerformance.  Shouldn't it do that once a new document load starts or something?
Comment 4 User image Mihai Sucan [:msucan] 2013-02-15 05:02:15 PST
Given the Gecko patch here, we should move the bug to Core, and file a different bug for the Web Console.
Comment 5 User image Rob Campbell [:rc] (:robcee) 2013-02-26 06:23:31 PST
moved to Core: DOM: Other for lack of a better component.

added bug 845315 for the Console Output work.
Comment 6 User image Andrea Marchesini [:baku] 2013-03-07 05:31:54 PST
Created attachment 722193 [details] [diff] [review]
WIP

Boris, I need a feedback :)
Do you see big big errors til now?
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2013-03-07 10:37:16 PST
I'm not going to be able to get to this until Monday.
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2013-03-14 11:55:03 PDT
Comment on attachment 722193 [details] [diff] [review]
WIP

I don't quite follow the nsGlobalWindow change.  Why is that change OK?  And why do we need to reset the entry manager there?

I'm trying to understand why we update the timings on request add and again on remove but not in between.  If we don't need to do it in between, why do we need to do it on both add and remove?

Might be good to have versions of GetName and GetEntryType that just return a const nsAString& so you don't need the string temporaries to compare.

>+++ b/dom/bindings/Bindings.conf

Please just use classnames and header files that won't require adding stuff here.

What clears the docloader's mPerformance?  If nothing, why is it ok to not clear it?

I didn't read all the implementation details carefully yet, because it wasn't clear whether you want me to.  Let me know if you do.
Comment 9 User image Patrick McManus [:mcmanus] 2013-03-14 13:36:11 PDT
awesome to see this is being done - let me know if I can help. (or :mayhemer - he did a bunch of the nav timing work)
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2013-03-14 13:43:09 PDT
I think mayhemer may be a good reviewer for the guts of this, in fact.
Comment 11 User image Steve Workman [:sworkman] (INACTIVE) 2013-07-08 16:08:21 PDT
Assigning this to Adrian to work on during his internship.
Comment 12 User image Adrian Lungu 2013-08-14 13:08:45 PDT
Created attachment 790409 [details] [diff] [review]
Resource Timing intrface

I brought back the Resource Timing interface and I've also tried to address the existing feedback (related to the interface). 
I tried to use only the existing files (nsPerformance and Performance.webidl), but it looks like the Binder will automatically include files like "mozilla/dom/PerformanceEntry.h", so I couldn't declare the new classes in the existing "nsPerformance.h" file. I ended up using new files for both the PerformanceEntry and the PerformanceResourceTiming.
Please let me know if I am going in the right directions, so far.
Thanks!
Comment 13 User image Adrian Lungu 2013-08-14 13:14:51 PDT
Created attachment 790410 [details] [diff] [review]
Resource Timing intrface (+ test)

+ The test file.
Comment 14 User image Honza Bambas (:mayhemer) 2013-08-15 08:50:43 PDT
Adrian, what feedback do you want from me?  Is this API specified? (I presume it is).  Can you link to the spec here in the bug?  If you want feedback on webidl changes, then I'm NOT the right person.
Comment 15 User image Honza Bambas (:mayhemer) 2013-08-15 08:51:42 PDT
BTW, the idl files are completely missing any documentation so I absolutely don't know what they represent and are purposed for.
Comment 16 User image Adrian Lungu 2013-08-15 11:28:16 PDT
Specs

Resource Timing : http://www.w3.org/TR/resource-timing/

Interfaces:
Performance Entry : http://www.w3.org/TR/performance-timeline/#performanceentry
Performance Resource Timing : http://www.w3.org/TR/resource-timing/#performanceresourcetiming
Comment 17 User image Adrian Lungu 2013-08-15 11:35:52 PDT
Created attachment 790874 [details] [diff] [review]
Resource Timing intrface (spec links updated)

I've updated the position of the spec links. Now, they are placed in the file's header.
Comment 18 User image Adrian Lungu 2013-08-15 13:11:27 PDT
(In reply to Honza Bambas (:mayhemer) from comment #15)
> BTW, the idl files are completely missing any documentation so I absolutely
> don't know what they represent and are purposed for.

I see that most of the idl files have only the spec link in their header. Is there any other documentation that should be placed in these files?
Comment 19 User image Boris Zbarsky [:bz] (still a bit busy) 2013-08-21 18:49:01 PDT
> I ended up using new files for both the PerformanceEntry and the
> PerformanceResourceTiming.

For what it's worth, that seems better to me anyway.  You could have done the other with explicit HeaderFile annotations, but better not to.
Comment 20 User image Adrian Lungu 2013-08-21 19:15:09 PDT
Created attachment 793794 [details] [diff] [review]
bug-822480-fix.patch

This is still work in progress, but I added more logic and also improved the JS interface.
I tried to use as much as I could from the existing navigation timing. One of the biggest challenge was to find a proper place to build the resource timing objects. I ended up choosing "nsDocLoader::OnStartRequest", but I don't know if it's actually the best choice.
I put some temporary comments in the files and also some debug printing code that will be removed later.
Comment 21 User image Boris Zbarsky [:bz] (still a bit busy) 2013-08-21 20:04:02 PDT
Comment on attachment 793794 [details] [diff] [review]
bug-822480-fix.patch

The spec link in Performance.webidl isn't linking to where that partial interface is defined.  Please fix the link?

Putting things in nsDocLoader::OnStartRequest is not great because it doesn't do what the spec says to for images (e.g. it has no way to only add one entry when image loads are coalesced) and it has no way to determine the initiatorType.  What you probably want instead is to add code to the various places that actually open a channel... Or possibly to channels themselves (note that per spec, data: channels are not supposed to add resource timing entries, for example).  We don't really have a very good setup for this in necko, unfortunately.  :(  On the other hand, it's not like the spec clearly defines what should happen here; see the feedback I just sent in.  Maybe the right solution for now is to do this at the places that open a channel and not add the channel if it's not an nsITimedChannel or something...

You shouldn't need the contentType from the channel for this stuff.  The entryType is not the channel contentType; it's "resource" for PerformanceResourceTiming entries.

This is not implementing http://www.w3.org/TR/resource-timing/#performanceresourcetiming-methods yet, right?
Comment 22 User image Adrian Lungu 2013-08-22 11:02:21 PDT
(In reply to Boris Zbarsky [:bz] from comment #21)
> Comment on attachment 793794 [details] [diff] [review]
> bug-822480-fix.patch
> 
> The spec link in Performance.webidl isn't linking to where that partial
> interface is defined.  Please fix the link?
> 
> Putting things in nsDocLoader::OnStartRequest is not great because it
> doesn't do what the spec says to for images (e.g. it has no way to only add
> one entry when image loads are coalesced) and it has no way to determine the
> initiatorType.  What you probably want instead is to add code to the various
> places that actually open a channel... Or possibly to channels themselves
> (note that per spec, data: channels are not supposed to add resource timing
> entries, for example).  We don't really have a very good setup for this in
> necko, unfortunately.  :(  On the other hand, it's not like the spec clearly
> defines what should happen here; see the feedback I just sent in.  Maybe the
> right solution for now is to do this at the places that open a channel and
> not add the channel if it's not an nsITimedChannel or something...
I see.. Thanks for the tips!
> 
> You shouldn't need the contentType from the channel for this stuff.  The
> entryType is not the channel contentType; it's "resource" for
> PerformanceResourceTiming entries.
> 
> This is not implementing
> http://www.w3.org/TR/resource-timing/#performanceresourcetiming-methods yet,
> right?
Yes. I am aware of this, but it's just not implemented yet. I wanted to start getting some feedback for the timings fetching before I'll start implementing the management of the entry list.
Comment 23 User image Boris Zbarsky [:bz] (still a bit busy) 2013-08-22 12:01:38 PDT
Note that a response to one of the spec issues I raised makes it clear that the intent is that PerformanceResourceTiming entries are only added to the entry list when the load completes.
Comment 24 User image Adrian Lungu 2013-08-22 13:16:23 PDT
(In reply to Boris Zbarsky [:bz] from comment #23)
> Note that a response to one of the spec issues I raised makes it clear that
> the intent is that PerformanceResourceTiming entries are only added to the
> entry list when the load completes.

I was thinking to use 2 entries arrays: one for the resources that were started (but not completed) and one the resources that were completed. Once a resource starts loading, I would add it to the first array (so I can track it without making it available through the "getEntries()" methods). When the load completes, I would just move the entry to the other list and insert it in the right place (to keep the entries sorted with respect to starTime).
Do you think that this approach would be fine?
Comment 25 User image Boris Zbarsky [:bz] (still a bit busy) 2013-08-22 19:02:50 PDT
That seems reasonable if we need to create the entries up front.  If we can just create them at the point when the load completes, so much the better.
Comment 26 User image Adrian Lungu 2013-08-26 23:23:01 PDT
Created attachment 795864 [details] [diff] [review]
Version 3 (add the entry after the resource is loaded)

So, I managed to add the entry only after the resource is loaded, but this lead to some extra changes in nsITimedChannel.idl, nsDOMNavigationTiming and nsPerformance. Could you, please, take a look and tell me if this would be the right way to continue the implementation?
So far, I implemented it only for images (as a proof of concept) and I will extend it to all the other resources afterwards.
Comment 27 User image Adrian Lungu 2013-08-27 00:38:40 PDT
Comment on attachment 795864 [details] [diff] [review]
Version 3 (add the entry after the resource is loaded)

Wrong flags..
Comment 28 User image Boris Zbarsky [:bz] (still a bit busy) 2013-08-27 10:46:10 PDT
Comment on attachment 795864 [details] [diff] [review]
Version 3 (add the entry after the resource is loaded)

I'm not sure why we need an accessor on documents to get the performance object and so forth.  Can we not just get it from the loadgroup (via the window, which we can get from the loadgroup) at the point when the load is finishing up?

If we do want to get it from the document, we should be getting it from the document's inner window, not the outer's current inner.

Apart from that seems fairly reasonable...
Comment 29 User image Adrian Lungu 2013-08-27 18:25:53 PDT
Created attachment 796398 [details] [diff] [review]
Version 4 (the nsPerformance object is retrived in the nsHttpChannel via nsDocShell)

This approach should be closer to the one we were talking about on IRC.
Also, I've added the attribute "initiatorType" in nsITimedChannel. I think that we have the greatest chances to retrieve the initiator type when the channel is being created.
Comment 30 User image Boris Zbarsky [:bz] (still a bit busy) 2013-08-27 19:22:11 PDT
Comment on attachment 796398 [details] [diff] [review]
Version 4 (the nsPerformance object is retrived in the nsHttpChannel via nsDocShell)

>+  nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(loadContext);

This will do the wrong thing in SVG resource documents.

That's why we have an nsILoadContext interface, in fact: otherwise you could just try to QueryNotificationCallbacks for nsIDocShell, and fail.

What you want is loadContext->GetAssociatedWindow().

That will always give you an outer window, unfortunately; that's the main drawback of trying to do this from inside necko.  :(  To help with that, we should not add entries for channels that were not successful, maybe, so that canceled loads during a page transition do not end up in the new page's .performance.

I don't think you need an mDocumentPerformance member...  which is good, because in its current form it would leak.

The mDOMTiming member is write-only, no?  Is it needed?
Comment 31 User image Adrian Lungu 2013-08-28 10:29:27 PDT
(In reply to Boris Zbarsky [:bz] from comment #30)
> Comment on attachment 796398 [details] [diff] [review]
> Version 4 (the nsPerformance object is retrived in the nsHttpChannel via
> nsDocShell)
> 
> >+  nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(loadContext);
> 
> This will do the wrong thing in SVG resource documents.
> 
> That's why we have an nsILoadContext interface, in fact: otherwise you could
> just try to QueryNotificationCallbacks for nsIDocShell, and fail.
> 
> What you want is loadContext->GetAssociatedWindow().
> 
> That will always give you an outer window, unfortunately; that's the main
> drawback of trying to do this from inside necko.  :(  To help with that, we
> should not add entries for channels that were not successful, maybe, so that
> canceled loads during a page transition do not end up in the new page's
> .performance.
In the current implementation, entries are added only after they were loaded, so this should work.
> 
> I don't think you need an mDocumentPerformance member...  which is good,
> because in its current form it would leak.
> 
> The mDOMTiming member is write-only, no?  Is it needed?
The mDOMTiming member is used to track the redirects. We could remove it, but tracking the redirects from the outside of the nsHTTPChannel could mean that we will have to add the entries during "OnStartRequest" and not "OnStopRequest". And this will lead to the other problem (inner vs outer window).
Do you think that I should find another way of tracking the redirects?
Comment 32 User image Boris Zbarsky [:bz] (still a bit busy) 2013-08-28 10:38:22 PDT
> The mDOMTiming member is used to track the redirects.

Oh, I see.  I'd missed where we read from it.

Could you just track those two timestamps directly instead of adding an entire new object allocation and whatnot?
Comment 33 User image Adrian Lungu 2013-08-28 10:54:00 PDT
(In reply to Boris Zbarsky [:bz] from comment #32)
> > The mDOMTiming member is used to track the redirects.
> 
> Oh, I see.  I'd missed where we read from it.
> 
> Could you just track those two timestamps directly instead of adding an
> entire new object allocation and whatnot?

Yes, I can do that, but it will result in some code duplication. Would that be ok?
Comment 34 User image Boris Zbarsky [:bz] (still a bit busy) 2013-08-28 11:02:53 PDT
I think it should be; the duplication will be pretty minimal.

In fact, I expect the current setup in the patch is broken anyway since the information is not copied to the post-redirect channel.
Comment 35 User image Adrian Lungu 2013-08-30 13:54:14 PDT
So, while doing some tests to check if the timings are fetched correctly, I noticed that "connectionStart" end "connectionEnd" are set to "0" most of the times (even for the main page - the main page will be filtered because it is not actually a resource).  I checked the specs and it says the same thing for both the navigation timing[1] and the resource timing[2]: "If a persistent connection [RFC 2616] is used or the resource is retrieved from relevant application caches or local resources, this attribute must return value of domainLookupEnd".
The problem is that the code is doing something different. The only setters for these two metrics are in nsHttpTransaction::OnTransportStatus()[3]. If the metrics are not set here, they will remain 0 (and not set to the value of "domainLookupEnd"). Also, there is no check for this case in the nsPerformance code[4]. Would this be a navigation timing bug?

Another thing that I tried to figure out in the last days was the initiatorType[5]. It looks like I need to get the html tag that uses the resource. Could you point me a starting point for my search, please?

Thanks!

[1] http://www.w3.org/TR/navigation-timing/#dom-performancetiming-connectstart
[2] http://www.w3.org/TR/resource-timing/#dom-performanceresourcetiming-connectstart
[3] http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpTransaction.cpp#l442
[4] http://dxr.mozilla.org/mozilla-central/source/dom/base/nsPerformance.cpp#l66
[5] http://www.w3.org/TR/resource-timing/#initiatorType-attribute
Comment 36 User image Boris Zbarsky [:bz] (still a bit busy) 2013-08-30 14:07:56 PDT
nsPerformance looks like it will return the value it got from the channel, or if the channel value was not initialized the value of fetchStart or something like that, right  See TimeStampToDOMOrFetchStart.

Note that the spec here is moderately insane, and we may not implement it exactly; there are spec issues raised.

> It looks like I need to get the html tag that uses the resource

Sure does.  Which means that needs to be passed through down to whatever APIs actually set up the channel (e.g. loadImage).
Comment 37 User image Adrian Lungu 2013-08-30 14:16:30 PDT
(In reply to Boris Zbarsky [:bz] from comment #36)
> nsPerformance looks like it will return the value it got from the channel,
> or if the channel value was not initialized the value of fetchStart or
> something like that, right  See TimeStampToDOMOrFetchStart.
I see. So, should we do the same thing for the resource timing, to preserve the consistence?
Comment 38 User image Boris Zbarsky [:bz] (still a bit busy) 2013-08-30 14:19:02 PDT
Yes.
Comment 39 User image Adrian Lungu 2013-09-05 17:33:45 PDT
Created attachment 800491 [details] [diff] [review]
Version 5 (initiator type added)

Summary:
- I changed the connectStart / connectEnd metrics to be backward compatible with the navigation timing;
- I filtered the main page (so is not added as a performance entry anymore)
- I added the initiator type for most of the possible initiators[1]. I still have to figure out how things works for the <svg> tags (what kind of resources can be added and how). Also, there may be some cases that I've missed, but I will return to this again later, after implementing the entries management.

[1] http://www.w3.org/TR/resource-timing/#dom-performanceresourcetiming-initiatortype
Comment 40 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-06 12:24:54 PDT
Comment on attachment 800491 [details] [diff] [review]
Version 5 (initiator type added)

I would really appreciate breaking this up into independent changesets for things like "change the current performance timing code", "change all the image-loading scignatures", etc.  That would make it a lot simpler to review.  As it is, review on this is ending up O(N^2) in length of patch, and N is growing fast....
Comment 41 User image Adrian Lungu 2013-09-08 21:26:27 PDT
Created attachment 801357 [details] [diff] [review]
Resource Timing Interface (v1)

This patch contains only the JS interfaces and the C++ associated classes (no logic is added).
Comment 42 User image Adrian Lungu 2013-09-08 21:28:55 PDT
Created attachment 801358 [details] [diff] [review]
Build and add the performance entry

This patch add the logic for building and adding the performance entries.
Comment 43 User image Adrian Lungu 2013-09-08 21:31:26 PDT
Created attachment 801359 [details] [diff] [review]
Initiator type added (v1)

I had to make some changes in the interfaces for the image loading, so I can figure out the initiator of the resource.
Comment 44 User image Adrian Lungu 2013-09-08 21:35:02 PDT
I split the previous patch into 3 patches, each of them addressing a different sub-problem. Please, let me know if this is the approach that you suggested. 
Thanks!
Comment 45 User image Adrian Lungu 2013-09-09 13:05:04 PDT
I see that Chromium changed the specs a little bit and converted "onresourcetimingbufferfull" from "attribute Function" [1] to "attribute EventHandler" [2]. Should we do the same thing?
Also, I don't see any "attribute Function" in our code so I don't know how this should be translated into the cpp code.

[1] http://www.w3.org/TR/resource-timing/#performanceresourcetiming-methods
[2] http://lists.w3.org/Archives/Public/public-web-perf/2013Aug/0021.html
Comment 46 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-09 13:41:05 PDT
The right spec to look at is http://www.w3c-test.org/webperf/specs/ResourceTiming/ which has EventHandler.  That's what we should implement, in theory.

But this spec makes no sense right now.  I posted http://lists.w3.org/Archives/Public/public-web-perf/2013Sep/0026.html

It's hard to say what to implement here until the spec bustage is sorted out; for now just don't implement onresourcetimingbufferfull?
Comment 47 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-09 14:15:25 PDT
Comment on attachment 801359 [details] [diff] [review]
Initiator type added (v1)

Thanks, this is easier to review.  And hopefully won't need more revisions.  ;)

> +++ b/content/base/src/nsImageLoadingContent.cpp
> +                                 NS_LITERAL_STRING("img"),

This is wrong. There are other tagnames that go through this code. In particular, SVG images and image inputs.

> +++ b/content/base/src/nsObjectLoadingContent.cpp

> +      if (thisContent->NodeInfo()->Equals(nsGkAtoms::embed)) {
> +        timedChannel->SetInitiatorType(NS_LITERAL_STRING("embed"));
> +      } else if (thisContent->NodeInfo()->Equals(nsGkAtoms::object)) {
> +        timedChannel->SetInitiatorType(NS_LITERAL_STRING("object"));

Why not use NodeInfo()->LocalName()?

> +++ b/docshell/base/nsDocShell.cpp
> +        if (aFirstParty) {

aFirstParty indicates whether this is a user-initiated action. In particular, it will be true for a link click targeted at a subframe. Is this really what we want to be testing here?

> +++ b/dom/base/nsPerformance.cpp
> +    nsString defaultInitiator = NS_LITERAL_STRING("other");

Please use NS_NAMED_LITERAL_STRING. 

>+++ b/image/public/imgILoader.idl

Rev the IID.

>+++ b/netwerk/base/public/nsITimedChannel.idl

Likewise.

This is missing the changes to nsHttpChannel.h.

With those bits addressed, looks good.  Post an interdiff that addresses the review comments?
Comment 48 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-09 15:41:18 PDT
Comment on attachment 801358 [details] [diff] [review]
Build and add the performance entry

This seems to be copying a bunch of boilerplate from nsPerformanceTiming.  Can we avoid that?

At some point, it would be good to have a necko peer look at the bits here.
Comment 49 User image Adrian Lungu 2013-09-10 11:40:37 PDT
Created attachment 802505 [details] [diff] [review]
part3_v1_to_v2.diff

nsHttpChannel.h was not modified for this patch. The changes were applied to nsITimedChannel.idl.

if (aFirstParty){
  // It is the main page and not a resource
  timedChannel->SetNavigationEnabled(true);
} else {
  // It is a <frame> / <iframe> resource
  timedChannel->SetInitiatorType(NS_LITERAL_STRING("subdocument"));
}
Here, I am trying to differentiate the main document and the sub-documents (like "iframe") contained by the main document. I don't know if this is the right flag to use for this. I did some testing and this looked like a possible stating point.
Comment 50 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-10 11:47:39 PDT
> Here, I am trying to differentiate the main document and the sub-documents

aFirstParty tells you nothing about that.  You probably want IsFrame().
Comment 51 User image Adrian Lungu 2013-09-10 13:28:34 PDT
(In reply to Boris Zbarsky [:bz] from comment #48)
> Comment on attachment 801358 [details] [diff] [review]
> Build and add the performance entry
> 
> This seems to be copying a bunch of boilerplate from nsPerformanceTiming. 
> Can we avoid that?
All the methods from nsPerformanceTiming return a relative timing (the time elapsed from the beginning of the navigation). This timing can be used by the PerformanceResourceTiming, but only if the time stamp is valid. If the time stamp is not valid, it will return the fetchTime(). And fetchTime() is different for the document and for all the other resources. So, what I can do is to add some new methods in the nsPerformanceTiming that return the time stamp (which is absolute) and transform this time stamp in a DOMHighResTimeStamp in the PerfomanceResourceTiming class. But this will add a some extra logic in the nsPerformanceTiming. Do you think that it will worth?
> 
> At some point, it would be good to have a necko peer look at the bits here.
I talked to Honza and he can help us here.
Comment 52 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-10 13:44:04 PDT
Let's take a concrete example: PerformanceResourceTiming::RequestStart.

This gets a timestamp from the channel, like your new class, after doing a preference check your class should be doing too.

Then it calls TimeStampToDOMOrFetchStart() which returns the difference from mNavigationStartTimeStamp or the return value of GetFetchStart().

That's the same thing your code does.  Why are we ending up using different objects for the two, exactly?  I'm not saying use the document's timing, but the document's timing and the resource's timing seem like they should be subclasses of a common class or something, if not the same object.
Comment 53 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-10 13:44:32 PDT
Oh, and the split between nsPerformanceTiming and the DOMTiming stuff is ... weird.  Not sure why we have that.
Comment 54 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-10 13:57:59 PDT
Comment on attachment 802505 [details] [diff] [review]
part3_v1_to_v2.diff

This really needs more context... ;)  Don't forget the -U8.

> mInitiatorType

Just have a pure virtual GetInitiatorType() method that subclasses implement, please.

If the spec calls for SVGImageElement to have an initiator type of "svg", I claim that's a spec issue that you should raise.  A goal of the SVGWG is to make <svg:image> as much like <html:img> as they can.

The rest looks good.
Comment 55 User image Adrian Lungu 2013-09-10 15:59:27 PDT
(In reply to Boris Zbarsky [:bz] from comment #54) 
> If the spec calls for SVGImageElement to have an initiator type of "svg", I
> claim that's a spec issue that you should raise.  A goal of the SVGWG is to
> make <svg:image> as much like <html:img> as they can.
Again, there are some differences here, between the "W3C Candidate Recommendation"[1] and the "Editor's draft"[2]. 
For the first one, it is specified that the initiator type must have one of the next values: "css", "embed", "img", "link", "object", "script", "subdocument", "svg", "xmlhttprequest", "other". Also they mention that the "svg" initiator type applies if "the initiator is the <svg> element and all resources downloaded as children of the <svg> element."
In the "Editor's draft" it says that the initiatory type gets the "localName" of the element, if the initiator is an element. In this case, the <svg:image> would have "image" as initiator type, right?
So, should I make the changes to fallow the "Editor's draft" spec?

[1] http://www.w3.org/TR/resource-timing/#dom-performanceresourcetiming-initiatortype
[2] http://www.w3c-test.org/webperf/specs/ResourceTiming/#dom-performanceresourcetiming-initiatortype
Comment 56 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-10 17:52:15 PDT
I think you should follow the editor's draft, yes.
Comment 57 User image Adrian Lungu 2013-09-11 14:59:03 PDT
Created attachment 803309 [details] [diff] [review]
Build and add the performance entry (v2)

I uploaded the full patch (and not an interdiff) since there are a lot of changes and I think it would be more clear this way.
Since the resource timing requires gathering all the timings before the nsPerformanceTiming and nsDOMNavigationTiming objects are created, I moved all the code required to compute the redirect timings at the channel level (nsHttpChannel).
Now, the resource timing and the navigation timing use the same object (nsPerformanceTiming) to gather the time stamps from the nsITimedChannel.  Because the navigation timing returns a DOMTimeMilliSec and resource timing returns a DOMHighResTimeStamp, I had to make 2 methods in the nsPerformanceTiming for each metric. By the way, shouldn't the two performance metrics use the same precision or is it required a more accurate precision for the resources?
Comment 58 User image Adrian Lungu 2013-09-12 11:00:19 PDT
Created attachment 803846 [details] [diff] [review]
part3_v1_to_v3.diff

I've also added the entryType, which, according to the "Editor's Draft"[1], should be set to the constant value "resource".

[1] http://www.w3c-test.org/webperf/specs/ResourceTiming/#performanceresourcetiming
Comment 59 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-12 11:07:34 PDT
Comment on attachment 803846 [details] [diff] [review]
part3_v1_to_v3.diff

This part looks fine.  Still sorting through the other giant patch.  :(
Comment 60 User image Adrian Lungu 2013-09-12 11:25:24 PDT
(In reply to Boris Zbarsky [:bz] from comment #59)
> This part looks fine.  Still sorting through the other giant patch.  :(
Sorry for that. I could post an inter-diff, but there were so many changes compared to the previous version and I think that an inter-diff would just make things even worse.
Comment 61 User image Adrian Lungu 2013-09-12 11:30:02 PDT
Created attachment 803872 [details] [diff] [review]
Initiator type added (v3)

Although the inter-diff (between v1 and v3) is already here, I'm uploading the full patch as well (easier history tracking for later).
Comment 62 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-12 11:33:30 PDT
Yeah, I don't think an interdiff would help much.  ;)
Comment 63 User image Adrian Lungu 2013-09-12 17:19:16 PDT
Created attachment 804116 [details] [diff] [review]
Entries management (v1)

"onresourcetimingbufferfull" callback is missing for now. Should we file a new bug for this? Or do you think that this would be cleared soon enough?
Comment 64 User image Adrian Lungu 2013-09-12 17:55:52 PDT
Created attachment 804137 [details] [diff] [review]
Resource Timing Interface (v2)

I've added the missing webidl files + updated the spec links to point to the Editor's draft.
Comment 65 User image Adrian Lungu 2013-09-17 19:16:07 PDT
I have a couple of questions related to "setResourceTimingBufferSize" and "getEntries" methods.
The specs say that "If the maxSize parameter is less than the number of elements currently stored in the buffer, no elements in the buffer are to be removed. The maxSize parameter will apply only after the clearResourceTimings method is called."
What means "apply" in this context? Does it mean that the extra resources will not be removed but the number of entries returned by getEntries() will be adjusted to the new "maxSize"? Or does it mean that the method will continue to return the old number of "maxSize" entries and the new "maxSize" will apply only after "clearResourceTimings" method is called?

A brief example:
performance.setResourceTimingBufferSize(2)
performance.clearResourceTimings() // we want to make sure that the maxSize = 2.
// Add 2 resources (via xhr)
// The callback "onresourcetimingbufferfull" will be called, but we will ignore it
// performance.getEntries() will return the 2 entries
// Add 1 more resource
// Performance.getEntries() will return 2 entries - maxSize (what entries? The first 2 or the last (most recent) 2?) or 3 entries - all the entries from the buffer? 
performance.setResourceTimingBufferSize(1)
// performance.getEntries() will return the 1 (the new maxSize), 2 (the old maxSize) or 3 (all of them) entries? (since clearResourceTimings was not called yet)
Comment 66 User image Adrian Lungu 2013-09-18 14:39:40 PDT
Comment on attachment 804116 [details] [diff] [review]
Entries management (v1)

Review of attachment 804116 [details] [diff] [review]:
-----------------------------------------------------------------

I cancel the feedback since there are a couple of questions related to this patch that should be cleared first.
Comment 67 User image Honza Bambas (:mayhemer) 2013-09-19 06:52:56 PDT
Comment on attachment 803309 [details] [diff] [review]
Build and add the performance entry (v2)

Review of attachment 803309 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for taking this that long.  Busy.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ -1733,5 @@
>  
> -  // transfer timed channel enabled status
> -  nsCOMPtr<nsITimedChannel> timed(do_QueryInterface(newChannel));
> -  if (timed)
> -    timed->SetTimingEnabled(mTimingEnabled);

Why removed?  (I haven't read the patch that carefully, maybe it's clear)

Note: HttpBaseChannel is a base class for HttpChildChannel that runs in child processes when doing IPC.  And it should do timing as well...  These lines ensure that.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1792,5 @@
>           * the failed redirect). */
>          PopRedirectAsyncFunc(
>              &nsHttpChannel::ContinueAsyncRedirectChannelToURI);
>      }
> +    NotifyRedirect(mURI, upgradedURI);

Probably not the right place, see comments bellow.

@@ +4993,5 @@
> +    mLoadedURI = aURI;
> +}
> +
> +void
> +nsHttpChannel::NotifyRedirect(nsIURI* aOldURI, nsIURI* aNewURI)

This has to record the time we start the redirect load?  Or what exactly?

The (only) place you call this method is definitely not correct, but to tell you where exactly to put it depends on answers.

@@ +5003,5 @@
> +    mRedirectEndTimeStamp = mFetchStartTimeStamp;
> +
> +    // At the unload event time we don't really know the loading uri.
> +    // Need it for later check for unload timing access.
> +    mLoadedURI = aNewURI;

What is the exact purpose of mLoadedURI?  It seems like you are creating a redundant member in this heavily used and already large enough class.

@@ +5028,5 @@
> +                    break;
> +                }
> +            }
> +            // All we need to know is in mRedirectCheck now. Clear history.
> +            mRedirects.Clear();

I have not checked your same-origin check deeply, but I hope you can find a solution w/o an array of all URIs and this loop (btw, it seems like it is wrong anyway).  I think you can build your check algorithm just by keeping the info that target URI is same-origin as a flag and forward it to the following redirected channels.

Note that the old channel (that keeps your mRedirects data) goes away and the replacement (a.k.a new) channel then lives and continues the logic.  We only give the replacement channel mRedirectLimit-1 info, so that we prevent redirect loops.  You can give it your mSameOriginRedirect flag.  Also I am not sure you are checking the Timing-Allow-Origin header.

Another useful info for you can be that mOriginalURI is the URI the replacement channel is redirected from.

@@ +5147,5 @@
>                 "Unexpected request");
>      MOZ_ASSERT(!(mTransactionPump && mCachePump) || mCachedContentIsPartial,
>                 "If we have both pumps, the cache content must be partial");
>  
> +    NotifyFetchStart(mURI);

Just a note that this is the time we have received the HTTP response head completely.  When it is large, e.g. because of large cookies, then this is not exactly the time the response has started.

Depends on the spec if it wants to know the time we are getting data or metadata of the response.  But I think metric for data is OK.

@@ +5300,5 @@
>  
> +        // Register entry to the Performance resource timing
> +        nsPerformance* documentPerformance = GetPerformance();
> +        if (mTimingEnabled && !mNavigationEnabled && documentPerformance) {
> +            documentPerformance->AddEntry(this, this);

get documentPerformance only when mTimingEnabled && !mNavigationEnabled

really, _!_mNavigationEnabled ?
Comment 68 User image Adrian Lungu 2013-09-19 11:54:09 PDT
(In reply to Honza Bambas (:mayhemer) from comment #67)
I tried to answer to your questions and, also, I asked a couple of question in the "review" of the patch.
Thanks!
Comment 69 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-20 20:30:39 PDT
I'm sorry this is taking so long.  I should be able to deal with this on Monday.
Comment 70 User image Adrian Lungu 2013-09-23 10:46:19 PDT
Created attachment 808687 [details] [diff] [review]
part1_v2_to_v3.diff

I found some memory leaks caused by the new interfaces. These changes seems to fix them. Could you take a look over, please?
Comment 71 User image Honza Bambas (:mayhemer) 2013-09-24 11:57:39 PDT
(In reply to Adrian Lungu from comment #68)
> (In reply to Honza Bambas (:mayhemer) from comment #67)
> I tried to answer to your questions and, also, I asked a couple of question
> in the "review" of the patch.
> Thanks!

Where can I read it?
Comment 72 User image Adrian Lungu 2013-09-24 12:39:40 PDT
(In reply to Honza Bambas (:mayhemer) from comment #71)
> (In reply to Adrian Lungu from comment #68)
> > (In reply to Honza Bambas (:mayhemer) from comment #67)
> > I tried to answer to your questions and, also, I asked a couple of question
> > in the "review" of the patch.
> > Thanks!
> 
> Where can I read it?

It should be in the patch's review. I hope that the next link works:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=822480&attachment=803309
Comment 73 User image Honza Bambas (:mayhemer) 2013-09-24 13:14:44 PDT
(In reply to Adrian Lungu from comment #72)
> It should be in the patch's review. I hope that the next link works:
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=822480&attachment=803309

You need to publish the comments :)  It will be in splinter and in bugzilla as a new comment.
Comment 74 User image Adrian Lungu 2013-09-24 13:22:35 PDT
Comment on attachment 803309 [details] [diff] [review]
Build and add the performance entry (v2)

Review of attachment 803309 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ -1733,5 @@
>  
> -  // transfer timed channel enabled status
> -  nsCOMPtr<nsITimedChannel> timed(do_QueryInterface(newChannel));
> -  if (timed)
> -    timed->SetTimingEnabled(mTimingEnabled);

I moved this in nsHttpChannel.
There is something that I don't understand. You say that HttpChannelChild should gather timings as well, but neither HttpChannelChild or HttpBaseChannel (its base class) extends "nsITimedChannel". The only classes that extends nsITimedChannel (which is a required to gather these timings) are imgRequestProxy and nsHttpChannel.
Maybe we should consider some changes for HttpChannelChild (to extend nsItimedChannel and gather timings), but, right now, HttpChannelChild is not doing this.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4993,5 @@
> +    mLoadedURI = aURI;
> +}
> +
> +void
> +nsHttpChannel::NotifyRedirect(nsIURI* aOldURI, nsIURI* aNewURI)

Yes, we want to record the start time of the redirect load.

@@ +5028,5 @@
> +                    break;
> +                }
> +            }
> +            // All we need to know is in mRedirectCheck now. Clear history.
> +            mRedirects.Clear();

All the code that is used to determine the redirect timings was just moved from the "nsDOMNavigationTiming" (so we can use it to track redirects for both the main page and the resources). Now, I see that this won't work because the channel is being replaced at the redirect time. I will change this, using your suggestions.

@@ +5147,5 @@
>                 "Unexpected request");
>      MOZ_ASSERT(!(mTransactionPump && mCachePump) || mCachedContentIsPartial,
>                 "If we have both pumps, the cache content must be partial");
>  
> +    NotifyFetchStart(mURI);

According to the spec: "If there are no HTTP redirects or equivalent, this attribute must return the time immediately before the user agent starts to fetch the resource.
If there are HTTP redirects or equivalent, this attribute must return the time immediately before the user agent starts to fetch the final resource in the redirection."
I thought that this method was being called before sending the request to the server. This notify call should be moved to an earlier phase. "::BeginConnect"? I see that we are already recording a stamp here, used for telemetry: "asyncOpen"

@@ +5300,5 @@
>  
> +        // Register entry to the Performance resource timing
> +        nsPerformance* documentPerformance = GetPerformance();
> +        if (mTimingEnabled && !mNavigationEnabled && documentPerformance) {
> +            documentPerformance->AddEntry(this, this);

Yes, _!_mNavigationEnabled.
I tried to differentiate the main page from all the other resources. Since, we provide navigation information only for the main page, I named the flag that identifies the main page "mNavigationEnabled". There is a comment in the "nsITimedChannel" file that mention this.
Comment 75 User image Adrian Lungu 2013-09-24 13:23:49 PDT
(In reply to Honza Bambas (:mayhemer) from comment #73)
> (In reply to Adrian Lungu from comment #72)
> > It should be in the patch's review. I hope that the next link works:
> > https://bugzilla.mozilla.org/page.cgi?id=splinter.
> > html&bug=822480&attachment=803309
> 
> You need to publish the comments :)  It will be in splinter and in bugzilla
> as a new comment.

Ohh.. I didn't notice that publish button. My bad! Thank you for letting me know about that!
Comment 76 User image Adrian Lungu 2013-09-24 21:53:55 PDT
Created attachment 809650 [details] [diff] [review]
Build and add the performance entry (v3)

Unfortunately, the inter-diff didn't work because of some changes made in the previous patch (see "808687: part1_v2_to_v3.diff").
Most of the changes are related to the redirects and are located int: nsHttpChannel, nsPerformance and PerformanceResourceTiming.
While testing, I found more issues related to the redirects. For instance, the cross-domain check algorithm is different for the natigation timing and resource timing: for the navigation timing, the original uri must have the same domain as the redirected uri while for the resource timing, the referrer uri must have the same domain as the redirected uri. I tried to address all the issues I found.
For now, the cross-domain check algorithm (for resource timing) returns false for different domains. I will write another patch to address the "Timing-Allow-Origin Response Header" part.
Comment 77 User image Honza Bambas (:mayhemer) 2013-09-26 10:25:09 PDT
(In reply to Adrian Lungu from comment #74)
> Comment on attachment 803309 [details] [diff] [review]
> Build and add the performance entry (v2)
> 
> Review of attachment 803309 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ -1733,5 @@
> >  
> > -  // transfer timed channel enabled status
> > -  nsCOMPtr<nsITimedChannel> timed(do_QueryInterface(newChannel));
> > -  if (timed)
> > -    timed->SetTimingEnabled(mTimingEnabled);
> 
> I moved this in nsHttpChannel.
> There is something that I don't understand. You say that HttpChannelChild
> should gather timings as well, but neither HttpChannelChild or
> HttpBaseChannel (its base class) extends "nsITimedChannel". The only classes
> that extends nsITimedChannel (which is a required to gather these timings)
> are imgRequestProxy and nsHttpChannel.
> Maybe we should consider some changes for HttpChannelChild (to extend
> nsItimedChannel and gather timings), but, right now, HttpChannelChild is not
> doing this.

I'll check on this.  Maybe we need some story for e10s too.  The timing API is exposed to content, right?  Then it should work on child processes too.

A followup bug is sufficient, but before you file it, I'll check what the state is.

> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +4993,5 @@
> > +    mLoadedURI = aURI;
> > +}
> > +
> > +void
> > +nsHttpChannel::NotifyRedirect(nsIURI* aOldURI, nsIURI* aNewURI)
> 
> Yes, we want to record the start time of the redirect load.
> 
> @@ +5028,5 @@
> > +                    break;
> > +                }
> > +            }
> > +            // All we need to know is in mRedirectCheck now. Clear history.
> > +            mRedirects.Clear();
> 
> All the code that is used to determine the redirect timings was just moved
> from the "nsDOMNavigationTiming" (so we can use it to track redirects for
> both the main page and the resources). Now, I see that this won't work
> because the channel is being replaced at the redirect time. I will change
> this, using your suggestions.
> 
> @@ +5147,5 @@
> >                 "Unexpected request");
> >      MOZ_ASSERT(!(mTransactionPump && mCachePump) || mCachedContentIsPartial,
> >                 "If we have both pumps, the cache content must be partial");
> >  
> > +    NotifyFetchStart(mURI);
> 
> According to the spec: "If there are no HTTP redirects or equivalent, this
> attribute must return the time immediately before the user agent starts to
> fetch the resource.
> If there are HTTP redirects or equivalent, this attribute must return the
> time immediately before the user agent starts to fetch the final resource in
> the redirection."
> I thought that this method was being called before sending the request to
> the server. This notify call should be moved to an earlier phase.
> "::BeginConnect"? I see that we are already recording a stamp here, used for
> telemetry: "asyncOpen"

I think fetch start means when we start delivering the data.  But depends, Boris?

> 
> @@ +5300,5 @@
> >  
> > +        // Register entry to the Performance resource timing
> > +        nsPerformance* documentPerformance = GetPerformance();
> > +        if (mTimingEnabled && !mNavigationEnabled && documentPerformance) {
> > +            documentPerformance->AddEntry(this, this);
> 
> Yes, _!_mNavigationEnabled.
> I tried to differentiate the main page from all the other resources. Since,
> we provide navigation information only for the main page, I named the flag
> that identifies the main page "mNavigationEnabled". There is a comment in
> the "nsITimedChannel" file that mention this.

This doesn't explain well what it means.  What does this flag do?  What happens when it's true / false?  Who sets this flag?
Comment 78 User image Adrian Lungu 2013-09-26 10:52:51 PDT
(In reply to Honza Bambas (:mayhemer) from comment #77)
> > Yes, _!_mNavigationEnabled.
> > I tried to differentiate the main page from all the other resources. Since,
> > we provide navigation information only for the main page, I named the flag
> > that identifies the main page "mNavigationEnabled". There is a comment in
> > the "nsITimedChannel" file that mention this.
> 
> This doesn't explain well what it means.  What does this flag do?  What
> happens when it's true / false?  Who sets this flag?

This flag is set at load time, in nsDocShell::DoURILoad() and it is used here, to decide whether or not this should be added as a resource entry.
Yesterday, while writing some tests, I found a bug related to this part (iframes), so there will be some changes here.. I hope that I will have a fix by the end of the day.
Comment 79 User image Honza Bambas (:mayhemer) 2013-09-26 12:01:24 PDT
Comment on attachment 809650 [details] [diff] [review]
Build and add the performance entry (v3)

Review of attachment 809650 [details] [diff] [review]:
-----------------------------------------------------------------

Roughly went through the http changes.  I have more comments and questions.  Please check on the email I've sent you on how http channel works more in detail.

::: netwerk/base/public/nsITimedChannel.idl
@@ +18,5 @@
>    // channelCreationTime will be available even with this attribute set to
>    // false.
>    attribute boolean timingEnabled;
> +  // The navigation should be enabled only for the main page.
> +  [noscript] attribute boolean navigationEnabled;

is the name bound to the spec somehow?  I presume not when this is noscript.  "navigationEnabled" is misleading.  I don't understand what the "navigation" in this context means.  can we give it a different, more specific name?

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ -1733,5 @@
>  
> -  // transfer timed channel enabled status
> -  nsCOMPtr<nsITimedChannel> timed(do_QueryInterface(newChannel));
> -  if (timed)
> -    timed->SetTimingEnabled(mTimingEnabled);

This is bad to be removed from here, definitely.  Leave it here.  I still don't understand why you move it to nsHttpChannel.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1837,5 @@
>      rv = mRedirectChannel->AsyncOpen(mListener, mListenerContext);
>      if (NS_FAILED(rv)) {
>          return rv;
>      }
> +    TransferRedirectTimings();

Why here and not during the setup?

@@ +4126,5 @@
> +    // transfer timed channel enabled status to the new channel
> +    nsCOMPtr<nsITimedChannel> timedChannel(do_QueryInterface(newChannel));
> +    if (timedChannel) {
> +        timedChannel->SetNavigationEnabled(mNavigationEnabled);
> +        timedChannel->SetTimingEnabled(mTimingEnabled);

No, this both has to be done in Base channel.

@@ +4980,5 @@
> +    if (mRedirectStartTimeStamp.IsNull()) {
> +        mRedirectStartTimeStamp = mAsyncOpenTime;
> +    }
> +    // Record the end time of the last redirect
> +    mRedirectEndTimeStamp = mozilla::TimeStamp::Now();

I don't think this is right.

I'll check the spec.

@@ +4988,5 @@
> +nsHttpChannel::TransferRedirectTimings()
> +{
> +    // Transmit the old channel's redirect data, if it's a timed channel
> +    nsHttpChannel* httpRedirectChannel =
> +        static_cast<nsHttpChannel*> (mRedirectChannel.get());

NO!  Don't do this!  The redirect channel can be anything.  How do you think this can only be http channel?  You have interfaces for this, if not, create them.  Isn't nsITimedChannel the one?

::: netwerk/protocol/http/nsHttpChannel.h
@@ +381,5 @@
>  
> +    // Performance
> +    nsString                          mInitiatorType;
> +
> +    int16_t                           mRedirectCount;

I think this is a redundant information.  Do you really need it?  Can you use the redirect limit counter?  Or adjust it to give you the proper information?
Comment 80 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-26 13:07:57 PDT
Comment on attachment 808687 [details] [diff] [review]
part1_v2_to_v3.diff

Looks reasonable.

>+NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(PerformanceResourceTiming,PerformanceEntry)

Space after comma.
Comment 81 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-26 13:23:14 PDT
Comment on attachment 809650 [details] [diff] [review]
Build and add the performance entry (v3)

So the two methods per property thing makes no sense to me so far.  Especially since the conversion between them looks broken (goes through a 32-bit int).

Fundamentally, it seems to me like PerformanceTiming returns times that correspond to (in pseudocode)

  (Timestamp2 - Timestamp1).toIntegerMilliseconds() + initialWallClockTime.

where Timestamp1 is the root document navigation start timestamp, and initialWallClockTime is the corresponding wall-clock time in ms.

On the other hand, PerformanceResourceTiming returns

  (Timestamp2 - Timestamp1).toMilliseconds() + initialWallClockTime

with the same Timestamp1 and initialWallClockTime.

So it seems like your timing object could store Timestamp1 and initialWallclockTime and always return the second thing, as a double.  Consumers who wanted to then truncate to a 64-bit unsigned int could do that.

Am I just missing something?
'
In an ideal world, the API changes for the above would be separated from new functionality that builds on those API changes...
Comment 82 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-26 13:30:53 PDT
> Should we file a new bug for this?

Either way, as long as we don't turn this stuff on without implementing that part.  Separate bug and turned off for now would make sense to me.

> What means "apply" in this context? 

This number is used in the processing model step 21 to decide whether "primary buffer is not full" tests true.  What I _assume_ they mean to happen here is that setting the maxSize to below the current buffer size immediately considers the buffer full but does not cause anything to be discarded from the buffer.  Please send in spec feedback about this being unclear.

>A brief example:
>performance.setResourceTimingBufferSize(2)
>performance.clearResourceTimings()
>// Add 2 resources (via xhr)
>// The callback "onresourcetimingbufferfull" will be called

No.  It'll be called when we try to add the third entry.

>// performance.getEntries() will return the 2 entries
>// Add 1 more resource

This is where onresourcetimingbufferfull happens.

> // Performance.getEntries() will return 2 entries - maxSize (what entries? The
> first 2 or the last (most recent) 2?)

Two entries.  The first two.  See the processing model: if the buffer is full and we try to add an entry and the onresourcetimingbufferfull event doesn't make the buffer no longer full, then the new entry is just not added.

>performance.setResourceTimingBufferSize(1)
>// performance.getEntries() will return the 1 (the new maxSize), 2 (the old
>maxSize)

It'll return the current buffer, which still contains 2 entries.
Comment 83 User image Adrian Lungu 2013-09-26 16:41:51 PDT
Created attachment 810866 [details] [diff] [review]
Subdoc bugfix

This is a fix for a bug related to the iframes, which I found while I was writing tests. It also addresses the "mNavigationEnabled" flag which was pretty confusing.
For this fix, I had to save the parent's performance object, so I could add the subdocs as entries in their parent's entries list.
Comment 84 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-26 21:03:02 PDT
Comment on attachment 810866 [details] [diff] [review]
Subdoc bugfix

>+    GetDocShell()->GetSameTypeParentIgnoreBrowserAndAppBoundaries(

This doesn't match the IsFrame() checks in docshell.  Which behavior do you actually want?

In either case, there is no reason to go through the docshell here.  Use GetScriptableParent or GetRealParent and compare the result to this->GetOuterWindow() to see if it's a real parent?

Other than that and the formatting issues with '{' on the wrong line in nsHttpChannel, looks reasonable.
Comment 85 User image Adrian Lungu 2013-09-27 14:01:50 PDT
Created attachment 811367 [details] [diff] [review]
part2_fixIframe_interdiff

So, I want to differentiate the documents from the other resource (that "isDocument" flag). Documents must be added as resources to their parents entries list (and not to their own list). Also, I want to differentiate the main document (the one with no parent) from the other documents (like iframe) - "isMainDocument" flag, because the main document should not be added as a performance entry at all.

I will fix the "{" before the final review. I saw a couple of more places where these are not set right.
Comment 86 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-27 14:06:58 PDT
>+    GetRealParent(getter_AddRefs(parentWindow));

This still doesn't match what the IsFrame() checks in docshell do.  Again, do you want to be treating mozbrowser or app as toplevel?  I would think you do, but your code is not doing that right now.
Comment 87 User image Adrian Lungu 2013-09-27 14:31:19 PDT
(In reply to Boris Zbarsky [:bz] from comment #86)
> >+    GetRealParent(getter_AddRefs(parentWindow));
> 
> This still doesn't match what the IsFrame() checks in docshell do.  Again,
> do you want to be treating mozbrowser or app as toplevel?  I would think you
> do, but your code is not doing that right now.
Yes, I do.
I took a closer look to GetRealParent() / GetScriptableParent() and I think that the second one would be more appropriate to be used. So, you are saying that IsFrame() should not be used to check whether my document is on the top level or not, right? Actually, I am thinking that if I reach the top level document, "nsHttpChannel::GetPerformance()" should return a nullptr (since there is no parent's performance object) and I can just remove the "IsMainDocument" flag.
Do you think that this would be a better solution?
Comment 88 User image Adrian Lungu 2013-09-27 18:22:53 PDT
Created attachment 811454 [details] [diff] [review]
part0_preparePerformanceTiming_v1

I've extracted the part related to the timings (High res and Low res timings) in a different patch so it will be easier to review and change things.

You were saying that both PerformanceTiming and PerformanceResourceTiming are returning the same thing (but with a different precision) : (Timestamp2 - Timestamp1).toIntegerMilliseconds() (or .toMilliseconds()) + initialWallClockTime.
But it is a little bit different: the PerformanceResourceTiming returns a relative timing to the navigation start (without "+ initialWallClockTime"), while the PerfomanceTiming returns an absolute value (with "+ initialWallClockTime"). I did some tests in Chrome and Firefox Nightly and this would be the behavior.

I will try to explain my idea briefly:
I want to have a method that returns the relative timing at a high resolution ("(Timestamp2 - Timestamp1).toMilliseconds()"). These methods also checks if the timing is allowed and if Timesamp2 is Null the result will be replaced with the fetchStartHighRes value.
The high res methods can be used by the PerformanceResourceTiming without any significant changes (there will be only a check for the cross origin). Also, for each of this high res timings, there is a method that would truncate the high res value to an int, add it to the "initialWallClockTime" and obtain the absolute (low resolution) value requested by the PerformanceTiming.
Comment 89 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-27 18:42:39 PDT
> I think that the second one would be more appropriate to be used.

Yes, I agree.

> So, you are saying that IsFrame() should not be used to check whether my document is on
> the top level or not

No, I'm saying the opposite. IsFrame() is the right thing to use, and matches the behavior of GetScriptableParent().

> Do you think that this would be a better solution?

Yes.

> But it is a little bit different: the PerformanceResourceTiming returns a relative
> timing to the navigation start (without "+ initialWallClockTime")

Where is that specified?  (Please raise another spec issue for this spec not actually defining the zero time, unless I'm just missing it.  Or if you don't plan to raise all these spec issues please let me know ASAP so I can.)

But let's assume that this is the case.  In that case, what you need to store, in a shared implementation, is Timestamp1 and initialTime.  For the case of PerformanceTiming you would set initialTime to an actual wall-clock time taken at the same time as Timestamp1.  For the PerformanceResourceTiming case you would set it to 0.  Then you have a single codepath that uses the initialTime in its computations.

Then the callers can just use this value as-is for PerformanceResourceTiming or via truncation to 64-bit int for PerformanceTiming.
Comment 90 User image Adrian Lungu 2013-09-27 19:12:20 PDT
(In reply to Boris Zbarsky [:bz] from comment #89)
> > But it is a little bit different: the PerformanceResourceTiming returns a relative
> > timing to the navigation start (without "+ initialWallClockTime")
> 
> Where is that specified?  (Please raise another spec issue for this spec not
> actually defining the zero time, unless I'm just missing it.  Or if you
> don't plan to raise all these spec issues please let me know ASAP so I can.)
Yes, you are right. In the spec, the definition of the "Monotonic Clock" is the same for both the PerformanceTiming and PerformanceResourceTiming, although it is implemented different. Would you mind raising this spec issues, please? So far, I've never addressed to the W3C mailing list.
Thanks!
Comment 91 User image Boris Zbarsky [:bz] (still a bit busy) 2013-09-27 19:43:16 PDT
OK.  Posted http://lists.w3.org/Archives/Public/public-web-perf/2013Sep/0101.html
Comment 92 User image Adrian Lungu 2013-09-30 22:52:25 PDT
Created attachment 812449 [details] [diff] [review]
part0_preparePerformanceTiming_v1_to_v2.diff
Comment 93 User image Adrian Lungu 2013-10-01 01:31:18 PDT
Created attachment 812510 [details] [diff] [review]
part0_adjustRedirects_v1.patch

I extracted the part related to the redirects, since it's pretty independent and it will be easier to review / address review this way.
I understand that nsHttpChannel is not the only channel, but, for now, I left the navigation implementation as it was (it's working only for nsHttpChannels). Changing all these (to work on both nsHttpChannel and HttpChannelChild) would require some extra work and time, and I'm afraid that I won't have this time, since there are less than 3 weeks left from my internship. We could focus on implementing (and finishing) the performance timing only for the single process model and we can pref it off until the multi-process compatibility will be fixed. 

> > +    int16_t                           mRedirectCount;
> 
> I think this is a redundant information.  Do you really need it?  Can you
> use the redirect limit counter?  Or adjust it to give you the proper
> information?
I took a look over the mRedirectionLimit, but it looks like the maximum value for this is not a constant, but a variable (it set in the parent when the channel is opened), so I don't think it's quite reliable.

I set NotifyPossibleRedirect() call in ::OnStartRequest (if there is no request, these saved values will be just ignored later) and the TransferRedirectTIming in "AutoRedirectVetoNotifier::ReportRedirectResult" since here we know that the redirect was successful and we still have access to the redirected channel.
The redirect timings are saved only if TransferRedirectTimings is being called.
Comment 94 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 07:15:24 PDT
Adrian, does "part0_preparePerformanceTiming_v1_to_v2.diff" take into account the discussion in comment 89?
Comment 95 User image Adrian Lungu 2013-10-01 10:07:20 PDT
(In reply to Boris Zbarsky [:bz] from comment #94)
> Adrian, does "part0_preparePerformanceTiming_v1_to_v2.diff" take into
> account the discussion in comment 89?

Yes. I set a wall clock (that can be changed for the resource timing) and now both timings use a shared implementation (one will truncate the result to an int64_t and the other one will keep the double value).
Comment 96 User image Adrian Lungu 2013-10-01 17:10:25 PDT
Boris, I have one more question related to the base timestmap. Let's say that I have a document which includes an iframe which includes a picture. The iframe's timestamp0 will be the document navigationStart. But what will be the timestamp0 for the picture (included in the iframe): the navigationStart of the iframe or the navigationStart of the main (top) document?
Thanks!
Comment 97 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 17:51:00 PDT
The navigationStart of the iframe.
Comment 98 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 21:42:29 PDT
Comment on attachment 812449 [details] [diff] [review]
part0_preparePerformanceTiming_v1_to_v2.diff

OK.  So the mWallClock naming is not great, now that I look at it, since it's not necessarily a wall-clock time.  Maybe name it mZeroTime?

This change seems to change the way !mChannel works to no longer return FetchStart() in various cases but instead return 0?

Otherwise, starting to look better!
Comment 99 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-01 21:43:21 PDT
Comment on attachment 812510 [details] [diff] [review]
part0_adjustRedirects_v1.patch

Would like Honza to look this over first.
Comment 100 User image Adrian Lungu 2013-10-02 01:26:48 PDT
(In reply to Boris Zbarsky [:bz] from comment #98)
> Comment on attachment 812449 [details] [diff] [review]
> part0_preparePerformanceTiming_v1_to_v2.diff
> 
> OK.  So the mWallClock naming is not great, now that I look at it, since
> it's not necessarily a wall-clock time.  Maybe name it mZeroTime?
OK.
> 
> This change seems to change the way !mChannel works to no longer return
> FetchStart() in various cases but instead return 0?
Yes, it will return 0 if the mChannel is null, but this should never happen actually. I think that we could put an assert in the nsPerformance to test this, as well. Also, if mChannel is null than the channel can't provide timing (not even a FetchStart()), so all the return values should be "0". Right?
> 
> Otherwise, starting to look better!
Comment 101 User image Adrian Lungu 2013-10-02 01:30:49 PDT
Comment on attachment 812510 [details] [diff] [review]
part0_adjustRedirects_v1.patch

Review of attachment 812510 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4853,5 @@
> +void
> +nsHttpChannel::TransferRedirectTimings() {
> +    // Save the temporary values for the current channel
> +    mRedirectStartTimeStamp = mRedirectStartTemporary;
> +    mRedirectEndTimeStamp = mRedirectStartTemporary;

mRedirectEndTimeStamp = mRedirectEndTemporary;
This is fixed locally and the change will be uploaded with the next version of the patch.
Also, I don't know how good are these namings, "NotifyPossibleRedirect()" and "mRedirectStartTemporary" / "mRedirectEndTemporary", but basically that the idea: we assume that every time will be a redirect (so we store the timings in some local variables) and we save them only if a redirect occurred.
Comment 102 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-02 06:07:32 PDT
> Right?

Sounds plausible, at least.  ;)
Comment 103 User image Adrian Lungu 2013-10-02 13:40:35 PDT
Created attachment 813296 [details] [diff] [review]
part2_addEntries_v4.patch

This patch is more lightweight since a lot of the code was moved to "part0_" patches. It also puts together the fixes for the iframe / subdoc.
Comment 104 User image Boris Zbarsky [:bz] (still a bit busy) 2013-10-02 14:10:24 PDT
I won't get to this since mid-next-week sometime, due to summit stuff....
Comment 105 User image Adrian Lungu 2013-10-02 14:19:40 PDT
(In reply to Boris Zbarsky [:bz] from comment #104)
> I won't get to this since mid-next-week sometime, due to summit stuff....
No problem. I will upload one more patch (to address all those things related to the entries management), but it's perfectly fine to review them next week.
Thanks for the reviews!
Comment 106 User image Adrian Lungu 2013-10-02 17:51:33 PDT
Created attachment 813401 [details] [diff] [review]
part4_entriesManagement_v2.patch
Comment 107 User image Adrian Lungu 2013-10-02 17:53:09 PDT
Created attachment 813402 [details] [diff] [review]
part5_addPreference_v1.patch

For now, resource timing is enabled (so we can test it).
Comment 108 User image Honza Bambas (:mayhemer) 2013-10-08 07:25:43 PDT
(Hoping this is the spec you are working with, please fix it if that is wrong)
Comment 109 User image Honza Bambas (:mayhemer) 2013-10-08 08:05:06 PDT
To make me understand the spec...

In a scenario:
[ channe1 --redir--> channel2 --redir--> channel3 --redir--> channel4 ]

only channel4 (or data it collects) is exposed to content, right?

so:

channel4.redirectStart = channel2.fetchStart
channel4.redirectEnd = channel4.responseEnd
channel4.redirectCount = 3


In a scenario:

[ channel1 --redir--> channel2/redirect-vetoed ]

we expose channel1 only

so:

channel1.redirectStart = 0
channel1.redirectEnd = 0
channel1.redirectCount = 0


In a scenario:

[ channel1 --redir--> channel2 --redir--> channel3/redirect-vetoed ]

we expose channel2

so:

channel2.redirectStart = channel2.fetchStart
channel2.redirectEnd = channel2.responseEnd
channel2.redirectCount = 1


Is that all so?

I believe yes, then I think your code is unnecessarily complex (usage of mRedirect*Temporary at least, but also other drawbacks).  Since redirect channel is threw away on redirect veto, you can store the future information about the redirect in it.  It will either bubble the chain or automatically die with the channel when vetoed.

Based on that I suggest:

in SetupRedirectChannel (@HttpBaseChannel if possible please) set on the target channel:
- redirectCount to this.redirectCount+1
- redirectStart to this.redirectStart

Later, after AsyncOpen is called on the redirect target channel, *identified by LOAD_REPLACE load flag*, it can just record redirectStart same way as fetchStart, of course - if not already non-null.  In the above scenarios, LOAD_REPLACE && redirectStart.isNull() will then be true only for channel2, in all cases.

When recording responseEnd, then LOAD_REPLACE flagged channel can record redirectEnd the same way as responseEnd.


Makes sense?
Comment 110 User image Honza Bambas (:mayhemer) 2013-10-08 08:06:15 PDT
Comment on attachment 812510 [details] [diff] [review]
part0_adjustRedirects_v1.patch

Review of attachment 812510 [details] [diff] [review]:
-----------------------------------------------------------------

In general: for any newly added member and method ADD PROPER COMMENTS please.  To all of them.

::: netwerk/base/public/nsITimedChannel.idl
@@ +38,5 @@
> +  // The redirect attributes timings must be writeble, se we can transfer
> +  // the data from one channel to the redirected channel.
> +  [noscript] attribute TimeStamp redirectStart;
> +  [noscript] attribute TimeStamp redirectEnd;
> +  

white spaces

@@ +39,5 @@
> +  // the data from one channel to the redirected channel.
> +  [noscript] attribute TimeStamp redirectStart;
> +  [noscript] attribute TimeStamp redirectEnd;
> +  
> +  // This flag should be set to false only if a cros-domain redirect occured

cross

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4679,5 @@
>      return NS_OK;
>  }
>  
> +// We are returning non-null value for the redirect timings in all the cases.
> +// These values must be filtered afterwards if there are any cross-origin

filtered by the consumer or by the channel?

@@ +4696,5 @@
> +    return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsHttpChannel::GetRedirectStart(TimeStamp* _retval)

mozilla::TimeStamp ?

@@ +4850,5 @@
> +    mRedirectEndTemporary = mozilla::TimeStamp::Now();
> +}
> +
> +void
> +nsHttpChannel::TransferRedirectTimings() {

{ on a new line

@@ +4853,5 @@
> +void
> +nsHttpChannel::TransferRedirectTimings() {
> +    // Save the temporary values for the current channel
> +    mRedirectStartTimeStamp = mRedirectStartTemporary;
> +    mRedirectEndTimeStamp = mRedirectStartTemporary;

I'd like to avoid using the temp vars at all.  See the previous comment.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +416,5 @@
> +    mozilla::TimeStamp                mFetchStartTimeStamp;
> +    mozilla::TimeStamp                mRedirectStartTimeStamp;
> +    mozilla::TimeStamp                mRedirectEndTimeStamp;
> +    mozilla::TimeStamp                mRedirectStartTemporary;
> +    mozilla::TimeStamp                mRedirectEndTemporary;

These all need comments...
Comment 111 User image Honza Bambas (:mayhemer) 2013-10-08 08:18:56 PDT
Comment on attachment 813296 [details] [diff] [review]
part2_addEntries_v4.patch

Review of attachment 813296 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +9766,5 @@
>  
>      nsCOMPtr<nsITimedChannel> timedChannel(do_QueryInterface(channel));
>      if (timedChannel) {
>          timedChannel->SetTimingEnabled(true);
> +        timedChannel->SetIsDocument(true);

There is a load flag for this, use LOAD_INITIAL_DOCUMENT_URI or LOAD_DOCUMENT_URI check instead, worth to double check with Boris.

::: netwerk/base/public/nsITimedChannel.idl
@@ +40,5 @@
>    [noscript] attribute TimeStamp redirectStart;
>    [noscript] attribute TimeStamp redirectEnd;
> +
> +  // The isDocument flag should be enabled only for documents
> +  [noscript] attribute boolean isDocument;

"enabled" or "true"?  I presume when have used work "enabled", setting this attribute to true affects the behavior of the channel.  This comment needs to be more detailed.

E.g. who sets this attribute and when?  What effect it has?
Comment 112 User image Honza Bambas (:mayhemer) 2013-10-08 08:31:26 PDT
(In reply to Honza Bambas (:mayhemer) from comment #109)
> in SetupRedirectChannel (@HttpBaseChannel if possible please) set on the
> target channel:
> - redirectCount to this.redirectCount+1
> - redirectStart to this.redirectStart

And yes, you need to also carry the are-all-redirects-same-origin flag as well in some way.  

I'm ok with carrying the are-all-redirects-same-origin flag using a separate method.

But you can also just propagate that info by setting redirectCount to 0 on the target channel.  When LOAD_REPLACE is set on a channel, but redirectCount == 0, then it is indication of demand that redirect timing has not to be reported.

Entirely up to you how you gonna do this.
Comment 113 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-10-08 09:43:14 PDT
(TR/ is for trash.)
Comment 114 User image Adrian Lungu 2013-10-08 11:32:51 PDT
(In reply to Honza Bambas (:mayhemer) from comment #109)
> To make me understand the spec...
> 
> In a scenario:
> [ channe1 --redir--> channel2 --redir--> channel3 --redir--> channel4 ]
> 
> only channel4 (or data it collects) is exposed to content, right?
> 
> so:
> 
> channel4.redirectStart = channel2.fetchStart
channel4.redirectStart = channel1.fetchStart
("this attribute MUST return a DOMHighResTimeStamp with a time value equal to the starting time of the fetch that initiates the redirect." - which would be channel1)
Comment 115 User image Adrian Lungu 2013-10-08 16:20:11 PDT
Created attachment 814623 [details] [diff] [review]
part0_adjustRedirects_v1_to_v2.diff
Comment 116 User image Adrian Lungu 2013-10-08 18:08:31 PDT
Created attachment 814664 [details] [diff] [review]
part2_removeIsDocFlag.diff

As Honza suggested, I removed the IsDocument flag and used instead "mLoadFlags & LOAD_DOCUMENT_URI"
Comment 117 User image Honza Bambas (:mayhemer) 2013-10-09 09:47:34 PDT
Adrian, can you please provide also the full patch?  I'd like to check both.  Thanks.
Comment 118 User image Adrian Lungu 2013-10-09 10:35:45 PDT
Created attachment 814981 [details] [diff] [review]
part0_adjustRedirects_v2.patch
Comment 119 User image Adrian Lungu 2013-10-11 17:44:31 PDT
Created attachment 816176 [details] [diff] [review]
part6_testing_v1.patch

Tests for the resource timing API
Comment 120 User image Honza Bambas (:mayhemer) 2013-10-15 05:16:34 PDT
Comment on attachment 814981 [details] [diff] [review]
part0_adjustRedirects_v2.patch

Review of attachment 814981 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not familiar with the new spec.  And actually with the old one we currently implement either.

It would help to have an overview of what nsDOMNavigationTiming and nsPerformance are doing and implementing.  W/o it it's hard for me to figure out what all of this is doing.  I know I should read the spec, but it's quite time consuming.

Adrian, can you please help with providing some reasonable detailed overview?  I've also sent a private mail on this to you.  Thanks.

::: dom/base/nsDOMNavigationTiming.cpp
@@ -95,5 @@
> -  // Need it for later check for unload timing access.
> -  mLoadedURI = aNewURI;
> -
> -  mRedirects.AppendObject(aOldURI);
> -}

Why is this moving away (to nsPerformance)? maybe it's correct, I just need explanation.

::: dom/base/nsPerformance.cpp
@@ +85,5 @@
> +  return TimeStampToDOMHighResOrFetchStart(stamp);
> +}
> +
> +DOMTimeMilliSec
> +nsPerformanceTiming::RedirectStart() {

{ on new line

::: image/src/imgRequestProxy.h
@@ +15,5 @@
>  #include "nsITimedChannel.h"
>  #include "nsCOMPtr.h"
>  #include "nsAutoPtr.h"
>  #include "nsThreadUtils.h"
> +#include "mozilla/TimeStamp.h"

?

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1757,5 @@
> +
> +    // The RedirectEnd timestamp is equal to the previous channel response end.
> +    TimeStamp prevResponseEnd;
> +    oldTimedChannel->GetResponseEnd(&prevResponseEnd);
> +    newTimedChannel->SetRedirectEnd(prevResponseEnd);

hmmm.. redirect end has to be the new channel's fetch end, no?

@@ +1773,5 @@
> +HttpBaseChannel::SameOriginWithOriginalUri(nsIURI *aURI)
> +{
> +    nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
> +    nsresult rv = ssm->CheckSameOriginURI(aURI, mOriginalURI, false);
> +    return (NS_SUCCEEDED(rv)) ? true : false;

style of this file is 2 spaces indent

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +323,5 @@
> +  // A time value equal to the time immediately after receiving the last byte of
> +  // the response of the last redirect.
> +  mozilla::TimeStamp                mRedirectEndTimeStamp;
> +  // A flag that should be false only if a cross-domain redirect occurred
> +  uint32_t                          mAllRedirectsSameOrigin     : 1;

put this to other :1 flags in this file.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4829,5 @@
> +// Redirect tracking
> +//-----------------------------------------------------------------------------
> +
> +NS_IMETHODIMP
> +nsHttpChannel::GetAllRedirectsSameOrigin(bool *aAllRedirectsSameOrigin)

These are method of nsITimedChannel, put them under 

// nsHttpChannel::nsITimedChannel

comment as appropriate and according the file style
Comment 121 User image Honza Bambas (:mayhemer) 2013-10-15 05:50:38 PDT
Can you please also provide a complete patch (all patches folded together) ?   Thanks.
Comment 122 User image Adrian Lungu 2013-10-15 18:04:39 PDT
(In reply to Honza Bambas (:mayhemer) from comment #120)
> 
> ::: dom/base/nsDOMNavigationTiming.cpp
> @@ -95,5 @@
> > -  // Need it for later check for unload timing access.
> > -  mLoadedURI = aNewURI;
> > -
> > -  mRedirects.AppendObject(aOldURI);
> > -}
> 
> Why is this moving away (to nsPerformance)? maybe it's correct, I just need
> explanation.
All the redirect logic was moved from the nsDOMnavigationTiming to the channel level. This way the code can be used by both the NavigationTiming and the ResourceTiming (more details about this in the email).
> ::: image/src/imgRequestProxy.h
> @@ +15,5 @@
> >  #include "nsITimedChannel.h"
> >  #include "nsCOMPtr.h"
> >  #include "nsAutoPtr.h"
> >  #include "nsThreadUtils.h"
> > +#include "mozilla/TimeStamp.h"
> 
> ?
imgRequestProxy implements the nsITimedChannel interface (although I don't understand why since it's not gathering any timings). At build time, I was getting a compile error saying that the TimeStamp is not a known type (the nsITimedChannel is using forward declaration for the TimeStamp).
> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +1757,5 @@
> > +
> > +    // The RedirectEnd timestamp is equal to the previous channel response end.
> > +    TimeStamp prevResponseEnd;
> > +    oldTimedChannel->GetResponseEnd(&prevResponseEnd);
> > +    newTimedChannel->SetRedirectEnd(prevResponseEnd);
> 
> hmmm.. redirect end has to be the new channel's fetch end, no?
The redirect end has to be the last redirected channel's fetch end. We can assume that each redirect is the last one and overwrite it if a new redirect occurs.
Comment 123 User image Adrian Lungu 2013-10-15 18:39:23 PDT
Created attachment 817579 [details] [diff] [review]
resourceTimingFull (v1)

Resource timing - all parts merged.
The patch contains all the existing code for the feature (including testing).
Comment 124 User image Honza Bambas (:mayhemer) 2013-10-16 11:11:50 PDT
Comment on attachment 817579 [details] [diff] [review]
resourceTimingFull (v1)

Review of attachment 817579 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks very very much for the explanatory email!  It's almost worth to publish it as a wiki page - implementation overview ;)  I really mean it..


The patch: 

comments, comments, comments!!!  the more or even more the better.

f+ (but still just a feedback only)

Please address all the comments, if something is not clear/wrong ask immediately.  Double check all is addressed.  Then submit the new whole patch for review (yes, I think it's time to do the full review and get it to the tree).

We also need someone to check on the DOM parts.  I'll try to find the reviewer.

::: content/base/src/nsContentUtils.cpp
@@ +439,5 @@
>    Preferences::AddBoolVarCache(&sIsPerformanceTimingEnabled,
>                                 "dom.enable_performance", true);
>  
> +  Preferences::AddBoolVarCache(&sIsResourceTimingEnabled,
> +                                 "dom.enable_resource_timing", true);

indent

::: content/base/src/nsImageLoadingContent.cpp
@@ +823,1 @@
>                                   getter_AddRefs(req));

Just curious if the image load cannot find the LocalName it self... Can be done in a followup.

::: dom/base/PerformanceEntry.h
@@ +34,5 @@
> +  }
> +
> +  void GetName(nsAString& aName) const
> +  {
> +      aName = mName;

indention

::: dom/base/nsPerformance.cpp
@@ +29,5 @@
>  nsPerformanceTiming::nsPerformanceTiming(nsPerformance* aPerformance,
>                                           nsITimedChannel* aChannel)
>    : mPerformance(aPerformance),
> +    mChannel(aChannel),
> +    mFetchStart(0),

0.0

@@ +30,5 @@
>                                           nsITimedChannel* aChannel)
>    : mPerformance(aPerformance),
> +    mChannel(aChannel),
> +    mFetchStart(0),
> +    mZeroTime(0),

I'd rather see this initialized by the constructor maybe (not by the SetWallClock method).  According the code it's possible.  I want to avoid changing the zero time value after this object had been created and returned some timestamps.  It could introduce hard to find errors in timings potentially.

@@ +49,5 @@
> +    if (!IsValid()) {
> +      return 0;
> +    }
> +    TimeStamp stamp;
> +    mChannel->GetAsyncOpen(&stamp);

null check!

@@ +51,5 @@
> +    }
> +    TimeStamp stamp;
> +    mChannel->GetAsyncOpen(&stamp);
> +    MOZ_ASSERT(!stamp.IsNull(), "The fetch start time stamp should always be"
> +        "valid if the performance timing is enabled");

"... always bevalid ..."

@@ +52,5 @@
> +    TimeStamp stamp;
> +    mChannel->GetAsyncOpen(&stamp);
> +    MOZ_ASSERT(!stamp.IsNull(), "The fetch start time stamp should always be"
> +        "valid if the performance timing is enabled");
> +    mFetchStart = (!stamp.IsNull()) ? TimeStampToDOMHighRes(stamp) : 0;

0.0

@@ +117,5 @@
> +    return 0;
> +  }
> +  mozilla::TimeStamp stamp;
> +  mChannel->GetRedirectStart(&stamp);
> +  return TimeStampToDOMHighResOrFetchStart(stamp);

same-orig check rule here as well?  (probably not, just please check on it, and... :)  add a comment why it is not tested here)

@@ +391,5 @@
>  }
>  
> +void
> +nsPerformance::GetEntries(nsTArray<nsRefPtr<PerformanceEntry> >& retval)
> +{

Add MOZ_ASSERT(NS_IsMainThread()) to all these methods touching mEntries.  It might happen in the future we need to add a lock over mEntries.

@@ +395,5 @@
> +{
> +  retval.Clear();
> +  uint32_t count = mEntries.Length();
> +  for (uint32_t i = 0 ; i < count && i < mPrimaryBufferSize ; i++) {
> +    retval.AppendElement(mEntries[i]);

You can try using AppendElements(mEntries.Elements()).. just an optimization.

@@ +414,5 @@
> +}
> +
> +void
> +nsPerformance::GetEntriesByName(const nsAString& name,
> +                                const mozilla::dom::Optional< nsAString >& entryType,

<nsAString

@@ +429,5 @@
> +  }
> +}
> +
> +void
> +nsPerformance::ClearResourceTimings() {

{ on new line

@@ +434,5 @@
> +  mPrimaryBufferSize = mBufferSizeSet;
> +  uint32_t count = mEntries.Length();
> +  for (uint32_t i = 0 ; i < count ; i++) {
> +    mEntries.RemoveElementAt(0);
> +  }

mEntries.Clear() ;)

@@ +441,5 @@
> +void
> +nsPerformance::SetResourceTimingBufferSize(uint64_t maxSize) {
> +  mBufferSizeSet = maxSize;
> +  if (mBufferSizeSet < mEntries.Length()) {
> +    ; // call onresourcetimingbufferfull

remove ;

@@ +459,5 @@
> +    nsAutoCString name;
> +    nsAutoString initiatorType;
> +
> +    resourcesTimedChannel->GetInitiatorType(initiatorType);
> +    resourcesChannel->GetName(name);

You need GetOriginalURI()->GetSpec() here:

"The name attribute must return the resolved URL of the requested resource. This attribute must not change even if the fetch redirected to a different URL."

@@ +472,5 @@
> +
> +    performanceEntry->SetName(entryName);
> +    performanceEntry->SetEntryType(NS_LITERAL_STRING("resource"));
> +    performanceEntry->SetInitiatorType(!initiatorType.IsEmpty() ?
> +        initiatorType : NS_LITERAL_STRING("other"));

This code needs comment about the object chaining (not visible) and better put of blnak lines.  There must be a blank over new object creation.

And also why you set wall clock to 0.

@@ +499,5 @@
> +    const PerformanceEntry* aElem1,
> +    const PerformanceEntry* aElem2) const
> +{
> +  NS_ABORT_IF_FALSE(aElem1 && aElem2,
> +        "Trying to compare null performance entries");

"Trying to... has to be indented only by 4 spaces

::: dom/base/nsPerformance.h
@@ +34,2 @@
>    nsPerformanceTiming(nsPerformance* aPerformance,
>                        nsITimedChannel* aChannel);

pass the wallclock zero time here (give the arg good name) and also pass the channel to do a redir check.

comments what the args in detail are and that channel can be null and why

@@ +54,5 @@
> +  {
> +    mozilla::TimeDuration duration =
> +        aStamp - GetDOMTiming()->GetNavigationStartTimeStamp();
> +    return duration.ToMilliseconds() + mZeroTime;
> +  }

Comments on what is what are needed, what values can have (mainly mZeroTime)

@@ +60,4 @@
>    virtual JSObject* WrapObject(JSContext *cx,
>                                 JS::Handle<JSObject*> scope) MOZ_OVERRIDE;
>  
> +  void SetWallClock(DOMHighResTimeStamp aWallClock)

[ Moot by the suggested ctor changes, but left for reference ]

Since it's confusing, rename this to SetZeroTimeWallClockMs.  It emphasize what it actually is - time we do relative counts to in a wallclock format and [ms] resolution.

A comment what it means when set to 0 (that it is not a bug, but it has a purpose).

Depends on which of the declared types this method is using, add a comment into C++ that it's in [ms] resolution. (best to both declarations).

::: dom/tests/mochitest/general/test_resource_timing.html
@@ +1,4 @@
> +<!--
> +  Any copyright is dedicated to the Public Domain.
> +  http://creativecommons.org/publicdomain/zero/1.0/
> +-->

add a explanatory list of steps this test is doing.  it's important to have an overview otherwise it's hard to understand what the test is doing.

I'll check the test during the review

check whitespaces (ws)

::: dom/webidl/PerformanceEntry.webidl
@@ +9,5 @@
> + * Copyright © 2012 W3C® (MIT, ERCIM, Keio), All Rights Reserved. W3C
> + * liability, trademark and document use rules apply.
> + */
> +
> +interface PerformanceEntry {

{ on new line

::: dom/webidl/PerformanceResourceTiming.webidl
@@ +12,5 @@
> +
> +interface PerformanceResourceTiming : PerformanceEntry {
> +  readonly attribute DOMString initiatorType; 
> +  // "css", "embed", "img", "link", "object", "script", "subdocument", "svg", 
> +  // "xmlhttprequest", "other"

comments over a method/attr

::: image/src/imgRequestProxy.h
@@ +15,5 @@
>  #include "nsITimedChannel.h"
>  #include "nsCOMPtr.h"
>  #include "nsAutoPtr.h"
>  #include "nsThreadUtils.h"
> +#include "mozilla/TimeStamp.h"

still a bit strange to me.  but if it's easier then to find out why this doesn't compile w/o it, then leave it.

::: layout/style/ImageLoader.cpp
@@ +258,5 @@
>  
>    nsRefPtr<imgRequestProxy> request;
>    nsContentUtils::LoadImage(aURI, mDocument, aOriginPrincipal, aReferrer,
>                              nullptr, nsIRequest::LOAD_NORMAL,
> +                            NS_LITERAL_STRING("css"),

are you sure?

::: modules/libpref/src/init/all.js
@@ +109,5 @@
>  // Whether nonzero values can be returned from performance.timing.*
>  pref("dom.enable_performance", true);
>  
> +// Whether resource timing will be gathered and returned by performance.GetEntries*
> +pref("dom.enable_resource_timing", true);

are we sure this should be enabled by default?  maybe yes, just checking ;)

::: netwerk/base/public/nsITimedChannel.idl
@@ +17,5 @@
>    // Set this attribute to true to enable collection of timing data.
>    // channelCreationTime will be available even with this attribute set to
>    // false.
>    attribute boolean timingEnabled;
> +  

ws

@@ +65,5 @@
>    readonly attribute PRTime cacheReadEndTime;
> +  readonly attribute PRTime redirectStartTime;
> +  readonly attribute PRTime redirectEndTime;
> +  
> +  // The following are used for resource timing tracking.

remove or place correctly this comment

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +316,5 @@
>  
>    nsRefPtr<nsHttpHandler>           mHttpHandler;  // keep gHttpHandler alive
> +
> +  // Performance tracking
> +  // The initiator type (for this resouce)

Explain what "The initiator type" is.

@@ +318,5 @@
> +
> +  // Performance tracking
> +  // The initiator type (for this resouce)
> +  nsString                          mInitiatorType;
> +  // Number of redirects that occurred.

has occurred

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5108,5 @@
> +        // Register entry to the Performance resource timing
> +        nsPerformance* documentPerformance = GetPerformance();
> +        if (mTimingEnabled && documentPerformance) {
> +            documentPerformance->AddEntry(this, this);
> +        }

Move mTimingEnabled check into GetPerformance() - as the first one.  Purpose is to bypass the whole QI/GI dance when we actually don't need it (99% of cases).

IMPORTANT: add a comment that Performance->AddEntry si not thread safe and can only be called on the main thread.

IMPORTANT: the place you call AddEntry in the channel is wrong.  Move it right before you call mListener->OnStopRequest.  When we do authentication loops, the channel would be added more then ones.

@@ +6118,5 @@
>      return rv;
>  }
>  
> +NS_IMETHODIMP
> +nsHttpChannel::GetInitiatorType(nsAString & aInitiatorType)

the interface method should be under a comment with reference to the interface (see the rest of this file how it's done for other methods).
Comment 125 User image Adrian Lungu 2013-10-16 23:26:18 PDT
Created attachment 818258 [details] [diff] [review]
resourceTimingFull_v2.patch

This is the second version of the resource timing (full patch).
I hope that I've addressed all the feedback from the previous version. Also, here are some responses to your comments (where I thought that I should not change the code; at least until we discuss them).

> @@ +395,5 @@
> > +{
> > +  retval.Clear();
> > +  uint32_t count = mEntries.Length();
> > +  for (uint32_t i = 0 ; i < count && i < mPrimaryBufferSize ; i++) {
> > +    retval.AppendElement(mEntries[i]);
> 
> You can try using AppendElements(mEntries.Elements()).. just an optimization.
The thing is that I don't want to append all the elements. "mPrimaryBufferSize" could be smaller than the mEntries.Length().

> @@ +258,5 @@
> >  
> >    nsRefPtr<imgRequestProxy> request;
> >    nsContentUtils::LoadImage(aURI, mDocument, aOriginPrincipal, aReferrer,
> >                              nullptr, nsIRequest::LOAD_NORMAL,
> > +                            NS_LITERAL_STRING("css"),
> 
> are you sure?
Yes. "A class that handles style system image loads (other image loads are handled by the nodes in the content tree)" - comment from the top of the ImageLoader.cpp
Comment 126 User image Adrian Lungu 2013-10-16 23:28:36 PDT
Created attachment 818259 [details] [diff] [review]
resourceTimingFull_v1_to_v2.diff

I'm also attaching the inter-diff between the v1 and v2 of the full patches. Maybe it will help in the review process.
Comment 127 User image Honza Bambas (:mayhemer) 2013-10-17 03:01:11 PDT
(In reply to Adrian Lungu from comment #125)
> Created attachment 818258 [details] [diff] [review]
> resourceTimingFull_v2.patch

> The thing is that I don't want to append all the elements.
> "mPrimaryBufferSize" could be smaller than the mEntries.Length().
> 
retval.AppendElements(mEntries.Elements(), std::max(count, mPrimaryBufferSize));
mxr is your friend here ;)


https://tbpl.mozilla.org/?tree=Try&rev=6c727119e67b
Comment 128 User image Honza Bambas (:mayhemer) 2013-10-17 05:15:45 PDT
Comment on attachment 818259 [details] [diff] [review]
resourceTimingFull_v1_to_v2.diff

Review of attachment 818259 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsPerformance.cpp
@@ +59,4 @@
>        return 0;
>      }
>      TimeStamp stamp;
>      mChannel->GetAsyncOpen(&stamp);

where is the null check here?

@@ +323,5 @@
>        "valid resource");
>    if (!mChannel) {
>      return false;
>    }
>    return true;

or just:

return !!mChannel;

@@ +423,5 @@
>    return dom::PerformanceBinding::Wrap(cx, scope, this);
>  }
>  
>  void
> +nsPerformance::GetEntries(nsTArray<nsRefPtr<PerformanceEntry>>& retval)

> >  !!!

@@ +430,4 @@
>    retval.Clear();
>    uint32_t count = mEntries.Length();
>    for (uint32_t i = 0 ; i < count && i < mPrimaryBufferSize ; i++) {
>      retval.AppendElement(mEntries[i]);

as noted, but since this is kinda sensitive I leave it up to you to change the code as proposed and then test it well or just let this suboptimal code in.

@@ +461,5 @@
>          (!entryType.WasPassed() ||
>           mEntries[i]->GetEntryType().Equals(entryType.Value()))) {
>        retval.AppendElement(mEntries[i]);
>      }
>    }

some blank lines to emphasize the blocks that logically belong together would be nice.

@@ +472,2 @@
>    mPrimaryBufferSize = mBufferSizeSet;
> +  mEntries.Clear();

MOZ_ASSERT(main-thread)

@@ +483,1 @@
>    }

MOZ_ASSERT(main-thread)

@@ +512,5 @@
>      NS_ConvertUTF8toUTF16 entryName(name);
>  
> +    // The nsPerformanceTiming object will gather the timings from the
> +    // nsIHttpChannel argument. The nsIHttpChannel is required to check
> +    // cross-origin redirects.

The nsIHttpChannel is required to - sounds like the channel has to do it.  Please rephrase to make more clear why you are passing the channel to the ctor

@@ +514,5 @@
> +    // The nsPerformanceTiming object will gather the timings from the
> +    // nsIHttpChannel argument. The nsIHttpChannel is required to check
> +    // cross-origin redirects.
> +    // The last argument is the "zero time" (offset). Since we don't want
> +    // any offset for the resource timing, this will be set to "0".

Since we don't want any offset - we don't want it because we want the timing be relative, right?

@@ +527,5 @@
>  
>      performanceEntry->SetName(entryName);
>      performanceEntry->SetEntryType(NS_LITERAL_STRING("resource"));
> +    // If the initiator type had no valid value, then set it to the default
> +    // ("other) value.

"other"

@@ +532,2 @@
>      performanceEntry->SetInitiatorType(!initiatorType.IsEmpty() ?
>          initiatorType : NS_LITERAL_STRING("other"));

!initiatorType.IsEmpty()
  ? initiatorType
  : NS_LITERAL_STRING("other")

::: dom/base/nsPerformance.h
@@ +33,5 @@
>  
> +/**
> + * @param   aPerformance
> + *          The parent object ("performance.timing") for the navigation timing
> + *          (can't be null).

timing belonging to what object?

@@ +41,5 @@
> + * @param   aHttpChannel
> + *          An nsIHttpChannel used by the resource timing cross-domain check
> + *          algorithm. This argument is null for the navigation timing
> + *          (navigation timing uses another algorithm for the cross-domain
> + *          redirects).

which channel this is?

@@ +45,5 @@
> + *          redirects).
> + * @param   aZeroTime
> + *          The offset that will be added to the duration of each event. This
> + *          argument should be equal to performance.navigationStart for
> + *          navigation timing and "0" for the resource timing.

to the "duration" ? shouldn't it be to the timestamp of each event?  result of calls to timing API is always a timestamp (absolute or relative) not actually a duration

@@ +74,5 @@
>    inline DOMHighResTimeStamp TimeStampToDOMHighResOrFetchStart(TimeStamp aStamp)
>    {
>      return (!aStamp.IsNull()) ?
>          TimeStampToDOMHighRes(aStamp) :
>          FetchStartHighRes();

forgot to note last time, better syntax is:

condition
  ? return-on-true
  : return-on-false;

@@ +80,5 @@
>  
> +  /**
> +   * @param   aStamp
> +   *          The TimeStamp recorded for a specific event. This TimeStamp can't
> +   *          be null.

maybe add a MOZ_ASSERT, but I think the duration counting asserts already.. up to you

@@ +81,5 @@
> +  /**
> +   * @param   aStamp
> +   *          The TimeStamp recorded for a specific event. This TimeStamp can't
> +   *          be null.
> +   * @return  the duration of an event with a given TimeStamp, relative to the

it doesn't return duration, it always return a timestamp, but ones a wallclock time, and ones a relative time to nav start.

otherwise a good comment.

@@ +94,5 @@
>    inline DOMHighResTimeStamp TimeStampToDOMHighRes(TimeStamp aStamp) const
>    {
>      mozilla::TimeDuration duration =
>          aStamp - GetDOMTiming()->GetNavigationStartTimeStamp();
>      return duration.ToMilliseconds() + mZeroTime;

I'd like to have more comments also about the algorithm here:

1. first we count the duration from navigation start to have a relative timestamp
2. we return the timestamp as milliseconds, shifted by time of the navigation start (mZeroTime) that can be 0 - result of this method is a timestamp relative to navigation start - or logically equal to NavigationStartTimeStamp - result of this method is a unix timestamp wallclock time

@@ +198,4 @@
>    nsRefPtr<nsPerformance> mPerformance;
>    nsCOMPtr<nsITimedChannel> mChannel;
>    DOMHighResTimeStamp mFetchStart;
> +  // This is an offset that will be added to each timing ([ms] resolution)

mention that this can have only two values:
- logically equal to NavigationStartTimeStamp > results are wallclock
- null > result are relative to nav start

::: dom/tests/mochitest/general/test_resource_timing.html
@@ +10,5 @@
> +  getEntriesByName() and getEntriesByType().
> +  
> +  As a next step, we check that the entries contain the correct information
> +  ("checkEntries()" method).
> +  The test checks that the entries contains all the required members, that the

entries contain + ws

@@ +24,5 @@
> +  resource timing: window.performance.setResourceTimingBufferSize(1) and
> +  window.performance.clearResourceTimings();
> +
> +  The last tests will verify that the xhr resources are also added as entries
> +  to our performance object.

maybe a dumb question: why isn't this part of the first test?

@@ +30,5 @@
> +  Meanwhile, the iframe from the page will get loaded
> +  (resource_timing_iframe.html).
> +  The script from this iframe will check that the iframe itself was not added
> +  as an entry (to itself). Also, it will check that the image included in the
> +  iframe was added as an entry. A last check is a double check: no subdocuments

was added as an entry - of what?

@@ +32,5 @@
> +  The script from this iframe will check that the iframe itself was not added
> +  as an entry (to itself). Also, it will check that the image included in the
> +  iframe was added as an entry. A last check is a double check: no subdocuments
> +  were added as entries for this iframe's performance object. The parent (this
> +  window) will be called at the end of the tests.

will be called - what does that mean?

::: dom/webidl/PerformanceResourceTiming.webidl
@@ +11,5 @@
>   */
>  
> +interface PerformanceResourceTiming : PerformanceEntry 
> +{
> +  // A string with the same value as the localName of that element.

localName of that element - hmm... you mean "name of the element that initiated the load" ?

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +67,1 @@
>    , mRedirectCount(0)

Probably not the best place ;)

../../../../netwerk/protocol/http/HttpBaseChannel.cpp:65:5: error: field 'mHttpHandler' will be initialized after field 'mAllRedirectsSameOrigin' [-Werror,-Wreorder]
  , mHttpHandler(gHttpHandler)
Comment 129 User image Honza Bambas (:mayhemer) 2013-10-17 05:15:58 PDT
Comment on attachment 818258 [details] [diff] [review]
resourceTimingFull_v2.patch

Review of attachment 818258 [details] [diff] [review]:
-----------------------------------------------------------------

r- for red try run

- fix the build and push to try (I think you should have Level 1 commit access, if not then I can push for you your draft patches)
- fix also the comments on the idiff

::: dom/base/nsPerformance.cpp
@@ +476,5 @@
> +void
> +nsPerformance::SetResourceTimingBufferSize(uint64_t maxSize)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  mBufferSizeSet = maxSize;

OK, "if the maxSize parameter is less than the number of elements currently stored in the buffer, no elements in the buffer are to be removed"

but when there are fewer entries, the limit should grow - what if I want the buffer just enlarge and leave what is already recorded? the spec doesn't say that this custom limit has to be taken into affect only after ClearResourceTimings() has been sub-sequentially called.

::: dom/base/nsPerformance.h
@@ +300,5 @@
>    nsRefPtr<nsPerformanceNavigation> mNavigation;
> +  nsTArray<nsRefPtr<PerformanceEntry>> mEntries;
> +  nsRefPtr<nsPerformance> mParentPerformance;
> +  uint64_t mBufferSizeSet;
> +  uint64_t mPrimaryBufferSize;

I haven't spot before, but both these seem not to be initialized!

"the user agent should store at least 150"

::: dom/tests/mochitest/general/resource_timing_iframe.html
@@ +1,1 @@
> +<!DOCTYPE HTML>

comments what this is doing, tabs to spaces

::: dom/tests/mochitest/general/test_resource_timing.html
@@ +1,4 @@
> +<!--
> +  Any copyright is dedicated to the Public Domain.
> +  http://creativecommons.org/publicdomain/zero/1.0/
> +

close the license header correctly.  then open a new one for your comment.


convert all tabs to TWO spaces
indention is in general two spaces, for all new files

@@ +48,5 @@
> +SimpleTest.waitForExplicitFinish();
> +
> +var mainWindowTestsDone = false;
> +var iframeTestsCompleted = 0;
> +var iframeTestsTotal = 3;

better is when the iframe calls some iframeFinished method on the parent test.  this is quite fragile and unnecessarily overcomplicated.

@@ +53,5 @@
> +
> +window.onload = function() {
> +	// Here, we should have 6 entries (1 css, 1 js, 3 png, 1 html) since the image was loaded.
> +	ok(window.performance.getEntries().length == 6, "Performance.getEntries() " +
> +		"should return 6 entries. Returned " + window.performance.getEntries().length);

use is(window.performance.getEntries().length, 6, ...)  and simplify the comment

@@ +58,5 @@
> +
> +	ok(!!window.performance, "Performance object should exist");
> +	ok(!!window.performance.getEntries, "Performance.getEntries() should exist");
> +	ok(!!window.performance.getEntriesByName, "Performance.getEntriesByName() should exist");
> +	ok(!!window.performance.getEntriesByType, "Performance.getEntriesByType() should exist");

shouldn't be these 4 as the first ones?

@@ +60,5 @@
> +	ok(!!window.performance.getEntries, "Performance.getEntries() should exist");
> +	ok(!!window.performance.getEntriesByName, "Performance.getEntriesByName() should exist");
> +	ok(!!window.performance.getEntriesByType, "Performance.getEntriesByType() should exist");
> +
> +	ok(!!window.performance.getEntriesByType("resource").length > 0,

!!window.performance.getEntriesByType("resource").length > 0

I think window.performance.getEntriesByType("resource").length > 0  or even only window.performance.getEntriesByType("resource").length is enough

applies to other check as well

@@ +93,5 @@
> +
> +	makeXhr("test-data.json", firstCheck);
> +}
> +
> +function checkEntries(anEntryList) {

hm.. used just ones, maybe inline this method?  up to you, the test code would be more readable when just flat/linear and not unnecessarily over-structured.

@@ +136,5 @@
> +	                 ', ', prop, ' = ', entry[prop]].join(''));
> +	        }
> +		}
> +	}
> +	

ws

@@ +138,5 @@
> +		}
> +	}
> +	
> +	ok(anEntryList[0].initiatorType === "script", "The first entry should be a .js file. " +
> +		"Returned " + anEntryList[0].initiatorType);

you should use is() for this.

@@ +146,5 @@
> +		"Returned " + anEntryList[2].initiatorType);
> +	ok(anEntryList[3].initiatorType === "object", "The fourth entry should be a .png file from an <object> tag. " +
> +		"Returned " + anEntryList[3].initiatorType);
> +	ok(anEntryList[4].initiatorType === "embed", "The fifth entry should be a .png file from an <embed> tag. " +
> +		"Returned " + anEntryList[4].initiatorType);

also check other properties, definitely the name, and also that entries are ordered by start time.

@@ +171,5 @@
> +function secondCheck() {
> +	// Since the buffer max size was set to '1', 'peformance.getEntries()' should
> +	// return only  '1' entry (first xhr results).
> +	ok(window.performance.getEntries().length == 1, "The second xhr entry should not be" +
> +		" returned since the buffer size was set to 1.");

check the name, this will make sure that the newer resource has been recorded

@@ +188,5 @@
> +function makeXhr(aUrl, aCallback) {
> +	var xmlhttp = new XMLHttpRequest();
> +    xmlhttp.onload = aCallback;
> +    xmlhttp.open("get", aUrl, true);
> +	xmlhttp.send();

indent

@@ +191,5 @@
> +    xmlhttp.open("get", aUrl, true);
> +	xmlhttp.send();
> +}
> +
> +function checkArraysHaveSameElementsInSameOrder(array1, array2) {

btw, functions can also be nested (this is used only on one place)

@@ +203,5 @@
> +	}
> +	return true;
> +}
> +
> +function ok_wrapper(result, desc) {

comments what this is

::: dom/webidl/Performance.webidl
@@ +27,5 @@
> +// http://www.w3c-test.org/webperf/specs/PerformanceTimeline/#sec-window.performance-attribute
> +partial interface Performance {
> +  PerformanceEntryList getEntries();
> +  PerformanceEntryList getEntriesByType(DOMString entryType);
> +  PerformanceEntryList getEntriesByName(DOMString name, optional DOMString 

ws

@@ +31,5 @@
> +  PerformanceEntryList getEntriesByName(DOMString name, optional DOMString 
> +  	entryType);
> +};
> +
> +// http://www.w3c-test.org/webperf/specs/PerformanceTimeline/#sec-window.performance-attribute

the spec ref is wrong

should be http://www.w3.org/TR/resource-timing/#performanceresourcetiming-methods
Comment 130 User image Honza Bambas (:mayhemer) 2013-10-17 05:17:07 PDT
Oh, and there are also your new test failures on windows (the only platform that has built).
Comment 131 User image Adrian Lungu 2013-10-17 19:13:50 PDT
Created attachment 818812 [details] [diff] [review]
resourceTimingFull_v3.patch

Resource timing full patch (v3)
feedback addressed + patch submitted on try server:
https://tbpl.mozilla.org/?tree=Try&rev=e96a9c384318

> @@ +24,5 @@
> > +  resource timing: window.performance.setResourceTimingBufferSize(1) and
> > +  window.performance.clearResourceTimings();
> > +
> > +  The last tests will verify that the xhr resources are also added as entries
> > +  to our performance object.
> 
> maybe a dumb question: why isn't this part of the first test?
I thought to combine the XHRs and the "setesourceTimingBufferSize()" + "clearResourceTimings()": I have to clear the entries and then add somehow some new entries. If I would clear the entries as part of the first tests, I couldn't do the rest of the tests, since I wouldn't any the entries (besieds 1 or 2 added after the XHRs).

> ::: dom/base/nsPerformance.cpp
> @@ +476,5 @@
> > +void
> > +nsPerformance::SetResourceTimingBufferSize(uint64_t maxSize)
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  mBufferSizeSet = maxSize;
> 
> OK, "if the maxSize parameter is less than the number of elements currently
> stored in the buffer, no elements in the buffer are to be removed"
> 
> but when there are fewer entries, the limit should grow - what if I want the
> buffer just enlarge and leave what is already recorded? the spec doesn't say
> that this custom limit has to be taken into affect only after
> ClearResourceTimings() has been sub-sequentially called.
The spec says "The maxSize parameter will apply only after the clearResourceTimings method is called." I talked to Boris about the meaning of "apply" (see Comment #82 - https://bugzilla.mozilla.org/show_bug.cgi?id=822480#c82) and as far as I understand the primary buffer size will change only after clearResourceTimings method is called.

> @@ +146,5 @@
> > +		"Returned " + anEntryList[2].initiatorType);
> > +	ok(anEntryList[3].initiatorType === "object", "The fourth entry should be a .png file from an <object> tag. " +
> > +		"Returned " + anEntryList[3].initiatorType);
> > +	ok(anEntryList[4].initiatorType === "embed", "The fifth entry should be a .png file from an <embed> tag. " +
> > +		"Returned " + anEntryList[4].initiatorType);
> 
> also check other properties, definitely the name, and also that entries are
> ordered by start time.
The order is being checked - Search for the comment "// The entries list should be in chronological order with respect to startTime". Also, the timings are being checked that are in the proper order for all the elements. I added checks for names, too.
Comment 132 User image Adrian Lungu 2013-10-17 23:56:46 PDT
Created attachment 818868 [details] [diff] [review]
resourceTimingFull_v4.patch

I noticed some problems (and tests failures) on mobile (both Android and B2G). On Android, it looks like the resources are loaded in a different order than on desktop. On B2G, the tests should not run at all (since resource timing is not implemented for it).
I changed the tests that check the entries name and initiator type, so that the order doesn't matter anymore. Also, if the platform is B2G, the tests should be skipped (it's only a test that checks that resource timing is prefed off).
I hope that the changes will work, since I can't test locally on a mobile platforms.
Comment 133 User image Adrian Lungu 2013-10-17 23:58:28 PDT
The try link for the previous patch (v4):
https://tbpl.mozilla.org/?tree=Try&rev=8c532ce3cab5
Comment 134 User image Honza Bambas (:mayhemer) 2013-10-18 08:59:40 PDT
Comment on attachment 818868 [details] [diff] [review]
resourceTimingFull_v4.patch

Review of attachment 818868 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab but please fix all the last comments before landing.

I'll try to find a dom peer to review.

::: dom/base/nsPerformance.h
@@ +34,5 @@
> +/**
> + * @param   aPerformance
> + *          The performance object (the JS parent).
> + *          This will be allow access to "window.performance.timing" attribute
> + *          for the navigation timing (can't be null).

will allow

@@ +91,5 @@
> +   * window.performance.timing.navigationStart).
> +   * 2. The second step is to add the specific offset of the API (mZeroTime).
> +   * This offset can be either 0 (for the resource timing - the timings returned
> +   * are relative timestamps), either equal to "performance.navigationStart"
> +   * (for navigation timing - the timings returned are absolute timestmaps).

'the two timestamps' - which timestamps?

'navigation start - window.performance.timing.navigationStart' - can be well understand as a math expression, this confusing ; use : or = instead of the - sign.

'offset of the API' - sounds weird

some overall tuning would be good anyway.

@@ +93,5 @@
> +   * This offset can be either 0 (for the resource timing - the timings returned
> +   * are relative timestamps), either equal to "performance.navigationStart"
> +   * (for navigation timing - the timings returned are absolute timestmaps).
> +   * @param   aStamp
> +   *          The TimeStamp recorded for a specific event (as milliseconds).

it's not milliseconds!  it's an arbitrary timestamp that it self cannot be used as a time, only when subtracted from another timestamp gives a 'duration' with an arbitrary precision - still not a place to say that it is in milliseconds or seconds or whatever.

just remove the 'milliseconds' note from the comment

@@ +100,5 @@
> +   *          navigationStart TimeStamp (the moment the user landed on the page)
> +   *          An "mZeroTime" offset will be added to the computed duration.
> +   *          The navigation timing will set the mZeroTime to
> +   *          "performance.navigationStart", while the resource timing will set
> +   *          the mZeroTime to "0".

still not sure the return value is described the best way...

suggestion:

@return number of milliseconds value as one of:
- relative to the navigation start time, time the user has landed on the page
- an absolute wall clock time since the unix epoch

Depends on how mZeroTime is set: for Resource Timing it is 0 causing the result be a relative time, for Navigation Timing it is set to "performance.navigationStart" causing the result be an absolute time

::: dom/tests/mochitest/general/resource_timing_iframe.html
@@ +3,5 @@
> +  http://creativecommons.org/publicdomain/zero/1.0/
> +-->
> +
> +<!--
> +  This file contains test for the Resource Timing and Performance Timeline APIs.

This file is a sub-test file at the first place.

@@ +34,5 @@
> +  var hasSubdocument = false;
> +  for (var i = 0 ; i < window.performance.getEntries().length ; i++) {
> +    var entry = window.performance.getEntries()[i];
> +    if (entry.initiatorType === "subdocument") {
> +      hasSubdocument = true;

suggestion: remove hasSubdocument flag at all, at this line call ok(false, "Unexpected iframe " + entry.name);

::: dom/tests/mochitest/general/test_resource_timing.html
@@ +34,5 @@
> +  The iframe contains a second script that will do some tests, as well, plus
> +  an image - its own resource.
> +  The script from the iframe will check that the iframe itself was not added
> +  as an entry (to itself). Also, it will check that its image was added as
> +  enentry to the iframe's performance object. 

enentry, ws

@@ +41,5 @@
> +  The parent's (this window) "ok_wrapper()" method will be called once the tests
> +  are completed.
> +
> +  When all the tests (from this document and from the iframe) are completed,
> +  SimpleTest.finish() is being called and the test ends.

remove the 'SimpleTest.finish() is being called and '

@@ +58,5 @@
> +if (navigator.userAgent.indexOf("Mobile") != -1) {
> +  ok(!SpecialPowers.getBoolPref("dom.enable_resource_timing"),
> +    "Resource timing is not implemented for b2g for the moment and it should be prefed off.");
> +  SimpleTest.finish();
> +}

definitely remove this after you add the test to the except file list.

@@ +66,5 @@
> +// the script starts running so we have to reload the page).
> +if (!SpecialPowers.getBoolPref("dom.enable_resource_timing")) {
> +	SpecialPowers.setBoolPref("dom.enable_resource_timing", true);
> +	location.reload(true);
> +}

and who reverts the pref back? rather run the test in a window (move this top level test to a new support file, do var ok = opener.ok and is = opener.is at the top).  Before you open the window, ensure the pref.  After the test is done (the test can signal using opener.onIamDone()) close the window, revert the pref to previous state and finish the test.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1775,5 @@
> +  nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
> +  nsresult rv = ssm->CheckSameOriginURI(aURI, mOriginalURI, false);
> +  return (NS_SUCCEEDED(rv))
> +      ? true
> +      : false;

go just with return NS_SUCCEEDED(rv);
Comment 135 User image Honza Bambas (:mayhemer) 2013-10-18 09:01:32 PDT
(In reply to Adrian Lungu from comment #132)
> Created attachment 818868 [details] [diff] [review]
> resourceTimingFull_v4.patch
> 
> I noticed some problems (and tests failures) on mobile (both Android and
> B2G). On Android, it looks like the resources are loaded in a different
> order than on desktop. 

Or you can also have a bug ;)  But I don't say it is this way, just from my experience, it is more probable the problem is in my changes than some 'mysterious' behavior of other code.

Also, remember that the resource is added to your array when OnStopRequest is called.  And it is not ensured in what order this happens, at all!  I can see you have changed the order test in v4, that is good.  I realize now the order cannot be tested at all :(

> On B2G, the tests should not run at all (since
> resource timing is not implemented for it).

Yes, the content process is a problem.  There is a list of files that has to be bypassed on b2g:

I think it's this one: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/b2g.json

> I changed the tests that check the entries name and initiator type, so that
> the order doesn't matter anymore. 

Yep.

> Also, if the platform is B2G, the tests
> should be skipped (it's only a test that checks that resource timing is
> prefed off).

Looking into the patch, this is not the proper way (however I know what you are trying to do).  Add the tests you want to bypass to the b2g.json file I refer.

> I hope that the changes will work, since I can't test locally on a mobile
> platforms.

Try is green.


Your objections to my r comments are all correct.
Comment 136 User image Honza Bambas (:mayhemer) 2013-10-18 09:20:34 PDT
jst, can you please take a look at the patch or find someone to forward the review to?  It needs a dom peer review (actually just review of the dom parts, the rest is 'r+ with comments' by me).
Comment 137 User image Adrian Lungu 2013-10-18 18:29:27 PDT
Created attachment 819277 [details] [diff] [review]
resourceTimingFull_v5.patch

I've addressed all the feedback from Comment134.
(https://bugzilla.mozilla.org/show_bug.cgi?id=822480#c134)

Try access revoked (will have to fix this afterwards).

I will be out for the next days, but I will get back on Thursday to address the review from the DOM peer + to add a wiki page with the doc.
Comment 138 User image Steve Workman [:sworkman] (INACTIVE) 2013-10-18 18:36:51 PDT
Comment on attachment 819277 [details] [diff] [review]
resourceTimingFull_v5.patch

try: https://tbpl.mozilla.org/?tree=Try&rev=3a18546886b5
Comment 139 User image Adrian Lungu 2013-10-24 10:49:38 PDT
Created attachment 821813 [details] [diff] [review]
resourceTimingFull_v5.patch (b2g fixed)

The previous patch had "resource_timing_iframe.html" added to "b2g.json". "resource_timing_iframe.html" is an additional file for the test. The test file is "test_resource_timing.html".
Now, the resource timing tests should be skipped for the b2g platform.
Comment 140 User image Adrian Lungu 2013-10-24 10:55:32 PDT
Created attachment 821816 [details] [diff] [review]
resourceTimingFull_v5.patch (proper patch attached)

In the previous attachment, I've uploaded the unmodified (wrong) patch.
Comment 141 User image Johnny Stenback (:jst, jst@mozilla.com) 2013-11-07 17:57:37 PST
Comment on attachment 821816 [details] [diff] [review]
resourceTimingFull_v5.patch (proper patch attached)

I finally managed to find some time to sit down and start reviewing this. I didn't finish though, but here's some comments for what I read through so far.

- In nsImageLoadingContent::LoadImage():

   rv = nsContentUtils::LoadImage(aNewURI, aDocument,
                                  aDocument->NodePrincipal(),
                                  aDocument->GetDocumentURI(),
                                  this, loadFlags,
+                                 content->NodeInfo()->LocalName(),

There should be no need for the NodeInfo() call there, nsIContent (or nsINode, which it inherits) has a LocalName() method on it, which does exactly that for you.

- In nsObjectLoadingContent::OpenChannel():

+      timedChannel->SetInitiatorType(thisContent->NodeInfo()->LocalName());

Same here.

- In nsXBLResourceLoader::LoadResources():

       nsContentUtils::LoadImage(url, doc, docPrincipal, docURL, nullptr,
-                                nsIRequest::LOAD_BACKGROUND,
+                                nsIRequest::LOAD_BACKGROUND, nsString(),

Use EmptyString() here instead of an nsString() object directly.

- In dom/base/PerformanceEntry.cpp:

+PerformanceEntry*
+PerformanceEntry::Clone(nsPerformance* aPerformance)
+{
+  PerformanceEntry* entry = new PerformanceEntry(aPerformance);
+  entry->SetName(mName);
+  entry->SetEntryType(mEntryType);
+
+  return entry;

I don't see this being called anywhere. Is it needed?

- In dom/base/PerformanceEntry.h:

+#define PERFORMANCE_MAX_ENTRY_SIZE 150

I don't see this used anywhere. Is it needed?

\ No newline at end of file

Please add a newline at end of file, in all places where this appears in the diff.

- In dom/base/PerformanceResourceTiming.cpp

The modeline comment at the very top of this file says to use 2 space indentation, but this file uses 4 space indentation. Please fix either of them, and I'd recommend fixing the indentation in the file to match that of the header file, and other files around this one.

+PerformanceEntry*
+PerformanceResourceTiming::Clone(nsPerformance* aPerformance)

I don't see this used either. Is it needed?

- In dom/base/nsPerformance.cpp:

+// This method will implement the timing allow check algorithm
+// http://w3c-test.org/webperf/specs/ResourceTiming/#timing-allow-check

Did the spec change here? It talks about a Timing-Allow-Origin header, which doesn't seem to be handled here at all. Is that intentional?

Note to self, continue from here.
Comment 142 User image Johnny Stenback (:jst, jst@mozilla.com) 2013-11-08 16:35:13 PST
Comment on attachment 821816 [details] [diff] [review]
resourceTimingFull_v5.patch (proper patch attached)

This is looking pretty good, a few more comments below. I'd like to look over an updated patch again before this lands. r- due to my previous comment and the stuff here, but this is close!

- In nsPerformance::SetResourceTimingBufferSize():

+  if (mBufferSizeSet < mEntries.Length()) {
+    // call onresourcetimingbufferfull
+  }

Either implement this call, or file a followup bug and reference it here.

+void
+nsPerformance::AddEntry(nsIHttpChannel* resourcesChannel,
+                        nsITimedChannel* resourcesTimedChannel)

"resources" in those argument names seem redundant, maybe drop them?

+    if (mEntries.Length() > mPrimaryBufferSize) {
+      // call onresourcetimingbufferfull
+    }

Same as earlier, implement this, or file a bug and reference it here.

- In dom/base/nsPerformance.h

+   * 2. The second step is to add any required offset (the mZeroTime). For now,
+   * this offset value is either 0 (for the resource timing), either equal to

Replace the second "either" with "or".

+  // High resolution (used by resource timing)
+  DOMHighResTimeStamp FetchStartHighRes();
+  DOMHighResTimeStamp RedirectStartHighRes();
+  DOMHighResTimeStamp RedirectEndHighRes();
+  DOMHighResTimeStamp DomainLookupStartHighRes();
+  DOMHighResTimeStamp DomainLookupEndHighRes();
...

Not a big deal, but could all these not be const, like the ones you're replacing were?

- In image/src/imgLoader.cpp:

                                 aCacheKey,
                                 aPolicy,
+                                nsString(),

Use EmptyString().

- In image/src/imgRequestProxy.h:

+#include "mozilla/TimeStamp.h"

How hard would it be to add this #include to the .cpp files that are affected by this change instead of to the header? (in the interest of not making more files than necessary read this header file)

- In layout/generic/nsImageFrame.cpp:

                        nullptr,
                        nullptr,      /* channel policy not needed */
+                       nsString(),

EmptyString().

- In layout/style/Loader.cpp, in Loader::LoadSheet():

     nsCOMPtr<nsIURI> referrerURI = aLoadData->GetReferrerURI();
     if (referrerURI)
       httpChannel->SetReferrer(referrerURI);
+
+    // Set the initiator type
+    nsCOMPtr<nsITimedChannel> timedChannel(do_QueryInterface(httpChannel));
+    if (timedChannel) {
+        if (aLoadData->mParentData) {
+          timedChannel->SetInitiatorType(NS_LITERAL_STRING("css"));
+        } else {
+          timedChannel->SetInitiatorType(NS_LITERAL_STRING("link"));
+        }

Fix the 4-space indentation there. We should run this change by some layout folks too, before this patch lands.

- In nsImageBoxFrame::UpdateImage():

       nsContentUtils::LoadImage(uri, doc, mContent->NodePrincipal(),
                                 doc->GetDocumentURI(), mListener, mLoadFlags,
-                                getter_AddRefs(mImageRequest));
+                                nsString(), getter_AddRefs(mImageRequest));

EmptyString().
 
- In nsTreeBodyFrame::GetImage():

                                                 nsIRequest::LOAD_NORMAL,
+                                                nsString(),

EmptyString()

- In nsMenuItemIconX::LoadIcon():

   nsresult rv = loader->LoadImage(aIconURI, nullptr, nullptr, nullptr, loadGroup, this,
                                    nullptr, nsIRequest::LOAD_NORMAL, nullptr,
-                                   nullptr, getter_AddRefs(mIconRequest));
+                                   nullptr, nsString(), getter_AddRefs(mIconRequest));

EmptyString().
Comment 143 User image Adrian Lungu 2013-11-09 11:27:18 PST
Created attachment 829770 [details] [diff] [review]
resourceTimingFull_v6.patch

I've addressed the review and submitted 2 new bugs for the missing pieces: "timing allow check algorithm" and "onresourcetimingbufferfull" callback:
https://bugzilla.mozilla.org/show_bug.cgi?id=936813
https://bugzilla.mozilla.org/show_bug.cgi?id=936814
Comment 144 User image Adrian Lungu 2013-11-09 11:45:18 PST
Created attachment 829772 [details] [diff] [review]
resourceTimingFull_v7.patch

I've missed a comment change from the review.

Also, trying to move the "#include "mozilla/TimeStamp.h" to the .cpp file is not working. I still get a compile error when the .h file tries to include the "nsITimedChannel.h" (which contains a forward declaration of the "Timestamp").

"FetchStartHighRes()" is not constant anymore. I preferred to cache its value in the "mFetchStart", since the "FetchStartHighRes()" is called very often (every time a timing is invalid). Since all the other methods ("RedirectStartHighRes()" and other similar methods) call "FetchStartHighRes()", all these methods are not const anymore.
Comment 145 User image Boris Zbarsky [:bz] (still a bit busy) 2013-11-09 15:24:10 PST
We should probably put this behind a pref (possibly defaulted to true on Nightly/Aurora) until the various bits that are still unimplemented are implemented....
Comment 146 User image Adrian Lungu 2013-11-11 01:16:52 PST
(In reply to Boris Zbarsky [:bz] from comment #145)
> We should probably put this behind a pref (possibly defaulted to true on
> Nightly/Aurora) until the various bits that are still unimplemented are
> implemented....

This is already behind a pref, which is default to false for now.
Comment 147 User image Boris Zbarsky [:bz] (still a bit busy) 2013-11-11 06:01:52 PST
Where?  All the WebIDL in the patch sure seems unconditional to me.
Comment 148 User image Adrian Lungu 2013-11-11 07:53:23 PST
"pref("dom.enable_resource_timing", false);" in all.js.
But this may be used only for unit tests.. Right?
Comment 149 User image Boris Zbarsky [:bz] (still a bit busy) 2013-11-11 11:32:18 PST
> "pref("dom.enable_resource_timing", false);" in all.js.

Sure, but how does that make "getEntries in performance" test false in JS?
Comment 150 User image Honza Bambas (:mayhemer) 2014-02-22 09:03:44 PST
Any chance this gets ever finished?
Comment 151 User image Boris Zbarsky [:bz] (still a bit busy) 2014-02-22 15:06:43 PST
What is this blocked on?  Just Adrian having time or someone else picking this up?
Comment 152 User image Steve Workman [:sworkman] (INACTIVE) 2014-02-24 10:42:48 PST
It was going to be on my list for this quarter, but got bumped in preference to other work. If Adrian can still work on it, great. If not, I'll keep it on my list.
Comment 153 User image Honza Bambas (:mayhemer) 2014-02-24 10:46:52 PST
I am just concerned about how fast this large and a bit of being finished (as I often see!) patch bitrots :((
Comment 154 User image Adrian Lungu 2014-02-24 12:17:50 PST
Hi guys,

I could find some time over the weekends to help this patch get committed, but I don't know what I should do next. I answered to the last review (Johnny's review) and waited for new directions. So, what would be the next steps or what is this patch missing?
Comment 155 User image Boris Zbarsky [:bz] (still a bit busy) 2014-02-24 12:55:10 PST
Comment 145?
Comment 156 User image Honza Bambas (:mayhemer) 2014-02-25 16:12:47 PST
I think you should know that :)  And as Boris mentions that, comment 145 is definitely one of them.
Comment 157 User image Fabien Ménager 2014-03-22 06:04:43 PDT
Any news about this patch ? 
Performance resource timing is something other browsers already implement (Chrome, IE10+) and it's sad Firefox doesn't have it yet.
Comment 158 User image Johnny Stenback (:jst, jst@mozilla.com) 2014-03-26 11:51:38 PDT
Agreed, we need to push this one over the finish line.
Comment 159 User image Valentin Gosu [:valentin] 2014-03-31 15:54:48 PDT
Created attachment 8399695 [details] [diff] [review]
Add in the Resource Timing API
Comment 160 User image Valentin Gosu [:valentin] 2014-03-31 15:56:44 PDT
Rebased the patch and working towards addressing comment 145
Comment 161 User image Valentin Gosu [:valentin] 2014-04-07 16:21:24 PDT
Just to make things clear, given the info in comment 149, I understand I should use the pref syntax (https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Pref) for getEntries. Should the pref cover the entire "partial interface Performance" blocks, or should I apply it only to the methods?

Is there anything else I should put behind a pref?
Comment 162 User image Boris Zbarsky [:bz] (still a bit busy) 2014-04-07 21:16:22 PDT
You should apply it only to the methods you want to be controlled by the pref.

> Is there anything else I should put behind a pref?

Just the web-exposed API.
Comment 163 User image Valentin Gosu [:valentin] 2014-04-08 07:40:09 PDT
Created attachment 8403294 [details] [diff] [review]
Add in the Resource Timing API
Comment 164 User image Valentin Gosu [:valentin] 2014-04-08 07:49:38 PDT
Created attachment 8403299 [details] [diff] [review]
pref_resourceTiming.patch

Added the pref extended attribute to the new methods and tested using the web console.
Comment 165 User image Boris Zbarsky [:bz] (still a bit busy) 2014-04-08 14:54:33 PDT
Comment on attachment 8403299 [details] [diff] [review]
pref_resourceTiming.patch

Great, thanks!
Comment 166 User image Valentin Gosu [:valentin] 2014-04-09 03:49:30 PDT
So are there any other issues keeping us from landing this?
Comment 167 User image Boris Zbarsky [:bz] (still a bit busy) 2014-04-09 18:47:15 PDT
Nothing on my end.  If it's green on try, it can land as far as I'm concerned.  Thanks!
Comment 168 User image Valentin Gosu [:valentin] 2014-04-10 11:54:52 PDT
Created attachment 8404899 [details] [diff] [review]
Add in the Resource Timing API
Comment 169 User image Valentin Gosu [:valentin] 2014-04-10 12:14:09 PDT
Comment on attachment 8404899 [details] [diff] [review]
Add in the Resource Timing API

Review of attachment 8404899 [details] [diff] [review]:
-----------------------------------------------------------------

https://tbpl.mozilla.org/?tree=Try&rev=071864fa0f4c

Tests are green (so far), except for the new e10s tests.
https://tbpl.mozilla.org/php/getParsedLog.php?id=37584799&tree=Try&full=1#error2

::: dom/base/nsPerformance.cpp
@@ +146,5 @@
> +{
> +  // We have to check if all the redirect URIs had the same origin (since there
> +  // is no check in RedirectStartHighRes())
> +  bool sameOrigin;
> +  mChannel->GetAllRedirectsSameOrigin(&sameOrigin);

Apparently, mChannel is null in the e10s tests.
Comment 170 User image Valentin Gosu [:valentin] 2014-04-10 17:31:44 PDT
Created attachment 8405102 [details] [diff] [review]
res_timing_e10s.patch

I fixed the crashes with some carefully placed ifs (and I removed the asserts), however one test still failed on e10s.

INFO TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug570341.html | Expected navigationStart to happen before fetchStart, got navigationStart = 1397172160445, fetchStart = 0

So I changed the default returned value from 0 to mZeroTime (according to the description in the header).
Comment 171 User image Valentin Gosu [:valentin] 2014-04-11 10:29:25 PDT
Turns out there's another test failing on e10s, which I can't figure out how to fix without a channel

16 INFO TEST-UNEXPECTED-FAIL | /tests/docshell/test/test_bug668513.html | Expected navigation.redirectCount == 1 on an server redirected navigation - got 0, expected 1

Any ideas on how I could get the redirectCount in this case? Reverting to the old way doesn't seem like a good choice either.
Comment 172 User image Boris Zbarsky [:bz] (still a bit busy) 2014-04-11 10:30:22 PDT
Honza, thoughts?
Comment 173 User image Johnny Stenback (:jst, jst@mozilla.com) 2014-04-11 15:21:51 PDT
Looks good to me too, pending the e10s stuff gets sorted out. Thanks for taking this on!
Comment 174 User image Valentin Gosu [:valentin] 2014-04-15 06:21:56 PDT
The problem was that HttpChannelChild did not implement nsITimedChannel, so it appeared to be null. I'm currently working to implement all the nsITimedChannel getters. Only redirectCount is needed to get all the tests to pass, but I'm thinking the others should be there as well.
Comment 175 User image Valentin Gosu [:valentin] 2014-04-16 18:49:49 PDT
Created attachment 8407949 [details] [diff] [review]
imported patch pref_resourceTiming.patch
Comment 176 User image Valentin Gosu [:valentin] 2014-04-16 18:50:19 PDT
Created attachment 8407950 [details] [diff] [review]
Fix for e10s
Comment 177 User image Valentin Gosu [:valentin] 2014-04-16 18:58:00 PDT
Comment on attachment 8407950 [details] [diff] [review]
Fix for e10s

Moved nsITimedChannel implementation to HttpBaseChannel so it would be inherited by HttpChannelChild.
The redirect count is passed from the parent to the child in RecvOnStartRequest.
mZeroTime is returned instead of 0.
Comment 178 User image Boris Zbarsky [:bz] (still a bit busy) 2014-04-16 19:41:42 PDT
Comment on attachment 8407950 [details] [diff] [review]
Fix for e10s

The DOM parts look fine, but I'd like a necko peer to review the necko bits.
Comment 179 User image Honza Bambas (:mayhemer) 2014-04-18 07:09:56 PDT
Comment on attachment 8407950 [details] [diff] [review]
Fix for e10s

Review of attachment 8407950 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +350,5 @@
> +  TimeStamp                         mCacheReadEnd;
> +  // copied from the transaction before we null out mTransaction
> +  // so that the timing can still be queried from OnStopRequest
> +  TimingStruct                      mTransactionTimings;
> +

no blank line here

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +52,5 @@
>    LOG(("Creating HttpChannelChild @%x\n", this));
>  
> +  mChannelCreationTime = PR_Now();
> +  mChannelCreationTimestamp = TimeStamp::Now();
> +  mAsyncOpenTime = TimeStamp::Now();

Is mAsyncOpenTime correct here?

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +600,5 @@
>      if (secInfoSer)
>        NS_SerializeToString(secInfoSer, secInfoSerialization);
>    }
>  
> +  uint16_t redirect_count = 0;

redirectCount

@@ +601,5 @@
>        NS_SerializeToString(secInfoSer, secInfoSerialization);
>    }
>  
> +  uint16_t redirect_count = 0;
> +  mChannel->GetRedirectCount(&redirect_count);

did you want chan-> ?

how did you test this?

::: netwerk/protocol/http/PHttpChannel.ipdl
@@ +17,5 @@
>  using class nsHttpHeaderArray from "nsHttpHeaderArray.h";
>  using class nsHttpResponseHead from "nsHttpResponseHead.h";
>  using struct nsHttpAtom from "nsHttp.h";
>  using mozilla::net::NetAddr from "mozilla/net/DNS.h";
> +using mozilla::TimeStamp from "mozilla/TimeStamp.h";

why?
Comment 180 User image Valentin Gosu [:valentin] 2014-04-18 15:07:50 PDT
(In reply to Honza Bambas (:mayhemer) from comment #179)
> > +  mAsyncOpenTime = TimeStamp::Now();
> 
> Is mAsyncOpenTime correct here?

Probably not, but it's needed for the case where we divert a channel, and asyncOpen does not get called. It seems as a fine approximation for the actual time, and it passes the tests.  We could handle any other issues in Bug 658819.

> > +  mChannel->GetRedirectCount(&redirect_count);
> 
> did you want chan-> ?

I used mChannel because it's also used in the call to SendOnStartRequest.

> how did you test this?

docshell/test/test_bug668513.html --e10s used to fail because of a wrong redirect count. I checked, and the tests seem to pass with both chan and mChannel.

https://tbpl.mozilla.org/?tree=Try&rev=a2ed7a9e1430
Comment 181 User image Valentin Gosu [:valentin] 2014-04-18 15:27:05 PDT
Created attachment 8409153 [details] [diff] [review]
pref_resourceTiming.patch
Comment 182 User image Valentin Gosu [:valentin] 2014-04-18 15:28:30 PDT
Created attachment 8409156 [details] [diff] [review]
res_timing_e10s.patch

Note You need to log in before you can comment on or make changes to this bug.