Please see the spec [1]. [1] https://w3c.github.io/server-timing/#the-dfn-performanceservertiming-dfn-interface

Summary: - Add PerformanceServerTiming.webidl. - We already have nsIServerTiming interface defined in nsITimedChannel.idl, so this patch basically convert nsIServerTiming to PerformanceServerTiming. Please take a look. Thanks.

Test steps: 1. Create a XHR to get serverTiming.sjs. 2. In serverTiming.sjs, add server-timing data to both response and trailer headers. 3. Check if we can get the data from PerformanceResourceTiming and if the data is correct. Thanks.

Comment on attachment 8941012 [details] [diff] [review] Part1: Implement PerformanceServerTiming Review of attachment 8941012 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/performance/PerformanceResourceTiming.cpp @@ +121,5 @@ > + return; > + } > + > + uint32_t length = 0; > + serverTimingArray->GetLength(&length); This can fail. ::: dom/performance/PerformanceServerTiming.h @@ +20,5 @@ > +class PerformanceServerTiming final : public nsISupports, > + public nsWrapperCache > +{ > +public: > + explicit PerformanceServerTiming(nsISupports* aParent, no explicit with more than 1 param. @@ +31,5 @@ > + > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(PerformanceServerTiming) > + > + virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override; 1. 80chars max. 2. no virtual for final classes

Please take a look. Thanks.

Rebase and carry r+. Thanks for the prompt review!

Carry r+.

Carry r+.

I'm going to fold Part 3 into Part 1 so bisection doesn't become a problem.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/aca79713b901 Part 1: Implement PerformanceServerTiming. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/f7667f5a63af Part 2: Test case. r=baku

Backed out 2 changesets (bug 1423495) for wpt failures at /server-timing/test_server_timing.html r=backout on a CLOSED TREE Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f7667f5a63af3664632c9c175ebb2b07b65bda89 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=155369745&repo=mozilla-inbound&lineNumber=6699 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/48053e1b869e128d3de797dadd12c03062fa1c5a

(In reply to Cristina Coroiu [:ccoroiu] from comment #11) > Backed out 2 changesets (bug 1423495) for wpt failures at > /server-timing/test_server_timing.html r=backout on a CLOSED TREE > > Failure push: > https://treeherder.mozilla.org/#/jobs?repo=mozilla- > inbound&revision=f7667f5a63af3664632c9c175ebb2b07b65bda89 > > Failure log: > https://treeherder.mozilla.org/logviewer.html#?job_id=155369745&repo=mozilla- > inbound&lineNumber=6699 > The failure is about not getting server timing entry from the load of test_server_timing.html [1]. However, according to [2], we don't report resource timing entry for document load and server timing is a part of resource timing. The spec [3] says that "All resources fetched by the current browsing [HTML5] or worker [WORKERS] context's MUST be included as PerformanceResourceTiming objects." It seems that our implementation about resource timing entry is correct. Valentin, since you wrote the code in [2], could you confirm that we really don't have to report resource timing entry for document load? Thanks. [1] https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/testing/web-platform/tests/server-timing/test_server_timing.html.sub.headers [2] https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/netwerk/protocol/http/HttpBaseChannel.cpp#4168-4170 [3] https://w3c.github.io/resource-timing/#resources-included

The document load does not get reported as a PerformanceResourceTiming entry, but as a PerformanceNavigationTiming entry (name="document", entryType: "navigation" - it extends PerformanceResourceTiming). It is the first entry in performance.getEntries(). The entry for the document is constructed here: https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/dom/performance/PerformanceMainThread.cpp#325

Why for this change: I found in test_server_timing.html [1], the test expects to get server timing data from PerformanceNavigationTiming. But, our current code creates PerformanceNavigationTiming quite late until JS code wants to access it. So, we have to create the document entry eariler if we found server timing headers. [1] https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/testing/web-platform/tests/server-timing/test_server_timing.html#13 Thanks.

Comment on attachment 8942132 [details] [diff] [review] Create doc entry from http channel Review of attachment 8942132 [details] [diff] [review]: ----------------------------------------------------------------- r=me provided that this passes all unit tests Also make sure to run attachment 8932225 [details] [diff] [review] locally. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +4170,4 @@ > return nullptr; > } > > + nsCOMPtr<nsIDocument> loadingDocument = innerWindow->GetDoc(); Are you sure this is the same as loadingDocument?

(In reply to Valentin Gosu [:valentin] from comment #15) > Comment on attachment 8942132 [details] [diff] [review] > Create doc entry from http channel > > Review of attachment 8942132 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me provided that this passes all unit tests > Also make sure to run attachment 8932225 [details] [diff] [review] locally. > > ::: netwerk/protocol/http/HttpBaseChannel.cpp > @@ +4170,4 @@ > > return nullptr; > > } > > > > + nsCOMPtr<nsIDocument> loadingDocument = innerWindow->GetDoc(); > > Are you sure this is the same as loadingDocument?

(In reply to Valentin Gosu [:valentin] from comment #15) > Comment on attachment 8942132 [details] [diff] [review] > Create doc entry from http channel > > Review of attachment 8942132 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me provided that this passes all unit tests > Also make sure to run attachment 8932225 [details] [diff] [review] locally. > > ::: netwerk/protocol/http/HttpBaseChannel.cpp > @@ +4170,4 @@ > > return nullptr; > > } > > > > + nsCOMPtr<nsIDocument> loadingDocument = innerWindow->GetDoc(); > > Are you sure this is the same as loadingDocument? Yes, this is not the same as loadingDocument. But, this seems to be the only way to pass test_server_timing.html. It's sad that I am not able to look it deeper for now.

(In reply to Kershaw Chang [:kershaw] from comment #17) > (In reply to Valentin Gosu [:valentin] from comment #15) > > Are you sure this is the same as loadingDocument? > > Yes, this is not the same as loadingDocument. But, this seems to be the only > way to pass test_server_timing.html. I'll take a look at it in the next few days. Thanks a lot for working on this! > It's sad that I am not able to look it deeper for now. :(

The main changes involve: - rebased the patches on the latest m-c - made the test run under HTTPS (since we restricted server timing support to secure origins in bug 1436517) - Made sure to check !TimingAllowed() in PerformanceTimingData::GetServerTiming - added a test to make sure we return an empty list for .serverTiming under plain-text HTTP - Added WPT meta info for expected failures (the tests run under plain-text HTTP at the moment). - The WPT tests reveal no big issues when run manually with HTTPS: https://w3c-test.org/server-timing/ - Fixed a few typos or otherwise minor issues.

Comment on attachment 8962919 [details] Bug 1423495 - Part1: Implement PerformanceServerTiming, https://reviewboard.mozilla.org/r/231726/#review237408 ::: dom/performance/PerformanceResourceTiming.cpp:97 (Diff revision 1) > + if (!serverTimingArray) { > + return; > + } > + > + uint32_t length = 0; > + if (NS_FAILED(serverTimingArray->GetLength(&length))) { NS_WARN_IF ::: dom/performance/PerformanceServerTiming.cpp:34 (Diff revision 1) > + return mozilla::dom::PerformanceServerTimingBinding::Wrap(aCx, this, aGivenProto); > +} > + > +void > +PerformanceServerTiming::GetName(nsAString& aName) const > +{ aName.Truncate() ::: dom/performance/PerformanceServerTiming.cpp:40 (Diff revision 1) > + if (!mServerTiming) { > + return; > + } > + > + nsAutoCString name; > + Unused << mServerTiming->GetName(name); I would do: if (NS_WARN_IF(NS_FAILED(...)) { return; } aName.Assign(...) ::: dom/performance/PerformanceServerTiming.cpp:52 (Diff revision 1) > + if (!mServerTiming) { > + return 0; > + } > + > + double duration = 0; > + Unused << mServerTiming->GetDuration(&duration); same here ::: dom/performance/PerformanceServerTiming.cpp:64 (Diff revision 1) > + if (!mServerTiming) { > + return; > + } > + > + nsAutoCString description; > + Unused << mServerTiming->GetDescription(description); and here

Comment on attachment 8962920 [details] Bug 1423495 - Part2: Test case, https://reviewboard.mozilla.org/r/231728/#review237410 ::: dom/performance/tests/serverTiming.sjs:2 (Diff revision 1) > + > +var respnseServerTiming = [{metric:"metric1", duration:"123.4", description:"description1"}, responseServerTiming ::: dom/performance/tests/serverTiming.sjs:24 (Diff revision 1) > + var body = "c\r\ndata reached\r\n3\r\nhej\r\n0\r\n"; > + > + response.seizePower(); > + response.write("HTTP/1.1 200 OK\r\n"); > + response.write("Content-Type: text/plain\r\n"); > + response.write(createServerTimingHeader(respnseServerTiming)); typo

Comment on attachment 8962921 [details] Bug 1423495 - Part3: Add PerformanceServerTiming to test_interface.js, https://reviewboard.mozilla.org/r/231730/#review237412 ::: dom/tests/mochitest/general/test_interfaces.js:775 (Diff revision 1) > {name: "PerformanceObserverEntryList", insecureContext: true}, > // IMPORTANT: Do not change this list without review from a DOM peer! > {name: "PerformanceResourceTiming", insecureContext: true}, > // IMPORTANT: Do not change this list without review from a DOM peer! > + {name: "PerformanceServerTiming", insecureContext: true}, > +// IMPORTANT: Do not change this list without review from a DOM peer! This interface is exposed to worker as well. You should touch dom/workers/test/*interfaces.js and the same file in dom/serviceworkers/test

Comment on attachment 8962922 [details] Bug 1423495 - Part4: Create doc entry form http channel if server timing headers are found for a document load https://reviewboard.mozilla.org/r/231732/#review237414

Comment on attachment 8962923 [details] Bug 1423495 - Part5: Fix WPT metadata to expect failures https://reviewboard.mozilla.org/r/231734/#review237416 ::: testing/web-platform/meta/server-timing/test_server_timing.html.ini:3 (Diff revision 1) > [test_server_timing.html] > [Untitled] > expected: FAIL What else is missing to make these test pass?

(In reply to Andrea Marchesini [:baku] from comment #29) > Comment on attachment 8962923 [details] > Bug 1423495 - Part5: Fix WPT metadata to expect failures > > [test_server_timing.html] > > [Untitled] > > expected: FAIL > > What else is missing to make these test pass? As it stands these tests can't pass. I filed a bug on the spec https://github.com/w3c/server-timing/issues/54 I intend to change the tests in a followup bug.

Comment on attachment 8964893 [details] Bug 1423495 - Part6: Use threadsafe refcounting for nsServerTiming https://reviewboard.mozilla.org/r/233620/#review239254 I guess it's because necko passes this interface around?

Comment on attachment 8964893 [details] Bug 1423495 - Part6: Use threadsafe refcounting for nsServerTiming https://reviewboard.mozilla.org/r/233620/#review239424

Comment on attachment 8962921 [details] Bug 1423495 - Part3: Add PerformanceServerTiming to test_interface.js, https://reviewboard.mozilla.org/r/231730/#review239530

Valentin, any word here?

I have all the tests passing now. Andrea, do you mind taking another look at Part 3? I also had to add the interface to test_worker_interfaces.js Most of the other changes are in attachment 8964893 [details] - I ended up keeping readonly attribute nsIArray serverTiming - we need it in the xpcshell tests.

All the 2 patches 3 are OK.

Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ecf85551434a Part1: Implement PerformanceServerTiming, r=baku https://hg.mozilla.org/integration/autoland/rev/ca970fc93a32 Part2: Test case, r=baku https://hg.mozilla.org/integration/autoland/rev/d06281128204 Part3: Add PerformanceServerTiming to test_interface.js, r=baku https://hg.mozilla.org/integration/autoland/rev/11804549931f Part4: Create doc entry form http channel if server timing headers are found for a document load r=baku https://hg.mozilla.org/integration/autoland/rev/26e3cf345802 Part5: Fix WPT metadata to expect failures r=baku https://hg.mozilla.org/integration/autoland/rev/fa8202661c16 Part6: Use threadsafe refcounting for nsServerTiming r=baku,nwgh

https://hg.mozilla.org/mozilla-central/rev/ecf85551434a https://hg.mozilla.org/mozilla-central/rev/ca970fc93a32 https://hg.mozilla.org/mozilla-central/rev/d06281128204 https://hg.mozilla.org/mozilla-central/rev/11804549931f https://hg.mozilla.org/mozilla-central/rev/26e3cf345802 https://hg.mozilla.org/mozilla-central/rev/fa8202661c16

Mentioned on Firefox 61 for developers: https://developer.mozilla.org/en-US/Firefox/Releases/61#APIs New reference docs: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Server-Timing https://developer.mozilla.org/en-US/docs/Web/API/PerformanceServerTiming https://developer.mozilla.org/en-US/docs/Web/API/PerformanceServerTiming/description https://developer.mozilla.org/en-US/docs/Web/API/PerformanceServerTiming/duration https://developer.mozilla.org/en-US/docs/Web/API/PerformanceServerTiming/name https://developer.mozilla.org/en-US/docs/Web/API/PerformanceResourceTiming/serverTiming Compat data updates: https://github.com/mdn/browser-compat-data/pull/2147 https://github.com/mdn/browser-compat-data/pull/2148 Let me know if this looks good to you.