Add in the Resource Timing API

RESOLVED FIXED in mozilla31

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: andym, Assigned: valentin)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla31
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [timing][performance][standards][netpanel][firebug], URL)

Attachments

(3 attachments, 48 obsolete attachments)

118.60 KB, patch
Details | Diff | Splinter Review
1.24 KB, patch
Details | Diff | Splinter Review
34.24 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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/
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.
Component: Developer Tools → Developer Tools: Console
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [timing][performance][standards]
Whiteboard: [timing][performance][standards] → [timing][performance][standards][netpanel]
Assignee: nobody → amarchesini
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!
Attachment #712873 - Flags: feedback?(bzbarsky)
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?
Attachment #712873 - Flags: feedback?(bzbarsky) → feedback-
Given the Gecko patch here, we should move the bug to Core, and file a different bug for the Web Console.
Component: Developer Tools: Console → DOM: Other
Product: Firefox → Core
Blocks: 845315
moved to Core: DOM: Other for lack of a better component.

added bug 845315 for the Console Output work.
Created attachment 722193 [details] [diff] [review]
WIP

Boris, I need a feedback :)
Do you see big big errors til now?
Attachment #712873 - Attachment is obsolete: true
Attachment #722193 - Flags: review?(bzbarsky)
I'm not going to be able to get to this until Monday.
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.
Attachment #722193 - Flags: review?(bzbarsky)
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)
I think mayhemer may be a good reviewer for the guts of this, in fact.
Component: DOM: Other → DOM
Product: Core → Core
Assigning this to Adrian to work on during his internship.
Assignee: amarchesini → alungu
Status: NEW → ASSIGNED

Comment 12

4 years ago
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!
Attachment #790409 - Flags: feedback?(honzab.moz)
Attachment #790409 - Flags: feedback?(bzbarsky)

Comment 13

4 years ago
Created attachment 790410 [details] [diff] [review]
Resource Timing intrface (+ test)

+ The test file.
Attachment #790409 - Attachment is obsolete: true
Attachment #790409 - Flags: feedback?(honzab.moz)
Attachment #790409 - Flags: feedback?(bzbarsky)
Attachment #790410 - Flags: feedback?(honzab.moz)
Attachment #790410 - Flags: feedback?(bzbarsky)
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.
BTW, the idl files are completely missing any documentation so I absolutely don't know what they represent and are purposed for.

Comment 16

4 years ago
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

4 years ago
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.
Attachment #790410 - Attachment is obsolete: true
Attachment #790410 - Flags: feedback?(honzab.moz)
Attachment #790410 - Flags: feedback?(bzbarsky)

Comment 18

