Enable Performance Observer in nightly by default

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
a month ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

({dev-doc-complete})

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed, relnote-firefox -)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Comment 2

3 years ago
Forgot to modify test_interfaces.html and test_serviceworker_interfaces.js.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=431242e9f441
(Assignee)

Comment 3

3 years ago
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
Depends on: 1165796
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
(Assignee)

Comment 8

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

Comment 11

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

Comment 12

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

Comment 13

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

Comment 16

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

Comment 17

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

Comment 18

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

Comment 20

3 years ago
Filed bug 1275235.
(Assignee)

Updated

3 years ago
Depends on: 1276490
(Assignee)

Comment 21

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

Comment 24

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cb8a9f6e77e8
Status: NEW → RESOLVED
Last Resolved: 3 years ago
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
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)
(Assignee)

Comment 32

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

Comment 35

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

Comment 36

2 years ago
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!

Updated

2 years ago
Depends on: 1398477
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.