Step 2.4 of Clients.matchAll() says: "For each service worker client client in targetClients, in the most recently focused order for window clients" Here: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-getall We currently return window Client objects in the order that the Window was created. Rather than try to resort the entire set of all documents all the time, we should probably just sort the results we know we are going to return from matchAll().

I'm going to work this in preparation for bug 1293277. I need some of the underlying support in nsGlobalWindow and the tests. The duplicate bits that will get thrown away after bug 1293277 will be very small.

Work-in-progress on the test. This is taking the most time since there is some unexpected focus behavior. For example, frames can't be focused unless an ancestor window is already focused, etc. Anyway, the guts of the test are written. I'll add the specific test cases and expectations tomorrow.

Updated test, but some weird behavior to figure out.

Update the patch to track focus time when the document replaces the about:blank document in an already focused window.

Fix creation order issues for controlled clients.

Hmm. The P4 patch is not updating when I upload it for some reason.

Try build shows the test does not work in non-e10s mode. I can confirm the same locally.

So, I'm seeing different behavior between e10s and non-e10s. Is this ok, or should one of them be considered bugged? Olli, can you help me figure it out? The difference I am seeing is that the window.focus() methods I'm calling in the test are ignored in non-e10s mode, but honored in e10s mode. Tracing the code its because in non-e10s the window is considered hidden and hits the short-circuit: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#7511 So, why are the tabs hidden in non-e10s, but not in e10s? In non-e10s we have a parent frame and fail this condition: https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6155 The frame->IsVisibleConsideringAncestors() is failing here: https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#367 I believe the "deck" selection means the tab is not selected. This is indeed the case in the test. In contrast, e10s has no parent. So we never check the frame deck selection. Instead we check the tree owner: https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6171 I haven't verified this part, but it seems that this code just checks to see if the xul window is visible: https://dxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsContentTreeOwner.cpp#672 This means that if a browser.xul is visible then we consider *all* the tabs within that xul window visible. Olli, does this e10s behavior sound correct? Assuming the non-e10s behavior is more correct, how can I go about testing this kind of API depending on window focus? Is it possible in wpt at all or must I go to browser chrome?

I guess I can change the test to create frames instead of using window.open(). I still wonder about comment 17, though. Should I file a bug in e10s focus()?

Needs some more cleanup, but this works in e10s and non-e10s locally.

Cleaned up test.

Comment on attachment 8841033 [details] [diff] [review] P1 Track the last focus time on the nsIDocument. r=smaug The Clients API requires returning Client objects for windows in "most recently focused" order. See step 2.5 of: https://w3c.github.io/ServiceWorker/#clients-getall This patch is a first step to implement this. It makes us track the last focus time on nsIDocument objects.

Comment on attachment 8840140 [details] [diff] [review] P2 Track last focus time on ServiceWorkerClient. r=smaug This patch then reads the last focus time from the document and stores it on our ServiceWorkerClientInfo.

Comment on attachment 8841034 [details] [diff] [review] P3 Sort output of Clients.matchAll(). r=smaug This patch changes our ServiceWorkerManager::GetAllClients() method to implement the new sorting. We have the focus time now, but we still need the Client creation order. We effectively get this out of the nsIObserverService by virtue of the document calling AddObserver() here: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#4708 I plan to replace this bit soon in bug 1293277 by storing a proper creation time for Clients. Also note that observer service EnumerateObservers() gives us the reverse order because of: https://dxr.mozilla.org/mozilla-central/source/xpcom/ds/nsObserverList.cpp#76 Note, we only support window Client objects at the moment, so we don't need to worry about Worker objects yet.

Comment on attachment 8841763 [details] [diff] [review] P4 Add a WPT test validating Clients.matchAll() result order. a=smaug This adds a WPT test for the Clients.matchAll() list ordering. Note, I could only test it for nested iframes because its really hard to use window.open() and window.focus() in a WPT test.

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #17) > So, I'm seeing different behavior between e10s and non-e10s. Is this ok, or > should one of them be considered bugged? Olli, can you help me figure it > out? > > The difference I am seeing is that the window.focus() methods I'm calling in > the test are ignored in non-e10s mode, but honored in e10s mode. What you mean with "ignored"? > I haven't verified this part, but it seems that this code just checks to see > if the xul window is visible: > > https://dxr.mozilla.org/mozilla-central/source/xpfe/appshell/ > nsContentTreeOwner.cpp#672 That code doesn't run in child process. TabChild::GetVisibility is the relevant bit here. See bug 666365 > > This means that if a browser.xul is visible then we consider *all* the tabs > within that xul window visible. > > Olli, does this e10s behavior sound correct? Assuming the non-e10s behavior > is more correct, I think it does sound. > how can I go about testing this kind of API depending on > window focus? Is it possible in wpt at all or must I go to browser chrome? Hmm, not sure.

