Closed Bug 1271487 Opened 7 years ago Closed 7 years ago

Enable Performance Observer in nightly by default

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed
relnote-firefox --- -

People

(Reporter: hiro, Assigned: hiro)

References

Details

(Keywords: dev-doc-complete, Whiteboard: btpp-active)

Attachments

(1 file)

MozReview Request: Bug 1271487 - Enable PerformanceObserver API in nightly by default. r?baku
58 bytes, text/x-review-board-request
baku
: review+
Details 
      No description provided.
Forgot to modify test_interfaces.html and test_serviceworker_interfaces.js.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=431242e9f441
Forgot also PerformanceObserverEntryList.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12a763300593
Have you reviewed the spec recently? It has got, at least based on the date, some updates recently.
And please send Intent to Ship message to dev.platform
Comment on attachment 8750636 [details]
MozReview Request: Bug 1271487 - Enable PerformanceObserver API in nightly by default. r?baku

Given that baku reviewed the implementation, maybe he could review this too.
Attachment #8750636 - Flags: review?(bugs) → review?(amarchesini)
Assignee: nobody → hiikezoe
Whiteboard: btpp-active
(In reply to Olli Pettay [:smaug] from comment #5)
> Have you reviewed the spec recently? It has got, at least based on the date,
> some updates recently.

Yes, I checked it yesterday and checked it again today.  Yesterday I did not notice but today I remembered that Frame Timing API has been changed.  We should modify or drop them at[1].  I've open bug 1271846 for it.
 
[1] https://dxr.mozilla.org/mozilla-central/rev/043082cb7bd8490c60815f67fbd1f33323ad7663/dom/base/PerformanceObserver.cpp#139
So after landing bug 1271846, could we enable PerformanceObserver? bug 1271846 is after all about not exposing wrong kind of data via the observer, and nothing says Frame Timing needs to be implemented before PerformanceObserver.
Comment on attachment 8750636 [details]
MozReview Request: Bug 1271487 - Enable PerformanceObserver API in nightly by default. r?baku

https://reviewboard.mozilla.org/r/51571/#review50264

But also fix the URL https://w3c.github.io/performance-timeline/#the-performanceobserver-interface in dom/webidl/PerformanceObserver.webidl
Attachment #8750636 - Flags: review?(amarchesini) → review+
(In reply to Olli Pettay [:smaug] from comment #9)
> So after landing bug 1271846, could we enable PerformanceObserver? bug
> 1271846 is after all about not exposing wrong kind of data via the observer,
> and nothing says Frame Timing needs to be implemented before
> PerformanceObserver.

Yes, I think so.
Comment on attachment 8750636 [details]
MozReview Request: Bug 1271487 - Enable PerformanceObserver API in nightly by default. r?baku

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51571/diff/1-2/
Attachment #8750636 - Flags: review?(bugs)
Comment on attachment 8750636 [details]
MozReview Request: Bug 1271487 - Enable PerformanceObserver API in nightly by default. r?baku

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51571/diff/2-3/
Attachment #8750636 - Attachment description: MozReview Request: Bug 1271487 - Enable PerformanceObserver API in nightly by default. r?smaug → MozReview Request: Bug 1271487 - Enable PerformanceObserver API in nightly by default. r?baku
Attachment #8750636 - Flags: review?(bugs)
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c88f53416291 - looks like you're going to have to do some work on web-platform-tests expectations before you can do that, https://treeherder.mozilla.org/logviewer.html#?job_id=28492955&repo=mozilla-inbound
Ah, I have not noticed the web platform test has been merged in our tree after I had a try in comment 3.
I am sorry for taking your time.
Comment on attachment 8750636 [details]
MozReview Request: Bug 1271487 - Enable PerformanceObserver API in nightly by default. r?baku

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51571/diff/3-4/
Andrea, could you please take a look this again?

There were two problem;

a) We don't integrate Navigation Timing into Performance Observer.
 Though I did not notice that Chrome does not integrate it either.  I dropped 'navigation' from valid types.
b) Timeout occured on 'An observer disconnected after a mark must receive the mark' test
 I guess it's a spec bug.  I would like to handle it apart from this bug.

Thank you,
Do we have a bug open to integrate navigation timing into performance observer?
Filed bug 1275235.
Depends on: 1276490
Comment on attachment 8750636 [details]
MozReview Request: Bug 1271487 - Enable PerformanceObserver API in nightly by default. r?baku

There was an intermittent failure on a try[1], but it will be fixed by bug 1276490.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=21421337&repo=try#L15163

Hi, Andrea, can you please review the part of changes against web-platform-tests?

A try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=657984a2903a
Attachment #8750636 - Flags: review+ → review?(amarchesini)
Attachment #8750636 - Flags: review?(amarchesini) → review+
Comment on attachment 8750636 [details]
MozReview Request: Bug 1271487 - Enable PerformanceObserver API in nightly by default. r?baku

