Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2244003002:
Materialized font style for TrayItemMore type system tray rows. (Closed)
DescriptionMaterialized font style for TrayItemMore type system tray rows.
Added a centralized style provider that can be re-used across all system tray rows eventually. This CL currently applies the font style to the TrayItemMore types only.
BUG=637846
Committed: https://crrev.com/3dfc90f7830de7eeb75d4574396be01f8373e88a
Cr-Commit-Position: refs/heads/master@{#419145}
Patch Set 1 #Patch Set 2 : Added TrayPopupItemStyle. #Patch Set 3 : Improved the TrayPopupItemStyle concept. #
Total comments: 14
Patch Set 4 : Merge branch 'master' into md_system_menu_bluetooth #Patch Set 5 : Addressed comments from patch set 3 and polished up a bit. #Patch Set 6 : Updated TrayPopupItemStyle to use the default FontList. #
Total comments: 32
Patch Set 7 : Deferred generic TrayItemPopupStyle application to TrayItemMore. #
Total comments: 10
Patch Set 8 : Addressed tdanderson@ comments from patch set 7. #Patch Set 9 : Merge branch 'master' into tray_item_popup_style #
Total comments: 4
Patch Set 10 : Re-ordered static function in Label. #
Messages
Total messages: 60 (42 generated)
The CQ bit was checked by bruthig@chromium.org to run a CQ dry run
Description was changed from ========== Materialized default bluetooth system tray row. BUG=632129 ========== to ========== Materialized default bluetooth system tray row. BUG=632129 ==========
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
bruthig@chromium.org changed reviewers: + tdanderson@chromium.org
Terry, here is the work I've done so far for materializing the default blue-tooth row in the system menu. I will leave it to your discretion as to whether you want to shepherd this along and get it committed in my absence. Overview: Patch set 2 adds a centralized Style provider that may not be mature enough for production but I think is a step in the right direction and is ok behind the ash-md flag. Patch set 1 has a solution for changing the look of the row but does a naive job at applying the style, i.e. the style is contained within the TrayItemMore class and thus not reusable to other tray items.
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, can you take a look from a high level and let me know what you think of the TrayPopupItemStyle concept?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
So far so good, let me know when it's ready for review. https://chromiumcodereview.appspot.com/2244003002/diff/40001/ash/common/syste... File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right): https://chromiumcodereview.appspot.com/2244003002/diff/40001/ash/common/syste... ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Can you also add a BUG=637846 to the CL description to track the fact that this CL lays the groundwork for theme standardization in the menu? https://chromiumcodereview.appspot.com/2244003002/diff/40001/ash/common/syste... ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:73: SetStyle(base::MakeUnique<TrayPopupItemStyle>( Put this behind an if (is_material) ? https://chromiumcodereview.appspot.com/2244003002/diff/40001/ash/common/syste... File ash/common/system/tray/tray_item_more.cc (right): https://chromiumcodereview.appspot.com/2244003002/diff/40001/ash/common/syste... ash/common/system/tray/tray_item_more.cc:85: style()->SetupLabel(label_); I wonder if it would be better to invoke this whenever |label_| is instantiated, and if it's possible for Label (or a derived type) could do the setup for us to avoid a manual call to SetupLabel()? https://chromiumcodereview.appspot.com/2244003002/diff/40001/ash/common/syste... File ash/common/system/tray/tray_item_more.h (right): https://chromiumcodereview.appspot.com/2244003002/diff/40001/ash/common/syste... ash/common/system/tray/tray_item_more.h:25: // A view with a chevron ('>') on the right edge. Clicking on the view brings up Maybe update this documentation while you're here. https://chromiumcodereview.appspot.com/2244003002/diff/40001/ash/common/syste... ash/common/system/tray/tray_item_more.h:68: // Current visual style of this item. consider specifying here (or in the class-level documentation of TrayPopupItemStyle) that it is only used in MD. https://chromiumcodereview.appspot.com/2244003002/diff/40001/ash/common/syste... File ash/common/system/tray/tray_popup_item_style.cc (right): https://chromiumcodereview.appspot.com/2244003002/diff/40001/ash/common/syste... ash/common/system/tray/tray_popup_item_style.cc:26: } nit: newline
Description was changed from ========== Materialized default bluetooth system tray row. BUG=632129 ========== to ========== Materialized default bluetooth system tray row. BUG=632129 BUG=637846 ==========
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 now ready for full review. Can you PTAL? https://codereview.chromium.org/2244003002/diff/40001/ash/common/system/chrom... File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right): https://codereview.chromium.org/2244003002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/09/08 16:11:02, tdanderson wrote: > Can you also add a BUG=637846 to the CL description to track the fact that this > CL lays the groundwork for theme standardization in the menu? Done. https://codereview.chromium.org/2244003002/diff/40001/ash/common/system/chrom... ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:73: SetStyle(base::MakeUnique<TrayPopupItemStyle>( On 2016/09/08 16:11:02, tdanderson wrote: > Put this behind an if (is_material) ? Done. https://codereview.chromium.org/2244003002/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_item_more.cc (right): https://codereview.chromium.org/2244003002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_item_more.cc:85: style()->SetupLabel(label_); On 2016/09/08 16:11:02, tdanderson wrote: > I wonder if it would be better to invoke this whenever |label_| is instantiated, > and if it's possible for Label (or a derived type) could do the setup for us to > avoid a manual call to SetupLabel()? I'm not sure I follow 100%. The style will need to be re-applied to a Label whenever the style changes so doing this during construction only wouldn't be sufficient. However, if we made Label extend TrayPopupItemStyleObserver and add the |label_| as an observer than perhaps we can make it a little more automatic. I don't think we should do this until the Style concept is mature enough and promoted into the ui namespace. https://codereview.chromium.org/2244003002/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_item_more.h (right): https://codereview.chromium.org/2244003002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_item_more.h:25: // A view with a chevron ('>') on the right edge. Clicking on the view brings up On 2016/09/08 16:11:02, tdanderson wrote: > Maybe update this documentation while you're here. Updated, WDYT? https://codereview.chromium.org/2244003002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_item_more.h:68: // Current visual style of this item. On 2016/09/08 16:11:02, tdanderson wrote: > consider specifying here (or in the class-level documentation of > TrayPopupItemStyle) that it is only used in MD. Done. https://codereview.chromium.org/2244003002/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_popup_item_style.cc (right): https://codereview.chromium.org/2244003002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_popup_item_style.cc:26: } On 2016/09/08 16:11:02, tdanderson wrote: > nit: newline Done.
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 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.
Ben, please see my comments below. https://codereview.chromium.org/2244003002/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_item_more.cc (right): https://codereview.chromium.org/2244003002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_item_more.cc:85: style()->SetupLabel(label_); On 2016/09/12 13:45:34, bruthig wrote: > On 2016/09/08 16:11:02, tdanderson wrote: > > I wonder if it would be better to invoke this whenever |label_| is > instantiated, > > and if it's possible for Label (or a derived type) could do the setup for us > to > > avoid a manual call to SetupLabel()? > > I'm not sure I follow 100%. The style will need to be re-applied to a Label > whenever the style changes so doing this during construction only wouldn't be > sufficient. Ah, disregard my comment then - in my first skim through I thought you only needed to do this once. > However, if we made Label extend TrayPopupItemStyleObserver and add > the |label_| as an observer than perhaps we can make it a little more automatic. > I don't think we should do this until the Style concept is mature enough and > promoted into the ui namespace. Agreed. https://codereview.chromium.org/2244003002/diff/40001/ash/common/system/tray/... File ash/common/system/tray/tray_item_more.h (right): https://codereview.chromium.org/2244003002/diff/40001/ash/common/system/tray/... ash/common/system/tray/tray_item_more.h:25: // A view with a chevron ('>') on the right edge. Clicking on the view brings up On 2016/09/12 13:45:34, bruthig wrote: > On 2016/09/08 16:11:02, tdanderson wrote: > > Maybe update this documentation while you're here. > > Updated, WDYT? lg https://codereview.chromium.org/2244003002/diff/100001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2244003002/diff/100001/ash/BUILD.gn#newcode2 ash/BUILD.gn:2: # Use of this source code is governed by a BSD-style license that can be nit: "blue-tooth" -> "Bluetooth" in the CL title and anywhere else in this CL. Also please include a brief CL description. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/chro... File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right): https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:90: const bool is_bluetooth_available = delegate->GetBluetoothAvailable(); I don't think you need to give this a name since you're only using it once. Ditto for line 112. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:100: // UpdateContent() should be called due to the style change above. Did you mean to actually call UpdateContent() here? It seems that you should be, otherwise the icon doesn't get set. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:107: void UpdateContent() override { private seems like a better choice than protected since BluetoothDefaultView has no derived classes. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_item_more.cc (right): https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_item_more.cc:48: UpdateContent(); If I'm not mistaken, this call to UpdateContent() invokes the function at line 83 (since it's in the constructor) whereas the call on line 80 will invoke the subclass-specific UpdateContent() overrides, which is ill-advised. It seems like you should: * Make TrayItemMore::UpdateContent() private, non-virtual, and call it something else, say UpdateTrayItemMoreContent() for the sake of argument. * Call UpdateTrayItemMoreContent() on line 48 instead of UpdateContent(). * I think you want line 80 to stay as-is. * Keep TrayItemMore::UpdateContent() as virtual and protected, but it will just call through to UTIMC(). https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_item_more.h (right): https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_item_more.h:8: #include <memory> is this needed? https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_item_more.h:52: // Overridden from ActionableView. nit: just "// ActionableView:" while you're here. Ditto for line 55. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_popup_item_style.cc (right): https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.cc:29: observers_.RemoveObserver(observer); should you check HasObserver() before calling RemoveObserver() ? https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.cc:56: return SkColorSetRGB(0x64, 0x64, 0x64); Consider moving this to tray_constants (or at the very least defining it as a constant in this file). https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.cc:91: break; can you add a NOTREACHED() somewhere in here? https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_popup_item_style.h (right): https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:22: // Central style provider for the system tray popup items. Makes it easier to I'd prefer "rows", "system menu rows", or just "the system menu" rather than "tray popup items". I'm also not super happy with having "tray popup" in the class name but I'm OK with leaving that as-is and seeing how this class evolves (e.g., it is likely this will be used for things other than the system menu, such as the opt-in IME menu, notification center, etc) https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:29: // Active and clickable nit: . at end of comments https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:38: enum class FontStyle { I'm having a hard time trying to figure out what all these are going to map to based on the names. Maybe some re-naming or comments would be appropriate? * TITLE: The text in the title row of the default views (i.e., the thing I'm working on)? * MAIN_PANEL_SECTION_ROW: The label for the default views. Perhaps call it something like DEFAULT_VIEW_LABEL? Use of "panel" is not accurate since it's not a panel. * SUB_PANEL_SECTION_ROW: The label for the rows in the detailed views? (Wouldn't they be the same as the above)? Same comment about renaming to something like DETAILED_VIEW_LABEL or similar. * SYSTEM_INFO: ? * CAPTION: ? * BUTTON: Is this the text used in text buttons (e.g., "Log out" in the top user row)? What would you do in the case of the Cast row where there is a label *and* a button, each possibly needing their own style? https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:48: virtual ~TrayPopupItemStyle(); should this actually be virtual?
Description was changed from ========== Materialized default bluetooth system tray row. BUG=632129 BUG=637846 ========== to ========== Materialized default Bluetooth system tray row. Centralized the font style and color in a "Style provider" for all text in the system menu. BUG=632129 BUG=637846 ==========
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...
Description was changed from ========== Materialized default Bluetooth system tray row. Centralized the font style and color in a "Style provider" for all text in the system menu. BUG=632129 BUG=637846 ========== to ========== Materialized font style for TrayItemMore type system tray rows. Added a centralized style provider that can be re-used across all system tray rows eventually. This CL currently applies the font style to the TrayItemMore types only. BUG=637846 ==========
Terry, I've updated the CL to remove the generic application of the style to the TrayItemMore. It turns out avoiding virtual function calls from the constructor is going to require a considerable amount of effort, which I will defer to a follow-up CL. The plan is to get the style provider in ASAP so others can start using it. Can you PTAL? https://codereview.chromium.org/2244003002/diff/100001/ash/BUILD.gn File ash/BUILD.gn (right): https://codereview.chromium.org/2244003002/diff/100001/ash/BUILD.gn#newcode2 ash/BUILD.gn:2: # Use of this source code is governed by a BSD-style license that can be On 2016/09/12 18:51:47, tdanderson wrote: > nit: "blue-tooth" -> "Bluetooth" in the CL title and anywhere else in this CL. > Also please include a brief CL description. Done. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/chro... File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right): https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:90: const bool is_bluetooth_available = delegate->GetBluetoothAvailable(); On 2016/09/12 18:51:47, tdanderson wrote: > I don't think you need to give this a name since you're only using it once. > Ditto for line 112. Done. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:100: // UpdateContent() should be called due to the style change above. On 2016/09/12 18:51:47, tdanderson wrote: > Did you mean to actually call UpdateContent() here? It seems that you should be, > otherwise the icon doesn't get set. It will be called via TrayItemMore::OnTrayPopupItemStyleUpdated() as per the comment. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:107: void UpdateContent() override { On 2016/09/12 18:51:47, tdanderson wrote: > private seems like a better choice than protected since BluetoothDefaultView has > no derived classes. Done. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_item_more.h (right): https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_item_more.h:8: #include <memory> On 2016/09/12 18:51:47, tdanderson wrote: > is this needed? Yup, for unique_ptr<>. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_item_more.h:52: // Overridden from ActionableView. On 2016/09/12 18:51:47, tdanderson wrote: > nit: just "// ActionableView:" while you're here. Ditto for line 55. Done. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_popup_item_style.cc (right): https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.cc:29: observers_.RemoveObserver(observer); On 2016/09/12 18:51:47, tdanderson wrote: > should you check HasObserver() before calling RemoveObserver() ? It would be unnecessary. ObserverListBase::RemoveObserver() is a no-op if it doesn't contain the observer. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.cc:56: return SkColorSetRGB(0x64, 0x64, 0x64); On 2016/09/12 18:51:47, tdanderson wrote: > Consider moving this to tray_constants (or at the very least defining it as a > constant in this file). Made it a constant in this file and kept TODO. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.cc:91: break; On 2016/09/12 18:51:47, tdanderson wrote: > can you add a NOTREACHED() somewhere in here? I'm not exactly sure what you are asking. FTR there will be compile time errors if not all cases of the enum are handled. Is that sufficient? https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_popup_item_style.h (right): https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:22: // Central style provider for the system tray popup items. Makes it easier to On 2016/09/12 18:51:47, tdanderson wrote: > I'd prefer "rows", "system menu rows", or just "the system menu" rather than > "tray popup items". I'm also not super happy with having "tray popup" in the > class name but I'm OK with leaving that as-is and seeing how this class evolves > (e.g., it is likely this will be used for things other than the system menu, > such as the opt-in IME menu, notification center, etc) Done. Let's discuss alternatives offline. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:29: // Active and clickable On 2016/09/12 18:51:47, tdanderson wrote: > nit: . at end of comments Done. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:38: enum class FontStyle { FTR I took this from the "System menu typography" section here: https://folio.googleplex.com/cros-core-ui/spec#%2F2.1%20-%20System%20menu%20-... On 2016/09/12 18:51:47, tdanderson wrote: > I'm having a hard time trying to figure out what all these are going to map to > based on the names. Maybe some re-naming or comments would be appropriate? Done, re-named and added docs. WDYT? > > * TITLE: The text in the title row of the default views (i.e., the thing I'm > working on)? > > * MAIN_PANEL_SECTION_ROW: The label for the default views. Perhaps call it > something like DEFAULT_VIEW_LABEL? Use of "panel" is not accurate since it's not > a panel. > > * SUB_PANEL_SECTION_ROW: The label for the rows in the detailed views? (Wouldn't > they be the same as the above)? Same comment about renaming to something like > DETAILED_VIEW_LABEL or similar. Apparently the default view items and the detailed view items are not the same. > > * SYSTEM_INFO: ? > > * CAPTION: ? > > * BUTTON: Is this the text used in text buttons (e.g., "Log out" in the top user > row)? What would you do in the case of the Cast row where there is a label *and* > a button, each possibly needing their own style? Yes, this text is used for the 'Harmony' buttons, e.g. "Sign out", "Stop", "Lock", etc. For the cast row the "Casting to living room" text would use MAIN_PANEL_SECTION_ROW and the "Stop" button would use BUTTON. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:48: virtual ~TrayPopupItemStyle(); On 2016/09/12 18:51:47, tdanderson wrote: > should this actually be virtual? Removed.
lgtm with nits https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/chro... File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right): https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/chro... ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:100: // UpdateContent() should be called due to the style change above. On 2016/09/15 18:56:29, bruthig wrote: > On 2016/09/12 18:51:47, tdanderson wrote: > > Did you mean to actually call UpdateContent() here? It seems that you should > be, > > otherwise the icon doesn't get set. > > It will be called via TrayItemMore::OnTrayPopupItemStyleUpdated() as per the > comment. I read the 'should' in this comment as "[you the developer] should make sure you call UpdateContent() at some point because the style has changed." I suggest changing 'should' to 'will', optionally 'will be called by TIM::OTPISU()'. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_popup_item_style.cc (right): https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.cc:29: observers_.RemoveObserver(observer); On 2016/09/15 18:56:29, bruthig wrote: > On 2016/09/12 18:51:47, tdanderson wrote: > > should you check HasObserver() before calling RemoveObserver() ? > > It would be unnecessary. ObserverListBase::RemoveObserver() is a no-op if it > doesn't contain the observer. Acknowledged. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.cc:91: break; On 2016/09/15 18:56:29, bruthig wrote: > On 2016/09/12 18:51:47, tdanderson wrote: > > can you add a NOTREACHED() somewhere in here? > > I'm not exactly sure what you are asking. FTR there will be compile time errors > if not all cases of the enum are handled. Is that sufficient? I thought the style guide required a default: NOTREACHED() but apparently it does not. OK to leave as-is. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... File ash/common/system/tray/tray_popup_item_style.h (right): https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:22: // Central style provider for the system tray popup items. Makes it easier to On 2016/09/15 18:56:30, bruthig wrote: > On 2016/09/12 18:51:47, tdanderson wrote: > > I'd prefer "rows", "system menu rows", or just "the system menu" rather than > > "tray popup items". I'm also not super happy with having "tray popup" in the > > class name but I'm OK with leaving that as-is and seeing how this class > evolves > > (e.g., it is likely this will be used for things other than the system menu, > > such as the opt-in IME menu, notification center, etc) > > Done. Let's discuss alternatives offline. Acknowledged. https://codereview.chromium.org/2244003002/diff/100001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:38: enum class FontStyle { On 2016/09/15 18:56:30, bruthig wrote: > FTR I took this from the "System menu typography" section here: > https://folio.googleplex.com/cros-core-ui/spec#%2F2.1%20-%20System%20menu%20-... > > On 2016/09/12 18:51:47, tdanderson wrote: > > I'm having a hard time trying to figure out what all these are going to map to > > based on the names. Maybe some re-naming or comments would be appropriate? > > Done, re-named and added docs. WDYT? lg > > > > > * TITLE: The text in the title row of the default views (i.e., the thing I'm > > working on)? > > > > * MAIN_PANEL_SECTION_ROW: The label for the default views. Perhaps call it > > something like DEFAULT_VIEW_LABEL? Use of "panel" is not accurate since it's > not > > a panel. > > > > * SUB_PANEL_SECTION_ROW: The label for the rows in the detailed views? > (Wouldn't > > they be the same as the above)? Same comment about renaming to something like > > DETAILED_VIEW_LABEL or similar. > > Apparently the default view items and the detailed view items are not the same. Oh, strange. Thanks for clarifying. > > > > > * SYSTEM_INFO: ? > > > > * CAPTION: ? > > > > * BUTTON: Is this the text used in text buttons (e.g., "Log out" in the top > user > > row)? What would you do in the case of the Cast row where there is a label > *and* > > a button, each possibly needing their own style? > > Yes, this text is used for the 'Harmony' buttons, e.g. "Sign out", "Stop", > "Lock", etc. For the cast row the "Casting to living room" text would use > MAIN_PANEL_SECTION_ROW and the "Stop" button would use BUTTON. > > Got it, thanks. https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... File ash/common/system/tray/tray_item_more.h (right): https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... ash/common/system/tray/tray_item_more.h:3: // found in the LICENSE file. nit: please update actual CL title to "Materialized font style for TrayItemMore type system tray rows." (it still says Bluetooth) nit: format the CL description at 80-char lines. https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... File ash/common/system/tray/tray_popup_item_style.h (right): https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:46: SYSTEM_INFO, I'm not super excited about the SYSTEM_INFO name since it's very specific. But it's fine to leave it as-is, it can hopefully evolve into a better name as various surfaces start using TrayPopupItemStyle. https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:50: // "Lock", Cast's "Stop", etc). Sign out / lock are sort of in flux, just use Cast's "Stop" as an example here. https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:65: ColorStyle color_style() const { return color_style_; } The member this returns is non-const, so the accessor should also be non-const (ditto for line 70). https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:91: }; DISALLOW_COPY_AND_ASSIGN?
bruthig@chromium.org changed reviewers: + sky@chromium.org
sky@, can you take a quick look at: - ash/BUILD.gn - ui/views/* https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... File ash/common/system/tray/tray_item_more.h (right): https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... ash/common/system/tray/tray_item_more.h:3: // found in the LICENSE file. On 2016/09/15 19:49:33, tdanderson wrote: > nit: please update actual CL title to "Materialized font style for TrayItemMore > type system tray rows." (it still says Bluetooth) > > nit: format the CL description at 80-char lines. Done. https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... File ash/common/system/tray/tray_popup_item_style.h (right): https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:46: SYSTEM_INFO, On 2016/09/15 19:49:33, tdanderson wrote: > I'm not super excited about the SYSTEM_INFO name since it's very specific. But > it's fine to leave it as-is, it can hopefully evolve into a better name as > various surfaces start using TrayPopupItemStyle. Acknowledged.
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 bruthig@chromium.org to run a CQ dry run
https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... File ash/common/system/tray/tray_popup_item_style.h (right): https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:50: // "Lock", Cast's "Stop", etc). On 2016/09/15 19:49:34, tdanderson wrote: > Sign out / lock are sort of in flux, just use Cast's "Stop" as an example here. Done. https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:65: ColorStyle color_style() const { return color_style_; } On 2016/09/15 19:49:33, tdanderson wrote: > The member this returns is non-const, so the accessor should also be non-const > (ditto for line 70). Discussed offline. Summary: ColorStyle and FontStyle are pod's and thus are returned by copy. This means editing them won't update the TrayPopupItemStyle and wouldn't violate the const declarations here. https://codereview.chromium.org/2244003002/diff/120001/ash/common/system/tray... ash/common/system/tray/tray_popup_item_style.h:91: }; On 2016/09/15 19:49:34, tdanderson wrote: > DISALLOW_COPY_AND_ASSIGN? 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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
LGTM with the following addressed. https://codereview.chromium.org/2244003002/diff/160001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2244003002/diff/160001/ui/views/controls/labe... ui/views/controls/label.cc:35: const gfx::FontList& Label::GetDefaultFontList() { When you move the declaration move the definition too. https://codereview.chromium.org/2244003002/diff/160001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2244003002/diff/160001/ui/views/controls/labe... ui/views/controls/label.h:25: static const gfx::FontList& GetDefaultFontList(); Style guide says constructor/destructor before other functions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2244003002/diff/160001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2244003002/diff/160001/ui/views/controls/labe... ui/views/controls/label.cc:35: const gfx::FontList& Label::GetDefaultFontList() { On 2016/09/15 22:51:01, sky wrote: > When you move the declaration move the definition too. Done. https://codereview.chromium.org/2244003002/diff/160001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2244003002/diff/160001/ui/views/controls/labe... ui/views/controls/label.h:25: static const gfx::FontList& GetDefaultFontList(); On 2016/09/15 22:51:01, sky wrote: > Style guide says constructor/destructor before other functions. Done.
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, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2244003002/#ps180001 (title: "y")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_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 bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by bruthig@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 ========== Materialized font style for TrayItemMore type system tray rows. Added a centralized style provider that can be re-used across all system tray rows eventually. This CL currently applies the font style to the TrayItemMore types only. BUG=637846 ========== to ========== Materialized font style for TrayItemMore type system tray rows. Added a centralized style provider that can be re-used across all system tray rows eventually. This CL currently applies the font style to the TrayItemMore types only. BUG=637846 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Materialized font style for TrayItemMore type system tray rows. Added a centralized style provider that can be re-used across all system tray rows eventually. This CL currently applies the font style to the TrayItemMore types only. BUG=637846 ========== to ========== Materialized font style for TrayItemMore type system tray rows. Added a centralized style provider that can be re-used across all system tray rows eventually. This CL currently applies the font style to the TrayItemMore types only. BUG=637846 Committed: https://crrev.com/3dfc90f7830de7eeb75d4574396be01f8373e88a Cr-Commit-Position: refs/heads/master@{#419145} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3dfc90f7830de7eeb75d4574396be01f8373e88a Cr-Commit-Position: refs/heads/master@{#419145} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
