Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2190963002:
Ash window cycle ui: don't create mirrored layers for previews until (Closed)
|
Created:
4 years, 4 months ago by Evan Stade Modified:
4 years, 4 months ago Reviewers:
sky CC:
chromium-reviews, kalyank, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAsh window cycle ui: don't create mirrored layers for previews until
we have to.
BUG=626111
Committed: https://crrev.com/444a0348e883795e23c2f821e3d8380da4518313
Cr-Commit-Position: refs/heads/master@{#410762}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use OnVisibleBoundsChanged #
Total comments: 1
Patch Set 3 : original approach #Patch Set 4 : no invalidatelayout #Patch Set 5 : loc-- #
Messages
Total messages: 27 (13 generated)
Description was changed from ========== CrOS window cycle ui: don't create mirrored layers for previews until we have to. BUG=626111 ========== to ========== Ash window cycle ui: don't create mirrored layers for previews until we have to. BUG=626111 ==========
estade@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2190963002/diff/1/ash/wm/window_mirror_view.cc File ash/wm/window_mirror_view.cc (right): https://codereview.chromium.org/2190963002/diff/1/ash/wm/window_mirror_view.c... ash/wm/window_mirror_view.cc:51: InvalidateLayout(); Are you sure you want that and not overriding GetNeedsNotificationWhenVisibleBoundsChange to return true?
https://codereview.chromium.org/2190963002/diff/1/ash/wm/window_mirror_view.cc File ash/wm/window_mirror_view.cc (right): https://codereview.chromium.org/2190963002/diff/1/ash/wm/window_mirror_view.c... ash/wm/window_mirror_view.cc:51: InvalidateLayout(); On 2016/08/01 20:55:57, sky wrote: > Are you sure you want that and not overriding > GetNeedsNotificationWhenVisibleBoundsChange to return true? a) We don't really want notifications after the initial visible bounds change, and it seems like using that approach would result in more code rather than less. Then we'd need to also override OnVisibleBoundsChanged and init the layer owner there and call Layout manually. Doesn't seem much better in my head but I can try it if you prefer that path. b) GetNeedsNotificationWhenVisibleBoundsChange says it's only used by NativeViewHost, but it looks like that's not true, does the documentation need updating?
https://codereview.chromium.org/2190963002/diff/1/ash/wm/window_mirror_view.cc File ash/wm/window_mirror_view.cc (right): https://codereview.chromium.org/2190963002/diff/1/ash/wm/window_mirror_view.c... ash/wm/window_mirror_view.cc:51: InvalidateLayout(); On 2016/08/01 22:11:14, Evan Stade (sorta ooo) wrote: > On 2016/08/01 20:55:57, sky wrote: > > Are you sure you want that and not overriding > > GetNeedsNotificationWhenVisibleBoundsChange to return true? > > a) We don't really want notifications after the initial visible bounds change, > and it seems like using that approach would result in more code rather than > less. That's no different than the if (!layer_owner_) here, right? > Then we'd need to also override OnVisibleBoundsChanged and init the layer > owner there and call Layout manually. Doesn't seem much better in my head but I > can try it if you prefer that path. I suggested using GetNeedsNotificationWhenVisibleBoundsChange because you want to do something when the visible bounds change, and the InvalidateLayout() call here invalidates the layout of all ancestors. That seems like overkill for what you're trying to do. But what you have here certainly works, so if you feel strongly I'm ok with it. > b) GetNeedsNotificationWhenVisibleBoundsChange says it's only used by > NativeViewHost, but it looks like that's not true, does the documentation need > updating?
https://codereview.chromium.org/2190963002/diff/1/ash/wm/window_mirror_view.cc File ash/wm/window_mirror_view.cc (right): https://codereview.chromium.org/2190963002/diff/1/ash/wm/window_mirror_view.c... ash/wm/window_mirror_view.cc:51: InvalidateLayout(); On 2016/08/02 16:21:27, sky wrote: > On 2016/08/01 22:11:14, Evan Stade (sorta ooo) wrote: > > On 2016/08/01 20:55:57, sky wrote: > > > Are you sure you want that and not overriding > > > GetNeedsNotificationWhenVisibleBoundsChange to return true? > > > > a) We don't really want notifications after the initial visible bounds change, > > and it seems like using that approach would result in more code rather than > > less. > > That's no different than the if (!layer_owner_) here, right? > > > Then we'd need to also override OnVisibleBoundsChanged and init the layer > > owner there and call Layout manually. Doesn't seem much better in my head but > I > > can try it if you prefer that path. > > I suggested using GetNeedsNotificationWhenVisibleBoundsChange because you want > to do something when the visible bounds change, and the InvalidateLayout() call > here invalidates the layout of all ancestors. That seems like overkill for what > you're trying to do. But what you have here certainly works, so if you feel > strongly I'm ok with it. > > > b) GetNeedsNotificationWhenVisibleBoundsChange says it's only used by > > NativeViewHost, but it looks like that's not true, does the documentation need > > updating? > OK, I'll try it the other way because it's easier to fairly evaluate when it's actually in front of you, but I probably won't get to it till Friday so this review may be dormant for a few days.
On 2016/08/02 18:22:55, Evan Stade wrote: > https://codereview.chromium.org/2190963002/diff/1/ash/wm/window_mirror_view.cc > File ash/wm/window_mirror_view.cc (right): > > https://codereview.chromium.org/2190963002/diff/1/ash/wm/window_mirror_view.c... > ash/wm/window_mirror_view.cc:51: InvalidateLayout(); > On 2016/08/02 16:21:27, sky wrote: > > On 2016/08/01 22:11:14, Evan Stade (sorta ooo) wrote: > > > On 2016/08/01 20:55:57, sky wrote: > > > > Are you sure you want that and not overriding > > > > GetNeedsNotificationWhenVisibleBoundsChange to return true? > > > > > > a) We don't really want notifications after the initial visible bounds > change, > > > and it seems like using that approach would result in more code rather than > > > less. > > > > That's no different than the if (!layer_owner_) here, right? > > > > > Then we'd need to also override OnVisibleBoundsChanged and init the layer > > > owner there and call Layout manually. Doesn't seem much better in my head > but > > I > > > can try it if you prefer that path. > > > > I suggested using GetNeedsNotificationWhenVisibleBoundsChange because you want > > to do something when the visible bounds change, and the InvalidateLayout() > call > > here invalidates the layout of all ancestors. That seems like overkill for > what > > you're trying to do. But what you have here certainly works, so if you feel > > strongly I'm ok with it. > > > > > b) GetNeedsNotificationWhenVisibleBoundsChange says it's only used by > > > NativeViewHost, but it looks like that's not true, does the documentation > need > > > updating? > > > > OK, I'll try it the other way because it's easier to fairly evaluate when it's > actually in front of you, but I probably won't get to it till Friday so this > review may be dormant for a few days. here's the version you proposed (I believe) in ps2. I think I prefer the original one, so I've re-uploaded it as ps3.
https://codereview.chromium.org/2190963002/diff/20001/ash/wm/window_mirror_vi... File ash/wm/window_mirror_view.cc (right): https://codereview.chromium.org/2190963002/diff/20001/ash/wm/window_mirror_vi... ash/wm/window_mirror_view.cc:47: InvalidateLayout(); The main thing I don't particularly like about either patch is the explcit call to invalidatelayout. Can you clarify why it is needed? If you change InitLayoutOwner() (in either patch) to call Layout() does the need for it go away?
yes, that works (updated)
The CQ bit was checked by estade@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 checked by estade@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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by estade@chromium.org
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
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Ash window cycle ui: don't create mirrored layers for previews until we have to. BUG=626111 ========== to ========== Ash window cycle ui: don't create mirrored layers for previews until we have to. BUG=626111 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Ash window cycle ui: don't create mirrored layers for previews until we have to. BUG=626111 ========== to ========== Ash window cycle ui: don't create mirrored layers for previews until we have to. BUG=626111 Committed: https://crrev.com/444a0348e883795e23c2f821e3d8380da4518313 Cr-Commit-Position: refs/heads/master@{#410762} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/444a0348e883795e23c2f821e3d8380da4518313 Cr-Commit-Position: refs/heads/master@{#410762} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
