Ensure that Service Worker clients are always returned in MRU order (1)

Ensure that Service Worker clients are always returned in MRU order (1)

Adds an ordering condition to ServiceWorkerVersion.

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-matchall-method

1. (Chromium) This CL.
2. (Blink) https://codereview.chromium.org/1286123004/

BUG=461411
Committed: https://crrev.com/65c5354524fb6ecb696b0fd4e423b1fd19a0a385
Cr-Commit-Position: refs/heads/master@{#344197}

[jeremyarcher, 2015-08-13 04:37:51]

Patchset #1 (id:1) has been deleted

[jeremyarcher, 2015-08-13 04:39:46]

jeremyarcher@google.com changed reviewers:
+ falken@chromium.org, nhiroki@chromium.org

[nhiroki, 2015-08-13 05:56:32]

Looks good overall. Can you add tests?

https://codereview.chromium.org/1285373002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1285373002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:385:
base::TimeTicks::Now(), host_client_type);
Non-window clients seem to always overcome window clients. It could be more
natural to prioritize window clients, that is, passing base::TimeTicks() here.

https://codereview.chromium.org/1285373002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1301:
std::sort(clients);
Can you add comments about why we sort clients here?

[jeremyarcher, 2015-08-14 05:22:06]

I've added a fairly basic layout test to the blink side of things, but I worry
that it will be too flaky. (It does consistently work on my machine, for what
it's worth.)

https://codereview.chromium.org/1285373002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1285373002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:385:
base::TimeTicks::Now(), host_client_type);
On 2015/08/13 05:56:31, nhiroki wrote:
> Non-window clients seem to always overcome window clients. It could be more
> natural to prioritize window clients, that is, passing base::TimeTicks() here.

Fixed.

https://codereview.chromium.org/1285373002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1301:
std::sort(clients);
On 2015/08/13 05:56:31, nhiroki wrote:
> Can you add comments about why we sort clients here?

Fixed.

[nhiroki, 2015-08-17 03:57:56]

https://codereview.chromium.org/1285373002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1285373002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:6: 
nit: "#include <algorithm>" for std::sort.

https://codereview.chromium.org/1285373002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1297: };
Can you move this to unnamed namespace?

https://codereview.chromium.org/1285373002/diff/40001/content/common/service_...
File content/common/service_worker/service_worker_client_info.h (right):

https://codereview.chromium.org/1285373002/diff/40001/content/common/service_...
content/common/service_worker/service_worker_client_info.h:23:
ServiceWorkerClientInfo(const ServiceWorkerClientInfo& a);
Probably this is not necessary (a compiler automatically generates a copy ctor)

[nhiroki, 2015-08-17 03:59:20]

On 2015/08/14 05:22:06, jeremyarcher wrote:
> I've added a fairly basic layout test to the blink side of things, but I worry
> that it will be too flaky. (It does consistently work on my machine, for what
> it's worth.)

Thank you for adding tests. I'll make a comment in that review.

[jeremyarcher, 2015-08-17 04:51:29]

PTAL

https://codereview.chromium.org/1285373002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1285373002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:6: 
On 2015/08/17 03:57:55, nhiroki wrote:
> nit: "#include <algorithm>" for std::sort.

Done.

https://codereview.chromium.org/1285373002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1297: };
On 2015/08/17 03:57:55, nhiroki wrote:
> Can you move this to unnamed namespace?

Done.

https://codereview.chromium.org/1285373002/diff/40001/content/common/service_...
File content/common/service_worker/service_worker_client_info.h (right):

https://codereview.chromium.org/1285373002/diff/40001/content/common/service_...
content/common/service_worker/service_worker_client_info.h:23:
ServiceWorkerClientInfo(const ServiceWorkerClientInfo& a);
On 2015/08/17 03:57:55, nhiroki wrote:
> Probably this is not necessary (a compiler automatically generates a copy
ctor)

Done.

[falken, 2015-08-17 06:28:32]

I think this lgtm, please wait for nhiroki's review and for the tests to be
ready though

https://codereview.chromium.org/1285373002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/1285373002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:456: web_contents
? web_contents->GetLastActiveTime() : base::TimeTicks(),
I'm curious when web_contents would be null? Anyone knows?

[nhiroki, 2015-08-17 06:43:22]

lgtm with a nit

https://codereview.chromium.org/1285373002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1285373002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1300: }
We already have an unnamed namespace starting at line 61. Can you move this
struct there?

[jeremyarcher, 2015-08-17 06:51:57]

Fixed!

I'll wait until LGTM on https://codereview.chromium.org/1286123004/ before
submitting.

https://codereview.chromium.org/1285373002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/1285373002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:456: web_contents
? web_contents->GetLastActiveTime() : base::TimeTicks(),
On 2015/08/17 06:28:32, falken wrote:
> I'm curious when web_contents would be null? Anyone knows?

I'm honestly not sure- the comment below in web_contents.h suggests so, but that
might be less necessary post-split.

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

https://codereview.chromium.org/1285373002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1285373002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1300: }
On 2015/08/17 06:43:22, nhiroki wrote:
> We already have an unnamed namespace starting at line 61. Can you move this
> struct there?

Ah, whoops. Added.

[jeremyarcher, 2015-08-18 08:58:32]

On 2015/08/17 06:51:57, jeremyarcher wrote:
> Fixed!
> 
> I'll wait until LGTM on https://codereview.chromium.org/1286123004/ before
> submitting.
> 
>
https://codereview.chromium.org/1285373002/diff/60001/content/browser/service...
> File content/browser/service_worker/service_worker_provider_host.cc (right):
> 
>
https://codereview.chromium.org/1285373002/diff/60001/content/browser/service...
> content/browser/service_worker/service_worker_provider_host.cc:456:
web_contents
> ? web_contents->GetLastActiveTime() : base::TimeTicks(),
> On 2015/08/17 06:28:32, falken wrote:
> > I'm curious when web_contents would be null? Anyone knows?
> 
> I'm honestly not sure- the comment below in web_contents.h suggests so, but
that
> might be less necessary post-split.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
> 
>
https://codereview.chromium.org/1285373002/diff/60001/content/browser/service...
> File content/browser/service_worker/service_worker_version.cc (right):
> 
>
https://codereview.chromium.org/1285373002/diff/60001/content/browser/service...
> content/browser/service_worker/service_worker_version.cc:1300: }
> On 2015/08/17 06:43:22, nhiroki wrote:
> > We already have an unnamed namespace starting at line 61. Can you move this
> > struct there?
> 
> Ah, whoops. Added.

