Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2633643002:
[ash-md] Reduces dimensions of texture layers in overview mode (Closed)
|
Created:
3 years, 11 months ago by varkha Modified:
3 years, 11 months ago Reviewers:
tdanderson CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ash-md] Reduces dimensions of texture layers in overview mode
This CL simplifies the view structure of the visible items in overview
mode and reduces the dimensions of the texture layers used.
Previously each item was a widget (with a default texture layer)
that was sllightly bigger than the window and had a LabelButton covering
most of the area with a custom background. That background was only made
non-transparent in a caption above the window so the texture layer was
in fact unnecessarily large.
With this change the widget is created with a ui::LAYER_NOT_DRAWN layer
and only the narrow visible background behind the caption has a texture
layer (necessary to implement rounded corners for the visible caption).
The views structure has also been simplified. The view is covered by a
simple button (to capture events) and a simple Label is used in place
of the LabelButton.
BUG=537050, 624608
TEST=NONE - no visible change
Review-Url: https://codereview.chromium.org/2633643002
Cr-Commit-Position: refs/heads/master@{#444129}
Committed: https://chromium.googlesource.com/chromium/src/+/d4824ca45ebcccbdd2869310f366c1b178a4be85
Patch Set 1 #Patch Set 2 : [ash-md] Reduces dimensions of texture layers in overview mode (test) #Patch Set 3 : [ash-md] Reduces dimensions of texture layers in overview mode (a11y test) #
Total comments: 22
Patch Set 4 : [ash-md] Reduces dimensions of texture layers in overview mode (nits) #Patch Set 5 : [ash-md] Reduces dimensions of texture layers in overview mode (test) #
Messages
Total messages: 35 (26 generated)
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@, this is another optimization inspired by reveman@'s doc. This should significantly reduce the overall memory consumption for the textures in overview mode. I imagine that before this change each window (consider that most users have most windows maximized) was represented in overview mode by a texture layer (in addition to the window itself) slightly larger than the original window. This change makes that layer NOT_DRAWN and only uses texture layers for the non-transparent portion of the text captions background. Please take a look, Thanks!
Description was changed from ========== [ash-md] Reduces dimensions of texture layers in overview mode This CL simplifies the view structure of the visible items in overview mode and reduces the dimensions of the texture layers used. Previously each item was a widget (with a default texture layer) that was sllightly bigger than the window and had a LabelButton covering most of the area with a custom background. That background was only made non-transparent in a caption above the window so the texture layer was in fact unnecessarily large. With this change the widget is created with a ui::LAYER_NOT_DRAWN layer and only the narrow visible background behind the caption has a texture layer (necessary to implement rounded corners for the visible caption). The views structure has also been simplified. The view is covered by a simple button (to capture events) and a simple Label is used in place of the LabelButton. BUG=537050 TEST=NONE - no visible change ========== to ========== [ash-md] Reduces dimensions of texture layers in overview mode This CL simplifies the view structure of the visible items in overview mode and reduces the dimensions of the texture layers used. Previously each item was a widget (with a default texture layer) that was sllightly bigger than the window and had a LabelButton covering most of the area with a custom background. That background was only made non-transparent in a caption above the window so the texture layer was in fact unnecessarily large. With this change the widget is created with a ui::LAYER_NOT_DRAWN layer and only the narrow visible background behind the caption has a texture layer (necessary to implement rounded corners for the visible caption). The views structure has also been simplified. The view is covered by a simple button (to capture events) and a simple Label is used in place of the LabelButton. BUG=537050, 624608 TEST=NONE - no visible change ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: This issue passed the CQ dry run.
To measure effectiveness of this I have installed a trace when animations start and end and counted number of times cc::Scheduler::DidCommit() gets called during the overview mode entry animation. I can definitely see an improvement [1] and it is variable depending on amount of work (number of windows). For some random browser window size I saw from 5% to 40% improvement in FPS with the biggest improvements in case of medium-high number of windows (8-24). With more windows the improvement was still significant, with fewer windows (1-4) the animation was smooth to begin with. So I think this is indeed a useful change. [1] https://docs.google.com/a/google.com/spreadsheets/d/1_PaXz0tsyA279TTRgVDOW3LJ...
To measure effectiveness of this I have installed a trace when animations start and end and counted number of times cc::Scheduler::DidCommit() gets called during the overview mode entry animation. I can definitely see an improvement [1] and it is variable depending on amount of work (number of windows). For some random browser window size I saw from 5% to 40% improvement in FPS with the biggest improvements in case of medium-high number of windows (8-24). With more windows the improvement was still significant, with fewer windows (1-4) the animation was smooth to begin with. So I think this is indeed a useful change. [1] https://docs.google.com/a/google.com/spreadsheets/d/1_PaXz0tsyA279TTRgVDOW3LJ...
LGTM with some comments below. The data collection you performed is much appreciated, thanks! https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:136: void ResetListener() { listener_ = nullptr; } I know this is re-use of existing code, but I'm not entirely convinced why we need to have the ability to reset the listener (i.e., why we need to call ResetListener() on line 446), and I don't find the comment on line 135 helpful. Can you give a quick explanation for what I'm missing? https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:147: class OverviewLabel : public views::Label { I wonder if introducing this class is really necessary since it does not have any of its own members and the only method it overrides is GetClassName(). Instead, consider instantiating an object as a views::Label followed immediately by lines 150-157. https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:149: OverviewLabel(const base::string16& text) : views::Label(text) { nit: explicit https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:156: // rendering. Does not actually set the label's background color. nit: line breaks in this comment block https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:302: const char* GetClassName() const override { return "RoundedContainerView"; } Thanks for adding this. https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:363: // Container View that has an item label and a close button as children. This could use some clarification about what CaptionContainerView owns: the label, close button, as well as the window region. With the comment as-is, line 392 makes it seem like |listener_button_| is only covering the label and close button. https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:375: background_->AddChildView(close_button_); optional: as you explained in person, it may be worthwhile providing a comment that the close button has a higher z-order than the listener button (i.e., is not obscured by the listener button) and is thus still able to receive its own events. https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:766: void WindowSelectorItem::UpdateButtonAccessibilityName() { I see you are removing IDS_ASH_OVERVIEW_CLOSE_ITEM_BUTTON_ACCESSIBLE_NAME which contained the string "Close". Does this mean that a ChromeVox user has no way of closing windows in overview mode (i.e., they can just traverse the windows and select one of them)? https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.h:160: // Updates the button's accessibility name. I assume that "button" here refers to the ShieldButton you have introduced in the .cc. But without looking at the .cc it's not clear what "button" means in this context for someone who is just reading through the header. Perhaps re-name this function (and update documentation) to just UpdateAccessibilityName() or similar? https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.h:196: std::unique_ptr<views::Widget> window_label_; So |window_label_| owns |caption_container_view_|, and then |caption_container_view_| owns both |window_label_view_| and |close_button_|, right? If so I find the naming a bit misleading here. Seeing |window_label_| in code makes me think it just represents the label and not the close button. Consider renaming |window_label_| to something more general, such as |window_header_widget_| or similar. https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.h:201: // Container view that owns |window_label_button_view_| and |close_button_|. nit: update |window_label_button_view_| to its new name
On 2017/01/16 at 17:45:03, varkha wrote: > To measure effectiveness of this I have installed a trace when animations start and end and counted number of times cc::Scheduler::DidCommit() gets called during the overview mode entry animation. I can definitely see an improvement [1] and it is variable depending on amount of work (number of windows). For some random browser window size I saw from 5% to 40% improvement in FPS with the biggest improvements in case of medium-high number of windows (8-24). With more windows the improvement was still significant, with fewer windows (1-4) the animation was smooth to begin with. > So I think this is indeed a useful change. > > [1] https://docs.google.com/a/google.com/spreadsheets/d/1_PaXz0tsyA279TTRgVDOW3LJ... Thanks for this data! Would it be possible to create an ash_perftests that would run this and other performance tests? We could then add that to the dashboard an track further improvements and regressions across important ChromeOS devices.
On 2017/01/17 02:07:18, reveman wrote: > On 2017/01/16 at 17:45:03, varkha wrote: > > To measure effectiveness of this I have installed a trace when animations > start and end and counted number of times cc::Scheduler::DidCommit() gets > called during the overview mode entry animation. I can definitely see an > improvement [1] and it is variable depending on amount of work (number of > windows). For some random browser window size I saw from 5% to 40% improvement > in FPS with the biggest improvements in case of medium-high number of windows > (8-24). With more windows the improvement was still significant, with fewer > windows (1-4) the animation was smooth to begin with. > > So I think this is indeed a useful change. > > > > [1] > https://docs.google.com/a/google.com/spreadsheets/d/1_PaXz0tsyA279TTRgVDOW3LJ... > > Thanks for this data! Would it be possible to create an ash_perftests that would > run this and other performance tests? We could then add that to the dashboard an > track further improvements and regressions across important ChromeOS devices. I am working now to generalize this for ui::LayerAnimations. I am trying to make an API that would be usable by front-end UI devs and would log UMA performance (smoothness factor) for desired animated surfaces. I'll keep you in the loop.
https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:136: void ResetListener() { listener_ = nullptr; } On 2017/01/16 23:43:29, tdanderson wrote: > I know this is re-use of existing code, but I'm not entirely convinced why we > need to have the ability to reset the listener (i.e., why we need to call > ResetListener() on line 446), and I don't find the comment on line 135 helpful. > Can you give a quick explanation for what I'm missing? Done (edited comment for clarity). https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:147: class OverviewLabel : public views::Label { On 2017/01/16 23:43:29, tdanderson wrote: > I wonder if introducing this class is really necessary since it does not have > any of its own members and the only method it overrides is GetClassName(). > Instead, consider instantiating an object as a views::Label followed immediately > by lines 150-157. Done. https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:149: OverviewLabel(const base::string16& text) : views::Label(text) { On 2017/01/16 23:43:29, tdanderson wrote: > nit: explicit Acknowledged. https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:156: // rendering. Does not actually set the label's background color. On 2017/01/16 23:43:29, tdanderson wrote: > nit: line breaks in this comment block Done. https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:302: const char* GetClassName() const override { return "RoundedContainerView"; } On 2017/01/16 23:43:29, tdanderson wrote: > Thanks for adding this. Acknowledged. https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:363: // Container View that has an item label and a close button as children. On 2017/01/16 23:43:29, tdanderson wrote: > This could use some clarification about what CaptionContainerView owns: the > label, close button, as well as the window region. With the comment as-is, line > 392 makes it seem like |listener_button_| is only covering the label and close > button. Done. https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:375: background_->AddChildView(close_button_); On 2017/01/16 23:43:29, tdanderson wrote: > optional: as you explained in person, it may be worthwhile providing a comment > that the close button has a higher z-order than the listener button (i.e., is > not obscured by the listener button) and is thus still able to receive its own > events. Done. https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:766: void WindowSelectorItem::UpdateButtonAccessibilityName() { On 2017/01/16 23:43:29, tdanderson wrote: > I see you are removing IDS_ASH_OVERVIEW_CLOSE_ITEM_BUTTON_ACCESSIBLE_NAME which > contained the string "Close". Does this mean that a ChromeVox user has no way of > closing windows in overview mode (i.e., they can just traverse the windows and > select one of them)? This was already the case. I feel that the Ctrl+W provides adequate albeit not discoverable way to close windows in overview using keyboard and having a tab stop for close button would hinder overview mode usability. IDS_ASH_OVERVIEW_CLOSE_ITEM_BUTTON_ACCESSIBLE_NAME was essentially unused for a while so I have removed it. https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector_item.h (right): https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.h:160: // Updates the button's accessibility name. On 2017/01/16 23:43:30, tdanderson wrote: > I assume that "button" here refers to the ShieldButton you have introduced in > the .cc. But without looking at the .cc it's not clear what "button" means in > this context for someone who is just reading through the header. Perhaps re-name > this function (and update documentation) to just UpdateAccessibilityName() or > similar? Done. https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.h:196: std::unique_ptr<views::Widget> window_label_; On 2017/01/16 23:43:30, tdanderson wrote: > So |window_label_| owns |caption_container_view_|, and then > |caption_container_view_| owns both |window_label_view_| and |close_button_|, > right? If so I find the naming a bit misleading here. Seeing |window_label_| in > code makes me think it just represents the label and not the close button. > Consider renaming |window_label_| to something more general, such as > |window_header_widget_| or similar. Done. https://codereview.chromium.org/2633643002/diff/40001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.h:201: // Container view that owns |window_label_button_view_| and |close_button_|. On 2017/01/16 23:43:30, tdanderson wrote: > nit: update |window_label_button_view_| to its new name Done.
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: This issue passed the CQ dry run.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org Link to the patchset: https://codereview.chromium.org/2633643002/#ps80001 (title: "[ash-md] Reduces dimensions of texture layers in overview mode (test)")
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": 80001, "attempt_start_ts": 1484685205021230,
"parent_rev": "47ce132c79896db4fe4832a96b097adc643e6e99", "commit_rev":
"d4824ca45ebcccbdd2869310f366c1b178a4be85"}
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Reduces dimensions of texture layers in overview mode This CL simplifies the view structure of the visible items in overview mode and reduces the dimensions of the texture layers used. Previously each item was a widget (with a default texture layer) that was sllightly bigger than the window and had a LabelButton covering most of the area with a custom background. That background was only made non-transparent in a caption above the window so the texture layer was in fact unnecessarily large. With this change the widget is created with a ui::LAYER_NOT_DRAWN layer and only the narrow visible background behind the caption has a texture layer (necessary to implement rounded corners for the visible caption). The views structure has also been simplified. The view is covered by a simple button (to capture events) and a simple Label is used in place of the LabelButton. BUG=537050, 624608 TEST=NONE - no visible change ========== to ========== [ash-md] Reduces dimensions of texture layers in overview mode This CL simplifies the view structure of the visible items in overview mode and reduces the dimensions of the texture layers used. Previously each item was a widget (with a default texture layer) that was sllightly bigger than the window and had a LabelButton covering most of the area with a custom background. That background was only made non-transparent in a caption above the window so the texture layer was in fact unnecessarily large. With this change the widget is created with a ui::LAYER_NOT_DRAWN layer and only the narrow visible background behind the caption has a texture layer (necessary to implement rounded corners for the visible caption). The views structure has also been simplified. The view is covered by a simple button (to capture events) and a simple Label is used in place of the LabelButton. BUG=537050, 624608 TEST=NONE - no visible change Review-Url: https://codereview.chromium.org/2633643002 Cr-Commit-Position: refs/heads/master@{#444129} Committed: https://chromium.googlesource.com/chromium/src/+/d4824ca45ebcccbdd2869310f366... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/d4824ca45ebcccbdd2869310f366... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
