Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2865713003:
Remove ash dependency. (Closed)
DescriptionRemove ash dependency.
Root window lists can be retrieved by observing EnvObserver callbacks.
Thus, ash/shell.cc is no longer needed. This intermediate step will
enable moving devtools out of ash.
BUG=711341
Review-Url: https://codereview.chromium.org/2865713003
Cr-Commit-Position: refs/heads/master@{#475365}
Committed: https://chromium.googlesource.com/chromium/src/+/4de8e9492ec943cc142295d28f34308655ae8ec1
Patch Set 1 : nits #Patch Set 2 : . #Patch Set 3 : nits. #
Total comments: 12
Patch Set 4 : . #
Total comments: 10
Patch Set 5 : use window coordinates to set bounds #
Total comments: 2
Patch Set 6 : address comments. #Patch Set 7 : rebase #
Total comments: 3
Patch Set 8 : nits #
Messages
Total messages: 55 (46 generated)
The CQ bit was checked by thanhph@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 ========== Remove ash dependency. BUG=711341 ========== to ========== Remove ash dependency. Root window lists can be retrieved by observing EnvObserver callbacks. Thus, ash/shell.cc is no longer needed. This intermediate step will enable moving devtools out of ash. BUG=711341 ==========
Description was changed from ========== Remove ash dependency. Root window lists can be retrieved by observing EnvObserver callbacks. Thus, ash/shell.cc is no longer needed. This intermediate step will enable moving devtools out of ash. BUG=711341 ========== to ========== Remove ash dependency. Root window lists can be retrieved by observing EnvObserver callbacks. Thus, ash/shell.cc is no longer needed. This intermediate step will enable moving devtools out of ash. BUG=711341 ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by thanhph@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...
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thanhph@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...
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by thanhph@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanhph@chromium.org changed reviewers: + sadrul@chromium.org
Hi Sadrul, Please review my patch. Thanks, Thanh.
Mostly nits. https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:349: if (!root_windows().empty()) { Can you DCHECK() here? i.e. can we ever reach here without any root windows? https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:351: root_windows()[0]->GetChildById(kShellWindowId_OverlayContainer); Can you just use the root-window itself as the parent? https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.h (right): https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.h:68: void OnWindowInitialized(aura::Window* window) override{}; override {} Or move the impl in the .cc file. https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:287: GetHighlightingWindow(dom_agent()->root_windows()[root_window_index]) First assert that |root_window_index| is valid. https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:357: aura::Window* root_window = dom_agent()->root_windows()[0]; Can we move GetPrimaryRootWindow() as a function in AshDevToolsTest, which returns dom_agent()->root_windows()[0]? Because having the function makes it easier to read the code. https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:678: aura::Window::Windows root_windows = dom_agent()->root_windows(); const auto& root_windows = dom_agent()->root_windows();
The CQ bit was checked by thanhph@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Sadrul. Please review my new patch. Thanh. https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:349: if (!root_windows().empty()) { On 2017/05/23 17:32:47, sadrul wrote: > Can you DCHECK() here? i.e. can we ever reach here without any root windows? Done. https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:351: root_windows()[0]->GetChildById(kShellWindowId_OverlayContainer); On 2017/05/23 17:32:47, sadrul wrote: > Can you just use the root-window itself as the parent? Done. https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.h (right): https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.h:68: void OnWindowInitialized(aura::Window* window) override{}; On 2017/05/23 17:32:47, sadrul wrote: > override {} > > Or move the impl in the .cc file. Done, I use override {} https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:287: GetHighlightingWindow(dom_agent()->root_windows()[root_window_index]) On 2017/05/23 17:32:47, sadrul wrote: > First assert that |root_window_index| is valid. Done. https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:357: aura::Window* root_window = dom_agent()->root_windows()[0]; On 2017/05/23 17:32:47, sadrul wrote: > Can we move GetPrimaryRootWindow() as a function in AshDevToolsTest, which > returns dom_agent()->root_windows()[0]? Because having the function makes it > easier to read the code. Done. https://codereview.chromium.org/2865713003/diff/100001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:678: aura::Window::Windows root_windows = dom_agent()->root_windows(); On 2017/05/23 17:32:47, sadrul wrote: > const auto& root_windows = dom_agent()->root_windows(); I removed this test for now as we discussed. Added a TODO to support multiple displays in the future.
https://codereview.chromium.org/2865713003/diff/120001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2865713003/diff/120001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:348: params.parent = root_windows()[0]; Add a TODO here that we would want to find the correct root window. https://codereview.chromium.org/2865713003/diff/120001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2865713003/diff/120001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:190: DCHECK(dom_agent()->root_windows().size()); This DCHECK is not needed. https://codereview.chromium.org/2865713003/diff/120001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:192: GetPrimaryRootWindow()->GetChildById(kShellWindowId_DefaultContainer); Remove this. https://codereview.chromium.org/2865713003/diff/120001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:212: AshTestBase::SetUp(); Add a comment explaining why this needs to happen after the dom-agent is created. https://codereview.chromium.org/2865713003/diff/120001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:678: // InitializeHighlightingWidget(). Can you file a bug to fix highlighting widget for multiple displays, and refer to that bug from this TODO?
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/2865713003/diff/160001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2865713003/diff/160001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:386: widget_for_highlighting_->GetNativeWindow()->SetBounds(origin_in_window); gfx::Rect bounds = window_and_bounds.second; gfx::Point origin = bounds.origin(); screen_position_client->ConvertPointFromScreen(root, &origin); bounds.set_origin(origin); widget_for_highlighting_->GetNativeWindow()->SetBounds(bounds);
Patchset #6 (id:180001) has been deleted
The CQ bit was checked by thanhph@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by thanhph@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Patchset #8 (id:240001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:220001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Sadrul. Please review my new patch. Thanh. https://codereview.chromium.org/2865713003/diff/120001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2865713003/diff/120001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:348: params.parent = root_windows()[0]; On 2017/05/25 04:21:03, sadrul wrote: > Add a TODO here that we would want to find the correct root window. We don't need this anymore. https://codereview.chromium.org/2865713003/diff/120001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2865713003/diff/120001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:190: DCHECK(dom_agent()->root_windows().size()); On 2017/05/25 04:21:03, sadrul wrote: > This DCHECK is not needed. Done. https://codereview.chromium.org/2865713003/diff/120001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:192: GetPrimaryRootWindow()->GetChildById(kShellWindowId_DefaultContainer); On 2017/05/25 04:21:03, sadrul wrote: > Remove this. Done. https://codereview.chromium.org/2865713003/diff/120001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:212: AshTestBase::SetUp(); On 2017/05/25 04:21:03, sadrul wrote: > Add a comment explaining why this needs to happen after the dom-agent is > created. Done. https://codereview.chromium.org/2865713003/diff/120001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:678: // InitializeHighlightingWidget(). On 2017/05/25 04:21:03, sadrul wrote: > Can you file a bug to fix highlighting widget for multiple displays, and refer > to that bug from this TODO? Done. https://codereview.chromium.org/2865713003/diff/160001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2865713003/diff/160001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:386: widget_for_highlighting_->GetNativeWindow()->SetBounds(origin_in_window); On 2017/05/26 20:09:56, sadrul wrote: > gfx::Rect bounds = window_and_bounds.second; > gfx::Point origin = bounds.origin(); > screen_position_client->ConvertPointFromScreen(root, &origin); > bounds.set_origin(origin); > widget_for_highlighting_->GetNativeWindow()->SetBounds(bounds); Done. I use gfx::Rect bounds(window_and_bounds.second) instead of const_cast.
lgtm https://codereview.chromium.org/2865713003/diff/250001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_dom_agent.cc (right): https://codereview.chromium.org/2865713003/diff/250001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:350: params.parent = nullptr; Don't need to set this. https://codereview.chromium.org/2865713003/diff/250001/ash/devtools/ash_devto... ash/devtools/ash_devtools_dom_agent.cc:374: window_and_bounds.first->GetRootWindow()); Use |root| https://codereview.chromium.org/2865713003/diff/250001/ash/devtools/ash_devto... File ash/devtools/ash_devtools_unittest.cc (right): https://codereview.chromium.org/2865713003/diff/250001/ash/devtools/ash_devto... ash/devtools/ash_devtools_unittest.cc:698: // multiple displays. https://crbug.com/726831. Let's make sure we add the test back in the follow up CL in https://codereview.chromium.org/2899783002/
The CQ bit was checked by thanhph@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thanhph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2865713003/#ps270001 (title: "nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 270001, "attempt_start_ts": 1496082413081920,
"parent_rev": "1af51020c7f360753dd77954fff621e5d1896ea6", "commit_rev":
"4de8e9492ec943cc142295d28f34308655ae8ec1"}
Message was sent while issue was closed.
Description was changed from ========== Remove ash dependency. Root window lists can be retrieved by observing EnvObserver callbacks. Thus, ash/shell.cc is no longer needed. This intermediate step will enable moving devtools out of ash. BUG=711341 ========== to ========== Remove ash dependency. Root window lists can be retrieved by observing EnvObserver callbacks. Thus, ash/shell.cc is no longer needed. This intermediate step will enable moving devtools out of ash. BUG=711341 Review-Url: https://codereview.chromium.org/2865713003 Cr-Commit-Position: refs/heads/master@{#475365} Committed: https://chromium.googlesource.com/chromium/src/+/4de8e9492ec943cc142295d28f34... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:270001) as https://chromium.googlesource.com/chromium/src/+/4de8e9492ec943cc142295d28f34... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