4 years ago
(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?
> 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

4 years ago
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.
Attachment #790874 - Attachment is obsolete: true
Attachment #793794 - Flags: feedback?(honzab.moz)
Attachment #793794 - Flags: feedback?(bzbarsky)
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?
Attachment #793794 - Flags: feedback?(bzbarsky) → feedback+

Comment 22

4 years ago
(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.
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

4 years ago
(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?
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

4 years ago
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.
Attachment #793794 - Attachment is obsolete: true
Attachment #793794 - Flags: feedback?(honzab.moz)
Attachment #795864 - Flags: review?(honzab.moz)
Attachment #795864 - Flags: review?(bzbarsky)

Comment 27

4 years ago
Comment on attachment 795864 [details] [diff] [review]
Version 3 (add the entry after the resource is loaded)

Wrong flags..
Attachment #795864 - Flags: review?(honzab.moz)
Attachment #795864 - Flags: review?(bzbarsky)
Attachment #795864 - Flags: feedback?(honzab.moz)
Attachment #795864 - Flags: feedback?(bzbarsky)
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...
Attachment #795864 - Flags: feedback?(bzbarsky) → feedback+

Comment 29

4 years ago
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.
Attachment #795864 - Attachment is obsolete: true
Attachment #795864 - Flags: feedback?(honzab.moz)
Attachment #796398 - Flags: feedback?(bzbarsky)
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?
Attachment #796398 - Flags: feedback?(bzbarsky) → feedback+

Comment 31

4 years ago
(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?
Flags: needinfo?(bzbarsky)
> 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?
Flags: needinfo?(bzbarsky)

Comment 33

4 years ago
(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?
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

4 years ago
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
Flags: needinfo?(bzbarsky)
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).
Flags: needinfo?(bzbarsky)

Comment 37

4 years ago
(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?
Yes.

Comment 39

4 years ago
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
Attachment #796398 - Attachment is obsolete: true
Attachment #800491 - Flags: feedback?(bzbarsky)
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....
Attachment #800491 - Flags: feedback?(bzbarsky) → feedback-

Comment 41

4 years ago
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

4 years ago
Created attachment 801358 [details] [diff] [review]
Build and add the performance entry

This patch add the logic for building and adding the performance entries.
Attachment #801358 - Flags: feedback?(bzbarsky)

Comment 43

4 years ago
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.
Attachment #800491 - Attachment is obsolete: true
Attachment #801359 - Flags: feedback?(bzbarsky)

Comment 44

4 years ago
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

4 years ago
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
Flags: needinfo?(bzbarsky)
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?
Flags: needinfo?(bzbarsky)
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?
Attachment #801359 - Flags: feedback?(bzbarsky) → feedback+
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.
Attachment #801358 - Flags: feedback?(bzbarsky) → feedback+

Comment 49

4 years ago
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.
Attachment #802505 - Flags: feedback?(bzbarsky)
> 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

4 years ago
(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.
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
Oh, and the split between nsPerformanceTiming and the DOMTiming stuff is ... weird.  Not sure why we have that.
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.
Attachment #802505 - Flags: feedback?(bzbarsky) → feedback+

Comment 55

4 years ago
(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
Flags: needinfo?(bzbarsky)
I think you should follow the editor's draft, yes.
Flags: needinfo?(bzbarsky)

Comment 57

4 years ago
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?
Attachment #801358 - Attachment is obsolete: true
Attachment #803309 - Flags: feedback?(honzab.moz)
Attachment #803309 - Flags: feedback?(bzbarsky)

Comment 58

4 years ago
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
Attachment #802505 - Attachment is obsolete: true
Attachment #803846 - Flags: feedback?(bzbarsky)
Comment on attachment 803846 [details] [diff] [review]
part3_v1_to_v3.diff

This part looks fine.  Still sorting through the other giant patch.  :(
Attachment #803846 - Flags: feedback?(bzbarsky) → feedback+

Comment 60

4 years ago
(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

4 years ago
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).
Attachment #801359 - Attachment is obsolete: true
Attachment #803872 - Flags: feedback+
Yeah, I don't think an interdiff would help much.  ;)

Comment 63

4 years ago
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?
Attachment #804116 - Flags: feedback?(bzbarsky)

Comment 64

4 years ago
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.
Attachment #801357 - Attachment is obsolete: true

Comment 65

4 years ago
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)
Flags: needinfo?(bzbarsky)

Comment 66

4 years ago
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.
Attachment #804116 - Flags: feedback?(bzbarsky)
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 ?
Attachment #803309 - Flags: feedback?(honzab.moz) → feedback-

Comment 68

4 years ago
(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!
Flags: needinfo?(honzab.moz)
I'm sorry this is taking so long.  I should be able to deal with this on Monday.

Comment 70

4 years ago
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?
Attachment #808687 - Flags: feedback?(bzbarsky)
(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?
Flags: needinfo?(honzab.moz)

Comment 72

4 years ago
(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
(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

4 years ago
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

4 years ago
(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

4 years ago
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.
Attachment #803309 - Attachment is obsolete: true
Attachment #803309 - Flags: feedback?(bzbarsky)
Attachment #809650 - Flags: feedback?(honzab.moz)
Attachment #809650 - Flags: feedback?(bzbarsky)
(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

4 years ago
(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 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?
Attachment #809650 - Flags: feedback?(honzab.moz)
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.
Attachment #808687 - Flags: feedback?(bzbarsky) → feedback+
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...
Attachment #809650 - Flags: feedback?(bzbarsky) → feedback-
> 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.
Flags: needinfo?(bzbarsky)

Comment 83

4 years ago
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.
Attachment #810866 - Flags: feedback?(honzab.moz)
Attachment #810866 - Flags: feedback?(bzbarsky)
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.
Attachment #810866 - Flags: feedback?(bzbarsky) → feedback+

Comment 85

4 years ago
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.
>+    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

4 years ago
(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

4 years ago
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.
Attachment #811454 - Flags: feedback?(bzbarsky)
> 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

4 years ago
(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!
OK.  Posted http://lists.w3.org/Archives/Public/public-web-perf/2013Sep/0101.html
Attachment #811454 - Flags: feedback?(bzbarsky)

Comment 92

4 years ago
Created attachment 812449 [details] [diff] [review]
part0_preparePerformanceTiming_v1_to_v2.diff
Attachment #812449 - Flags: feedback?(bzbarsky)

Comment 93

4 years ago
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.
Attachment #812510 - Flags: feedback?(honzab.moz)
Attachment #812510 - Flags: feedback?(bzbarsky)
Adrian, does "part0_preparePerformanceTiming_v1_to_v2.diff" take into account the discussion in comment 89?
Flags: needinfo?(alungu)

Comment 95

4 years ago
(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).
Flags: needinfo?(alungu)

Comment 96

4 years ago
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!
Flags: needinfo?(bzbarsky)
The navigationStart of the iframe.
Flags: needinfo?(bzbarsky)
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!
Attachment #812449 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 812510 [details] [diff] [review]
part0_adjustRedirects_v1.patch

Would like Honza to look this over first.
Attachment #812510 - Flags: feedback?(bzbarsky)

Comment 100

4 years ago
(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

4 years ago
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.
> Right?

Sounds plausible, at least.  ;)
Keywords: dev-doc-needed

Comment 103

4 years ago
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.
Attachment #809650 - Attachment is obsolete: true
Attachment #810866 - Attachment is obsolete: true
Attachment #811367 - Attachment is obsolete: true
Attachment #810866 - Flags: feedback?(honzab.moz)
Attachment #813296 - Flags: feedback?(honzab.moz)
Attachment #813296 - Flags: feedback?(bzbarsky)
I won't get to this since mid-next-week sometime, due to summit stuff....

Comment 105

4 years ago
(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

4 years ago
Created attachment 813401 [details] [diff] [review]
part4_entriesManagement_v2.patch
Attachment #804116 - Attachment is obsolete: true
Attachment #813401 - Flags: feedback?(bzbarsky)

Comment 107

4 years ago
Created attachment 813402 [details] [diff] [review]
part5_addPreference_v1.patch

For now, resource timing is enabled (so we can test it).
Attachment #813402 - Flags: feedback?(bzbarsky)
(Hoping this is the spec you are working with, please fix it if that is wrong)
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?
Flags: needinfo?(alungu)
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...
Attachment #812510 - Flags: feedback?(honzab.moz)
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?
Attachment #813296 - Flags: feedback?(honzab.moz)
(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.
(TR/ is for trash.)

Comment 114

4 years ago
(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)
Flags: needinfo?(alungu)

Comment 115

4 years ago
Created attachment 814623 [details] [diff] [review]
part0_adjustRedirects_v1_to_v2.diff
Attachment #814623 - Flags: feedback?(honzab.moz)

Comment 116

4 years ago
Created attachment 814664 [details] [diff] [review]
part2_removeIsDocFlag.diff

As Honza suggested, I removed the IsDocument flag and used instead "mLoadFlags & LOAD_DOCUMENT_URI"
Attachment #814664 - Flags: feedback?(honzab.moz)
Attachment #814664 - Flags: feedback?(bzbarsky)
Adrian, can you please provide also the full patch?  I'd like to check both.  Thanks.
Attachment #814664 - Flags: feedback?(honzab.moz) → feedback+

Comment 118

4 years ago
Created attachment 814981 [details] [diff] [review]
part0_adjustRedirects_v2.patch
Attachment #812510 - Attachment is obsolete: true
Attachment #814623 - Attachment is obsolete: true
Attachment #814623 - Flags: feedback?(honzab.moz)
Attachment #814981 - Flags: feedback?(honzab.moz)

Updated

4 years ago
Attachment #813296 - Flags: feedback?(bzbarsky) → feedback?(honzab.moz)

Updated

4 years ago
Attachment #813401 - Flags: feedback?(bzbarsky) → feedback?(honzab.moz)

Updated

4 years ago
Attachment #813402 - Flags: feedback?(bzbarsky) → feedback?(honzab.moz)

Comment 119

4 years ago
Created attachment 816176 [details] [diff] [review]
part6_testing_v1.patch

Tests for the resource timing API
Attachment #816176 - Flags: feedback?(honzab.moz)
Attachment #816176 - Flags: feedback?(bzbarsky)
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
Attachment #814981 - Flags: feedback?(honzab.moz) → feedback-
Can you please also provide a complete patch (all patches folded together) ?   Thanks.

Comment 122

4 years ago
(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

4 years ago
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).
Attachment #817579 - Flags: feedback?(honzab.moz)
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).
Attachment #817579 - Flags: feedback?(honzab.moz) → feedback+

Comment 125

4 years ago
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
Attachment #722193 - Attachment is obsolete: true
Attachment #803846 - Attachment is obsolete: true
Attachment #803872 - Attachment is obsolete: true
Attachment #804137 - Attachment is obsolete: true
Attachment #808687 - Attachment is obsolete: true
Attachment #811454 - Attachment is obsolete: true
Attachment #812449 - Attachment is obsolete: true
Attachment #813296 - Attachment is obsolete: true
Attachment #813401 - Attachment is obsolete: true
Attachment #813402 - Attachment is obsolete: true
Attachment #814664 - Attachment is obsolete: true
Attachment #814981 - Attachment is obsolete: true
Attachment #816176 - Attachment is obsolete: true
Attachment #817579 - Attachment is obsolete: true
Attachment #813296 - Flags: feedback?(honzab.moz)
Attachment #813401 - Flags: feedback?(honzab.moz)
Attachment #813402 - Flags: feedback?(honzab.moz)
Attachment #814664 - Flags: feedback?(bzbarsky)
Attachment #816176 - Flags: feedback?(honzab.moz)
Attachment #816176 - Flags: feedback?(bzbarsky)
Attachment #818258 - Flags: review?(honzab.moz)

Comment 126

4 years ago
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.
(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 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)
Attachment #818259 - Flags: review-
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
Attachment #818258 - Flags: review?(honzab.moz) → review-
Oh, and there are also your new test failures on windows (the only platform that has built).

Comment 131

4 years ago
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.
Attachment #818258 - Attachment is obsolete: true
Attachment #818259 - Attachment is obsolete: true
Attachment #818812 - Flags: review?(honzab.moz)

Comment 132

4 years ago
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.
Attachment #818812 - Attachment is obsolete: true
Attachment #818812 - Flags: review?(honzab.moz)
Attachment #818868 - Flags: review?(honzab.moz)

Comment 133

4 years ago
The try link for the previous patch (v4):
https://tbpl.mozilla.org/?tree=Try&rev=8c532ce3cab5
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);
Attachment #818868 - Flags: review?(honzab.moz) → review+
(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.
Attachment #818868 - Flags: review?(jst)
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).
Flags: needinfo?(jst)

Comment 137

4 years ago
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.
Attachment #818868 - Attachment is obsolete: true
Attachment #818868 - Flags: review?(jst)
Attachment #819277 - Flags: review?(jst)
Comment on attachment 819277 [details] [diff] [review]
resourceTimingFull_v5.patch

try: https://tbpl.mozilla.org/?tree=Try&rev=3a18546886b5

Comment 139

4 years ago
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.
Attachment #819277 - Attachment is obsolete: true
Attachment #819277 - Flags: review?(jst)
Attachment #821813 - Flags: review?(jst)

Comment 140

4 years ago
Created attachment 821816 [details] [diff] [review]
resourceTimingFull_v5.patch (proper patch attached)

In the previous attachment, I've uploaded the unmodified (wrong) patch.
Attachment #821813 - Attachment is obsolete: true
Attachment #821813 - Flags: review?(jst)
Attachment #821816 - Flags: review?(jst)
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 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().
Attachment #821816 - Flags: review?(jst) → review+

Updated

4 years ago
Blocks: 936813

Updated

4 years ago
Depends on: 936814

Updated

4 years ago
Blocks: 936814
No longer depends on: 936814

Comment 143

4 years ago
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
Attachment #821816 - Attachment is obsolete: true

Comment 144

4 years ago
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.
Attachment #829770 - Attachment is obsolete: true
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

4 years ago
(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.
Where?  All the WebIDL in the patch sure seems unconditional to me.

Comment 148

4 years ago
"pref("dom.enable_resource_timing", false);" in all.js.
But this may be used only for unit tests.. Right?
> "pref("dom.enable_resource_timing", false);" in all.js.

Sure, but how does that make "getEntries in performance" test false in JS?
Any chance this gets ever finished?
What is this blocked on?  Just Adrian having time or someone else picking this up?
Flags: needinfo?(adrian.lungu89)
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.
I am just concerned about how fast this large and a bit of being finished (as I often see!) patch bitrots :((

Comment 154

4 years ago
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?
Flags: needinfo?(adrian.lungu89) → needinfo?(honzab.moz)
Comment 145?
I think you should know that :)  And as Boris mentions that, comment 145 is definitely one of them.
Flags: needinfo?(honzab.moz)

Comment 157

4 years ago
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.
Agreed, we need to push this one over the finish line.
Flags: needinfo?(jst)
(Assignee)

Comment 159

3 years ago
Created attachment 8399695 [details] [diff] [review]
Add in the Resource Timing API
(Assignee)

Updated

3 years ago
Attachment #829772 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Assignee: adrian.lungu89 → valentin.gosu
(Assignee)

Comment 160

3 years ago
Rebased the patch and working towards addressing comment 145
Whiteboard: [timing][performance][standards][netpanel] → [timing][performance][standards][netpanel][firebug]
(Assignee)

Comment 161

3 years ago
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?
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 163

3 years ago
Created attachment 8403294 [details] [diff] [review]
Add in the Resource Timing API
(Assignee)

Updated

3 years ago
Attachment #8399695 - Attachment is obsolete: true
(Assignee)

Comment 164

3 years ago
Created attachment 8403299 [details] [diff] [review]
pref_resourceTiming.patch

Added the pref extended attribute to the new methods and tested using the web console.
Attachment #8403299 - Flags: review?(bzbarsky)
Comment on attachment 8403299 [details] [diff] [review]
pref_resourceTiming.patch

Great, thanks!
Attachment #8403299 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 166

3 years ago
So are there any other issues keeping us from landing this?
Flags: needinfo?(jst)
Flags: needinfo?(bzbarsky)
Nothing on my end.  If it's green on try, it can land as far as I'm concerned.  Thanks!
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 168

3 years ago
Created attachment 8404899 [details] [diff] [review]
Add in the Resource Timing API
(Assignee)

Updated

3 years ago
Attachment #8403294 - Attachment is obsolete: true
(Assignee)

Comment 169

3 years ago
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.
(Assignee)

Comment 170

3 years ago
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).
Attachment #8405102 - Flags: review?(bzbarsky)
(Assignee)

Updated

3 years ago
Attachment #8405102 - Flags: review?(bzbarsky)
(Assignee)

Comment 171

3 years ago
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.
Flags: needinfo?(bzbarsky)
Honza, thoughts?
Flags: needinfo?(bzbarsky) → needinfo?(honzab.moz)
Looks good to me too, pending the e10s stuff gets sorted out. Thanks for taking this on!
Flags: needinfo?(jst)
(Assignee)

Comment 174

3 years ago
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.
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 175

3 years ago
Created attachment 8407949 [details] [diff] [review]
imported patch pref_resourceTiming.patch
(Assignee)

Comment 176

3 years ago
Created attachment 8407950 [details] [diff] [review]
Fix for e10s
(Assignee)

Updated

3 years ago
Attachment #8405102 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8407949 - Attachment is obsolete: true
(Assignee)

Comment 177

3 years ago
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.
Attachment #8407950 - Flags: review?(bzbarsky)
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.
Attachment #8407950 - Flags: review?(honzab.moz)
Attachment #8407950 - Flags: review?(bzbarsky)
Attachment #8407950 - Flags: review+
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?
Attachment #8407950 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 180

3 years ago
(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
(Assignee)

Comment 181

3 years ago
Created attachment 8409153 [details] [diff] [review]
pref_resourceTiming.patch
(Assignee)

Updated

3 years ago
Attachment #8403299 - Attachment is obsolete: true
(Assignee)

Comment 182

3 years ago
Created attachment 8409156 [details] [diff] [review]
res_timing_e10s.patch
Attachment #8407950 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a4b034ec82c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f27f91556752
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5679c8bc3fe
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0a4b034ec82c
https://hg.mozilla.org/mozilla-central/rev/f27f91556752
https://hg.mozilla.org/mozilla-central/rev/a5679c8bc3fe
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Blocks: 1002855

Updated

3 years ago
Depends on: 1071527
Blocks: 1089923
Depends on: 1185256
Depends on: 1246956
Depends on: 1254688
You need to log in before you can comment on or make changes to this bug.