Closed Bug 1263722 Opened 8 years ago Closed 6 years ago

There should be a PerformanceEntry for the document load

Categories

(Core :: Networking, defect, P2)

Product:
Core
Component:
Networking
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
relnote-firefox --- 58+
firefox58 --- fixed

People

(Reporter: bzbarsky, Assigned: valentin)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [necko-next])

Attachments

(2 files, 1 obsolete file)

Bug 1263722 - Implement performance navigation timing
58 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1263722 - Add PerformanceNavigationTiming to test_interfaces.js
59 bytes, text/x-review-board-request
baku
: review+
Details 
https://www.w3.org/TR/2016/WD-navigation-timing-2-20160225/#processing-model step 2 creates a PerformanceNavigationTiming with name set to "document" and entryType set to "navigation".  

PerformanceNavigationTiming inherits from PerformanceResourceTiming which inherits from PerformanceEntry, per https://www.w3.org/TR/2016/WD-resource-timing-20160225/#performanceresourcetiming

The spec for Performance.getEntries/getEntriesByName/getEntriesByType at https://www.w3.org/TR/2013/REC-performance-timeline-20131212/#sec-window.performance-attribute talks about returning all PerformanceEntries that satisfy certain criteria.  This means that:

1)  performance.getEntries() is never an empty list, modulo the list-clearing
    stuff: it always contains the PerformanceNavigationTiming for the document
    load.
2)  performance.getEntriesByType("navigation") should return a list that includes
    the PerformanceNavigationTiming for the document load.
3)  performance.getEntriesByName("document") should return a list that includes
    the PerformanceNavigationTiming for the document load.

Looks to me like we don't implement PerformanceNavigationTiming at all...
Flags: needinfo?(valentin.gosu)
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-active]
More of a feedback request than a review. I still need to make sure we follow the spec correctly and add/change tests.
Comment on attachment 8742151 [details]
MozReview Request: Bug 1263722 - Fix includes in unified mode r=bz

https://reviewboard.mozilla.org/r/46955/#review43847

::: dom/base/DOMMatrix.h:19
(Diff revision 1)
>  #include "mozilla/ErrorResult.h"
>  #include "nsCOMPtr.h"
>  #include "mozilla/dom/BindingDeclarations.h"
>  #include "mozilla/dom/TypedArray.h"
>  #include "mozilla/gfx/Matrix.h" // for Matrix4x4
> +#include "mozilla/dom/DOMPoint.h"

This shouldn't be needed.  This header only uses DOMPoint in already_AddRefed return values, and the forward-decl is enough for that.

::: dom/base/PostMessageEvent.h:11
(Diff revision 1)
>  
>  #ifndef mozilla_dom_PostMessageEvent_h
>  #define mozilla_dom_PostMessageEvent_h
>  
>  #include "mozilla/dom/StructuredCloneHolder.h"
> +#include "nsIDocument.h"

Why is this needed, exactly?  I don't think it should be needed here.
Attachment #8742151 - Flags: review?(bzbarsky)
Comment on attachment 8742152 [details]
Bug 1263722 - Implement performance navigation timing

https://reviewboard.mozilla.org/r/46957/#review43849

The lack of any sort of tests in this patch is not encouraging.  There need to be some.

Also, if there is someone else who can review this, please ask them.  I can do it if there is absolutely no one else, I guess...
Attachment #8742152 - Flags: review?(bzbarsky)
Valentin: what's the status with this bug?  Should we keep it necko-active?
Flags: needinfo?(valentin.gosu)
Moving it to [necko-next]. Need to write WPT tests for it.
Flags: needinfo?(valentin.gosu)
Whiteboard: [necko-active] → [necko-next]
Depends on: 1393837
Attachment #8742151 - Attachment is obsolete: true
Comment on attachment 8742152 [details]
Bug 1263722 - Implement performance navigation timing

https://reviewboard.mozilla.org/r/46957/#review178952