Switched from activate (when the window is rendered) to focus (when it is
highlighted).
I ended up putting most of the code in RenderFrameHost, rather than WebContents,
because
the latter tracks frames separately. Thoughts?

[nhiroki, 2015-08-19 03:45:50]

Looks good. Let me make one nit comment before stamping this again.

https://codereview.chromium.org/1285373002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/1285373002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_provider_host.cc:442:
render_frame_host->GetRenderWidgetHost();
I wonder if we can call LastFocusTime() w/o exposing it to content API
(content/public) because GetRenderWidgetHost() returns RenderWidgetHostImpl*.

If this is correct, can you ...

(1) discard changes in content/public/browser/render_widget_host.h?
(2) move the definition of RenderWidgetHostImpl::LastFocusTime into
render_widget_host_impl.h and renames the function to last_focus_time()?

[jeremyarcher, 2015-08-19 05:26:46]

On 2015/08/19 03:45:50, nhiroki wrote:
> Looks good. Let me make one nit comment before stamping this again.
> 
>
https://codereview.chromium.org/1285373002/diff/100001/content/browser/servic...
> File content/browser/service_worker/service_worker_provider_host.cc (right):
> 
>
https://codereview.chromium.org/1285373002/diff/100001/content/browser/servic...
> content/browser/service_worker/service_worker_provider_host.cc:442:
> render_frame_host->GetRenderWidgetHost();
> I wonder if we can call LastFocusTime() w/o exposing it to content API
> (content/public) because GetRenderWidgetHost() returns RenderWidgetHostImpl*.
> 
> If this is correct, can you ...
> 
> (1) discard changes in content/public/browser/render_widget_host.h?
> (2) move the definition of RenderWidgetHostImpl::LastFocusTime into
> render_widget_host_impl.h and renames the function to last_focus_time()?

Bah- digging around a bit more, it turns out RenderWidgetHost is set
to nullptr for <iframe>'s. For take three, I've moved the focus handling
logic to the FrameTree, which supports all the scenarios I could think of
(windows, tabs, and <iframes>), at least in mousing around on Linux.

I've also renamed the function to last_focus_time(), and removed the edit
in content/public. PTAL when you get a moment.

[nhiroki, 2015-08-19 07:42:33]

LGTM

https://codereview.chromium.org/1285373002/diff/120001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1285373002/diff/120001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:13: #include
"base/memory/scoped_vector.h"
nit: can you include "base/time/time.h"?

https://codereview.chromium.org/1285373002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/1285373002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:339: 
Please keep this blank line for readability.

https://codereview.chromium.org/1285373002/diff/120001/content/common/service...
File content/common/service_worker/service_worker_client_info.h (right):

https://codereview.chromium.org/1285373002/diff/120001/content/common/service...
content/common/service_worker/service_worker_client_info.h:27: base::TimeTicks
last_active_time,
nit: last_focus_time

https://codereview.chromium.org/1285373002/diff/120001/content/common/service...
content/common/service_worker/service_worker_client_info.h:43: base::TimeTicks
last_active_time;
ditto

[jeremyarcher, 2015-08-19 07:47:03]