Comment on attachment 8841033 [details] [diff] [review] P1 Track the last focus time on the nsIDocument. r=smaug >+ // Update the last focus time on any affected documents >+ if (aWindow && aWindow != mFocusedWindow) { >+ const TimeStamp now(TimeStamp::Now()); >+ for (nsIDocument* doc = aWindow->GetDoc(); GetExtantDoc() >+ doc; >+ doc = doc->GetParentDocument()) { >+ doc->SetLastFocusTime(now); So this means several documents may have the same time stamp. Is that ok per spec? I don't read it that way. I would just update aWindow's timestamp. But the spec is very unclear here. What does "WindowClient objects that have been focused are placed first sorted in the most recently focused order" mean? ServiceWorker spec refers to HTML spec as if WindowClient was some object HTML spec had anything to do with. Elsewhere ServiceWorker spec seems to refer to the relevant browsing context, and if so, the timestamp should be in the outerwindow or docshell, not in document. https://w3c.github.io/ServiceWorker/#dom-windowclient-focus

Comment on attachment 8840140 [details] [diff] [review] P2 Track last focus time on ServiceWorkerClient. r=smaug So, I think you'll need to update this to get mLastFocusTime from outerWindow, assuming outerWindow isn't null, and otherwise set mLastFocusTime to TimeStamp(). That fixed, r+

Comment on attachment 8841034 [details] [diff] [review] P3 Sort output of Clients.matchAll(). r=smaug >+ // The observer service gives us the list in reverse creation order. >+ // We need to maintain creation order, so reverse the list before >+ // processing. >+ docList.Reverse(); This looks a bit scary to rely on observer service implementation. I would have probably added some counter to nsIDocument to tell the service worker registration order But I guess this is fine if we have tests for this.

(In reply to Olli Pettay [:smaug] from comment #27) > >+ doc; > >+ doc = doc->GetParentDocument()) { > >+ doc->SetLastFocusTime(now); > So this means several documents may have the same time stamp. Is that ok per > spec? > I don't read it that way. > I would just update aWindow's timestamp. I have an open spec issue about this: https://github.com/w3c/ServiceWorker/issues/1080 And Jake (spec editor) and I discussed in IRC here: http://logs.glob.uno/?c=freenode%23whatwg&s=23+Feb+2017&e=23+Feb+2017&h=focus#c1021602 > But the spec is very unclear here. What does "WindowClient objects that have > been focused are placed first sorted in the most recently focused order" > mean? ServiceWorker spec refers to HTML spec as if WindowClient was some > object HTML spec had anything to do with. > Elsewhere ServiceWorker spec seems to refer to the relevant browsing > context, and if so, the timestamp should be in the outerwindow or docshell, > not in document. > https://w3c.github.io/ServiceWorker/#dom-windowclient-focus Well, all the focus code is oriented towards documents. It seems like storing it on other entities would be a bit unusual. I think we are trying to conform to what document.hasFocus() does. Also, the client concept corresponds to the environment or environment settings: https://w3c.github.io/ServiceWorker/#service-worker-client-concept Navigating a window or calling document.open() should create a new Client. So I think its better associated with the inner window. My plan in bug 1293277 is to have the inner window's ClientSource pull the focus time from the document when it calls nsIDocument::HasFocus(). It would feel weird to call nsIDocument::HasFocus(), but then get the focus time from some other object that may or may not have been associated with the document when focus happened. Does any of this change your mind?

The spec maps focus handling to browsing context, if to anything, so it is not my mind which needs to change but the spec ;) So at least file a spec bug. Right now it is pretty much unimplementable, plenty of guess-work happening here.

Comment on attachment 8841033 [details] [diff] [review] P1 Track the last focus time on the nsIDocument. r=smaug >+ // Update the last focus time on any affected documents >+ if (aWindow && aWindow != mFocusedWindow) { >+ const TimeStamp now(TimeStamp::Now()); >+ for (nsIDocument* doc = aWindow->GetDoc(); GetExtantDoc() But ok, based on the feedback, perhaps we can take this.

