Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2129213003:
[ash-md] Hides frame headers in overview mode not using a mask (Closed)
|
Created:
4 years, 5 months ago by varkha Modified:
4 years, 5 months ago Reviewers:
tdanderson CC:
chromium-reviews, kalyank, sadrul, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ash-md] Hides frame headers in overview mode not using a mask
Mask layers are expensive and we will want to avoid using them when
there are too many windows.
Trying therefore to find a solution to hide the frame headers that does
not rely on masks.
Also fixes the mask update using SetFillsBoundsOpaquely(false).
BUG=624431
Committed: https://crrev.com/c54b330fedbd77ed0d54532ec98418f7ce7ff6a6
Cr-Commit-Position: refs/heads/master@{#404573}
Patch Set 1 #
Total comments: 5
Patch Set 2 : [ash-md] Hides frame headers in overview mode not using a mask (using SetAlphaShape) #
Total comments: 18
Patch Set 3 : [ash-md] Hides frame headers in overview mode not using a mask (comments) #
Messages
Total messages: 39 (27 generated)
Description was changed from ========== [ash-md] Hides frame headers in overview mode not using a mask There is a problem with clearing mask layers when the textures are reused. Trying therefore to find a solution to hide the frame headers that does not rely on masks. BUG=624612 ========== to ========== [ash-md] Hides frame headers in overview mode not using a mask There is a problem with clearing mask layers when the textures are reused. Trying therefore to find a solution to hide the frame headers that does not rely on masks. BUG=624431 ==========
The CQ bit was checked by varkha@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...
varkha@chromium.org changed reviewers: + tdanderson@chromium.org
tdanderson@, can you please take a first look? Doing this will allow us to put in a logic to at least restrict when masks are used in hope that doing this will allow avoiding the resource-hungry cases. Thanks!
lgtm with a nit and a couple of questions https://codereview.chromium.org/2129213003/diff/1/ash/common/wm/overview/scop... File ash/common/wm/overview/scoped_transform_overview_window.cc (right): https://codereview.chromium.org/2129213003/diff/1/ash/common/wm/overview/scop... ash/common/wm/overview/scoped_transform_overview_window.cc:183: OverviewContentMask(int radius); explicit https://codereview.chromium.org/2129213003/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/2129213003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:357: void BrowserNonClientFrameViewAsh::OnOverviewModeStarting() { As a side question, why aren't we making any MD changes to BNCFVMus? https://codereview.chromium.org/2129213003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:363: SetVisible(false); Correct me if I'm mistaken, but isn't this setting *all* of the non-client areas to be invisible, including the entire window border? Is that what we want to be doing?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I am exploring other ways of masking so I will hold on to this for now. Thanks! https://codereview.chromium.org/2129213003/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right): https://codereview.chromium.org/2129213003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:357: void BrowserNonClientFrameViewAsh::OnOverviewModeStarting() { On 2016/07/08 18:51:55, tdanderson wrote: > As a side question, why aren't we making any MD changes to BNCFVMus? Good question, I will follow up with sky if it is expected at this stage. https://codereview.chromium.org/2129213003/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:363: SetVisible(false); On 2016/07/08 18:51:55, tdanderson wrote: > Correct me if I'm mistaken, but isn't this setting *all* of the non-client areas > to be invisible, including the entire window border? Is that what we want to be > doing? For ash browser frame this is the only visible portion at the moment, no (at least in the borders that we use to scale the overview)?
The CQ bit was checked by varkha@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by varkha@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...
tdanderson@, let me know if you can take a look before you come back. If not - no worries, I will find somebody new. I have found that I can use ui::Layer::SetAlphaShape to effectively mask the window header. I may still have very small rounded corner discrepancies because of the http://crbug.com/626080 but at least the headers will not get exposed. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2129213003/diff/20001/ash/common/wm/overview/... File ash/common/wm/overview/scoped_transform_overview_window.cc (right): https://codereview.chromium.org/2129213003/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/scoped_transform_overview_window.cc:428: window()->GetLayer()->SetAlphaShape(base::WrapUnique(region)); Self review: this could be done only for windows that need to mask a header (i.e. non-zero TOP_VIEW_INSET) rather than for all windows.
A few questions and comments below: https://chromiumcodereview.appspot.com/2129213003/diff/20001/ash/common/wm/ov... File ash/common/wm/overview/scoped_transform_overview_window.cc (right): https://chromiumcodereview.appspot.com/2129213003/diff/20001/ash/common/wm/ov... ash/common/wm/overview/scoped_transform_overview_window.cc:183: OverviewContentMask(float radius); Make this constructor explicit https://chromiumcodereview.appspot.com/2129213003/diff/20001/ash/common/wm/ov... ash/common/wm/overview/scoped_transform_overview_window.cc:409: &transform != &original_transform_) { Hmm, shouldn't all of SetTransform() be a no-op if this is true? i.e., void ScopedTransformOverviewWindow::SetTransform(...) { if (&transform == &original_transform) return; ... } That, or perhaps I don't understand why you're including this check. https://chromiumcodereview.appspot.com/2129213003/diff/20001/ash/common/wm/ov... ash/common/wm/overview/scoped_transform_overview_window.cc:412: mask_->layer()->SetBounds(bounds); I don't understand why you're setting bounds with an origin of (0,0) (from line 410) rather than the true bounds. https://chromiumcodereview.appspot.com/2129213003/diff/20001/ash/common/wm/ov... ash/common/wm/overview/scoped_transform_overview_window.cc:415: if (!alpha_shape_set_) { I don't think you need |alpha_shape_set_| at all. Assuming that calling window()->SetMasksToBounds(true) is a no-op if SMTB(true) has already been called on window(), then it seems that lines 415-421 could be replaced with: SkRegion* alpha_shape = window()->GetLayer()->alpha_shape(); if (!original_alpha_shape_ && alpha_shape) original_alpha_shape_.reset(new SKRegion(*alpha_shape)); window()->SetMasksToBounds(true); Furthermore, line 297 seems unnecessary anyway since (iiuc) RestoreWindow() is called right before |this| is destroyed. https://chromiumcodereview.appspot.com/2129213003/diff/20001/ash/common/wm/ov... ash/common/wm/overview/scoped_transform_overview_window.cc:416: if (window()->GetLayer()->alpha_shape()) {} since call to reset() spans two lines https://chromiumcodereview.appspot.com/2129213003/diff/20001/ash/common/wm/ov... File ash/common/wm/overview/scoped_transform_overview_window.h (right): https://chromiumcodereview.appspot.com/2129213003/diff/20001/ash/common/wm/ov... ash/common/wm/overview/scoped_transform_overview_window.h:140: // Original window shape. From line 416 in scoped_transform_overview_window.cc, it looks like not every window has an alpha shape. Can you modify the comments on line 140 and 143 accordingly? e.g., "The original window shape, if one exists" and "True after the shape has been set, or after we determine that the window had no shape" or something. https://chromiumcodereview.appspot.com/2129213003/diff/20001/ash/common/wm/ov... ash/common/wm/overview/scoped_transform_overview_window.h:141: std::unique_ptr<SkRegion> original_alpha_shape_; Maybe it's just because "alpha shape" is new terminology to me, but I think this would be more readable if it were called |original_window_shape_|. https://chromiumcodereview.appspot.com/2129213003/diff/20001/ash/common/wm/ov... ash/common/wm/overview/scoped_transform_overview_window.h:144: bool alpha_shape_set_; I don't think you need this (see my comments below), but if you do, rename to |original_alpha_shape_set_| (or |original_window_shape_set_| if you change the other member's name).
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by varkha@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...
Thanks. Also the mask should work with enne@'s helpful tip (although it is probably still expensive so we should disable it for the case of many windows). https://codereview.chromium.org/2129213003/diff/20001/ash/common/wm/overview/... File ash/common/wm/overview/scoped_transform_overview_window.cc (right): https://codereview.chromium.org/2129213003/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/scoped_transform_overview_window.cc:183: OverviewContentMask(float radius); On 2016/07/09 21:23:21, tdanderson (OOO until July 18) wrote: > Make this constructor explicit Done. https://codereview.chromium.org/2129213003/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/scoped_transform_overview_window.cc:409: &transform != &original_transform_) { On 2016/07/09 21:23:21, tdanderson (OOO until July 18) wrote: > Hmm, shouldn't all of SetTransform() be a no-op if this is true? i.e., > > void ScopedTransformOverviewWindow::SetTransform(...) { > if (&transform == &original_transform) > return; > > ... > } > > That, or perhaps I don't understand why you're including this check. No. This method is used to both set the transform on a a window that scales it into overview and also to revert back to original transform. But when reverting there is no point to refresh the header mask since it is about to be removed anyway. See a call site above in RestoreWindow(). https://codereview.chromium.org/2129213003/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/scoped_transform_overview_window.cc:412: mask_->layer()->SetBounds(bounds); On 2016/07/09 21:23:21, tdanderson (OOO until July 18) wrote: > I don't understand why you're setting bounds with an origin of (0,0) (from line > 410) rather than the true bounds. This is for a mask layer - my understanding is that this is using coordinates relative to the window()->GetLayer(). https://codereview.chromium.org/2129213003/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/scoped_transform_overview_window.cc:415: if (!alpha_shape_set_) { On 2016/07/09 21:23:21, tdanderson (OOO until July 18) wrote: > I don't think you need |alpha_shape_set_| at all. Assuming that calling > window()->SetMasksToBounds(true) is a no-op if SMTB(true) has already been > called on window(), then it seems that lines 415-421 could be replaced with: > > SkRegion* alpha_shape = window()->GetLayer()->alpha_shape(); > if (!original_alpha_shape_ && alpha_shape) > original_alpha_shape_.reset(new SKRegion(*alpha_shape)); > window()->SetMasksToBounds(true); > > Furthermore, line 297 seems unnecessary anyway since (iiuc) RestoreWindow() is > called right before |this| is destroyed. Done. https://codereview.chromium.org/2129213003/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/scoped_transform_overview_window.cc:416: if (window()->GetLayer()->alpha_shape()) On 2016/07/09 21:23:21, tdanderson (OOO until July 18) wrote: > {} since call to reset() spans two lines Done. https://codereview.chromium.org/2129213003/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/scoped_transform_overview_window.cc:428: window()->GetLayer()->SetAlphaShape(base::WrapUnique(region)); On 2016/07/09 21:07:32, varkha wrote: > Self review: this could be done only for windows that need to mask a header > (i.e. non-zero TOP_VIEW_INSET) rather than for all windows. Done. https://codereview.chromium.org/2129213003/diff/20001/ash/common/wm/overview/... File ash/common/wm/overview/scoped_transform_overview_window.h (right): https://codereview.chromium.org/2129213003/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/scoped_transform_overview_window.h:140: // Original window shape. On 2016/07/09 21:23:21, tdanderson (OOO until July 18) wrote: > From line 416 in scoped_transform_overview_window.cc, it looks like not every > window has an alpha shape. Can you modify the comments on line 140 and 143 > accordingly? e.g., "The original window shape, if one exists" and "True after > the shape has been set, or after we determine that the window had no shape" or > something. Done. https://codereview.chromium.org/2129213003/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/scoped_transform_overview_window.h:141: std::unique_ptr<SkRegion> original_alpha_shape_; On 2016/07/09 21:23:21, tdanderson (OOO until July 18) wrote: > Maybe it's just because "alpha shape" is new terminology to me, but I think this > would be more readable if it were called |original_window_shape_|. Done. https://codereview.chromium.org/2129213003/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/scoped_transform_overview_window.h:144: bool alpha_shape_set_; On 2016/07/09 21:23:21, tdanderson (OOO until July 18) wrote: > I don't think you need this (see my comments below), but if you do, rename to > |original_alpha_shape_set_| (or |original_window_shape_set_| if you change the > other member's name). Done.
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by varkha@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...
Patch set 3 lgtm
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 varkha@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-md] Hides frame headers in overview mode not using a mask There is a problem with clearing mask layers when the textures are reused. Trying therefore to find a solution to hide the frame headers that does not rely on masks. BUG=624431 ========== to ========== [ash-md] Hides frame headers in overview mode not using a mask There is a problem with clearing mask layers when the textures are reused. Trying therefore to find a solution to hide the frame headers that does not rely on masks. BUG=624431 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Hides frame headers in overview mode not using a mask There is a problem with clearing mask layers when the textures are reused. Trying therefore to find a solution to hide the frame headers that does not rely on masks. BUG=624431 ========== to ========== [ash-md] Hides frame headers in overview mode not using a mask There is a problem with clearing mask layers when the textures are reused. Trying therefore to find a solution to hide the frame headers that does not rely on masks. BUG=624431 Committed: https://crrev.com/c54b330fedbd77ed0d54532ec98418f7ce7ff6a6 Cr-Commit-Position: refs/heads/master@{#404573} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c54b330fedbd77ed0d54532ec98418f7ce7ff6a6 Cr-Commit-Position: refs/heads/master@{#404573}
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Hides frame headers in overview mode not using a mask There is a problem with clearing mask layers when the textures are reused. Trying therefore to find a solution to hide the frame headers that does not rely on masks. BUG=624431 Committed: https://crrev.com/c54b330fedbd77ed0d54532ec98418f7ce7ff6a6 Cr-Commit-Position: refs/heads/master@{#404573} ========== to ========== [ash-md] Hides frame headers in overview mode not using a mask Mask layers are expensive and we will want to avoid using them when there are too many windows. Trying therefore to find a solution to hide the frame headers that does not rely on masks. Also fixes the mask update using SetFillsBoundsOpaquely(false). BUG=624431 Committed: https://crrev.com/c54b330fedbd77ed0d54532ec98418f7ce7ff6a6 Cr-Commit-Position: refs/heads/master@{#404573} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
