Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2755673003:
Allow parallel creation of windows
DescriptionAllow parallel creation of windows
BUG=666958
Patch Set 1 #
Total comments: 14
Patch Set 2 : fwang's and tonikitoo's comments #Patch Set 3 : fix comments #
Total comments: 12
Patch Set 4 : Add WindowTreeDataInternal #Patch Set 5 : fix comment #
Total comments: 6
Patch Set 6 : nits #
Depends on Patchset: Messages
Total messages: 43 (32 generated)
The CQ bit was checked by maksim.sisov@chromium.org 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...
fwang@igalia.com changed reviewers: + fwang@igalia.com
Seems a good start. I think the main issue is that we should try and reuse the window_tree_data_list_ vector instead of creating your own. https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... services/ui/demo/mus_demo_external.cc:62: } Do you really need this post task stuff? I think you can just append all WindowTreeData in one go and wait for the OnRooWindowEmbed calls. Also there are many comments explaining this to update. https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... services/ui/demo/mus_demo_external.cc:68: window_tree_host_mus_list_.push_back(std::move(mus)); I believe you should keep appending to window_tree_data_list_ https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... services/ui/demo/mus_demo_external.cc:81: DCHECK(it != window_tree_host_mus_list_.end()); This logic exists in RemoveWindowTreeData, so it should probably be shared. This is possible if you use window_tree_data_list_ instead of your own vector. https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... services/ui/demo/mus_demo_external.cc:86: window_tree_host_mus_list_.erase(it); No need to do that if your use window_tree_data_list_ https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... File services/ui/demo/mus_demo_external.h (right): https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... services/ui/demo/mus_demo_external.h:32: void OnRootWindowEmbed(aura::WindowTreeHostMus* window_tree_host) final; What about OnEmbedRootReady, to be consistent with OnEmbedRootDestroyed? I guess the OnEmbed in the parent class should be final. https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... services/ui/demo/mus_demo_external.h:40: window_tree_host_mus_list_; I think you can just keep this in WindowTreeData.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tonikitoo@igalia.com changed reviewers: + tonikitoo@igalia.com
https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... services/ui/demo/mus_demo_external.cc:84: InitWindowTreeData(std::move(*it)); now that you are passing a valid unique_ptr always to WindowTreeData::Init, we should remove the null-check in it (see line 71): 69 void WindowTreeData::Init( 70 std::unique_ptr<aura::WindowTreeHostMus> window_tree_host) { 71 if (window_tree_host) { 72 window_tree_host->InitHost(); 73 window_tree_host->Show(); 74 SetWindowTreeHost(std::move(window_tree_host)); 75 }
The CQ bit was checked by msisov@igalia.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: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by msisov@igalia.com to run a CQ dry run
Description was changed from ========== Allow parallel creation of windows BUG= ========== to ========== Allow parallel creation of windows BUG= ==========
msisov@igalia.com changed reviewers: - maksim.sisov@chromium.org
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: No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... services/ui/demo/mus_demo_external.cc:62: } On 2017/03/15 14:37:56, fwang wrote: > Do you really need this post task stuff? I think you can just append all > WindowTreeData in one go and wait for the OnRooWindowEmbed calls. > > Also there are many comments explaining this to update. Done. https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... services/ui/demo/mus_demo_external.cc:68: window_tree_host_mus_list_.push_back(std::move(mus)); On 2017/03/15 14:37:56, fwang wrote: > I believe you should keep appending to window_tree_data_list_ Done. https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... services/ui/demo/mus_demo_external.cc:81: DCHECK(it != window_tree_host_mus_list_.end()); On 2017/03/15 14:37:56, fwang wrote: > This logic exists in RemoveWindowTreeData, so it should probably be shared. This > is possible if you use window_tree_data_list_ instead of your own vector. Ok, I've made FindWindowTreeData, which returns an iterator for a requested WindowTreeData https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... services/ui/demo/mus_demo_external.cc:84: InitWindowTreeData(std::move(*it)); On 2017/03/15 15:34:54, tonikitoo wrote: > now that you are passing a valid unique_ptr always to WindowTreeData::Init, we > should remove the null-check in it (see line 71): > > 69 void WindowTreeData::Init( > 70 std::unique_ptr<aura::WindowTreeHostMus> window_tree_host) { > 71 if (window_tree_host) { > 72 window_tree_host->InitHost(); > 73 window_tree_host->Show(); > 74 SetWindowTreeHost(std::move(window_tree_host)); > 75 } Logic is changed. So it's still applicable. https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... services/ui/demo/mus_demo_external.cc:86: window_tree_host_mus_list_.erase(it); On 2017/03/15 14:37:56, fwang wrote: > No need to do that if your use window_tree_data_list_ Done. https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... File services/ui/demo/mus_demo_external.h (right): https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... services/ui/demo/mus_demo_external.h:32: void OnRootWindowEmbed(aura::WindowTreeHostMus* window_tree_host) final; On 2017/03/15 14:37:56, fwang wrote: > What about OnEmbedRootReady, to be consistent with OnEmbedRootDestroyed? > > I guess the OnEmbed in the parent class should be final. > Done. https://codereview.chromium.org/2755673003/diff/1/services/ui/demo/mus_demo_e... services/ui/demo/mus_demo_external.h:40: window_tree_host_mus_list_; On 2017/03/15 14:37:56, fwang wrote: > I think you can just keep this in WindowTreeData. Done.
The CQ bit was checked by maksim.sisov@chromium.org 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...
Description was changed from ========== Allow parallel creation of windows BUG= ========== to ========== Allow parallel creation of windows BUG=666958 ==========
OK, looks better! I still think we can improve how the append & init are done (see below). https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... services/ui/demo/mus_demo.h:63: void InitWindowTreeDataExternal(aura::WindowTreeHostMus* window_tree_host); I would make HasPendingWindowTreeData protected and move this function specific to "external mode" into the MusDemoExternal derived class. But actually, I think we no longer really need HasPendingWindowTreeData, now that the last element is no longer the pending child and use FindWindowTreeData instead. https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... services/ui/demo/mus_demo_external.cc:73: void MusDemoExternal::OpenNewWindow(size_t window_count) { window_index https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... services/ui/demo/mus_demo_external.cc:75: window_tree_client(), GetSquareSizeForWindow(window_count))); window_index https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... services/ui/demo/mus_demo_external.cc:81: InitWindowTreeDataExternal(window_tree_host); I seems you can get rid of InitWindowTreeDataExternal and just implement the init in the body of OnEmbedRootReady. https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... File services/ui/demo/mus_demo_external.h (right): https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... services/ui/demo/mus_demo_external.h:25: void OpenNewWindow(size_t window_count); Maybe this can be window_index? https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/window... File services/ui/demo/window_tree_data.cc (right): https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/window... services/ui/demo/window_tree_data.cc:70: std::unique_ptr<aura::WindowTreeHostMus> window_tree_host) { So I think you can remove the parameter and assume DCHECK(window_tree_host_) here. Then WindowTreeData could have a constructor that accepts a std::unique_ptr<aura::WindowTreeHostMus> parameter and performs what is in the if (window_tree_host) { } below. Finally, MusDemoInternal will be able to use the constructor to append the WindowTreeData and then call Init on the data without passing any std::unique_ptr<aura::WindowTreeHostMus>. And MusDemoExternal won't need to pass a nullptr (which is a bit weird).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... services/ui/demo/mus_demo.h:63: void InitWindowTreeDataExternal(aura::WindowTreeHostMus* window_tree_host); On 2017/03/16 08:59:48, fwang wrote: > I would make HasPendingWindowTreeData protected and move this function specific > to "external mode" into the MusDemoExternal derived class. But actually, I think > we no longer really need HasPendingWindowTreeData, now that the last element is > no longer the pending child and use FindWindowTreeData instead. Agree, HasPendingWindowTreeData is not longer needed. We just check that WindowTreeData is not initialized when Init() is called and that InitWindowTreeData https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... File services/ui/demo/mus_demo_external.cc (right): https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... services/ui/demo/mus_demo_external.cc:73: void MusDemoExternal::OpenNewWindow(size_t window_count) { On 2017/03/16 08:59:49, fwang wrote: > window_index Done. https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... services/ui/demo/mus_demo_external.cc:75: window_tree_client(), GetSquareSizeForWindow(window_count))); On 2017/03/16 08:59:49, fwang wrote: > window_index Done. https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... services/ui/demo/mus_demo_external.cc:81: InitWindowTreeDataExternal(window_tree_host); On 2017/03/16 08:59:48, fwang wrote: > I seems you can get rid of InitWindowTreeDataExternal and just implement the > init in the body of OnEmbedRootReady. Done. https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... File services/ui/demo/mus_demo_external.h (right): https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/mus_de... services/ui/demo/mus_demo_external.h:25: void OpenNewWindow(size_t window_count); On 2017/03/16 08:59:49, fwang wrote: > Maybe this can be window_index? Done. https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/window... File services/ui/demo/window_tree_data.cc (right): https://codereview.chromium.org/2755673003/diff/40001/services/ui/demo/window... services/ui/demo/window_tree_data.cc:70: std::unique_ptr<aura::WindowTreeHostMus> window_tree_host) { On 2017/03/16 08:59:49, fwang wrote: > So I think you can remove the parameter and assume DCHECK(window_tree_host_) > here. > > Then WindowTreeData could have a constructor that accepts a > std::unique_ptr<aura::WindowTreeHostMus> parameter and performs what is in the > if (window_tree_host) { } below. > > Finally, MusDemoInternal will be able to use the constructor to append the > WindowTreeData and then call Init on the data without passing any > std::unique_ptr<aura::WindowTreeHostMus>. And MusDemoExternal won't need to pass > a nullptr (which is a bit weird). If window_tree_host->InitHost(); and window_tree_host->Show() go into ctor, then in case of external demo, those two would be called before OnEmbedRootReady, which is wrong, I guess.
On 2017/03/16 09:21:32, maksims wrote: > If window_tree_host->InitHost(); and window_tree_host->Show() go > into ctor, then in case of external demo, those two would be called before > OnEmbedRootReady, which is wrong, I guess. Yes, I meant add a new constructor WindowTreeData(std::unique_ptr<aura::WindowTreeHostMus>, int square_size) that would call WindowTreeData(int square_size) and do the inithost, show and setwindowtreehost. This would only be used in internal mode. But now that I think about it, it would even be better to have such a constructor in a class WindowTreeDataInternal deriving from WindowTreeData and MusDemoInternal would do AppendWindowTreeData(base::MakeUnique<WindowTreeDataInternal>(...)). This would be similar to what is done in MusDemoExternal.
The CQ bit was checked by maksim.sisov@chromium.org 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...
On 2017/03/16 09:33:51, fwang wrote: > On 2017/03/16 09:21:32, maksims wrote: > > If window_tree_host->InitHost(); and window_tree_host->Show() go > > into ctor, then in case of external demo, those two would be called before > > OnEmbedRootReady, which is wrong, I guess. > > Yes, I meant add a new constructor > > WindowTreeData(std::unique_ptr<aura::WindowTreeHostMus>, int square_size) > > that would call WindowTreeData(int square_size) and do the inithost, show and > setwindowtreehost. This would only be used in internal mode. > > But now that I think about it, it would even be better to have such a > constructor in a class WindowTreeDataInternal deriving from WindowTreeData and > MusDemoInternal would do > AppendWindowTreeData(base::MakeUnique<WindowTreeDataInternal>(...)). This would > be similar to what is done in MusDemoExternal. Agree. Done!
The CQ bit was checked by maksim.sisov@chromium.org 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/2755673003/diff/80001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2755673003/diff/80001/services/ui/demo/mus_de... services/ui/demo/mus_demo.h:60: std::unique_ptr<aura::WindowTreeHostMus> window_tree_host); Shoudn't this be removed? https://codereview.chromium.org/2755673003/diff/80001/services/ui/demo/mus_de... File services/ui/demo/mus_demo_internal.cc (right): https://codereview.chromium.org/2755673003/diff/80001/services/ui/demo/mus_de... services/ui/demo/mus_demo_internal.cc:92: window_tree_data->Init(); Maybe this can be moved into the WindowTreeDataInternal constructor. https://codereview.chromium.org/2755673003/diff/80001/services/ui/demo/window... File services/ui/demo/window_tree_data.h (right): https://codereview.chromium.org/2755673003/diff/80001/services/ui/demo/window... services/ui/demo/window_tree_data.h:30: bool IsInitialized() const { return window_delegate_; } Now that we remove haspendingdata, I wonder if this really needs to be public?
maksim.sisov@chromium.org changed reviewers: + kylechar@chromium.org
The CQ bit was checked by maksim.sisov@chromium.org 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...
kylechar@, please review. https://codereview.chromium.org/2755673003/diff/80001/services/ui/demo/mus_de... File services/ui/demo/mus_demo.h (right): https://codereview.chromium.org/2755673003/diff/80001/services/ui/demo/mus_de... services/ui/demo/mus_demo.h:60: std::unique_ptr<aura::WindowTreeHostMus> window_tree_host); On 2017/03/16 10:54:55, fwang wrote: > Shoudn't this be removed? Done. https://codereview.chromium.org/2755673003/diff/80001/services/ui/demo/mus_de... File services/ui/demo/mus_demo_internal.cc (right): https://codereview.chromium.org/2755673003/diff/80001/services/ui/demo/mus_de... services/ui/demo/mus_demo_internal.cc:92: window_tree_data->Init(); On 2017/03/16 10:54:55, fwang wrote: > Maybe this can be moved into the WindowTreeDataInternal constructor. I think it's better to leave it as it is in order to better understand the logic, when someone looks the code here. https://codereview.chromium.org/2755673003/diff/80001/services/ui/demo/window... File services/ui/demo/window_tree_data.h (right): https://codereview.chromium.org/2755673003/diff/80001/services/ui/demo/window... services/ui/demo/window_tree_data.h:30: bool IsInitialized() const { return window_delegate_; } On 2017/03/16 10:54:55, fwang wrote: > Now that we remove haspendingdata, I wonder if this really needs to be public? Done.
informal l-g-t-m
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_...)
msisov@igalia.com changed reviewers: - maksim.sisov@chromium.org
kylechar@chromium.org changed reviewers: - kylechar@chromium.org |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