Comment on attachment 8841763 [details] [diff] [review] P4 Add a WPT test validating Clients.matchAll() result order. a=smaug rs+

> The spec maps focus handling to browsing context, if to anything, so it is > not my mind which needs to change but the spec ;) The spec does mention browsing context in various places, but the definition is actually "environment or environment settings object": https://w3c.github.io/ServiceWorker/#dfn-service-worker-client Linking to: https://html.spec.whatwg.org/multipage/webappapis.html#environment https://html.spec.whatwg.org/multipage/webappapis.html#environment-settings-object The spec does get the browsing context from Client environment in a number of places. For example, step 3.1 of: https://w3c.github.io/ServiceWorker/#client-focus Says: "Let browsingContext be the context object’s associated service worker client's global object's browsing context." AFAICT there are only two places the spec mistakenly still calls a Client a browsing context: * Step 10.5 of https://w3c.github.io/ServiceWorker/#service-worker-postmessage * `clients` comment in https://w3c.github.io/ServiceWorker/#serviceworkerglobalscope-interface Filed a spec issue: https://github.com/w3c/ServiceWorker/issues/1083

(In reply to Olli Pettay [:smaug] from comment #29) > This looks a bit scary to rely on observer service implementation. > I would have probably added some counter to nsIDocument to tell the service > worker registration order > But I guess this is fine if we have tests for this. I plan to replace this in the coming weeks with Clients API rewrite in bug 1293277. It didn't seem worth it to add another field on nsIDocument that I would be removing so soon.

Update WPT manifest.

Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d19ad1c1c214 P1 Track the last focus time on the nsIDocument. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/e54c8d21e4c1 P2 Track last focus time on ServiceWorkerClient. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/274999e28c07 P3 Sort output of Clients.matchAll(). r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/b53d88cb7099 P4 Add a WPT test validating Clients.matchAll() result order. a=smaug

Backed out Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b53d88cb7099db7cf46c687c22b28947d0f4ccc5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=80655234&repo=mozilla-inbound [task 2017-02-28T16:21:44.985737Z] 16:21:44 INFO - TEST-START | dom/workers/test/serviceworkers/test_claim.html [task 2017-02-28T16:21:45.494793Z] 16:21:45 INFO - ERROR: claim_worker_2 failed to capture clients. [task 2017-02-28T16:27:11.513784Z] 16:27:11 INFO - TEST-INFO | started process screentopng [task 2017-02-28T16:27:12.154789Z] 16:27:12 INFO - TEST-INFO | screentopng: exit 0 [task 2017-02-28T16:27:12.155204Z] 16:27:12 INFO - Buffered messages logged at 16:21:45 [task 2017-02-28T16:27:12.159517Z] 16:27:12 INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | parent exists. [task 2017-02-28T16:27:12.162596Z] 16:27:12 INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | controller changed event received. [task 2017-02-28T16:27:12.164599Z] 16:27:12 INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Client was claimed and received controllerchange event. [task 2017-02-28T16:27:12.167559Z] 16:27:12 INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Claim should resolve with undefined. [task 2017-02-28T16:27:12.169520Z] 16:27:12 INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Received message from claiming worker. [task 2017-02-28T16:27:12.171471Z] 16:27:12 INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Worker doesn't control any client before claim. [task 2017-02-28T16:27:12.173537Z] 16:27:12 INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Worker should claim 2 clients. [task 2017-02-28T16:27:12.175752Z] 16:27:12 INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Claim should resolve with undefined. [task 2017-02-28T16:27:12.178056Z] 16:27:12 INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Client received message from claiming worker. [task 2017-02-28T16:27:12.181450Z] 16:27:12 INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | MatchAll clients count before claim should be 0 [task 2017-02-28T16:27:12.183393Z] 16:27:12 INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | MatchAll clients count after claim should be 2 [task 2017-02-28T16:27:12.185306Z] 16:27:12 INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Controlling service worker has the correct url. [task 2017-02-28T16:27:12.187335Z] 16:27:12 INFO - TEST-PASS | dom/workers/test/serviceworkers/test_claim.html | Client was claimed and received controllerchange event. [task 2017-02-28T16:27:12.189166Z] 16:27:12 INFO - Buffered messages finished [task 2017-02-28T16:27:12.191486Z] 16:27:12 INFO - TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_claim.html | Test timed out. [task 2017-02-28T16:27:12.193166Z] 16:27:12 INFO - reportError@SimpleTest/TestRunner.js:120:7 [task 2017-02-28T16:27:12.194859Z] 16:27:12 INFO - TestRunner._checkForHangs@SimpleTest/TestRunner.js:141:7 [task 2017-02-28T16:27:12.196556Z] 16:27:12 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5 [task 2017-02-28T16:27:12.198259Z] 16:27:12 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5 [task 2017-02-28T16:27:12.199982Z] 16:27:12 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5 [task 2017-02-28T16:27:12.201902Z] 16:27:12 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5 [task 2017-02-28T16:27:12.203587Z] 16:27:12 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5 [task 2017-02-28T16:27:12.205308Z] 16:27:12 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5 [task 2017-02-28T16:27:12.207178Z] 16:27:12 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5 [task 2017-02-28T16:27:12.208906Z] 16:27:12 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5 [task 2017-02-28T16:27:12.210646Z] 16:27:12 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5 [task 2017-02-28T16:27:12.212393Z] 16:27:12 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5 [task 2017-02-28T16:27:12.214049Z] 16:27:12 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:162:5 [task 2017-02-28T16:27:12.215716Z] 16:27:12 INFO - TestRunner.runTests@SimpleTest/TestRunner.js:379:5 [task 2017-02-28T16:27:12.217290Z] 16:27:12 INFO - RunSet.runtests@SimpleTest/setup.js:190:3 [task 2017-02-28T16:27:12.218920Z] 16:27:12 INFO - RunSet.runall@SimpleTest/setup.js:169:5 [task 2017-02-28T16:27:12.220523Z] 16:27:12 INFO - hookupTests@SimpleTest/setup.js:262:5 [task 2017-02-28T16:27:12.222252Z] 16:27:12 INFO - parseTestManifest@http://mochi.test:8888/manifestLibrary.js:36:5 [task 2017-02-28T16:27:12.224002Z] 16:27:12 INFO - getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:49:11 [task 2017-02-28T16:27:12.225755Z] 16:27:12 INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3 [task 2017-02-28T16:27:12.227550Z] 16:27:12 INFO - hookup@SimpleTest/setup.js:242:5 [task 2017-02-28T16:27:12.229401Z] 16:27:12 INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp:11:1 See also https://treeherder.mozilla.org/logviewer.html#?job_id=80657084&repo=mozilla-inbound dom/workers/test/serviceworkers/test_serviceworker_interfaces.html | Test timed out.

Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee625ebbd40 Backed out changeset b53d88cb7099 https://hg.mozilla.org/integration/mozilla-inbound/rev/d90cb83b58bf Backed out changeset 274999e28c07 https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6fd1b64337 Backed out changeset e54c8d21e4c1 https://hg.mozilla.org/integration/mozilla-inbound/rev/901d34864971 Backed out changeset d19ad1c1c214 for failing dom/workers/test/serviceworkers/test_claim.html. r=backout

So I accidentally lost the logic that excludes Client objects from other registrations. I'll add that back in. It appears, though, that we've never fully implemented step 2.2.3.1 of: https://w3c.github.io/ServiceWorker/#clients-getall We should not only be checking the registration, but verifying the Client is controlled by the exact ServiceWorker instance calling `clients.matchAll()`. I'll file a follow-on bug to address that. I could just fix it, but it needs a test which might take some work.

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #41) > I'll file a follow-on bug to address that. I could just fix it, but it > needs a test which might take some work. Filed bug 1343308.

Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e1c10d908957 P1 Track the last focus time on the nsIDocument. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/f0ef771861c2 P2 Track last focus time on ServiceWorkerClient. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/eda3f225fb73 P3 Sort output of Clients.matchAll(). r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/69a692df37c9 P4 Add a WPT test validating Clients.matchAll() result order. a=smaug

https://hg.mozilla.org/mozilla-central/rev/e1c10d908957 https://hg.mozilla.org/mozilla-central/rev/f0ef771861c2 https://hg.mozilla.org/mozilla-central/rev/eda3f225fb73 https://hg.mozilla.org/mozilla-central/rev/69a692df37c9

I've made sure that this update is covered on the matchAll() ref doc: https://developer.mozilla.org/en-US/docs/Web/API/Clients/matchAll#Return_value I've also added a note to the Fx54 release notes: https://developer.mozilla.org/en-US/Firefox/Releases/54#Web_workers_and_Service_Workers Let me know if that sounds ok. Thanks!