https://reviewboard.mozilla.org/r/51571/#review52848
https://hg.mozilla.org/mozilla-central/rev/cb8a9f6e77e8
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Release Note Request (optional, but appreciated)
[Why is this notable]: New feature
[Suggested wording]: [https://w3c.github.io/performance-timeline/#the-performanceobserver-interface PerformanceObserver] enabled
[Links (documentation, blog post, etc)]: https://w3c.github.io/performance-timeline/#the-performanceobserver-interface
relnote-firefox: --- → ?
I'm following WebPerf WG f2f remotely. Sounds like PerformanceObserver may get some changes to its behavior at least.
Added to https://developer.mozilla.org/en-US/Firefox/Releases/49#Others
Not sure it is worth adding the product release notes.
Ping me if you disagree
relnote-firefox: ? → -
smaug, is this something we could let ride the trains in its current form?  I'd like to use some [code that depends on having PerformanceObserver](https://github.com/GoogleChrome/tti-polyfill) sooner rather than later, since we don't have built tti support just yet...
Flags: needinfo?(bugs)
At least need to change the prefs and test_interfaces.
And read the spec again. hiro, any opinion on.

dmose, could you file a bug to enable the API outside Nightly too.
Flags: needinfo?(hikezoe)
Flags: needinfo?(dmose)
Flags: needinfo?(bugs)
Done.
Flags: needinfo?(dmose)
As far as I can tell there is still an interoperability issue.  For this issue, we have still failure cases[1] in web platform tests.

The spec author agreed to drop the test case once [2], but it seems to defer to Level 3 spec [3].
I don't know what chrome currently does for the test case, but if chrome still passes the test cases, I think we should match the behavior to chrome before shipping.

[1] https://hg.mozilla.org/mozilla-central/file/44121dbcac6a/testing/web-platform/meta/performance-timeline/po-disconnect.any.js.ini
[2] https://github.com/w3c/performance-timeline/issues/66#issuecomment-282396844
[3] https://github.com/w3c/performance-timeline/issues/66#issuecomment-287438989
Flags: needinfo?(hikezoe)
I've pinged the Chrome bug for this.[1] Hopefully we can get a commitment to fixing Chrome to match the spec so we can update the web-platform-test and ship this.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=607324
(In reply to Brian Birtles (:birtles) from comment #33)
> I've pinged the Chrome bug for this.[1] Hopefully we can get a commitment to
> fixing Chrome to match the spec so we can update the web-platform-test and
> ship this.

The Chrome implementation has been fixed now.

Curiously, they now appear to pass po-disconnect.any.html.[1] I'm not sure why.

Hiro, I guess we just need to submit a PR to fix po-disconnect.any.html and po-disconnect.any.worker.html (or, probably better still, do it in m-c and upstream it). And send an Intent to ship.

Is there anything else we need to do before enabling this?

Further discussion should probably move to bug 1386021 I suppose.

[1] https://chromium.googlesource.com/chromium/src.git/+/7df7884ee789a33cd850c04d6d462aaacb6df9f0%5E%21/#F0
Flags: needinfo?(hikezoe)
(In reply to Brian Birtles (:birtles) from comment #34)
> (In reply to Brian Birtles (:birtles) from comment #33)
> > I've pinged the Chrome bug for this.[1] Hopefully we can get a commitment to
> > fixing Chrome to match the spec so we can update the web-platform-test and
> > ship this.
> 
> The Chrome implementation has been fixed now.
> 
> Curiously, they now appear to pass po-disconnect.any.html.[1] I'm not sure
> why.
> 
> Hiro, I guess we just need to submit a PR to fix po-disconnect.any.html and
> po-disconnect.any.worker.html (or, probably better still, do it in m-c and
> upstream it).

As far as I read the latest test in m-c, it seems to me that the test has been already fixed. And surprisingly it already runs on our CI. From an autoland commit [1];

[task 2017-09-01T05:23:16.102673Z] 05:23:16     INFO - TEST-START | /performance-timeline/po-disconnect.any.html
[task 2017-09-01T05:23:16.191654Z] 05:23:16     INFO - PID 7590 | 1504243396185	Marionette	DEBUG	Register listener.js for window 2147483671
[task 2017-09-01T05:23:18.323101Z] 05:23:18     INFO - ...
[task 2017-09-01T05:23:18.323445Z] 05:23:18     INFO - TEST-OK | /performance-timeline/po-disconnect.any.html | took 2221ms
[task 2017-09-01T05:23:18.324349Z] 05:23:18     INFO - TEST-START | /performance-timeline/po-disconnect.any.worker.html
[task 2017-09-01T05:23:18.393024Z] 05:23:18     INFO - PID 7590 | 1504243398391	Marionette	DEBUG	Register listener.js for window 2147483674
[task 2017-09-01T05:23:20.597117Z] 05:23:20     INFO - ...
[task 2017-09-01T05:23:20.597308Z] 05:23:20     INFO - TEST-OK | /performance-timeline/po-disconnect.any.worker.html | took 2271ms

Our annotation scheme for these files seems to be broken.  Also, as per the log for the autoland commit, there seems to be other issue. (e.g. po-callback-mutate.any.html is timed out).  We need to check them. Anyway po-disconnect should work fine now. I did push a try. Keeping ni to me.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b7f7d82a2e01b71a96953cd4f0029cc5e9a1bf2

[1] https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=127647775
I've opened four bugs for remaining issues that cause some failures in wpt. All bugs block bug 1386021.
Flags: needinfo?(hikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> I've opened four bugs for remaining issues that cause some failures in wpt.
> All bugs block bug 1386021.

*And* you submitted patches for all of them! Thank you!
Depends on: 1398477
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.