::: dom/performance/Performance.h:58
(Diff revision 2)
>                         JS::Handle<JSObject*> aGivenProto) override;
>  
> -  void GetEntries(nsTArray<RefPtr<PerformanceEntry>>& aRetval);
> +  virtual void GetEntries(nsTArray<RefPtr<PerformanceEntry>>& aRetval);
>  
> -  void GetEntriesByType(const nsAString& aEntryType,
> +  virtual void GetEntriesByType(const nsAString& aEntryType,
>                          nsTArray<RefPtr<PerformanceEntry>>& aRetval);

indentation

::: dom/performance/Performance.h:61
(Diff revision 2)
>  
> -  void GetEntriesByType(const nsAString& aEntryType,
> +  virtual void GetEntriesByType(const nsAString& aEntryType,
>                          nsTArray<RefPtr<PerformanceEntry>>& aRetval);
>  
> -  void GetEntriesByName(const nsAString& aName,
> +  virtual void GetEntriesByName(const nsAString& aName,
>                          const Optional<nsAString>& aEntryType,

indentation here as well

::: dom/performance/PerformanceMainThread.h:54
(Diff revision 2)
>  
> +  // The GetEntries* methods need to be overriden in order to add the
> +  // the document entry of type navigation.
> +  virtual void GetEntries(nsTArray<RefPtr<PerformanceEntry>>& aRetval) override;
> +  virtual void GetEntriesByType(const nsAString& aEntryType,
> +                        nsTArray<RefPtr<PerformanceEntry>>& aRetval) override;

can you fix the indentation here and in the following method?

::: dom/performance/PerformanceMainThread.cpp:343
(Diff revision 2)
> +  aRetval.Sort(PerformanceEntryComparator());
> +}
> +
> +void
> +PerformanceMainThread::GetEntriesByType(const nsAString& aEntryType,
> +              nsTArray<RefPtr<PerformanceEntry>>& aRetval)

indentation

::: dom/performance/PerformanceMainThread.cpp:365
(Diff revision 2)
> +  Performance::GetEntriesByType(aEntryType, aRetval);
> +}
> +
> +void
> +PerformanceMainThread::GetEntriesByName(const nsAString& aName,
> +              const Optional<nsAString>& aEntryType,

indentation

::: dom/performance/PerformanceNavigationTiming.h:48
(Diff revision 2)
> +    return 0;
> +  }
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> +
> +  virtual DOMHighResTimeStamp UnloadEventStart();

no virtual for final classes.

::: dom/performance/PerformanceNavigationTiming.h:58
(Diff revision 2)
> +  virtual DOMHighResTimeStamp DomContentLoadedEventEnd();
> +  virtual DOMHighResTimeStamp DomComplete();
> +  virtual DOMHighResTimeStamp LoadEventStart();
> +  virtual DOMHighResTimeStamp LoadEventEnd() const;
> +  virtual NavigationType Type();
> +  virtual uint16_t RedirectCount();

all of them and the DTOR should not be virtual.

::: dom/performance/PerformanceNavigationTiming.cpp:97
(Diff revision 2)
> +uint16_t
> +PerformanceNavigationTiming::RedirectCount()
> +{
> +  return mTiming->GetRedirectCount();
> +}
> +

no extra line here.
Attachment #8742152 - Flags: review?(amarchesini) → review+
Comment on attachment 8907531 [details]
Bug 1263722 - Add PerformanceNavigationTiming to test_interfaces.js

https://reviewboard.mozilla.org/r/179236/#review184360
Attachment #8907531 - Flags: review?(amarchesini) → review+
Thanks for the review. I'll land this in fx58.
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P2
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6827b7bf9116
Implement performance navigation timing r=baku
https://hg.mozilla.org/integration/autoland/rev/24116d4c6b85
Add PerformanceNavigationTiming to test_interfaces.js r=baku
This also triggers these mochitest failures on mac: https://treeherder.mozilla.org/logviewer.html#?job_id=133056823&repo=autoland

> Assertion failure: 0 == rv, at /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:292

Also on shutdown.
I added mDocEntry to the cycle collection.
I was able to reproduce the leak locally, and verified that it went away.
Try also looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd6578f0aa920de6bf04d1c731baa9002aab0e7e
Flags: needinfo?(valentin.gosu)
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/eb45d53b45db
Implement performance navigation timing r=baku
https://hg.mozilla.org/integration/autoland/rev/ab239d8e44ca
Add PerformanceNavigationTiming to test_interfaces.js r=baku
https://hg.mozilla.org/mozilla-central/rev/eb45d53b45db
https://hg.mozilla.org/mozilla-central/rev/ab239d8e44ca
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1403154
Depends on: 1403178
Depends on: 1403926
As luck would have it, one of the Google tech writers had written the docs for this already ;-)

I've updated the all the browser support information, and added a note to the Fx58 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/58#New_APIs

https://developer.mozilla.org/en-US/docs/Web/API/PerformanceNavigationTiming (and child pages)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.