Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(370)

Issue 2439093002: Reland of "Added common layout framework for system menu rows." (Closed)

Created:
4 years, 2 months ago by bruthig
Modified:
4 years, 1 month ago
Reviewers:
tdanderson, sky
CC:
chromium-reviews, kalyank, sadrul, tfarina
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Reland of "Added common layout framework for system menu rows." Added common layout framework for system menu rows. Currently most of the system menu rows define their own layouts even though many rows should have the same layout. Additionally we have to support two separate layout schemes while material design is being implemented. Thus this CL lays the ground work to make it easier to support multiple layout schemes and a centralized layout source. Follow-up TODO: - wire in the new layouts to the system menu rows BUG=657669 Originally landed here: https://chromiumcodereview.appspot.com/2414103003/ Reverted here: https://codereview.chromium.org/2441923004/ Committed: https://crrev.com/44fcf74b6ff76829c51a504693629c6278558bab Cr-Commit-Position: refs/heads/master@{#427851}

Patch Set 1 #

Patch Set 2 : Made the ThreeViewLayout own the container views. #

Total comments: 1

Patch Set 3 : Reworked ThreeViewLayout to a TriView that extends View instead of a LayoutManager. #

Total comments: 15

Patch Set 4 : Addressed review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1297 lines, -7 lines) Patch
M ash/BUILD.gn View 1 2 3 chunks +8 lines, -0 lines 0 comments Download
A ash/common/system/tray/size_range_layout.h View 1 chunk +112 lines, -0 lines 0 comments Download
A ash/common/system/tray/size_range_layout.cc View 1 chunk +103 lines, -0 lines 0 comments Download
A ash/common/system/tray/size_range_layout_unittest.cc View 1 chunk +182 lines, -0 lines 0 comments Download
M ash/common/system/tray/tray_constants.h View 1 chunk +12 lines, -0 lines 0 comments Download
M ash/common/system/tray/tray_constants.cc View 2 chunks +16 lines, -0 lines 0 comments Download
A ash/common/system/tray/tray_popup_layout_factory.h View 1 2 3 1 chunk +49 lines, -0 lines 0 comments Download
A ash/common/system/tray/tray_popup_layout_factory.cc View 1 2 1 chunk +85 lines, -0 lines 0 comments Download
A ash/common/system/tray/tri_view.h View 1 2 3 1 chunk +158 lines, -0 lines 0 comments Download
A ash/common/system/tray/tri_view.cc View 1 2 3 1 chunk +165 lines, -0 lines 0 comments Download
A ash/common/system/tray/tri_view_unittest.cc View 1 2 1 chunk +328 lines, -0 lines 0 comments Download
M ui/views/BUILD.gn View 1 chunk +2 lines, -0 lines 0 comments Download
A ui/views/test/test_layout_manager.h View 1 chunk +45 lines, -0 lines 0 comments Download
A ui/views/test/test_layout_manager.cc View 1 chunk +26 lines, -0 lines 0 comments Download
M ui/views/test/test_views.h View 2 chunks +2 lines, -2 lines 0 comments Download
M ui/views/test/test_views.cc View 1 chunk +4 lines, -5 lines 0 comments Download

Messages

Total messages: 26 (13 generated)
bruthig
sky@, can you please take a look at this re-land attempt? Patch set 1 is ...
4 years, 2 months ago (2016-10-21 15:51:02 UTC) #2
sky
You forced me to actually look at this now. Having a LayoutManager (ThreeViewLayout) add views ...
4 years, 2 months ago (2016-10-21 16:35:11 UTC) #5
bruthig
On 2016/10/21 16:35:11, sky wrote: > You forced me to actually look at this now. ...
4 years, 2 months ago (2016-10-21 18:49:47 UTC) #8
sky
On Fri, Oct 21, 2016 at 11:49 AM, <bruthig@chromium.org> wrote: > On 2016/10/21 16:35:11, sky ...
4 years, 2 months ago (2016-10-21 20:13:03 UTC) #9
bruthig
tdanderson@, I've reworked this CL quite a bit to re-land it. Can you please take ...
4 years, 2 months ago (2016-10-22 00:16:30 UTC) #13
sky
views LGTM https://codereview.chromium.org/2439093002/diff/40001/ash/common/system/tray/size_range_layout.h File ash/common/system/tray/size_range_layout.h (right): https://codereview.chromium.org/2439093002/diff/40001/ash/common/system/tray/size_range_layout.h#newcode47 ash/common/system/tray/size_range_layout.h:47: static gfx::Size AbsoluteMinSize(); style nit: style guide ...
4 years, 1 month ago (2016-10-24 15:33:13 UTC) #16
tdanderson
lgtm with comments below https://codereview.chromium.org/2439093002/diff/40001/ash/common/system/tray/tray_popup_layout_factory.h File ash/common/system/tray/tray_popup_layout_factory.h (right): https://codereview.chromium.org/2439093002/diff/40001/ash/common/system/tray/tray_popup_layout_factory.h#newcode40 ash/common/system/tray/tray_popup_layout_factory.h:40: // into a clickable View. ...
4 years, 1 month ago (2016-10-26 19:29:13 UTC) #17
bruthig
Addressed comments. Take a look if you wish but not necessary. https://codereview.chromium.org/2439093002/diff/40001/ash/common/system/tray/size_range_layout.h File ash/common/system/tray/size_range_layout.h (right): ...
4 years, 1 month ago (2016-10-26 21:24:16 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2439093002/60001
4 years, 1 month ago (2016-10-26 21:24:59 UTC) #21
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 1 month ago (2016-10-26 22:31:21 UTC) #22
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/44fcf74b6ff76829c51a504693629c6278558bab Cr-Commit-Position: refs/heads/master@{#427851}
4 years, 1 month ago (2016-10-26 22:33:38 UTC) #24
sky
https://codereview.chromium.org/2439093002/diff/40001/ash/common/system/tray/size_range_layout.h File ash/common/system/tray/size_range_layout.h (right): https://codereview.chromium.org/2439093002/diff/40001/ash/common/system/tray/size_range_layout.h#newcode47 ash/common/system/tray/size_range_layout.h:47: static gfx::Size AbsoluteMinSize(); On 2016/10/26 21:24:16, bruthig wrote: > ...
4 years, 1 month ago (2016-10-26 23:37:14 UTC) #25
alancutter (OOO until 2018)
4 years, 1 month ago (2016-10-27 04:20:50 UTC) #26
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2460443002/ by alancutter@chromium.org.

The reason for reverting is: TriViewTest.ViewsRemovedOnRemoveAllChildren is
leaking on Linux Chromium OS ASan LSan Tests (1)
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2....

Powered by Google App Engine
This is Rietveld 408576698