Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2905593002:
Clients.matchAll() should return ordered clients by type/focus time/creation time (Closed)
|
Created:
3 years, 7 months ago by xiaofengzhang Modified:
3 years, 6 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, jam, nhiroki, kinuko+serviceworker, horo+watch_chromium.org, blink-reviews, darin-cc_chromium.org, kinuko+watch, tzik, blink-worker-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionClients.matchAll() should return ordered clients by type/focus time/creation time
According to current service worker specification, sort matchedClients such that:
1) WindowClient objects are always placed before worker clients.
2) WindowClient objects that have been focused are placed first sorted in the
most recently focused order, and WindowClient objects that have never been
focused are placed in their creation order.
3) WorkerClients are placed in their creation order.
Current implementation only considers the focus time.
BUG=701233
Review-Url: https://codereview.chromium.org/2905593002
Cr-Commit-Position: refs/heads/master@{#476935}
Committed: https://chromium.googlesource.com/chromium/src/+/5c3497c94f604dee3cfe64ca84a7828a7b9bec04
Patch Set 1 #
Total comments: 3
Patch Set 2 : Totally follow the matchAll specification #
Total comments: 11
Patch Set 3 : Address shimazu's comments #16 #
Total comments: 25
Patch Set 4 : Address shimazu's comments #24 and leon's #21 #
Total comments: 8
Patch Set 5 : Address shimazu's comments #31 #Patch Set 6 : for test failure #Patch Set 7 : Address shimazu's comments #31 and fix the test failure #Messages
Total messages: 58 (37 generated)
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2905593002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2905593002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_client_utils.cc:372: // Find the subgroud that ever been focused subgroud --> subgroup Find the subgroud that ever been focused -- > Find the subgroup that had ever been focused
https://codereview.chromium.org/2905593002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2905593002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_client_utils.cc:317: base::TimeTicks(), host_client_type); Use host->create_time() ? https://codereview.chromium.org/2905593002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_client_utils.cc:513: base::TimeTicks(), base::TimeTicks(), provider_host->client_type()); Use provider_host->create_time() ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Clients.matchAll() should return non-focused controlled windows in creation order part#1 According to current service worker specification, window client objects that have never been focused should be placed in their creation order, but Chromium didn't honoring that algorithm, and only concerns the "last focused time". This patch is the first one to fix the bug. With it, one failed test case can pass now, some others not yet. But I am wondering the other failed cases may not related with the matchAll algorithm. BUG=701233 ========== to ========== The implementation of clients.matchAll() should follow the specification According to current service worker specification, sort matchedClients such that: 1) WindowClient objects are always placed before worker clients. 2) WindowClient objects that have been focused are placed first sorted in the most recently focused order, and WindowClient objects that have never been focused are placed in their creation order. 3) WorkerClients are placed in their creation order. Current implementation only considers the focus time. BUG=701233 ==========
xiaofeng.zhang@intel.com changed reviewers: + shimazu@chromium.org
Hi shimazu, PTAL, thanks :-)
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== The implementation of clients.matchAll() should follow the specification According to current service worker specification, sort matchedClients such that: 1) WindowClient objects are always placed before worker clients. 2) WindowClient objects that have been focused are placed first sorted in the most recently focused order, and WindowClient objects that have never been focused are placed in their creation order. 3) WorkerClients are placed in their creation order. Current implementation only considers the focus time. BUG=701233 ========== to ========== The implementation of clients.matchAll() should follow the specification According to current service worker specification, sort matchedClients such that: 1) WindowClient objects are always placed before worker clients. 2) WindowClient objects that have been focused are placed first sorted in the most recently focused order, and WindowClient objects that have never been focused are placed in their creation order. 3) WorkerClients are placed in their creation order. Current implementation only considers the focus time. BUG=701233 ==========
Description was changed from ========== The implementation of clients.matchAll() should follow the specification According to current service worker specification, sort matchedClients such that: 1) WindowClient objects are always placed before worker clients. 2) WindowClient objects that have been focused are placed first sorted in the most recently focused order, and WindowClient objects that have never been focused are placed in their creation order. 3) WorkerClients are placed in their creation order. Current implementation only considers the focus time. BUG=701233 ========== to ========== Clients.matchAll() should return ordered clients by type/focus time/creation time According to current service worker specification, sort matchedClients such that: 1) WindowClient objects are always placed before worker clients. 2) WindowClient objects that have been focused are placed first sorted in the most recently focused order, and WindowClient objects that have never been focused are placed in their creation order. 3) WorkerClients are placed in their creation order. Current implementation only considers the focus time. BUG=701233 ==========
Thanks for working on this:) For the nested iframe test, I'm guessing the problem is coming from different |last_focus_time_|s which are owned by FrameTreeNode. It's directly set by TimeTicks::Now() in FrameTreeNode::DidFocus individually. https://cs.chromium.org/chromium/src/content/browser/frame_host/frame_tree_no... https://codereview.chromium.org/2905593002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2905593002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:329: std::unique_ptr<ServiceWorkerClients> clients(new ServiceWorkerClients); Could you use base::MakeUnique<ServiceWorkerClients>() instead? https://codereview.chromium.org/2905593002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:376: ServiceWorkerClients* clients) { Could you change the type to std::unique_ptr<ServiceWorkerClients> and use std::move() to passing the argument? https://codereview.chromium.org/2905593002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:381: clients->begin(), clients->end(), [](ServiceWorkerClientInfo info) { I feel it's more simple to implement operator< in ServiceWorkerClientInfo. WDYT? https://codereview.chromium.org/2905593002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:544: ServiceWorkerClients clients; How about creating std::unique_ptr<ServiceWorkerClients> here and pass it by std::move() in the both paths: - GetWindowClients -> DidGetWindowClients -> DidGetClients - DidGetClients
And do you think we should modify the clients-matchall-order.https-expected.txt in other platforms like ./third_party/WebKit/LayoutTests/platform/win7/, etc.. https://codereview.chromium.org/2905593002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2905593002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:329: std::unique_ptr<ServiceWorkerClients> clients(new ServiceWorkerClients); On 2017/05/25 08:56:26, shimazu wrote: > Could you use base::MakeUnique<ServiceWorkerClients>() instead? Done. https://codereview.chromium.org/2905593002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:359: }; Sorry, this part is not needed https://codereview.chromium.org/2905593002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:376: ServiceWorkerClients* clients) { On 2017/05/25 08:56:26, shimazu wrote: > Could you change the type to std::unique_ptr<ServiceWorkerClients> and use > std::move() to passing the argument? Done. https://codereview.chromium.org/2905593002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:381: clients->begin(), clients->end(), [](ServiceWorkerClientInfo info) { On 2017/05/25 08:56:26, shimazu wrote: > I feel it's more simple to implement operator< in ServiceWorkerClientInfo. > WDYT? Thanks shimazu. please see the cl#2. The reasons I keep it here instead of ServiceWorkerClientInfo is that I think ServiceWorkerClientInfo is only for holding the information of service worker client, and std::sort() maybe not clear enough without a comparison function. WDYT? https://codereview.chromium.org/2905593002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:544: ServiceWorkerClients clients; On 2017/05/25 08:56:26, shimazu wrote: > How about creating std::unique_ptr<ServiceWorkerClients> here and pass it by > std::move() in the both paths: > - GetWindowClients -> DidGetWindowClients -> DidGetClients > - DidGetClients > Done.
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
leon.han@intel.com changed reviewers: + leon.han@intel.com
https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:357: if (a.last_focus_time != b.last_focus_time) I suggest we keep the comparison algorithm strictly with step 2.5 described at spec: https://w3c.github.io/ServiceWorker/#dom-clients-matchall Client type is the first priority, then focus time, then creation time. Thus it'd be clearer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> And do you think we should modify the clients-matchall-order.https-expected.txt in other platforms like ./third_party/WebKit/LayoutTests/platform/win7/, etc.. Yes, I think so. I also think we might be able to remove them because the behavior should not depend on platforms. Could you try to remove them? https://codereview.chromium.org/2905593002/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2905593002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:359: }; On 2017/05/26 04:01:28, xiaofengzhang wrote: > Sorry, this part is not needed Acknowledged. https://codereview.chromium.org/2905593002/diff/20001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:381: clients->begin(), clients->end(), [](ServiceWorkerClientInfo info) { On 2017/05/26 04:01:27, xiaofengzhang wrote: > On 2017/05/25 08:56:26, shimazu wrote: > > I feel it's more simple to implement operator< in ServiceWorkerClientInfo. > > WDYT? > > Thanks shimazu. please see the cl#2. > The reasons I keep it here instead of ServiceWorkerClientInfo is that I think > ServiceWorkerClientInfo is only for holding the information of service worker > client, and std::sort() maybe not clear enough without a comparison function. > WDYT? It looks good, thanks! https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:373: callback.Run(clients.get()); Could you change the argument to use std::unique_ptr? https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:378: ServiceWorkerClients* clients) { std::unique_ptr<ServiceWorkerClients> https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:389: } DidGetClients() https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:398: GetNonWindowClients(controller, options, clients.get()); Please move clients to GetNonWindowClients and add a return. https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:404: const ClientsCallback& callback) { Please add an argument like std::unique_ptr<ServiceWorkerClients> clients https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:424: base::MakeUnique<ServiceWorkerClients>()); Pass |clients| to DidGetWindowClients https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:431: &OnGetWindowClientsOnUI, clients_info, controller->script_url(), Pass |clients| to OnGetWindowClientsOnUI https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:513: std::unique_ptr<ServiceWorkerClients> clients; How about creating the instance of clients here and passing it around? https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:526: GetNonWindowClients(controller, options, clients.get()); Passing clients and calling DidGetClients at the end of GetNonWindowClients
https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:357: if (a.last_focus_time != b.last_focus_time) On 2017/05/26 05:48:38, leonhsl wrote: > I suggest we keep the comparison algorithm strictly with step 2.5 described at > spec: https://w3c.github.io/ServiceWorker/#dom-clients-matchall > Client type is the first priority, then focus time, then creation time. Thus > it'd be clearer. Acknowledged. I am working on the test cases failure, it should be caused by this function. https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:373: callback.Run(clients.get()); On 2017/05/26 08:10:07, shimazu wrote: > Could you change the argument to use std::unique_ptr? Acknowledged. https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:378: ServiceWorkerClients* clients) { On 2017/05/26 08:10:07, shimazu wrote: > std::unique_ptr<ServiceWorkerClients> Acknowledged. https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:398: GetNonWindowClients(controller, options, clients.get()); On 2017/05/26 08:10:07, shimazu wrote: > Please move clients to GetNonWindowClients and add a return. I am thinking if we move DidGetClients into GetNonWindowClients, it seems that the code here can't deal with case of client_type != kWebServiceWorkerClientWindow, which also needs DidGetClients? https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:513: std::unique_ptr<ServiceWorkerClients> clients; On 2017/05/26 08:10:07, shimazu wrote: > How about creating the instance of clients here and passing it around? Acknowledged.
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi shimazu, please help to review again, thanks :-) https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:357: if (a.last_focus_time != b.last_focus_time) On 2017/05/26 13:57:48, xiaofengzhang wrote: > On 2017/05/26 05:48:38, leonhsl wrote: > > I suggest we keep the comparison algorithm strictly with step 2.5 described at > > spec: https://w3c.github.io/ServiceWorker/#dom-clients-matchall > > Client type is the first priority, then focus time, then creation time. Thus > > it'd be clearer. > > Acknowledged. I am working on the test cases failure, it should be caused by > this function. Done. It seems the test cases' failure is because of null ptr at line 513 instead of here. https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:373: callback.Run(clients.get()); On 2017/05/26 08:10:07, shimazu wrote: > Could you change the argument to use std::unique_ptr? Done. https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:378: ServiceWorkerClients* clients) { On 2017/05/26 13:57:48, xiaofengzhang wrote: > On 2017/05/26 08:10:07, shimazu wrote: > > std::unique_ptr<ServiceWorkerClients> > > Acknowledged. Done. https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:389: } On 2017/05/26 08:10:07, shimazu wrote: > DidGetClients() Done. https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:398: GetNonWindowClients(controller, options, clients.get()); On 2017/05/26 08:10:07, shimazu wrote: > Please move clients to GetNonWindowClients and add a return. Done. https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:404: const ClientsCallback& callback) { On 2017/05/26 08:10:07, shimazu wrote: > Please add an argument like > std::unique_ptr<ServiceWorkerClients> clients Done. https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:424: base::MakeUnique<ServiceWorkerClients>()); On 2017/05/26 08:10:07, shimazu wrote: > Pass |clients| to DidGetWindowClients Done. https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:431: &OnGetWindowClientsOnUI, clients_info, controller->script_url(), On 2017/05/26 08:10:07, shimazu wrote: > Pass |clients| to OnGetWindowClientsOnUI Done. https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:513: std::unique_ptr<ServiceWorkerClients> clients; On 2017/05/26 08:10:07, shimazu wrote: > How about creating the instance of clients here and passing it around? Done. https://codereview.chromium.org/2905593002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:526: GetNonWindowClients(controller, options, clients.get()); On 2017/05/26 08:10:07, shimazu wrote: > Passing clients and calling DidGetClients at the end of GetNonWindowClients Done.
Thanks for updating! The patch looks mostly good. Let me add only a few comments. Also could you remove the relevant lines from third_party/LayoutTests/TestExpectations? https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:360: return a.client_type < b.client_type; I feel numerical comparison of enum is a bit scarey. How about comparison like the following? === // Clients for windows should be appeared earlier. if (a.client_type == blink::kWebServiceWorkerClientTypeWindow && b.client_type != blink::kWebServiceWorkerClientTypeWindow) { return true; } if (a.client_type != blink::kWebServiceWorkerClientTypeWindow && b.client_type == blink::kWebServiceWorkerClientTypeWindow) { return false; } // Clients focused recently should be appeared earlier. if (a.last_focus_time != b.last_forcus_time) return a.last_focus_time > b.last_focus_time; // Clients created before should be appeared earlier. return a.create_time <= b.create_time; https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:364: return a.create_time < b.create_time; I think it's better to use '<=' to preserve the order if all of parameters are the same. https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:417: for (auto& controllee : controller->controllee_map()) { Brackets are not necessary here.
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:110001) has been deleted
Patchset #6 (id:130001) has been deleted
The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #7 (id:170001) has been deleted
Patchset #7 (id:190001) has been deleted
Hi shimazu, please help review patch set #7, thanks. https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:360: return a.client_type < b.client_type; On 2017/05/29 01:45:13, shimazu wrote: > I feel numerical comparison of enum is a bit scarey. > > How about comparison like the following? > > === > > // Clients for windows should be appeared earlier. > if (a.client_type == blink::kWebServiceWorkerClientTypeWindow && > b.client_type != blink::kWebServiceWorkerClientTypeWindow) { > return true; > } > if (a.client_type != blink::kWebServiceWorkerClientTypeWindow && > b.client_type == blink::kWebServiceWorkerClientTypeWindow) { > return false; > } > > // Clients focused recently should be appeared earlier. > if (a.last_focus_time != b.last_forcus_time) > return a.last_focus_time > b.last_focus_time; > > // Clients created before should be appeared earlier. > return a.create_time <= b.create_time; Done. Thanks a lot :-) https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:364: return a.create_time < b.create_time; On 2017/05/29 01:45:13, shimazu wrote: > I think it's better to use '<=' to preserve the order if all of parameters are > the same. It seems "<=" will cause some test failure. https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:417: for (auto& controllee : controller->controllee_map()) { On 2017/05/29 01:45:13, shimazu wrote: > Brackets are not necessary here. Done. aha, I didn't want to add the brackets here, I don't remember why I did it, :-(
Thanks! Let me confirm the test failure just in case... I expect the change is possible to change some expected.txt because the order might be changed, but I didn't think it cause crashes. https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:364: return a.create_time < b.create_time; On 2017/06/01 01:00:42, xiaofengzhang wrote: > On 2017/05/29 01:45:13, shimazu wrote: > > I think it's better to use '<=' to preserve the order if all of parameters are > > the same. > > It seems "<=" will cause some test failure. Hmm... I didn't think this change cause crashes. Could you explain why this cause the failure? I'm worried that the test or somewhere might be broken.
On 2017/06/01 09:50:49, shimazu wrote: > Thanks! Let me confirm the test failure just in case... > I expect the change is possible to change some expected.txt because the order > might be changed, but I didn't think it cause crashes. > > https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... > File content/browser/service_worker/service_worker_client_utils.cc (right): > > https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... > content/browser/service_worker/service_worker_client_utils.cc:364: return > a.create_time < b.create_time; > On 2017/06/01 01:00:42, xiaofengzhang wrote: > > On 2017/05/29 01:45:13, shimazu wrote: > > > I think it's better to use '<=' to preserve the order if all of parameters > are > > > the same. > > > > It seems "<=" will cause some test failure. > > Hmm... I didn't think this change cause crashes. Could you explain why this > cause the failure? I'm worried that the test or somewhere might be broken. Actually I don't know the reason. I just tried several patches and each of them has minimal change compare with patch set 4th. And then found "<=" always cause two test failure(please see which two in patch set 5 test result).
On 2017/06/02 01:39:13, xiaofengzhang wrote: > On 2017/06/01 09:50:49, shimazu wrote: > > Thanks! Let me confirm the test failure just in case... > > I expect the change is possible to change some expected.txt because the order > > might be changed, but I didn't think it cause crashes. > > > > > https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... > > File content/browser/service_worker/service_worker_client_utils.cc (right): > > > > > https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... > > content/browser/service_worker/service_worker_client_utils.cc:364: return > > a.create_time < b.create_time; > > On 2017/06/01 01:00:42, xiaofengzhang wrote: > > > On 2017/05/29 01:45:13, shimazu wrote: > > > > I think it's better to use '<=' to preserve the order if all of parameters > > are > > > > the same. > > > > > > It seems "<=" will cause some test failure. > > > > Hmm... I didn't think this change cause crashes. Could you explain why this > > cause the failure? I'm worried that the test or somewhere might be broken. > > Actually I don't know the reason. I just tried several patches and each of them > has minimal change compare with patch set 4th. And then found "<=" always cause > two test failure(please see which two in patch set 5 test result). For the failure log, I can find any hint. But I tried several times CQ test with "<=", the failure logs are the same.
On 2017/06/02 01:41:10, xiaofengzhang wrote: > On 2017/06/02 01:39:13, xiaofengzhang wrote: > > On 2017/06/01 09:50:49, shimazu wrote: > > > Thanks! Let me confirm the test failure just in case... > > > I expect the change is possible to change some expected.txt because the > order > > > might be changed, but I didn't think it cause crashes. > > > > > > > > > https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... > > > File content/browser/service_worker/service_worker_client_utils.cc (right): > > > > > > > > > https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... > > > content/browser/service_worker/service_worker_client_utils.cc:364: return > > > a.create_time < b.create_time; > > > On 2017/06/01 01:00:42, xiaofengzhang wrote: > > > > On 2017/05/29 01:45:13, shimazu wrote: > > > > > I think it's better to use '<=' to preserve the order if all of > parameters > > > are > > > > > the same. > > > > > > > > It seems "<=" will cause some test failure. > > > > > > Hmm... I didn't think this change cause crashes. Could you explain why this > > > cause the failure? I'm worried that the test or somewhere might be broken. > > > > Actually I don't know the reason. I just tried several patches and each of > them > > has minimal change compare with patch set 4th. And then found "<=" always > cause > > two test failure(please see which two in patch set 5 test result). > > For the failure log, I can find any hint. But I tried several times CQ test with > "<=", the failure logs are the same. Sorry, I can find any hint --> I can't...
On 2017/06/01 09:50:49, shimazu wrote: > Thanks! Let me confirm the test failure just in case... > I expect the change is possible to change some expected.txt because the order > might be changed, but I didn't think it cause crashes. > > https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... > File content/browser/service_worker/service_worker_client_utils.cc (right): > > https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... > content/browser/service_worker/service_worker_client_utils.cc:364: return > a.create_time < b.create_time; > On 2017/06/01 01:00:42, xiaofengzhang wrote: > > On 2017/05/29 01:45:13, shimazu wrote: > > > I think it's better to use '<=' to preserve the order if all of parameters > are > > > the same. > > > > It seems "<=" will cause some test failure. > > Hmm... I didn't think this change cause crashes. Could you explain why this > cause the failure? I'm worried that the test or somewhere might be broken. Hi shimazu, do you have update on this?
On 2017/06/05 01:18:59, xiaofengzhang wrote: > On 2017/06/01 09:50:49, shimazu wrote: > > Thanks! Let me confirm the test failure just in case... > > I expect the change is possible to change some expected.txt because the order > > might be changed, but I didn't think it cause crashes. > > > > > https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... > > File content/browser/service_worker/service_worker_client_utils.cc (right): > > > > > https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... > > content/browser/service_worker/service_worker_client_utils.cc:364: return > > a.create_time < b.create_time; > > On 2017/06/01 01:00:42, xiaofengzhang wrote: > > > On 2017/05/29 01:45:13, shimazu wrote: > > > > I think it's better to use '<=' to preserve the order if all of parameters > > are > > > > the same. > > > > > > It seems "<=" will cause some test failure. > > > > Hmm... I didn't think this change cause crashes. Could you explain why this > > cause the failure? I'm worried that the test or somewhere might be broken. > > Hi shimazu, do you have update on this? Sorry for slow reply. I had another issue and couldn't work on this on Friday.. Let me investigate the issue on mac a bit.
Really thanks, lgtm :) https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_client_utils.cc (right): https://codereview.chromium.org/2905593002/diff/60001/content/browser/service... content/browser/service_worker/service_worker_client_utils.cc:364: return a.create_time < b.create_time; On 2017/06/01 09:50:49, shimazu wrote: > On 2017/06/01 01:00:42, xiaofengzhang wrote: > > On 2017/05/29 01:45:13, shimazu wrote: > > > I think it's better to use '<=' to preserve the order if all of parameters > are > > > the same. > > > > It seems "<=" will cause some test failure. > > Hmm... I didn't think this change cause crashes. Could you explain why this > cause the failure? I'm worried that the test or somewhere might be broken. Ah, sorry, that's my bad. C++ Compare requires comp(a, a) == false. http://en.cppreference.com/w/cpp/concept/Compare That means '=' operator isn't needed here.
The CQ bit was checked by xiaofeng.zhang@intel.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi, Thanks for mail. I'm on vacation so may respond slowly. Will get back office on 2017/06/09. For any emergency issue please contact me via phone.(+8613810834890) Thanks. BR, Han Leon
CQ is committing da patch.
Bot data: {"patchset_id": 210001, "attempt_start_ts": 1496627438855100,
"parent_rev": "8f43c3fa3816ec6b70168653eafd9b924412b426", "commit_rev":
"5c3497c94f604dee3cfe64ca84a7828a7b9bec04"}
Message was sent while issue was closed.
Description was changed from ========== Clients.matchAll() should return ordered clients by type/focus time/creation time According to current service worker specification, sort matchedClients such that: 1) WindowClient objects are always placed before worker clients. 2) WindowClient objects that have been focused are placed first sorted in the most recently focused order, and WindowClient objects that have never been focused are placed in their creation order. 3) WorkerClients are placed in their creation order. Current implementation only considers the focus time. BUG=701233 ========== to ========== Clients.matchAll() should return ordered clients by type/focus time/creation time According to current service worker specification, sort matchedClients such that: 1) WindowClient objects are always placed before worker clients. 2) WindowClient objects that have been focused are placed first sorted in the most recently focused order, and WindowClient objects that have never been focused are placed in their creation order. 3) WorkerClients are placed in their creation order. Current implementation only considers the focus time. BUG=701233 Review-Url: https://codereview.chromium.org/2905593002 Cr-Commit-Position: refs/heads/master@{#476935} Committed: https://chromium.googlesource.com/chromium/src/+/5c3497c94f604dee3cfe64ca84a7... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:210001) as https://chromium.googlesource.com/chromium/src/+/5c3497c94f604dee3cfe64ca84a7... |