On 2015/08/19 07:42:33, nhiroki wrote:
> LGTM
> 
>
https://codereview.chromium.org/1285373002/diff/120001/content/browser/frame_...
> File content/browser/frame_host/frame_tree_node.h (right):
> 
>
https://codereview.chromium.org/1285373002/diff/120001/content/browser/frame_...
> content/browser/frame_host/frame_tree_node.h:13: #include
> "base/memory/scoped_vector.h"
> nit: can you include "base/time/time.h"?
> 
>
https://codereview.chromium.org/1285373002/diff/120001/content/browser/servic...
> File content/browser/service_worker/service_worker_version.cc (left):
> 
>
https://codereview.chromium.org/1285373002/diff/120001/content/browser/servic...
> content/browser/service_worker/service_worker_version.cc:339: 
> Please keep this blank line for readability.
> 
>
https://codereview.chromium.org/1285373002/diff/120001/content/common/service...
> File content/common/service_worker/service_worker_client_info.h (right):
> 
>
https://codereview.chromium.org/1285373002/diff/120001/content/common/service...
> content/common/service_worker/service_worker_client_info.h:27: base::TimeTicks
> last_active_time,
> nit: last_focus_time
> 
>
https://codereview.chromium.org/1285373002/diff/120001/content/common/service...
> content/common/service_worker/service_worker_client_info.h:43: base::TimeTicks
> last_active_time;
> ditto

Fixed!

[jeremyarcher, 2015-08-19 07:48:00]

jeremyarcher@google.com changed reviewers:
+ tkent@chromium.org

[jeremyarcher, 2015-08-19 07:48:13]

On 2015/08/19 07:47:03, jeremyarcher wrote:
> On 2015/08/19 07:42:33, nhiroki wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/1285373002/diff/120001/content/browser/frame_...
> > File content/browser/frame_host/frame_tree_node.h (right):
> > 
> >
>
https://codereview.chromium.org/1285373002/diff/120001/content/browser/frame_...
> > content/browser/frame_host/frame_tree_node.h:13: #include
> > "base/memory/scoped_vector.h"
> > nit: can you include "base/time/time.h"?
> > 
> >
>
https://codereview.chromium.org/1285373002/diff/120001/content/browser/servic...
> > File content/browser/service_worker/service_worker_version.cc (left):
> > 
> >
>
https://codereview.chromium.org/1285373002/diff/120001/content/browser/servic...
> > content/browser/service_worker/service_worker_version.cc:339: 
> > Please keep this blank line for readability.
> > 
> >
>
https://codereview.chromium.org/1285373002/diff/120001/content/common/service...
> > File content/common/service_worker/service_worker_client_info.h (right):
> > 
> >
>
https://codereview.chromium.org/1285373002/diff/120001/content/common/service...
> > content/common/service_worker/service_worker_client_info.h:27:
base::TimeTicks
> > last_active_time,
> > nit: last_focus_time
> > 
> >
>
https://codereview.chromium.org/1285373002/diff/120001/content/common/service...
> > content/common/service_worker/service_worker_client_info.h:43:
base::TimeTicks
> > last_active_time;
> > ditto
> 
> Fixed!

+tkent

[jeremyarcher, 2015-08-19 07:50:56]

jeremyarcher@google.com changed reviewers:
+ kinuko@chromium.org
- tkent@chromium.org

[jeremyarcher, 2015-08-19 07:50:57]

-tkent
+kinuko

Apologies for the noise.

https://codereview.chromium.org/1285373002/diff/120001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1285373002/diff/120001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.h:13: #include
"base/memory/scoped_vector.h"
On 2015/08/19 07:42:33, nhiroki wrote:
> nit: can you include "base/time/time.h"?

Done.

https://codereview.chromium.org/1285373002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/1285373002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:339: 
On 2015/08/19 07:42:33, nhiroki wrote:
> Please keep this blank line for readability.

Done.

https://codereview.chromium.org/1285373002/diff/120001/content/common/service...
File content/common/service_worker/service_worker_client_info.h (right):

https://codereview.chromium.org/1285373002/diff/120001/content/common/service...
content/common/service_worker/service_worker_client_info.h:27: base::TimeTicks
last_active_time,
On 2015/08/19 07:42:33, nhiroki wrote:
> nit: last_focus_time

Done.

https://codereview.chromium.org/1285373002/diff/120001/content/common/service...
content/common/service_worker/service_worker_client_info.h:43: base::TimeTicks
last_active_time;
On 2015/08/19 07:42:33, nhiroki wrote:
> ditto

Done.

[tkent, 2015-08-19 07:52:15]

> 
> +tkent

I have no reason to review this CL.

[kinuko, 2015-08-19 08:46:02]

lgtm

[jeremyarcher, 2015-08-19 08:46:14]

Patchset #7 (id:140001) has been deleted

[jeremyarcher, 2015-08-19 08:48:12]

The CQ bit was checked by jeremyarcher@google.com

[jeremyarcher, 2015-08-19 08:48:12]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
nhiroki@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1285373002/#ps160001
(title: "Fix inconsistencies.")

[commit-bot: I haz the power, 2015-08-19 08:48:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1285373002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1285373002/160001

[commit-bot: I haz the power, 2015-08-19 10:57:13]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-08-19 10:57:52]

Patchset 7 (id:??) landed as
https://crrev.com/65c5354524fb6ecb696b0fd4e423b1fd19a0a385
Cr-Commit-Position: refs/heads/master@{#344197}