Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2875123002:
Check internal display RootWindow before getting the compositor. (Closed)
DescriptionCheck internal display RootWindow before getting the compositor.
When we find the id for the internal display it gets set and
Display::HasInternalDisplay() returns true. The internal display id
isn't reset, even when an internal display is removed. So even though we
might have an internal display id, there is no guarantee there is a
RootWindow for it.
BUG=721452
TEST=local device
Review-Url: https://codereview.chromium.org/2875123002
Cr-Commit-Position: refs/heads/master@{#471167}
Committed: https://chromium.googlesource.com/chromium/src/+/afd793fce0cf63ed0ac18a37a5a5f2f92e92d9a4
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use ShellPort. #
Total comments: 2
Patch Set 3 : Fix nits. #Patch Set 4 : Revert to patch 1 using Shell. #Messages
Total messages: 30 (20 generated)
wutao@chromium.org changed reviewers: + kylechar@chromium.org, oshima@chromium.org
Hi kylechar@, Please take a look. Thank you, Tao
The CQ bit was checked by wutao@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.
Sorry for not being able to catch this early. https://codereview.chromium.org/2875123002/diff/1/ash/wallpaper/wallpaper_con... File ash/wallpaper/wallpaper_controller.cc (right): https://codereview.chromium.org/2875123002/diff/1/ash/wallpaper/wallpaper_con... ash/wallpaper/wallpaper_controller.cc:421: display::Display::InternalDisplayId()); can you use ShellPort::GetRootWindowForDisplayId ?
Hi Oshima, PTAL. Thank you, Tao https://codereview.chromium.org/2875123002/diff/1/ash/wallpaper/wallpaper_con... File ash/wallpaper/wallpaper_controller.cc (right): https://codereview.chromium.org/2875123002/diff/1/ash/wallpaper/wallpaper_con... ash/wallpaper/wallpaper_controller.cc:421: display::Display::InternalDisplayId()); On 2017/05/11 21:03:45, oshima wrote: > can you use ShellPort::GetRootWindowForDisplayId ? sg. BTW, It seems the code wraps aura::Window with WmWindow and then I need to get aura::Window again.
The CQ bit was checked by wutao@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...
lgtm https://codereview.chromium.org/2875123002/diff/20001/ash/wallpaper/wallpaper... File ash/wallpaper/wallpaper_controller.cc (right): https://codereview.chromium.org/2875123002/diff/20001/ash/wallpaper/wallpaper... ash/wallpaper/wallpaper_controller.cc:423: root_window->aura_window() GetLayer()
Thanks. https://codereview.chromium.org/2875123002/diff/20001/ash/wallpaper/wallpaper... File ash/wallpaper/wallpaper_controller.cc (right): https://codereview.chromium.org/2875123002/diff/20001/ash/wallpaper/wallpaper... ash/wallpaper/wallpaper_controller.cc:423: root_window->aura_window() On 2017/05/11 22:29:26, oshima wrote: > GetLayer() Done.
The CQ bit was checked by wutao@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...
sadrul@chromium.org changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/2875123002/diff/1/ash/wallpaper/wallpaper_con... File ash/wallpaper/wallpaper_controller.cc (right): https://codereview.chromium.org/2875123002/diff/1/ash/wallpaper/wallpaper_con... ash/wallpaper/wallpaper_controller.cc:421: display::Display::InternalDisplayId()); On 2017/05/11 21:34:35, wutao wrote: > On 2017/05/11 21:03:45, oshima wrote: > > can you use ShellPort::GetRootWindowForDisplayId ? > > sg. > BTW, It seems the code wraps aura::Window with WmWindow and then I need to get > aura::Window again. Actually, we are trying to get rid of WmWindow. So directly using aura::Window here is better. https://bugs.chromium.org/p/chromium/issues/detail?id=671246
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/11 23:41:12, sadrul wrote: > https://codereview.chromium.org/2875123002/diff/1/ash/wallpaper/wallpaper_con... > File ash/wallpaper/wallpaper_controller.cc (right): > > https://codereview.chromium.org/2875123002/diff/1/ash/wallpaper/wallpaper_con... > ash/wallpaper/wallpaper_controller.cc:421: > display::Display::InternalDisplayId()); > On 2017/05/11 21:34:35, wutao wrote: > > On 2017/05/11 21:03:45, oshima wrote: > > > can you use ShellPort::GetRootWindowForDisplayId ? > > > > sg. > > BTW, It seems the code wraps aura::Window with WmWindow and then I need to get > > aura::Window again. > > Actually, we are trying to get rid of WmWindow. So directly using aura::Window > here is better. > > https://bugs.chromium.org/p/chromium/issues/detail?id=671246 It'll just change to aura::Window* right?
The CQ bit was checked by wutao@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/05/11 23:50:03, oshima wrote: > On 2017/05/11 23:41:12, sadrul wrote: > > > https://codereview.chromium.org/2875123002/diff/1/ash/wallpaper/wallpaper_con... > > File ash/wallpaper/wallpaper_controller.cc (right): > > > > > https://codereview.chromium.org/2875123002/diff/1/ash/wallpaper/wallpaper_con... > > ash/wallpaper/wallpaper_controller.cc:421: > > display::Display::InternalDisplayId()); > > On 2017/05/11 21:34:35, wutao wrote: > > > On 2017/05/11 21:03:45, oshima wrote: > > > > can you use ShellPort::GetRootWindowForDisplayId ? > > > > > > sg. > > > BTW, It seems the code wraps aura::Window with WmWindow and then I need to > get > > > aura::Window again. > > > > Actually, we are trying to get rid of WmWindow. So directly using aura::Window > > here is better. > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=671246 > > It'll just change to aura::Window* right? Talked offline. Will land the version in patch 1.
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 wutao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2875123002/#ps60001 (title: "Revert to patch 1 using Shell.")
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": 60001, "attempt_start_ts": 1494552216815030,
"parent_rev": "c294316ee266826158935c2fadadfd447b4a3a31", "commit_rev":
"afd793fce0cf63ed0ac18a37a5a5f2f92e92d9a4"}
Message was sent while issue was closed.
Description was changed from ========== Check internal display RootWindow before getting the compositor. When we find the id for the internal display it gets set and Display::HasInternalDisplay() returns true. The internal display id isn't reset, even when an internal display is removed. So even though we might have an internal display id, there is no guarantee there is a RootWindow for it. BUG=721452 TEST=local device ========== to ========== Check internal display RootWindow before getting the compositor. When we find the id for the internal display it gets set and Display::HasInternalDisplay() returns true. The internal display id isn't reset, even when an internal display is removed. So even though we might have an internal display id, there is no guarantee there is a RootWindow for it. BUG=721452 TEST=local device Review-Url: https://codereview.chromium.org/2875123002 Cr-Commit-Position: refs/heads/master@{#471167} Committed: https://chromium.googlesource.com/chromium/src/+/afd793fce0cf63ed0ac18a37a5a5... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/afd793fce0cf63ed0ac18a37a5a5... |
