Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2414103003:
Added common layout framework for system menu rows. (Closed)
DescriptionAdded 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
Committed: https://crrev.com/d7d592f487f9e03222d66c449b3ca772210b2b0b
Cr-Commit-Position: refs/heads/master@{#426626}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added tests #Patch Set 3 : Merge branch 'master' into md_system_menu_layout_mgr #
Total comments: 51
Patch Set 4 : Addressed review comments and fixed some build errors. #Patch Set 5 : Removed VIEWS_EXPORT from TestLayoutManager and replaced TestView with StaticSizedView. #
Total comments: 10
Patch Set 6 : Addressed review comments. #Patch Set 7 : Moved |layout_manager_| destruction order in ~View(). #
Messages
Total messages: 49 (27 generated)
bruthig@chromium.org changed reviewers: + sky@chromium.org, tdanderson@chromium.org
tdanderson@, can you please take a look? sky@, I have a couple of specific questions for you regarding this change. 1. I see that you recently removed LayoutManager::Uninstalled(). Would you be opposed to adding it back in? Please see the ThreeViewLayout::Uninstalled() method for how I am using it. 2. Do you think either of the SizeRangeLayout or the ThreeViewLayout is general enough to be put in views or should we just leave it in ash?
You are literally the third person in a week to bring up layout managers. Do you have a design doc you can point me at for what you are trying to do? As to Uninstalled(). I removed it for two reasons. 1. the only code in it was DCHECKs. 2. If you look at code like you have here: 43 if (layout_manager_) 44 layout_manager_->Uninstalled(host_); 45 layout_manager_ = std::move(layout_manager); You can see that immediately after Uninstalled() is called the LayoutManager is destroyed. (assuming the actual layout_manager_ is different) So, can't the destructor do whatever Uninstalled() would do?
On 2016/10/14 16:24:26, sky wrote: > You are literally the third person in a week to bring up layout managers. Do you > have a design doc you can point me at for what you are trying to do? I don't but I can create one for the purposes of promoting these ones to views. Is it ok with you if I delay the design doc until after submitting this to ash and defer the OWNER review to tdanderson@ because he has all of the context already? Out of curiosity, who else is talking to you about LayoutManagers? Is there any potential synergy between our efforts? > > As to Uninstalled(). I removed it for two reasons. > > 1. the only code in it was DCHECKs. > 2. If you look at code like you have here: > > 43 if (layout_manager_) > 44 layout_manager_->Uninstalled(host_); > 45 layout_manager_ = std::move(layout_manager); > > You can see that immediately after Uninstalled() is called the LayoutManager is > destroyed. (assuming the actual layout_manager_ is different) So, can't the > destructor do whatever Uninstalled() would do? I guess my original concern was that the LayoutManager that is being uninstalled wasn't always going to be destroyed immediately afterwards but as you pointed out this appears to be true everywhere. Do you think this requirement is worthy of a comment somewhere? Perhaps in the LayoutManager class documentation?
https://codereview.chromium.org/2414103003/diff/1/ash/common/system/tray/size... File ash/common/system/tray/size_range_layout.h (right): https://codereview.chromium.org/2414103003/diff/1/ash/common/system/tray/size... ash/common/system/tray/size_range_layout.h:41: class SizeRangeLayout : public views::LayoutManager { Unless I'm missing something this class seems problematic. It isn't uncommon for views you may want to use with this class to override functions that go to the LayoutManager. Even the example you give doesn't work as ImageView overrides GetPreferredSize. https://codereview.chromium.org/2414103003/diff/1/ash/common/system/tray/thre... File ash/common/system/tray/three_view_layout.h (right): https://codereview.chromium.org/2414103003/diff/1/ash/common/system/tray/thre... ash/common/system/tray/three_view_layout.h:35: class ThreeViewLayout : public views::LayoutManager { How is this different than BoxLayout? I get that it's a more restrictive and focused version, but why not add the functionality you need to BoxLayout?
On 2016/10/14 17:07:01, bruthig wrote: > On 2016/10/14 16:24:26, sky wrote: > > You are literally the third person in a week to bring up layout managers. Do > you > > have a design doc you can point me at for what you are trying to do? > > I don't but I can create one for the purposes of promoting these ones to views. > Is it ok with you if I delay the design doc until after submitting this to ash > and defer the OWNER review to tdanderson@ because he has all of the context > already? > > Out of curiosity, who else is talking to you about LayoutManagers? Is there any > potential synergy between our efforts? Sorry, I didn't respond to this question. There are two projects that touching layout that I'm aware of: . kylixrd@ and robliao@ are working on making layout easier. Current work is around an alignment/anchor type layout manager. . ellyjones@ is looking to make dialogs match harmony spec, which entails going through a bunch of layouts and making them match. I encouraged here to consider looking into separating the definition of the layout from the code and into a resource format. > > > > > As to Uninstalled(). I removed it for two reasons. > > > > 1. the only code in it was DCHECKs. > > 2. If you look at code like you have here: > > > > 43 if (layout_manager_) > > 44 layout_manager_->Uninstalled(host_); > > 45 layout_manager_ = std::move(layout_manager); > > > > You can see that immediately after Uninstalled() is called the LayoutManager > is > > destroyed. (assuming the actual layout_manager_ is different) So, can't the > > destructor do whatever Uninstalled() would do? > > I guess my original concern was that the LayoutManager that is being uninstalled > wasn't always going to be destroyed immediately afterwards but as you pointed > out this appears to be true everywhere. Do you think this requirement is worthy > of a comment somewhere? Perhaps in the LayoutManager class documentation?
Thanks Scott, the alignment/anchor layout manager sounds interesting and possibly applicable to my use cases. I will reach out to kylixrd@/robliao@. I've added some comments about the ThreeViewLayout differentiators, please take a look. If I haven't convinced you then perhaps we can jump on VC or something real time to come to an agreement quicker :) https://codereview.chromium.org/2414103003/diff/1/ash/common/system/tray/size... File ash/common/system/tray/size_range_layout.h (right): https://codereview.chromium.org/2414103003/diff/1/ash/common/system/tray/size... ash/common/system/tray/size_range_layout.h:41: class SizeRangeLayout : public views::LayoutManager { On 2016/10/14 22:11:12, sky wrote: > Unless I'm missing something this class seems problematic. It isn't uncommon for > views you may want to use with this class to override functions that go to the > LayoutManager. Even the example you give doesn't work as ImageView overrides > GetPreferredSize. Ah right, I guess I need to update the example code to wrap the ImageView in a raw container View and apply the layout manager to the container view. https://codereview.chromium.org/2414103003/diff/1/ash/common/system/tray/thre... File ash/common/system/tray/three_view_layout.h (right): https://codereview.chromium.org/2414103003/diff/1/ash/common/system/tray/thre... ash/common/system/tray/three_view_layout.h:35: class ThreeViewLayout : public views::LayoutManager { On 2016/10/14 22:11:12, sky wrote: > How is this different than BoxLayout? I get that it's a more restrictive and > focused version, but why not add the functionality you need to BoxLayout? It's different in that each of the Container views have their own layout manager set on them whereas a BoxLayout cannot do this because each 'cell' is not backed by a View. In the BoxLayout it would be incorrect to apply a sub layout manager to an arbitrary child because the BoxLayout doesn't know if the child view is a container view or not. e.g. It should not apply layout to a LabelButton child. Another difference is that a client of the ThreeViewLayout can add child views to any of the Container views in any order they wish and they will be laid out within that containers space. This is beneficial if the client only wants to add a child view to the END container and ensure it is aligned with the View above it, for example in the system menu rows. It may be possible to push the min/max preferred size behavior into the BoxLayout but I don't think it makes sense to push these other characteristics there because the BoxLayout has no knowledge of the children views when they are added. i.e. the BoxLayout can be installed on any container view that already has the children views added and it will behave nicely. The ThreeViewLayout on the other hand requires all child views to be added through its API. (It may be worth considering to extend the 'Three' view property to an arbitrary 'N' view property. But this isn't something I would want to do in this CL.)
On 2016/10/17 15:53:12, bruthig wrote: > Thanks Scott, the alignment/anchor layout manager sounds interesting and > possibly applicable to my use cases. I will reach out to kylixrd@/robliao@. > > I've added some comments about the ThreeViewLayout differentiators, please take > a look. If I haven't convinced you then perhaps we can jump on VC or something > real time to come to an agreement quicker :) I don't need a design doc, but what I'm missing most is where you intend to use this and if it's a good fit. You say the 'system menu'. Do you mean the menu reachable from each chrome window, or the tray menu? If the later could you add an owner of that code to ensure this is a good fit? > https://codereview.chromium.org/2414103003/diff/1/ash/common/system/tray/size... > File ash/common/system/tray/size_range_layout.h (right): > > https://codereview.chromium.org/2414103003/diff/1/ash/common/system/tray/size... > ash/common/system/tray/size_range_layout.h:41: class SizeRangeLayout : public > views::LayoutManager { > On 2016/10/14 22:11:12, sky wrote: > > Unless I'm missing something this class seems problematic. It isn't uncommon > for > > views you may want to use with this class to override functions that go to the > > LayoutManager. Even the example you give doesn't work as ImageView overrides > > GetPreferredSize. > > Ah right, I guess I need to update the example code to wrap the ImageView in a > raw container View and apply the layout manager to the container view. > > https://codereview.chromium.org/2414103003/diff/1/ash/common/system/tray/thre... > File ash/common/system/tray/three_view_layout.h (right): > > https://codereview.chromium.org/2414103003/diff/1/ash/common/system/tray/thre... > ash/common/system/tray/three_view_layout.h:35: class ThreeViewLayout : public > views::LayoutManager { > On 2016/10/14 22:11:12, sky wrote: > > How is this different than BoxLayout? I get that it's a more restrictive and > > focused version, but why not add the functionality you need to BoxLayout? > > It's different in that each of the Container views have their own layout manager > set on them whereas a BoxLayout cannot do this because each 'cell' is not backed > by a View. In the BoxLayout it would be incorrect to apply a sub layout manager > to an arbitrary child because the BoxLayout doesn't know if the child view is a > container view or not. e.g. It should not apply layout to a LabelButton child. > > Another difference is that a client of the ThreeViewLayout can add child views > to any of the Container views in any order they wish and they will be laid out > within that containers space. This is beneficial if the client only wants to > add a child view to the END container and ensure it is aligned with the View > above it, for example in the system menu rows. > > It may be possible to push the min/max preferred size behavior into the > BoxLayout but I don't think it makes sense to push these other characteristics > there because the BoxLayout has no knowledge of the children views when they are > added. i.e. the BoxLayout can be installed on any container view that already > has the children views added and it will behave nicely. The ThreeViewLayout on > the other hand requires all child views to be added through its API. > > (It may be worth considering to extend the 'Three' view property to an arbitrary > 'N' view property. But this isn't something I would want to do in this CL.)
On 2016/10/17 16:20:54, sky wrote: > On 2016/10/17 15:53:12, bruthig wrote: > > Thanks Scott, the alignment/anchor layout manager sounds interesting and > > possibly applicable to my use cases. I will reach out to kylixrd@/robliao@. > > > > I've added some comments about the ThreeViewLayout differentiators, please > take > > a look. If I haven't convinced you then perhaps we can jump on VC or > something > > real time to come to an agreement quicker :) > > I don't need a design doc, but what I'm missing most is where you intend to use > this and if it's a good fit. You say the 'system menu'. Do you mean the menu > reachable from each chrome window, or the tray menu? If the later could you add > an owner of that code to ensure this is a good fit? > I'm referring to the tray menu. tdanderson@, can you take a look and see if this is a good fit?
On 2016/10/17 16:27:57, bruthig wrote: > On 2016/10/17 16:20:54, sky wrote: > > On 2016/10/17 15:53:12, bruthig wrote: > > > Thanks Scott, the alignment/anchor layout manager sounds interesting and > > > possibly applicable to my use cases. I will reach out to kylixrd@/robliao@. > > > > > > I've added some comments about the ThreeViewLayout differentiators, please > > take > > > a look. If I haven't convinced you then perhaps we can jump on VC or > > something > > > real time to come to an agreement quicker :) > > > > I don't need a design doc, but what I'm missing most is where you intend to > use > > this and if it's a good fit. You say the 'system menu'. Do you mean the menu > > reachable from each chrome window, or the tray menu? If the later could you > add > > an owner of that code to ensure this is a good fit? > > > > I'm referring to the tray menu. > > tdanderson@, can you take a look and see if this is a good fit? Yes, I think this approach is a good fit for the layout of the various system menu rows. It can also eventually be used to simplify the layout code in the other native Ash bubbles (IME, palette, anything else someone wants to add). Ben, let me know when this is ready and I will give a more detailed review.
Description was changed from ========== 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. TODO in this CL - add tests Follow-up TODO: - wire in the new layouts to the system menu rows BUG= ========== to ========== 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= ==========
The CQ bit was checked by bruthig@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...
Terry, this is ready for full review, can you please take a look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Nice, I'm looking forward to seeing this in action. ash/ parts LGTM with some questions and comments below. https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/size_range_layout.cc (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/size_range_layout.cc:53: min_size_.SetToMax(gfx::Size()); You probably wanted this to be |max_size_| https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/size_range_layout.cc:76: // TODO(bruthig): Handle Views that override GetPreferredSize(). As discussed in person, can you append something like "... to make SizeRangeLayout suitable for general use in views" https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/size_range_layout.cc:95: if (layout_manager_) Regarding your 'if (layout_manager_)' checks in this file: if I understand correctly, the only way |layout_manager_| can be null is if someone called SetLayoutManager() with a null layout manager. Is that right? If so, how would you feel about enforcing a non-null parameter in SetLayoutManager() with a CHECK? https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/size_range_layout.h (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/size_range_layout.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Can you include a bug number in the CL description? Maybe file a new one to track changes common to the layout of various rows. https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/size_range_layout.h:23: // LayoutManager owned by this. i.e. this can be used to override the preferred nit: |this| instead of 'this'. https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/size_range_layout.h:46: static const int kMinSize; I don't think that |kMinSize| and |kMaxSize| need to be exposed in the header. Perhaps they could just be defined locally in the MinSize() and MaxSize() functions instead? https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/size_range_layout.h:57: static gfx::Size MaxSize(); I'm also not sure whether MinSize() and MaxSize() really need to be exposed in the header either. It seems the only place they're used is by SizeRangeLayoutTest. Maybe move them private since SRLT is a friend? If you do move them to private, consider adding public ResetMinSize() and ResetMaxSize() functions (which would call out to set_min/max_size() as you describe in the comments above). If you are keeping MinSize() and MaxSize(), I suggest naming them something different, since I think their current names make them too much like accessors. nit: set_min_size() in the documentation should be SetMinSize() instead. https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/size_range_layout_unittest.cc (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/size_range_layout_unittest.cc:137: TEST_F(SizeRangeLayoutTest, MaxSizeTakesPrecedenceOverMinSize) { Does max size win here because it was set after the min size? If so, I find it a bit worrying that the order of calls can matter. Say the current min size is (3, 5) and the current max size is (12, 11). I see two cases that are ill-defined: (1) If I try to set the max size to be (1, 2); here 1<3 and 2<5. (2) If I try to set the min size to be (10, 20); here only one of the values is problematic since 20>11. Consider some possible alternative ways of handling these, and make the expected behavior clear in the header documentation. One possibility could be to adjust both min and max sizes, e.g. In (1), the new min and max sizes both become (1, 2). In (2), the new min is (10, 20) and the new max becomes (12, 20). Though I'm not entirely sold on that because it violates a previously-set min or max constraint. You could instead make such operations no-ops or fail with a CHECK. https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/three_view_layout.cc (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/three_view_layout.cc:24: return views::BoxLayout::kVertical; A possible future consideration is to have both layout managers share the same enum. https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/three_view_layout.cc:82: views::View* container_view = GetContainer(container); nit: condense to one line? https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/three_view_layout.h (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/three_view_layout.h:34: // on each container with the same orientation as this has been created with. nit: |this| https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/three_view_layout.h:45: enum Container { START, CENTER, END }; Using enum class is preferred to enum (ditto for Orientation) https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/three_view_layout.h:87: // Sets the |insets| that all of the container spaces will be laid out within. Just reading the documentation here I find it a bit ambiguous. Are |insets| being applied individually to each of the three containers, or does |insets| surround the three containers as a group? https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/three_view_layout.h:94: // not-visible its space will be distributed to the other visible containers. And I assume that if a container transitions from not-visible to visible then the space is re-distributed accordingly across all three containers? (nit: consider mentioning this here) https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/three_view_layout_unittest.cc (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/three_view_layout_unittest.cc:18: class TestView : public views::View { should you specify a destructor for TestView and ThreeViewLayoutTest ? https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/three_view_layout_unittest.cc:93: EXPECT_EQ(26, GetBoundsInHost(end_child).x()); Consider specifying these as constants derived from the other constants you use in the test rather than as magic numbers. https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/three_view_layout_unittest.cc:265: TEST_F(ThreeViewLayoutTest, NonZeroFlexTakesPrecedenceOverMaxSize) { Can you document this expected precedence behavior somewhere in the ThreeViewLayout header? https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/tray_constants.cc (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/tray_constants.cc:106: const int kTrayPopupItemMinEndWidth[] = {48, 48, 48}; optional: share 48 with kMenuButtonSize https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/tray_popup_layout_factory.cc (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/tray_popup_layout_factory.cc:50: // be removed. This would allow us to drop the |host| parameter from this nit: wrap this in a TODO https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/tray_popup_layout_factory.h (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/tray_popup_layout_factory.h:43: static void ConfigureDefaultLayout(ThreeViewLayout* layout, Maybe it's the function name or the description, but I'm having a hard time understanding when and how this would be used by something other than as a helper function for InstallDefaultLayout(). Can you please clarify?
Description was changed from ========== 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= ========== to ========== 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 ==========
The CQ bit was checked by bruthig@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
tdanderson@, I've made some changes, can you take a second look? sky@, can you take a look at: - ash/BUILD.gn - ui/views/* https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... File ash/common/system/tray/size_range_layout.cc (right): https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/size_range_layout.cc:53: min_size_.SetToMax(gfx::Size()); On 2016/10/19 18:30:59, tdanderson wrote: > You probably wanted this to be |max_size_| D'oh, done! https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/size_range_layout.cc:76: // TODO(bruthig): Handle Views that override GetPreferredSize(). On 2016/10/19 18:30:59, tdanderson wrote: > As discussed in person, can you append something like "... to make > SizeRangeLayout suitable for general use in views" Removed. https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/size_range_layout.cc:95: if (layout_manager_) On 2016/10/19 18:30:59, tdanderson wrote: > Regarding your 'if (layout_manager_)' checks in this file: if I understand > correctly, the only way |layout_manager_| can be null is if someone called > SetLayoutManager() with a null layout manager. Is that right? If so, how would > you feel about enforcing a non-null parameter in SetLayoutManager() with a > CHECK? I've changed it to a DCHECK(). Do you think it's worth of a CHECK()? With this change I am able to remove those two TODO's regarding Views preferred size/height. :D https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... File ash/common/system/tray/size_range_layout.h (right): https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/size_range_layout.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/10/19 18:30:59, tdanderson wrote: > Can you include a bug number in the CL description? Maybe file a new one to > track changes common to the layout of various rows. Done. https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/size_range_layout.h:23: // LayoutManager owned by this. i.e. this can be used to override the preferred On 2016/10/19 18:30:59, tdanderson wrote: > nit: |this| instead of 'this'. Done. https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/size_range_layout.h:46: static const int kMinSize; On 2016/10/19 18:30:59, tdanderson wrote: > I don't think that |kMinSize| and |kMaxSize| need to be exposed in the header. > Perhaps they could just be defined locally in the MinSize() and MaxSize() > functions instead? Done. https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/size_range_layout.h:57: static gfx::Size MaxSize(); On 2016/10/19 18:30:59, tdanderson wrote: > I'm also not sure whether MinSize() and MaxSize() really need to be exposed in > the header either. It seems the only place they're used is by > SizeRangeLayoutTest. Maybe move them private since SRLT is a friend? If you do > move them to private, consider adding public ResetMinSize() and ResetMaxSize() > functions (which would call out to set_min/max_size() as you describe in the > comments above). > > If you are keeping MinSize() and MaxSize(), I suggest naming them something > different, since I think their current names make them too much like accessors. > > nit: set_min_size() in the documentation should be SetMinSize() instead. I don't want to move them to private because I envision them being used by other clients if they ever want to un-set a min/max OR more likely they want to only set a min/max for one axis. The following example would effectively only set an x-axis max size. gfx::Size max_size = SRL::MaxSize(); max_size.set_width(25); layout.SetMaxSize(max_size); I've renamed them, WDYT? https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... File ash/common/system/tray/size_range_layout_unittest.cc (right): https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/size_range_layout_unittest.cc:137: TEST_F(SizeRangeLayoutTest, MaxSizeTakesPrecedenceOverMinSize) { On 2016/10/19 18:30:59, tdanderson wrote: > Does max size win here because it was set after the min size? If so, I find it a > bit worrying that the order of calls can matter. Say the current min size is (3, > 5) and the current max size is (12, 11). I see two cases that are ill-defined: > > (1) If I try to set the max size to be (1, 2); here 1<3 and 2<5. > > (2) If I try to set the min size to be (10, 20); here only one of the values is > problematic since 20>11. > > Consider some possible alternative ways of handling these, and make the expected > behavior clear in the header documentation. One possibility could be to adjust > both min and max sizes, e.g. > > In (1), the new min and max sizes both become (1, 2). > > In (2), the new min is (10, 20) and the new max becomes (12, 20). > > Though I'm not entirely sold on that because it violates a previously-set min or > max constraint. You could instead make such operations no-ops or fail with a > CHECK. MaxSize wins because of the implementation of SizeRangeLayout::ClampSizeToRange(), it has nothing to do with the order in which the client calls Set(Min|Max)Size(). I've made it so that SetMinSize() may update the |max_size_| if the new |min_size| is larger than the |max_size_|, and similarly for the opposite way. I think this is the only other reasonable option and it's easier to understand. I feel like any of the other suggested options make the API much more difficult for clients to work with deterministically. https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... File ash/common/system/tray/three_view_layout.cc (right): https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/three_view_layout.cc:24: return views::BoxLayout::kVertical; On 2016/10/19 18:30:59, tdanderson wrote: > A possible future consideration is to have both layout managers share the same > enum. Acknowledged. https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/three_view_layout.cc:82: views::View* container_view = GetContainer(container); On 2016/10/19 18:30:59, tdanderson wrote: > nit: condense to one line? Done. https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... File ash/common/system/tray/three_view_layout.h (right): https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/three_view_layout.h:34: // on each container with the same orientation as this has been created with. On 2016/10/19 18:30:59, tdanderson wrote: > nit: |this| Done. https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/three_view_layout.h:45: enum Container { START, CENTER, END }; On 2016/10/19 18:30:59, tdanderson wrote: > Using enum class is preferred to enum (ditto for Orientation) Done. https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/three_view_layout.h:87: // Sets the |insets| that all of the container spaces will be laid out within. On 2016/10/19 18:30:59, tdanderson wrote: > Just reading the documentation here I find it a bit ambiguous. Are |insets| > being applied individually to each of the three containers, or does |insets| > surround the three containers as a group? Updated, WDYT? https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/three_view_layout.h:94: // not-visible its space will be distributed to the other visible containers. On 2016/10/19 18:30:59, tdanderson wrote: > And I assume that if a container transitions from not-visible to visible then > the space is re-distributed accordingly across all three containers? (nit: > consider mentioning this here) Updated. WDYT? https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... File ash/common/system/tray/three_view_layout_unittest.cc (right): https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/three_view_layout_unittest.cc:18: class TestView : public views::View { On 2016/10/19 18:31:00, tdanderson wrote: > should you specify a destructor for TestView and ThreeViewLayoutTest ? I don't think they are necessary, are they? https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/three_view_layout_unittest.cc:93: EXPECT_EQ(26, GetBoundsInHost(end_child).x()); On 2016/10/19 18:31:00, tdanderson wrote: > Consider specifying these as constants derived from the other constants you use > in the test rather than as magic numbers. Updated along with some of the other tests. Let me know if I haven't made them more clear. https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/three_view_layout_unittest.cc:265: TEST_F(ThreeViewLayoutTest, NonZeroFlexTakesPrecedenceOverMaxSize) { On 2016/10/19 18:30:59, tdanderson wrote: > Can you document this expected precedence behavior somewhere in the > ThreeViewLayout header? Done. And added another test because this applies to min size constraints as well. PTAL. https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_constants.cc (right): https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_constants.cc:106: const int kTrayPopupItemMinEndWidth[] = {48, 48, 48}; On 2016/10/19 18:31:00, tdanderson wrote: > optional: share 48 with kMenuButtonSize These will get reworked in a follow-up CL. https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_popup_layout_factory.cc (right): https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_popup_layout_factory.cc:50: // be removed. This would allow us to drop the |host| parameter from this On 2016/10/19 18:31:00, tdanderson wrote: > nit: wrap this in a TODO Done. https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_popup_layout_factory.h (right): https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_popup_layout_factory.h:43: static void ConfigureDefaultLayout(ThreeViewLayout* layout, On 2016/10/19 18:31:00, tdanderson wrote: > Maybe it's the function name or the description, but I'm having a hard time > understanding when and how this would be used by something other than as a > helper function for InstallDefaultLayout(). Can you please clarify? I envision this being used by menu rows that have more than one targetable space. It will allow these rows to set the layout for each targetable container (e.g. the audio row) Example: VolumeView::VolumeView() { ... mute_button_ = new CustomButton(); ThreeViewLayout* start_layout = new ThreeViewLayout(); TrayPopupLayoutFactory::ConfigureDefaultLayout(start_layout, START); mute_button_->SetLayoutManager(end_layout); start_layout.AddView(START, head_phone_image); center_view_ = new views::View(); ThreeViewLayout* center_layout = new ThreeViewLayout(); TrayPopupLayoutFactory::ConfigureDefaultLayout(center_layout, CENTER); center_view_->SetLayoutManager(center_layout); center_layout->AddView(CENTER, slider); more_region_ = views::View(); ThreeViewLayout* end_layout = new ThreeViewLayout(); TrayPopupLayoutFactory::ConfigureDefaultLayout(end_layout, END); more_region_.SetLayoutManager(end_layout); end_layout->AddView(END, detail_view_button_); ... } Do you think adding an example like this to the docs would help?
The CQ bit was checked by bruthig@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.
https://codereview.chromium.org/2414103003/diff/80001/ui/views/test/test_layo... File ui/views/test/test_layout_manager.h (right): https://codereview.chromium.org/2414103003/diff/80001/ui/views/test/test_layo... ui/views/test/test_layout_manager.h:34: gfx::Size preferred_size_ = gfx::Size(0, 0); Why the = gfx::Size(0, 0)? Isn't that the default? https://codereview.chromium.org/2414103003/diff/80001/ui/views/test/test_views.h File ui/views/test/test_views.h (right): https://codereview.chromium.org/2414103003/diff/80001/ui/views/test/test_view... ui/views/test/test_views.h:20: explicit StaticSizedView(const gfx::Size& size); How about making the size an optional parameter? Also, can you rename size to preferred_size as that better conveys which size this sets. https://codereview.chromium.org/2414103003/diff/80001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/2414103003/diff/80001/ui/views/view.cc#newcod... ui/views/view.cc:131: // Some layout managers hold a reference to the host that they are installed I have a mild preference to move this after deleting the children. That's what we do in ~Window. Does that work for your case?
https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/size_range_layout.cc (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/size_range_layout.cc:95: if (layout_manager_) On 2016/10/20 04:40:12, bruthig wrote: > On 2016/10/19 18:30:59, tdanderson wrote: > > Regarding your 'if (layout_manager_)' checks in this file: if I understand > > correctly, the only way |layout_manager_| can be null is if someone called > > SetLayoutManager() with a null layout manager. Is that right? If so, how would > > you feel about enforcing a non-null parameter in SetLayoutManager() with a > > CHECK? > > I've changed it to a DCHECK(). Do you think it's worth of a CHECK()? > > With this change I am able to remove those two TODO's regarding Views preferred > size/height. :D A DCHECK should be fine. https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/size_range_layout.h (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/size_range_layout.h:57: static gfx::Size MaxSize(); On 2016/10/20 04:40:12, bruthig wrote: > On 2016/10/19 18:30:59, tdanderson wrote: > > I'm also not sure whether MinSize() and MaxSize() really need to be exposed in > > the header either. It seems the only place they're used is by > > SizeRangeLayoutTest. Maybe move them private since SRLT is a friend? If you do > > move them to private, consider adding public ResetMinSize() and ResetMaxSize() > > functions (which would call out to set_min/max_size() as you describe in the > > comments above). > > > > If you are keeping MinSize() and MaxSize(), I suggest naming them something > > different, since I think their current names make them too much like > accessors. > > > > nit: set_min_size() in the documentation should be SetMinSize() instead. > > I don't want to move them to private because I envision them being used by other > clients if they ever want to un-set a min/max OR more likely they want to only > set a min/max for one axis. The following example would effectively only set an > x-axis max size. > > gfx::Size max_size = SRL::MaxSize(); > max_size.set_width(25); > layout.SetMaxSize(max_size); > > I've renamed them, WDYT? OK, sounds reasonable and the new names lg. https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/size_range_layout_unittest.cc (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/size_range_layout_unittest.cc:137: TEST_F(SizeRangeLayoutTest, MaxSizeTakesPrecedenceOverMinSize) { On 2016/10/20 04:40:12, bruthig wrote: > On 2016/10/19 18:30:59, tdanderson wrote: > > Does max size win here because it was set after the min size? If so, I find it > a > > bit worrying that the order of calls can matter. Say the current min size is > (3, > > 5) and the current max size is (12, 11). I see two cases that are ill-defined: > > > > (1) If I try to set the max size to be (1, 2); here 1<3 and 2<5. > > > > (2) If I try to set the min size to be (10, 20); here only one of the values > is > > problematic since 20>11. > > > > Consider some possible alternative ways of handling these, and make the > expected > > behavior clear in the header documentation. One possibility could be to adjust > > both min and max sizes, e.g. > > > > In (1), the new min and max sizes both become (1, 2). > > > > In (2), the new min is (10, 20) and the new max becomes (12, 20). > > > > Though I'm not entirely sold on that because it violates a previously-set min > or > > max constraint. You could instead make such operations no-ops or fail with a > > CHECK. > > MaxSize wins because of the implementation of > SizeRangeLayout::ClampSizeToRange(), it has nothing to do with the order in > which the client calls Set(Min|Max)Size(). > > I've made it so that SetMinSize() may update the |max_size_| if the new > |min_size| is larger than the |max_size_|, and similarly for the opposite way. > I think this is the only other reasonable option and it's easier to understand. > I feel like any of the other suggested options make the API much more difficult > for clients to work with deterministically. Works for me, thanks. https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/three_view_layout.h (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/three_view_layout.h:87: // Sets the |insets| that all of the container spaces will be laid out within. On 2016/10/20 04:40:12, bruthig wrote: > On 2016/10/19 18:30:59, tdanderson wrote: > > Just reading the documentation here I find it a bit ambiguous. Are |insets| > > being applied individually to each of the three containers, or does |insets| > > surround the three containers as a group? > > Updated, WDYT? New description is much clearer, thanks https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/three_view_layout.h:94: // not-visible its space will be distributed to the other visible containers. On 2016/10/20 04:40:12, bruthig wrote: > On 2016/10/19 18:30:59, tdanderson wrote: > > And I assume that if a container transitions from not-visible to visible then > > the space is re-distributed accordingly across all three containers? (nit: > > consider mentioning this here) > > Updated. WDYT? Looks good https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/three_view_layout_unittest.cc (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/three_view_layout_unittest.cc:18: class TestView : public views::View { On 2016/10/20 04:40:12, bruthig wrote: > On 2016/10/19 18:31:00, tdanderson wrote: > > should you specify a destructor for TestView and ThreeViewLayoutTest ? > > I don't think they are necessary, are they? My mistake, they're not. https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/three_view_layout_unittest.cc:93: EXPECT_EQ(26, GetBoundsInHost(end_child).x()); On 2016/10/20 04:40:12, bruthig wrote: > On 2016/10/19 18:31:00, tdanderson wrote: > > Consider specifying these as constants derived from the other constants you > use > > in the test rather than as magic numbers. > > Updated along with some of the other tests. Let me know if I haven't made them > more clear. lg, thanks https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/three_view_layout_unittest.cc:265: TEST_F(ThreeViewLayoutTest, NonZeroFlexTakesPrecedenceOverMaxSize) { On 2016/10/20 04:40:12, bruthig wrote: > On 2016/10/19 18:30:59, tdanderson wrote: > > Can you document this expected precedence behavior somewhere in the > > ThreeViewLayout header? > > Done. And added another test because this applies to min size constraints as > well. PTAL. lg, thanks https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/tray_constants.cc (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/tray_constants.cc:106: const int kTrayPopupItemMinEndWidth[] = {48, 48, 48}; On 2016/10/20 04:40:13, bruthig wrote: > On 2016/10/19 18:31:00, tdanderson wrote: > > optional: share 48 with kMenuButtonSize > > These will get reworked in a follow-up CL. Acknowledged. https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... File ash/common/system/tray/tray_popup_layout_factory.h (right): https://chromiumcodereview.appspot.com/2414103003/diff/40001/ash/common/syste... ash/common/system/tray/tray_popup_layout_factory.h:43: static void ConfigureDefaultLayout(ThreeViewLayout* layout, On 2016/10/20 04:40:13, bruthig wrote: > On 2016/10/19 18:31:00, tdanderson wrote: > > Maybe it's the function name or the description, but I'm having a hard time > > understanding when and how this would be used by something other than as a > > helper function for InstallDefaultLayout(). Can you please clarify? > > I envision this being used by menu rows that have more than one targetable > space. It will allow these rows to set the layout for each targetable container > (e.g. the audio row) > > Example: > > VolumeView::VolumeView() { > ... > mute_button_ = new CustomButton(); > ThreeViewLayout* start_layout = new ThreeViewLayout(); > TrayPopupLayoutFactory::ConfigureDefaultLayout(start_layout, START); > mute_button_->SetLayoutManager(end_layout); > start_layout.AddView(START, head_phone_image); > > center_view_ = new views::View(); > ThreeViewLayout* center_layout = new ThreeViewLayout(); > TrayPopupLayoutFactory::ConfigureDefaultLayout(center_layout, CENTER); > center_view_->SetLayoutManager(center_layout); > center_layout->AddView(CENTER, slider); > > more_region_ = views::View(); > ThreeViewLayout* end_layout = new ThreeViewLayout(); > TrayPopupLayoutFactory::ConfigureDefaultLayout(end_layout, END); > more_region_.SetLayoutManager(end_layout); > end_layout->AddView(END, detail_view_button_); > ... > } > > Do you think adding an example like this to the docs would help? Following up on our discussion in person, let's defer the decision of whether to make this method public or private until you start implementing the new audio row. https://chromiumcodereview.appspot.com/2414103003/diff/80001/ash/common/syste... File ash/common/system/tray/three_view_layout.h (right): https://chromiumcodereview.appspot.com/2414103003/diff/80001/ash/common/syste... ash/common/system/tray/three_view_layout.h:107: // Note that positive flex values will take precedence over size constraints. micro nit: 'non-zero' instead of 'positive' since you've already specified that flex values are not negative.
The CQ bit was checked by bruthig@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...
sky@, can you please take another look? https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_popup_layout_factory.h (right): https://codereview.chromium.org/2414103003/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_popup_layout_factory.h:43: static void ConfigureDefaultLayout(ThreeViewLayout* layout, On 2016/10/20 18:16:49, tdanderson wrote: > On 2016/10/20 04:40:13, bruthig wrote: > > On 2016/10/19 18:31:00, tdanderson wrote: > > > Maybe it's the function name or the description, but I'm having a hard time > > > understanding when and how this would be used by something other than as a > > > helper function for InstallDefaultLayout(). Can you please clarify? > > > > I envision this being used by menu rows that have more than one targetable > > space. It will allow these rows to set the layout for each targetable > container > > (e.g. the audio row) > > > > Example: > > > > VolumeView::VolumeView() { > > ... > > mute_button_ = new CustomButton(); > > ThreeViewLayout* start_layout = new ThreeViewLayout(); > > TrayPopupLayoutFactory::ConfigureDefaultLayout(start_layout, START); > > mute_button_->SetLayoutManager(end_layout); > > start_layout.AddView(START, head_phone_image); > > > > center_view_ = new views::View(); > > ThreeViewLayout* center_layout = new ThreeViewLayout(); > > TrayPopupLayoutFactory::ConfigureDefaultLayout(center_layout, CENTER); > > center_view_->SetLayoutManager(center_layout); > > center_layout->AddView(CENTER, slider); > > > > more_region_ = views::View(); > > ThreeViewLayout* end_layout = new ThreeViewLayout(); > > TrayPopupLayoutFactory::ConfigureDefaultLayout(end_layout, END); > > more_region_.SetLayoutManager(end_layout); > > end_layout->AddView(END, detail_view_button_); > > ... > > } > > > > Do you think adding an example like this to the docs would help? > > Following up on our discussion in person, let's defer the decision of whether to > make this method public or private until you start implementing the new audio > row. Acknowledged. https://codereview.chromium.org/2414103003/diff/80001/ash/common/system/tray/... File ash/common/system/tray/three_view_layout.h (right): https://codereview.chromium.org/2414103003/diff/80001/ash/common/system/tray/... ash/common/system/tray/three_view_layout.h:107: // Note that positive flex values will take precedence over size constraints. On 2016/10/20 18:16:49, tdanderson wrote: > micro nit: 'non-zero' instead of 'positive' since you've already specified that > flex values are not negative. Done. https://codereview.chromium.org/2414103003/diff/80001/ui/views/test/test_layo... File ui/views/test/test_layout_manager.h (right): https://codereview.chromium.org/2414103003/diff/80001/ui/views/test/test_layo... ui/views/test/test_layout_manager.h:34: gfx::Size preferred_size_ = gfx::Size(0, 0); On 2016/10/20 15:59:11, sky wrote: > Why the = gfx::Size(0, 0)? Isn't that the default? Removed. https://codereview.chromium.org/2414103003/diff/80001/ui/views/test/test_views.h File ui/views/test/test_views.h (right): https://codereview.chromium.org/2414103003/diff/80001/ui/views/test/test_view... ui/views/test/test_views.h:20: explicit StaticSizedView(const gfx::Size& size); On 2016/10/20 15:59:11, sky wrote: > How about making the size an optional parameter? Also, can you rename size to > preferred_size as that better conveys which size this sets. Done. https://codereview.chromium.org/2414103003/diff/80001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/2414103003/diff/80001/ui/views/view.cc#newcod... ui/views/view.cc:131: // Some layout managers hold a reference to the host that they are installed On 2016/10/20 15:59:11, sky wrote: > I have a mild preference to move this after deleting the children. That's what > we do in ~Window. Does that work for your case? The risk in doing so is that View may try to access a deleted child but I was able to include a test to guard against this so I've changed it as requested.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2414103003/diff/80001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/2414103003/diff/80001/ui/views/view.cc#newcod... ui/views/view.cc:131: // Some layout managers hold a reference to the host that they are installed On 2016/10/20 19:01:34, bruthig wrote: > On 2016/10/20 15:59:11, sky wrote: > > I have a mild preference to move this after deleting the children. That's what > > we do in ~Window. Does that work for your case? > > The risk in doing so is that View may try to access a deleted child but I was > able to include a test to guard against this so I've changed it as requested. Now that I look at this code again I think the way you had it is better, because apparently we won't call LayoutManager::ViewRemoved() because of the parent = NULL below. So, I think either move the reset where you had it, or make ViewRemoved be called. Sorry for the run around.
The CQ bit was checked by bruthig@chromium.org to run a CQ dry run
sky@, can you please take another look? https://codereview.chromium.org/2414103003/diff/80001/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/2414103003/diff/80001/ui/views/view.cc#newcod... ui/views/view.cc:131: // Some layout managers hold a reference to the host that they are installed On 2016/10/20 20:42:22, sky wrote: > On 2016/10/20 19:01:34, bruthig wrote: > > On 2016/10/20 15:59:11, sky wrote: > > > I have a mild preference to move this after deleting the children. That's > what > > > we do in ~Window. Does that work for your case? > > > > The risk in doing so is that View may try to access a deleted child but I was > > able to include a test to guard against this so I've changed it as requested. > > Now that I look at this code again I think the way you had it is better, because > apparently we won't call LayoutManager::ViewRemoved() because of the parent = > NULL below. So, I think either move the reset where you had it, or make > ViewRemoved be called. Sorry for the run around. Done.
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.
LGTM
The CQ bit was checked by bruthig@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/2414103003/#ps120001 (title: "Moved |layout_manager_| destruction order in ~View().")
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 ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://chromiumcodereview.appspot.com/2441923004/ by pkasting@chromium.org. The reason for reverting is: Many ash_unittests failures on Linux, see e.g. https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2....
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/d7d592f487f9e03222d66c449b3ca772210b2b0b Cr-Commit-Position: refs/heads/master@{#426626} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d7d592f487f9e03222d66c449b3ca772210b2b0b Cr-Commit-Position: refs/heads/master@{#426626} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
