Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2744463002:
Add VectorIconButton functionality to ImageButton. (Closed)
|
Created:
3 years, 9 months ago by Bret Modified:
3 years, 9 months ago CC:
chromium-reviews, asanka, msramek+watch_chromium.org, yusukes+watch_chromium.org, dcheng, markusheintz_, miu+watch_chromium.org, extensions-reviews_chromium.org, msw+watch_chromium.org, awdf+watch_chromium.org, raymes+watch_chromium.org, dbeam+watch-downloads_chromium.org, nona+watch_chromium.org, chromium-apps-reviews_chromium.org, vabr+watchlistpasswordmanager_chromium.org, Peter Beverloo, mlamouri+watch-notifications_chromium.org, hcarmona+bubble_chromium.org, gcasto+watchlist_chromium.org, bruthig+ink_drop_chromium.org, rouslan+bubble_chromium.org, groby+bubble_chromium.org, tfarina, shuchen+watch_chromium.org, James Su Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove VectorIconButton functionality into static utilities.
There are only a few users of VectorIconButton, and its hard-coded
nature means it is not very flexible. This patch moves its functionality
into two static utilities, one which creates an ImageButton with the
existing vector button styling, and another that updates the
ImageButton's images from an icon. The owner classes are now responsible
for updating the styling if the theme changes.
BUG=691895
Review-Url: https://codereview.chromium.org/2744463002
Cr-Commit-Position: refs/heads/master@{#458601}
Committed: https://chromium.googlesource.com/chromium/src/+/b7f329f1bcb344b270da73f0ef23650198886a0f
Patch Set 1 #Patch Set 2 : minor edits #Patch Set 3 : fix button_example #Patch Set 4 : fix cros build #
Total comments: 9
Patch Set 5 : WIP: use observer #
Total comments: 30
Patch Set 6 : wip: address high-level comments #
Total comments: 1
Patch Set 7 : use static utility #Patch Set 8 : tests #
Total comments: 24
Patch Set 9 : address comments #Patch Set 10 : comments 2 #Patch Set 11 : fix typo in tests #Patch Set 12 : fix cros build #
Total comments: 2
Patch Set 13 : check details #Messages
Total messages: 72 (28 generated)
Description was changed from ========== Add VectorIconButton functionality to ImageButton. There are only a few users of VectorIconButton, and its hard-coded nature means it is not very flexible. This patch moves its functionality into ImageButton and adds a function that provides a default button with a vector icon for existing users. This patch also removes the button padding in Harmony and fixes the find bar buttons to work with less padding. BUG=691895 ========== to ========== Add VectorIconButton functionality to ImageButton. There are only a few users of VectorIconButton, and its hard-coded nature means it is not very flexible. This patch moves its functionality into ImageButton and adds a function that provides a default button with a vector icon for existing users. This patch also removes the button padding in Harmony and fixes the find bar buttons to work with less padding. BUG=691895 ==========
bsep@chromium.org changed reviewers: + pkasting@chromium.org, sky@chromium.org
PTAL pkasting and sky
The CQ bit was checked by bsep@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_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 bsep@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by bsep@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
sky@chromium.org changed reviewers: + bruthig@chromium.org, estade@chromium.org
+estade and bruthig in case they are interested I'm definitely interested in having a *lot* less button classes. My understanding is bruthig@ is going to be tackling this very problem soon. https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... File ui/views/controls/button/image_button.h (right): https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... ui/views/controls/button/image_button.h:28: virtual SkColor GetVectorIconColor() const; Specifying the color should not be a requirement for using a vectoricon. Additionally as vectoricon support is implemented in terms of ImageSkia I don't think ImageButton needs first class support for vectoricons. Rather I think there should be a function (not declared in ImageButton, perhaps in vector_image_buttn.h) for configuring ImageButton from an vectoricon. Basically this function contains what you have in UpdateImagesFromVectorIcon. If you want a factory function it can be in the same file. As to how to update the image color when the theme changes that should be moved to ViewObserver. Specifically add OnThemeChanged and OnNativeThemeChanged to ViewObserver and call them from View.
https://codereview.chromium.org/2744463002/diff/50001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2744463002/diff/50001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:301: return gfx::Size(std::min(size().width(), kDefaultInkDropSize), |kDefaultInkDropSize| is somewhat of an arbitrary value and I've been wanting to remove it for a while. I feel like this change is only making more engrained. Can you extend ImageButton with a new FindInPageButton that overrides CreateInkDropRipple() and CreateInkDropHighlight() to return the right sized effects and remove the changes to InkDropHostView? I think that the Button refactor that I have planned will eventually purge this FindInPageButton. https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... File ui/views/controls/button/image_button.h (right): https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... ui/views/controls/button/image_button.h:26: class VIEWS_EXPORT ImageButtonDelegate : public ButtonListener { IMO I don't think coupling the click listener to the image properties provider is the right way to go. I'm pretty sure there are some use cases where the button pressed listener is and should be a completely different class than the image property provider. I'm not sure if this is in line with sky@'s suggestion or not but I've thought about this a bit in the past and I was thinking about adding an ImageProvider class that the ImageButton can accept. The ImageProvider could then be extended by VectorImageProvider, BitmapImageProvider, MyOwnCustomPaintedImageProvider, etc. Something along the lines of: class ImageProvider { unique_ptr<ImageSkia> GetImage(const Size&); } And then if you wanted to get super fancy you could have the ImageProvider have observers defined like below that ImageButton could inherit: class ImageProviderObserver{ virtual void ImageChanged(); } This would enable a few things such as: - The concrete ImageProvider could own and manage animations - We could have a small library of general ImageProviders that can be re-used. This would support composability which is the primary driving motivation to my planned button refactor. sky@, WDYT?
On 2017/03/09 05:04:42, bruthig wrote: > https://codereview.chromium.org/2744463002/diff/50001/ui/views/animation/ink_... > File ui/views/animation/ink_drop_host_view.cc (right): > > https://codereview.chromium.org/2744463002/diff/50001/ui/views/animation/ink_... > ui/views/animation/ink_drop_host_view.cc:301: return > gfx::Size(std::min(size().width(), kDefaultInkDropSize), > |kDefaultInkDropSize| is somewhat of an arbitrary value and I've been wanting to > remove it for a while. I feel like this change is only making more engrained. > Can you extend ImageButton with a new FindInPageButton that overrides > CreateInkDropRipple() and CreateInkDropHighlight() to return the right sized > effects and remove the changes to InkDropHostView? > > I think that the Button refactor that I have planned will eventually purge this > FindInPageButton. > > https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... > File ui/views/controls/button/image_button.h (right): > > https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... > ui/views/controls/button/image_button.h:26: class VIEWS_EXPORT > ImageButtonDelegate : public ButtonListener { > IMO I don't think coupling the click listener to the image properties provider > is the right way to go. I'm pretty sure there are some use cases where the > button pressed listener is and should be a completely different class than the > image property provider. > > I'm not sure if this is in line with sky@'s suggestion or not but I've thought > about this a bit in the past and I was thinking about adding an ImageProvider > class that the ImageButton can accept. The ImageProvider could then be extended > by VectorImageProvider, BitmapImageProvider, MyOwnCustomPaintedImageProvider, > etc. Something along the lines of: > > class ImageProvider { > unique_ptr<ImageSkia> GetImage(const Size&); > } > > And then if you wanted to get super fancy you could have the ImageProvider have > observers defined like below that ImageButton could inherit: > > class ImageProviderObserver{ > virtual void ImageChanged(); > } > > This would enable a few things such as: > > - The concrete ImageProvider could own and manage animations > - We could have a small library of general ImageProviders that can be re-used. > This would support composability which is the primary driving motivation to my > planned button refactor. > > sky@, WDYT? The interface you are suggesting is in some ways similar to ImageSkiaSource. I tend to think the size comes from the image, and not something used to produce the image. If you look at ImageSkia it has an inherent size. I think this is more the common case.
It sounds like there's some significant interface design discussion ongoing, so maybe I'll stay out until that's settled. If you want, feel free to clarify more whether you'd like sky and me to look at different files or different aspects of the design. Finally, it would be good to understand better the description comment about "removes the button padding in Harmony" and the practical effect that has. Does anything actually change appearance or behavior and, if so, what?
As to an interface like ImageProvider. I would rather not expose that as a core part of button. I would prefer to keep button entirely in terms of ImageSkia. If a user of the button wants to dynamically update the image it should call SetImage as appropriate. -Scott On Wed, Mar 8, 2017 at 9:04 PM, <bruthig@chromium.org> wrote: > > https://codereview.chromium.org/2744463002/diff/50001/ui/views/animation/ink_... > File ui/views/animation/ink_drop_host_view.cc (right): > > https://codereview.chromium.org/2744463002/diff/50001/ui/views/animation/ink_... > ui/views/animation/ink_drop_host_view.cc:301: return > gfx::Size(std::min(size().width(), kDefaultInkDropSize), > |kDefaultInkDropSize| is somewhat of an arbitrary value and I've been > wanting to remove it for a while. I feel like this change is only > making more engrained. Can you extend ImageButton with a new > FindInPageButton that overrides CreateInkDropRipple() and > CreateInkDropHighlight() to return the right sized effects and remove > the changes to InkDropHostView? > > I think that the Button refactor that I have planned will eventually > purge this FindInPageButton. > > https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... > File ui/views/controls/button/image_button.h (right): > > https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... > ui/views/controls/button/image_button.h:26: class VIEWS_EXPORT > ImageButtonDelegate : public ButtonListener { > IMO I don't think coupling the click listener to the image properties > provider is the right way to go. I'm pretty sure there are some use > cases where the button pressed listener is and should be a completely > different class than the image property provider. > > I'm not sure if this is in line with sky@'s suggestion or not but I've > thought about this a bit in the past and I was thinking about adding an > ImageProvider class that the ImageButton can accept. The ImageProvider > could then be extended by VectorImageProvider, BitmapImageProvider, > MyOwnCustomPaintedImageProvider, etc. Something along the lines of: > > class ImageProvider { > unique_ptr<ImageSkia> GetImage(const Size&); > } > > And then if you wanted to get super fancy you could have the > ImageProvider have observers defined like below that ImageButton could > inherit: > > class ImageProviderObserver{ > virtual void ImageChanged(); > } > > This would enable a few things such as: > > - The concrete ImageProvider could own and manage animations > - We could have a small library of general ImageProviders that can be > re-used. This would support composability which is the primary driving > motivation to my planned button refactor. > > sky@, WDYT? > > https://codereview.chromium.org/2744463002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I pondered the comments and if we want to avoid giving ImageButton knowledge of vector icons maybe I should just keep VectorIconButton for now? There's no reason I NEED to delete it, I can add the flexibility we need for Harmony there instead. Then we can rethink all the buttons at once. https://codereview.chromium.org/2744463002/diff/50001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2744463002/diff/50001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:301: return gfx::Size(std::min(size().width(), kDefaultInkDropSize), On 2017/03/09 05:04:42, bruthig wrote: > |kDefaultInkDropSize| is somewhat of an arbitrary value and I've been wanting to > remove it for a while. I feel like this change is only making more engrained. > Can you extend ImageButton with a new FindInPageButton that overrides > CreateInkDropRipple() and CreateInkDropHighlight() to return the right sized > effects and remove the changes to InkDropHostView? > > I think that the Button refactor that I have planned will eventually purge this > FindInPageButton. It's not just the find-in-page buttons, I modified those because they had a layout problem when we changed their size for Harmony. Here I'm trying to make sure the ink drop sizes are reasonable when the button is smaller than 24dip. I don't like the default constant either but I don't see how this is making it more ingrained; all buttons are relying on it either way. https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... File ui/views/controls/button/image_button.h (right): https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... ui/views/controls/button/image_button.h:26: class VIEWS_EXPORT ImageButtonDelegate : public ButtonListener { On 2017/03/09 05:04:42, bruthig wrote: > IMO I don't think coupling the click listener to the image properties provider > is the right way to go. I'm pretty sure there are some use cases where the > button pressed listener is and should be a completely different class than the > image property provider. > > I'm not sure if this is in line with sky@'s suggestion or not but I've thought > about this a bit in the past and I was thinking about adding an ImageProvider > class that the ImageButton can accept. The ImageProvider could then be extended > by VectorImageProvider, BitmapImageProvider, MyOwnCustomPaintedImageProvider, > etc. Something along the lines of: > > class ImageProvider { > unique_ptr<ImageSkia> GetImage(const Size&); > } > > And then if you wanted to get super fancy you could have the ImageProvider have > observers defined like below that ImageButton could inherit: > > class ImageProviderObserver{ > virtual void ImageChanged(); > } > > This would enable a few things such as: > > - The concrete ImageProvider could own and manage animations > - We could have a small library of general ImageProviders that can be re-used. > This would support composability which is the primary driving motivation to my > planned button refactor. > > sky@, WDYT? In practice I haven't seen any cases where it seems like the listener and color provider should be different classes. Most of the time the view's parent just implements all the necessary methods. I do agree it's conceptually wonky though. https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... ui/views/controls/button/image_button.h:28: virtual SkColor GetVectorIconColor() const; On 2017/03/09 04:05:02, sky wrote: > Specifying the color should not be a requirement for using a vectoricon. > Additionally as vectoricon support is implemented in terms of ImageSkia I don't > think ImageButton needs first class support for vectoricons. Rather I think > there should be a function (not declared in ImageButton, perhaps in > vector_image_buttn.h) for configuring ImageButton from an vectoricon. Basically > this function contains what you have in UpdateImagesFromVectorIcon. If you want > a factory function it can be in the same file. > > As to how to update the image color when the theme changes that should be moved > to ViewObserver. Specifically add OnThemeChanged and OnNativeThemeChanged to > ViewObserver and call them from View. Subclasses aren't required to override GetVectorIconColor, there's a default "return black" in the cc file. This is how VectorIconButton worked. To your larger point: basically you want a VectorIconButtonObserver that subclasses ViewObserver that takes the vector icon and calls SetImage when appropriate, and then a factory that wires it up, if I understand that right. That doesn't seem substantively different than just keeping VectorIconButton to me.
On 2017/03/09 22:32:37, Bret Sepulveda wrote: > I pondered the comments and if we want to avoid giving ImageButton knowledge of > vector icons maybe I should just keep VectorIconButton for now? There's no > reason I NEED to delete it, I can add the flexibility we need for Harmony there > instead. Then we can rethink all the buttons at once. Assuming we refactor the button classes eventually, which route makes it easier to understand and do that easily and effectively? Assuming we do not get around to refactoring the button classes, and they stay in the way you leave them, which way is clearer and simpler? My gut feeling is that going ahead and killing VectorIconButton, even if it isn't done in a way that achieves all of Scott's desires, might be better on these, but I also feel like my gut is especially poorly-informed here, so you should probably ignore it.
I really want fewer button classes, but I want it done in such a way that doesn't result in an overly complex class. I believe my recommendations head that way. I would appreciate it if you could take this on, but I completely understand if you don't have the time for this now. -Scott On Thu, Mar 9, 2017 at 2:32 PM, <bsep@chromium.org> wrote: > I pondered the comments and if we want to avoid giving ImageButton knowledge > of > vector icons maybe I should just keep VectorIconButton for now? There's no > reason I NEED to delete it, I can add the flexibility we need for Harmony > there > instead. Then we can rethink all the buttons at once. > > > https://codereview.chromium.org/2744463002/diff/50001/ui/views/animation/ink_... > File ui/views/animation/ink_drop_host_view.cc (right): > > https://codereview.chromium.org/2744463002/diff/50001/ui/views/animation/ink_... > ui/views/animation/ink_drop_host_view.cc:301: return > gfx::Size(std::min(size().width(), kDefaultInkDropSize), > On 2017/03/09 05:04:42, bruthig wrote: >> |kDefaultInkDropSize| is somewhat of an arbitrary value and I've been > wanting to >> remove it for a while. I feel like this change is only making more > engrained. >> Can you extend ImageButton with a new FindInPageButton that overrides >> CreateInkDropRipple() and CreateInkDropHighlight() to return the right > sized >> effects and remove the changes to InkDropHostView? >> >> I think that the Button refactor that I have planned will eventually > purge this >> FindInPageButton. > > It's not just the find-in-page buttons, I modified those because they > had a layout problem when we changed their size for Harmony. Here I'm > trying to make sure the ink drop sizes are reasonable when the button is > smaller than 24dip. I don't like the default constant either but I don't > see how this is making it more ingrained; all buttons are relying on it > either way. > > https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... > File ui/views/controls/button/image_button.h (right): > > https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... > ui/views/controls/button/image_button.h:26: class VIEWS_EXPORT > ImageButtonDelegate : public ButtonListener { > On 2017/03/09 05:04:42, bruthig wrote: >> IMO I don't think coupling the click listener to the image properties > provider >> is the right way to go. I'm pretty sure there are some use cases where > the >> button pressed listener is and should be a completely different class > than the >> image property provider. >> >> I'm not sure if this is in line with sky@'s suggestion or not but I've > thought >> about this a bit in the past and I was thinking about adding an > ImageProvider >> class that the ImageButton can accept. The ImageProvider could then > be extended >> by VectorImageProvider, BitmapImageProvider, > MyOwnCustomPaintedImageProvider, >> etc. Something along the lines of: >> >> class ImageProvider { >> unique_ptr<ImageSkia> GetImage(const Size&); >> } >> >> And then if you wanted to get super fancy you could have the > ImageProvider have >> observers defined like below that ImageButton could inherit: >> >> class ImageProviderObserver{ >> virtual void ImageChanged(); >> } >> >> This would enable a few things such as: >> >> - The concrete ImageProvider could own and manage animations >> - We could have a small library of general ImageProviders that can be > re-used. >> This would support composability which is the primary driving > motivation to my >> planned button refactor. >> >> sky@, WDYT? > > In practice I haven't seen any cases where it seems like the listener > and color provider should be different classes. Most of the time the > view's parent just implements all the necessary methods. I do agree it's > conceptually wonky though. > > https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... > ui/views/controls/button/image_button.h:28: virtual SkColor > GetVectorIconColor() const; > On 2017/03/09 04:05:02, sky wrote: >> Specifying the color should not be a requirement for using a > vectoricon. >> Additionally as vectoricon support is implemented in terms of > ImageSkia I don't >> think ImageButton needs first class support for vectoricons. Rather I > think >> there should be a function (not declared in ImageButton, perhaps in >> vector_image_buttn.h) for configuring ImageButton from an vectoricon. > Basically >> this function contains what you have in UpdateImagesFromVectorIcon. If > you want >> a factory function it can be in the same file. >> >> As to how to update the image color when the theme changes that should > be moved >> to ViewObserver. Specifically add OnThemeChanged and > OnNativeThemeChanged to >> ViewObserver and call them from View. > > Subclasses aren't required to override GetVectorIconColor, there's a > default "return black" in the cc file. This is how VectorIconButton > worked. > > To your larger point: basically you want a VectorIconButtonObserver that > subclasses ViewObserver that takes the vector icon and calls SetImage > when appropriate, and then a factory that wires it up, if I understand > that right. That doesn't seem substantively different than just keeping > VectorIconButton to me. > > https://codereview.chromium.org/2744463002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> Assuming we refactor the button classes eventually, which route makes it easier > to understand and do that easily and effectively? The way in this patch is probably not going to aid eventual refactoring. It'll be a pain to back out the ImageButtonDelegate changes if we decide to remove it. Scott's suggestion and leaving VectorIconButton both seem better, though they feel fairly similar to each other. > Assuming we do not get around to refactoring the button classes, and they stay > in the way you leave them, which way is clearer and simpler? On the other hand, I like the way I've done it here if it's going to stay like this. It's simpler than requiring a whole other class to manage vector icons, and I'm imagining ImageButtonDelegate being a vector that allows people to eliminate more subclasses in this way. > My gut feeling is that going ahead and killing VectorIconButton, even if it > isn't done in a way that achieves all of Scott's desires, might be better on > these, but I also feel like my gut is especially poorly-informed here, so you > should probably ignore it. That was my gut too, which is why I went ahead with this. But I don't have a good sense of what the ideal is.
Upon further reflection, I think I'll revert to refactoring VectorIconButton instead. That should be a less invasive change that still gives us what we need for Harmony. I would like to keep the InkDropHostView changes, and the addition of SetPadding to ImageButton since adding an empty border seems to be a common pattern.
https://codereview.chromium.org/2744463002/diff/50001/ui/views/animation/ink_... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2744463002/diff/50001/ui/views/animation/ink_... ui/views/animation/ink_drop_host_view.cc:301: return gfx::Size(std::min(size().width(), kDefaultInkDropSize), On 2017/03/09 22:32:37, Bret Sepulveda wrote: > On 2017/03/09 05:04:42, bruthig wrote: > > |kDefaultInkDropSize| is somewhat of an arbitrary value and I've been wanting > to > > remove it for a while. I feel like this change is only making more engrained. > > > Can you extend ImageButton with a new FindInPageButton that overrides > > CreateInkDropRipple() and CreateInkDropHighlight() to return the right sized > > effects and remove the changes to InkDropHostView? > > > > I think that the Button refactor that I have planned will eventually purge > this > > FindInPageButton. > > It's not just the find-in-page buttons, I modified those because they had a > layout problem when we changed their size for Harmony. Here I'm trying to make > sure the ink drop sizes are reasonable when the button is smaller than 24dip. I > don't like the default constant either but I don't see how this is making it > more ingrained; all buttons are relying on it either way. I guess I dislike the conditional 'min()' logic more than the constant itself. Prior to your change it was possible to statically determine what ink drop size was being used on a button. With this conditional change I'm now going to need to know the run time size of the button. This is going to make it harder to perform the button refactor and extract the ink drop as an effect. I will have to either reverse engineer the run time sizes or I'm going to have to dig up all of the designer specs. As a compromise, I'd be happy with introducing an InkDropHostView::SetDefaultInkDropSize() that clients can use instead if needed. This would at least white-list the buttons that require the behavior. https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... File ui/views/controls/button/image_button.h (right): https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... ui/views/controls/button/image_button.h:26: class VIEWS_EXPORT ImageButtonDelegate : public ButtonListener { On 2017/03/09 22:32:37, Bret Sepulveda wrote: > On 2017/03/09 05:04:42, bruthig wrote: > > IMO I don't think coupling the click listener to the image properties provider > > is the right way to go. I'm pretty sure there are some use cases where the > > button pressed listener is and should be a completely different class than the > > image property provider. > > > > I'm not sure if this is in line with sky@'s suggestion or not but I've thought > > about this a bit in the past and I was thinking about adding an ImageProvider > > class that the ImageButton can accept. The ImageProvider could then be > extended > > by VectorImageProvider, BitmapImageProvider, MyOwnCustomPaintedImageProvider, > > etc. Something along the lines of: > > > > class ImageProvider { > > unique_ptr<ImageSkia> GetImage(const Size&); > > } > > > > And then if you wanted to get super fancy you could have the ImageProvider > have > > observers defined like below that ImageButton could inherit: > > > > class ImageProviderObserver{ > > virtual void ImageChanged(); > > } > > > > This would enable a few things such as: > > > > - The concrete ImageProvider could own and manage animations > > - We could have a small library of general ImageProviders that can be re-used. > > > This would support composability which is the primary driving motivation to my > > planned button refactor. > > > > sky@, WDYT? > > In practice I haven't seen any cases where it seems like the listener and color > provider should be different classes. Most of the time the view's parent just > implements all the necessary methods. I do agree it's conceptually wonky though. As noted below, this is partly because we have many is-a relationships which IMO would have been better with a has-a relationship. But FTR ShelfView and BookmarkBarView are two instances that comes to mind that implement a ButtonListener::ButtonPressed() that handles more than 1 button. I think it is quite reasonable that all of the buttons shouldn't have to have the same color, which this pattern would encourage. https://codereview.chromium.org/2744463002/diff/50001/ui/views/controls/butto... ui/views/controls/button/image_button.h:28: virtual SkColor GetVectorIconColor() const; On 2017/03/09 22:32:37, Bret Sepulveda wrote: > On 2017/03/09 04:05:02, sky wrote: > > Specifying the color should not be a requirement for using a vectoricon. > > Additionally as vectoricon support is implemented in terms of ImageSkia I > don't > > think ImageButton needs first class support for vectoricons. Rather I think > > there should be a function (not declared in ImageButton, perhaps in > > vector_image_buttn.h) for configuring ImageButton from an vectoricon. > Basically > > this function contains what you have in UpdateImagesFromVectorIcon. If you > want > > a factory function it can be in the same file. > > > > As to how to update the image color when the theme changes that should be > moved > > to ViewObserver. Specifically add OnThemeChanged and OnNativeThemeChanged to > > ViewObserver and call them from View. > > Subclasses aren't required to override GetVectorIconColor, there's a default > "return black" in the cc file. This is how VectorIconButton worked. > > To your larger point: basically you want a VectorIconButtonObserver that > subclasses ViewObserver that takes the vector icon and calls SetImage when > appropriate, and then a factory that wires it up, if I understand that right. > That doesn't seem substantively different than just keeping VectorIconButton to > me. It's different because it decouples the button state handling (aka input event) logic from the buttons presentation. Thus it should be re-usable across any ImageButton or ImageButton descendant. We currently have ~100 Button descendants and one contributing factor is because many buttons have chosen this is-a relationship instead of the has-a one that sky@ is suggesting. In short, an enthusiastic +1 to sky@'s suggestion and sentiment.
sky@ and bruthig@, PTAL at work-in-progress patchset 5. I played around with VectorIconButton and started to see why you wanted it as an observer instead. I added VectorIconImageButtonObserver and changed FindBarView to use it. It should be possible to also change the other clients of VectorIconButton. What do you think?
I will let sky@ comment further as OWNER but I'm liking the way this is starting to look. Thanks for pursuing the possibilities here. https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.cc (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.cc:20: const gfx::VectorIcon& icon, nit: |icon| doesn't appear to be required/used. https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.cc:44: void VectorIconImageButtonObserver::UpdateButtonImages(View* view) { I'm not sure if this is already on your radar or not but I want to call it out. We should make sure that an icon will properly be set even if OnThemeChanged() or OnNativeThemeChanged() is not called. And ideally we would not add any additional requirements on to the clients to call 'Init()' or something like that. I believe this scenario will happen if the View/Button that the observer is being attached to, is already a in the View tree before the observer is attached.
I like it! https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.h (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:23: class VIEWS_EXPORT VectorIconImageButtonObserver : public ViewObserver { How about naming this VectorIconImageUpdater or something? I suggest that as the ViewObserver part of this class is an implementation detail. The main reason for this class is getting the vector icon and color. Also, add a description. https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:33: virtual const gfx::VectorIcon& GetVectorIcon(View* view) const = 0; Having these two functions private certainly works, but IMO is confusing. How about moving these two to the protected section and moving the ViewObserver implementation to the private section? https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:34: virtual SkColor GetVectorIconColor() const; Add descriptions of these functions. https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:39: VIEWS_EXPORT ImageButton* CreateDefaultVectorIconButton( Please add a comment. Also, make this return a unique_ptr so ownership is clear. Also, 'default' is a little unclear here. How about CreateImageButtonWithVectorIcon? https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:41: VectorIconImageButtonObserver* observer, WDYT of making the button own the observer? I realize that wouldn't work with how you have things in find_bar, but it's an easy change and I suspect less error prone. Do you think it's going to be common to want to share the observer?
No new patchset, just looking for input on potential directions I could take this. https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.cc (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.cc:44: void VectorIconImageButtonObserver::UpdateButtonImages(View* view) { On 2017/03/13 15:19:49, bruthig wrote: > I'm not sure if this is already on your radar or not but I want to call it out. > > We should make sure that an icon will properly be set even if OnThemeChanged() > or OnNativeThemeChanged() is not called. And ideally we would not add any > additional requirements on to the clients to call 'Init()' or something like > that. > > I believe this scenario will happen if the View/Button that the observer is > being attached to, is already a in the View tree before the observer is > attached. I fixed this by giving the observer an Attach method that adds itself as an observer and then updates the icon. https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.h (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:39: VIEWS_EXPORT ImageButton* CreateDefaultVectorIconButton( On 2017/03/13 16:21:57, sky wrote: > Please add a comment. Also, make this return a unique_ptr so ownership is clear. > Also, 'default' is a little unclear here. How about > CreateImageButtonWithVectorIcon? The name is good. I'm not sure how unique_ptr interacts with the child view/parent view relationship, it seems to me like it wouldn't work. And it'd be silly to make all callers call release() on the pointer. https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:41: VectorIconImageButtonObserver* observer, On 2017/03/13 16:21:57, sky wrote: > WDYT of making the button own the observer? I realize that wouldn't work with > how you have things in find_bar, but it's an easy change and I suspect less > error prone. Do you think it's going to be common to want to share the observer? I wanted to avoid giving ImageButton knowledge of this class, but it does make the ownership pattern easier. I could extract a base class, "ImageUpdater" that calls UpdateButtonImages when appropriate and make VectorIconImageUpdater a subclass, that way ImageButton remains ignorant of everything vector-icon-related. But I don't see any other places that would want to make their own ImageUpdater so I'm worried it would be a wasted base class. On the other hand most of the current VectorIconButton instances are close buttons with the same icon, so we could have a global "CloseIconImageUpdater" for all of them if we can share observers. I'm not sure how compelling that is.
https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/image_button.cc (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/image_button.cc:99: void ImageButton::SetPadding(const gfx::Insets& padding) { What is the motivation to add this method instead of calling SetBorder directly? https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.h (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:23: class VIEWS_EXPORT VectorIconImageButtonObserver : public ViewObserver { with ViewObserver I'm no longer sure this interface is all that helpful. It doesn't cut down on the number of overrides needed. Maybe the owners of the vector icon buttons should just be ViewObservers.
https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.cc (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.cc:44: void VectorIconImageButtonObserver::UpdateButtonImages(View* view) { On 2017/03/15 00:07:13, Bret Sepulveda wrote: > On 2017/03/13 15:19:49, bruthig wrote: > > I'm not sure if this is already on your radar or not but I want to call it > out. > > > > We should make sure that an icon will properly be set even if OnThemeChanged() > > or OnNativeThemeChanged() is not called. And ideally we would not add any > > additional requirements on to the clients to call 'Init()' or something like > > that. > > > > I believe this scenario will happen if the View/Button that the observer is > > being attached to, is already a in the View tree before the observer is > > attached. > > I fixed this by giving the observer an Attach method that adds itself as an > observer and then updates the icon. You shouldn't need this. OnNativeThemeChanged() is always called when the view becomes parented to a widget. https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.h (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:39: VIEWS_EXPORT ImageButton* CreateDefaultVectorIconButton( On 2017/03/15 00:07:13, Bret Sepulveda wrote: > On 2017/03/13 16:21:57, sky wrote: > > Please add a comment. Also, make this return a unique_ptr so ownership is > clear. > > Also, 'default' is a little unclear here. How about > > CreateImageButtonWithVectorIcon? > > The name is good. I'm not sure how unique_ptr interacts with the child > view/parent view relationship, it seems to me like it wouldn't work. And it'd be > silly to make all callers call release() on the pointer. Calling release() still makes ownership clear. But I'm ok without unique_ptr here. https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:41: VectorIconImageButtonObserver* observer, On 2017/03/15 00:07:13, Bret Sepulveda wrote: > On 2017/03/13 16:21:57, sky wrote: > > WDYT of making the button own the observer? I realize that wouldn't work with > > how you have things in find_bar, but it's an easy change and I suspect less > > error prone. Do you think it's going to be common to want to share the > observer? > > I wanted to avoid giving ImageButton knowledge of this class, but it does make > the ownership pattern easier. I could extract a base class, "ImageUpdater" that > calls UpdateButtonImages when appropriate and make VectorIconImageUpdater a > subclass, that way ImageButton remains ignorant of everything > vector-icon-related. But I don't see any other places that would want to make > their own ImageUpdater so I'm worried it would be a wasted base class. > > On the other hand most of the current VectorIconButton instances are close > buttons with the same icon, so we could have a global "CloseIconImageUpdater" > for all of them if we can share observers. I'm not sure how compelling that is. Sorry for not being clear. I was suggesting ownership is handled by overriding OnViewIsDeleting() and deleting this.
https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.cc (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.cc:44: void VectorIconImageButtonObserver::UpdateButtonImages(View* view) { > You shouldn't need this. OnNativeThemeChanged() is always called when the view > becomes parented to a widget. What if the View is parented to a Widget before the VectorIconImageButtonObserver is attached? This may not happen currently because the ImageButton is being created in the CreateDefaultVectorIconButton() factory function. But eventually if we have different effects following this pattern (e.g. InkDropEffect) then it doesn't make sense for them all to go through these factory functions that ensure this. Relying on factory functions to support this is similar to relying on concrete class constructors to wire everything together which is one contributing factor to the overgrowth of the Button hierarchy. https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.h (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:23: class VIEWS_EXPORT VectorIconImageButtonObserver : public ViewObserver { On 2017/03/15 15:04:02, Evan Stade wrote: > with ViewObserver I'm no longer sure this interface is all that helpful. It > doesn't cut down on the number of overrides needed. Maybe the owners of the > vector icon buttons should just be ViewObservers. I think the value of this class will grow as it gets enhanced. The current value of this class is that it derives a disabled icon color and applies it depending on the observed view's state. I don't know why we would want to duplicate that logic in multiple vector icon owners. As for naming, I propose SimpleVectorIconImageUpdater. I imagine we will have fancier image updater use cases in the future, also be based on vector icons, that won't make sense to bloat this class with.
https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.h (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:23: class VIEWS_EXPORT VectorIconImageButtonObserver : public ViewObserver { On 2017/03/15 15:53:20, bruthig wrote: > On 2017/03/15 15:04:02, Evan Stade wrote: > > with ViewObserver I'm no longer sure this interface is all that helpful. It > > doesn't cut down on the number of overrides needed. Maybe the owners of the > > vector icon buttons should just be ViewObservers. > > I think the value of this class will grow as it gets enhanced. The current > value of this class is that it derives a disabled icon color and applies it > depending on the observed view's state. I don't know why we would want to > duplicate that logic in multiple vector icon owners. > > As for naming, I propose SimpleVectorIconImageUpdater. I imagine we will have > fancier image updater use cases in the future, also be based on vector icons, > that won't make sense to bloat this class with. I see. I think this is breaking the Chromium inheritance rules against multiple inheritance though. Only one base class can have non-trivial/no-op functionality.
https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.cc (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.cc:44: void VectorIconImageButtonObserver::UpdateButtonImages(View* view) { On 2017/03/15 15:53:20, bruthig wrote: > > You shouldn't need this. OnNativeThemeChanged() is always called when the view > > becomes parented to a widget. > > What if the View is parented to a Widget before the > VectorIconImageButtonObserver is attached? You're suggesting the function take a View? In that case it could call view->GetWidget() and if non-null call OnNativeThemeChanged() directly. > This may not happen currently > because the ImageButton is being created in the CreateDefaultVectorIconButton() > factory function. But eventually if we have different effects following this > pattern (e.g. InkDropEffect) then it doesn't make sense for them all to go > through these factory functions that ensure this. Relying on factory functions > to support this is similar to relying on concrete class constructors to wire > everything together which is one contributing factor to the overgrowth of the > Button hierarchy. I'm not sure I follow. Bret suggested adding another function explicitly for the initial call. That doesn't seem necessary given OnNativeThemeChanged() covers this. Maybe I'm missing something?
https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.h (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:23: class VIEWS_EXPORT VectorIconImageButtonObserver : public ViewObserver { On 2017/03/15 16:59:25, Evan Stade wrote: > On 2017/03/15 15:53:20, bruthig wrote: > > On 2017/03/15 15:04:02, Evan Stade wrote: > > > with ViewObserver I'm no longer sure this interface is all that helpful. It > > > doesn't cut down on the number of overrides needed. Maybe the owners of the > > > vector icon buttons should just be ViewObservers. > > > > I think the value of this class will grow as it gets enhanced. The current > > value of this class is that it derives a disabled icon color and applies it > > depending on the observed view's state. I don't know why we would want to > > duplicate that logic in multiple vector icon owners. > > > > As for naming, I propose SimpleVectorIconImageUpdater. I imagine we will have > > fancier image updater use cases in the future, also be based on vector icons, > > that won't make sense to bloat this class with. > > I see. I think this is breaking the Chromium inheritance rules against multiple > inheritance though. Only one base class can have non-trivial/no-op > functionality. FWIW, our UI code defies that rule in 8 billion places. Arguably, we could do better, by holding a lot more things by value instead of pointer (move constructors make this palatable), using composition when we can, using templates and inner classes to implement functionality instead of virtual inheritance -- the Sean Parent talk I linked in a Chromium-dev thread recently talks a lot about this stuff in the last third of the talk. But I don't know how much appetite there is to make that change broadly (no one has talked about doing it, not even me), and I'm worried it's hard to get there piecemeal. And when the pattern in the UI code is to use multiple inheritance to do this, it seems less bad to also do so here? Anyway, not really arguing hard for one point of view.
https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.h (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:23: class VIEWS_EXPORT VectorIconImageButtonObserver : public ViewObserver { On 2017/03/15 18:03:02, Peter Kasting wrote: > On 2017/03/15 16:59:25, Evan Stade wrote: > > On 2017/03/15 15:53:20, bruthig wrote: > > > On 2017/03/15 15:04:02, Evan Stade wrote: > > > > with ViewObserver I'm no longer sure this interface is all that helpful. > It > > > > doesn't cut down on the number of overrides needed. Maybe the owners of > the > > > > vector icon buttons should just be ViewObservers. > > > > > > I think the value of this class will grow as it gets enhanced. The current > > > value of this class is that it derives a disabled icon color and applies it > > > depending on the observed view's state. I don't know why we would want to > > > duplicate that logic in multiple vector icon owners. > > > > > > As for naming, I propose SimpleVectorIconImageUpdater. I imagine we will > have > > > fancier image updater use cases in the future, also be based on vector > icons, > > > that won't make sense to bloat this class with. > > > > I see. I think this is breaking the Chromium inheritance rules against > multiple > > inheritance though. Only one base class can have non-trivial/no-op > > functionality. > > FWIW, our UI code defies that rule in 8 billion places. Evan sez: we've fixed a lot of these and are trying not to add more. I am on board with this sentiment.
https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.cc (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.cc:44: void VectorIconImageButtonObserver::UpdateButtonImages(View* view) { On 2017/03/15 17:01:54, sky wrote: > On 2017/03/15 15:53:20, bruthig wrote: > > > You shouldn't need this. OnNativeThemeChanged() is always called when the > view > > > becomes parented to a widget. > > > > What if the View is parented to a Widget before the > > VectorIconImageButtonObserver is attached? > > You're suggesting the function take a View? In that case it could call > view->GetWidget() and if non-null call OnNativeThemeChanged() directly. disclaimer: I'm not clear on what 'function' you mean here or where exactly Bret's original Attach() would live. I wasn't orignally suggesting a solution I was just requesting that it be possible to attach the observer after the button had been parented to a Widget. I think it is limiting to require the button to be parented to a Widget before the observers are added. sky@, this may be what you are suggesting but thinking about it now, I would probably explore adding a ViewObserver::AttachedToView(View* observed_view) method that is called when observers are added to a View. Then the VectorIconImageButtonObserver could set an image and use the theme if it's available. > > > This may not happen currently > > because the ImageButton is being created in the > CreateDefaultVectorIconButton() > > factory function. But eventually if we have different effects following this > > pattern (e.g. InkDropEffect) then it doesn't make sense for them all to go > > through these factory functions that ensure this. Relying on factory > functions > > to support this is similar to relying on concrete class constructors to wire > > everything together which is one contributing factor to the overgrowth of the > > Button hierarchy. > > I'm not sure I follow. Bret suggested adding another function explicitly for the > initial call. That doesn't seem necessary given OnNativeThemeChanged() covers > this. Maybe I'm missing something? https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.h (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:23: class VIEWS_EXPORT VectorIconImageButtonObserver : public ViewObserver { On 2017/03/15 16:59:25, Evan Stade wrote: > On 2017/03/15 15:53:20, bruthig wrote: > > On 2017/03/15 15:04:02, Evan Stade wrote: > > > with ViewObserver I'm no longer sure this interface is all that helpful. It > > > doesn't cut down on the number of overrides needed. Maybe the owners of the > > > vector icon buttons should just be ViewObservers. > > > > I think the value of this class will grow as it gets enhanced. The current > > value of this class is that it derives a disabled icon color and applies it > > depending on the observed view's state. I don't know why we would want to > > duplicate that logic in multiple vector icon owners. > > > > As for naming, I propose SimpleVectorIconImageUpdater. I imagine we will have > > fancier image updater use cases in the future, also be based on vector icons, > > that won't make sense to bloat this class with. > > I see. I think this is breaking the Chromium inheritance rules against multiple > inheritance though. Only one base class can have non-trivial/no-op > functionality. The FindBarViewButtonObserver found in find_bar_view.h only inherits from VectorIconImageButtonObserver. Not sure where we are violating the multiple inheritance restriction. To be honest I'd rather see a concrete re-usable VectorIconUpdater that maps view pointers to vector icons instead of having a specialized class like FindBarViewButtonObserver. Ex: FindBarView::FindBarView(...) { ... VectorIconUpdater* icon_updater = new VectorIconUpdator(); icon_updator->SetThemColorId(ui::NativeTheme::kColorId_TextfieldDefaultColor); icon_updater->AddImage(find_previous_button_, ui::kCaretUpIcon); icon_updater->AddImage(find_next_button_, ui::kCaretDownIcon); icon_updater->AddImage(close_button_, ui::kCloseIcon); AddObserver(icon_updater); ... }
https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.h (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:23: class VIEWS_EXPORT VectorIconImageButtonObserver : public ViewObserver { On 2017/03/15 21:41:21, bruthig wrote: > On 2017/03/15 16:59:25, Evan Stade wrote: > > On 2017/03/15 15:53:20, bruthig wrote: > > > On 2017/03/15 15:04:02, Evan Stade wrote: > > > > with ViewObserver I'm no longer sure this interface is all that helpful. > It > > > > doesn't cut down on the number of overrides needed. Maybe the owners of > the > > > > vector icon buttons should just be ViewObservers. > > > > > > I think the value of this class will grow as it gets enhanced. The current > > > value of this class is that it derives a disabled icon color and applies it > > > depending on the observed view's state. I don't know why we would want to > > > duplicate that logic in multiple vector icon owners. > > > > > > As for naming, I propose SimpleVectorIconImageUpdater. I imagine we will > have > > > fancier image updater use cases in the future, also be based on vector > icons, > > > that won't make sense to bloat this class with. > > > > I see. I think this is breaking the Chromium inheritance rules against > multiple > > inheritance though. Only one base class can have non-trivial/no-op > > functionality. > > The FindBarViewButtonObserver found in find_bar_view.h only inherits from > VectorIconImageButtonObserver. Not sure where we are violating the multiple > inheritance restriction. That's kind of an elaborate way to get around multiple inheritance. Are all six current VectorIconButtonDelegate implementers going to create a private class like this, e.g. DownloadItemViewButtonObserver? I guess I'm struggling to see how defining and owning a thunk-like class that overrides a half-interface, half-class base class which in turn overrides another related interface is a win over having VectorIconButton. If we really don't want VectorIconButton, I think we should just be folding the functionality into ImageButton, i.e. ImageButton::SetVectorIcon(gfx::VectorIcon, SkColor)
> I think we should just be folding the functionality into ImageButton, i.e. > ImageButton::SetVectorIcon(gfx::VectorIcon, SkColor) in fact, this could be a static utility. No need to be part of ImageButton. https://codereview.chromium.org/2744463002/diff/70001/chrome/browser/ui/views... File chrome/browser/ui/views/find_bar_view.cc (right): https://codereview.chromium.org/2744463002/diff/70001/chrome/browser/ui/views... chrome/browser/ui/views/find_bar_view.cc:362: if (view == parent_->find_previous_button_) to elaborate, this is also objectionable in my eyes. The output of GetVectorIcon depends on the inner workings of FindBarView, and it should be part of FindBarView. I'm pretty sure accessing a private member variable is a violation of style guidelines, and the resolution of providing accessors is also not great. What this indicates to me is that the FindBarView is the one that ought to be the observer.
I uploaded a new WIP patchset, let me know what you think. The main changes are: * Renamed VectorIconImageButtonObserver to VectorIconImageUpdater, and made it concrete. It takes an icon on construction instead of the subclass providing an icon. * Removed changes from ImageButton, the updater now owns itself (sort of). * Converted DownloadItemView to use the updater. * Made InkDropHostView::AnimateInkDrop public so I could eliminate DropDownButton. I went back and forth on this change, so let me know if you don't like this. https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/image_button.cc (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/image_button.cc:99: void ImageButton::SetPadding(const gfx::Insets& padding) { On 2017/03/15 15:04:02, Evan Stade wrote: > What is the motivation to add this method instead of calling SetBorder directly? I see a lot of places where we're doing SetBorder(CreateEmptyBorder(etc)) but nowhere where we're setting a different kind of border. So I figured I would make that pattern explicit here. https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.cc (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.cc:44: void VectorIconImageButtonObserver::UpdateButtonImages(View* view) { On 2017/03/15 21:41:21, bruthig wrote: > On 2017/03/15 17:01:54, sky wrote: > > On 2017/03/15 15:53:20, bruthig wrote: > > > > You shouldn't need this. OnNativeThemeChanged() is always called when the > > view > > > > becomes parented to a widget. > > > > > > What if the View is parented to a Widget before the > > > VectorIconImageButtonObserver is attached? > > > > You're suggesting the function take a View? In that case it could call > > view->GetWidget() and if non-null call OnNativeThemeChanged() directly. > > disclaimer: I'm not clear on what 'function' you mean here or where exactly > Bret's original Attach() would live. > > I wasn't orignally suggesting a solution I was just requesting that it be > possible to attach the observer after the button had been parented to a Widget. > I think it is limiting to require the button to be parented to a Widget before > the observers are added. > > sky@, this may be what you are suggesting but thinking about it now, I would > probably explore adding a ViewObserver::AttachedToView(View* observed_view) > method that is called when observers are added to a View. Then the > VectorIconImageButtonObserver could set an image and use the theme if it's > available. > > > > > > This may not happen currently > > > because the ImageButton is being created in the > > CreateDefaultVectorIconButton() > > > factory function. But eventually if we have different effects following > this > > > pattern (e.g. InkDropEffect) then it doesn't make sense for them all to go > > > through these factory functions that ensure this. Relying on factory > > functions > > > to support this is similar to relying on concrete class constructors to wire > > > everything together which is one contributing factor to the overgrowth of > the > > > Button hierarchy. > > > > I'm not sure I follow. Bret suggested adding another function explicitly for > the > > initial call. That doesn't seem necessary given OnNativeThemeChanged() covers > > this. Maybe I'm missing something? I think bruthig@ is right, that you could hypothetically create an image button, add it to the hierarchy, and THEN add this observer. Then the image wouldn't get set from the vector icon. Unless you mean adding an observer itself triggers OnNativeThemeChanged? But I don't see that code. I moved AddObserver inside an "Attach" method on the observer that updates the images too. But the ViewObserver::AttachedToView idea sounds reasonable to me, since then you don't have to call a different method (Attach vs AddObserver) to wire everything up correctly. https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.h (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:23: class VIEWS_EXPORT VectorIconImageButtonObserver : public ViewObserver { To bruthig's suggestion, I thought about doing something where the observer would have a map of button to icon, but since there's a variety of ways to fetch the icon color on update subclasses are probably still necessary. To estade's suggestion, I added ImageButton::SetVectorIcon in patchset 1, if you want to see how that worked out. The icon color needs to update on a theme change, so we can't just set-and-forget the icon color. https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.h:41: VectorIconImageButtonObserver* observer, On 2017/03/15 15:05:11, sky wrote: > On 2017/03/15 00:07:13, Bret Sepulveda wrote: > > On 2017/03/13 16:21:57, sky wrote: > > > WDYT of making the button own the observer? I realize that wouldn't work > with > > > how you have things in find_bar, but it's an easy change and I suspect less > > > error prone. Do you think it's going to be common to want to share the > > observer? > > > > I wanted to avoid giving ImageButton knowledge of this class, but it does make > > the ownership pattern easier. I could extract a base class, "ImageUpdater" > that > > calls UpdateButtonImages when appropriate and make VectorIconImageUpdater a > > subclass, that way ImageButton remains ignorant of everything > > vector-icon-related. But I don't see any other places that would want to make > > their own ImageUpdater so I'm worried it would be a wasted base class. > > > > On the other hand most of the current VectorIconButton instances are close > > buttons with the same icon, so we could have a global "CloseIconImageUpdater" > > for all of them if we can share observers. I'm not sure how compelling that > is. > > Sorry for not being clear. I was suggesting ownership is handled by overriding > OnViewIsDeleting() and deleting this. I did this and it works well.
https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... File ui/views/controls/button/vector_icon_image_button_observer.cc (right): https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... ui/views/controls/button/vector_icon_image_button_observer.cc:44: void VectorIconImageButtonObserver::UpdateButtonImages(View* view) { On 2017/03/15 23:39:19, Bret Sepulveda wrote: > On 2017/03/15 21:41:21, bruthig wrote: > > On 2017/03/15 17:01:54, sky wrote: > > > On 2017/03/15 15:53:20, bruthig wrote: > > > > > You shouldn't need this. OnNativeThemeChanged() is always called when > the > > > view > > > > > becomes parented to a widget. > > > > > > > > What if the View is parented to a Widget before the > > > > VectorIconImageButtonObserver is attached? > > > > > > You're suggesting the function take a View? In that case it could call > > > view->GetWidget() and if non-null call OnNativeThemeChanged() directly. > > > > disclaimer: I'm not clear on what 'function' you mean here or where exactly > > Bret's original Attach() would live. > > > > I wasn't orignally suggesting a solution I was just requesting that it be > > possible to attach the observer after the button had been parented to a > Widget. > > I think it is limiting to require the button to be parented to a Widget before > > the observers are added. > > > > sky@, this may be what you are suggesting but thinking about it now, I would > > probably explore adding a ViewObserver::AttachedToView(View* observed_view) > > method that is called when observers are added to a View. Then the > > VectorIconImageButtonObserver could set an image and use the theme if it's > > available. > > > > > > > > > This may not happen currently > > > > because the ImageButton is being created in the > > > CreateDefaultVectorIconButton() > > > > factory function. But eventually if we have different effects following > > this > > > > pattern (e.g. InkDropEffect) then it doesn't make sense for them all to go > > > > through these factory functions that ensure this. Relying on factory > > > functions > > > > to support this is similar to relying on concrete class constructors to > wire > > > > everything together which is one contributing factor to the overgrowth of > > the > > > > Button hierarchy. > > > > > > I'm not sure I follow. Bret suggested adding another function explicitly for > > the > > > initial call. That doesn't seem necessary given OnNativeThemeChanged() > covers > > > this. Maybe I'm missing something? > > I think bruthig@ is right, that you could hypothetically create an image button, > add it to the hierarchy, and THEN add this observer. Then the image wouldn't get > set from the vector icon. Unless you mean adding an observer itself triggers > OnNativeThemeChanged? But I don't see that code. > > I moved AddObserver inside an "Attach" method on the observer that updates the > images too. But the ViewObserver::AttachedToView idea sounds reasonable to me, > since then you don't have to call a different method (Attach vs AddObserver) to > wire everything up correctly. You could make that argument for any observer. I think whoever is attaching the observer to the view should know if they need to make additional calls before attaching the observer. I don't think we should add functions like ObserverAttached and ObserverDetached.
On 2017/03/15 23:39:20, Bret Sepulveda wrote: > To estade's suggestion, I added ImageButton::SetVectorIcon in patchset 1, if you > want to see how that worked out. The icon color needs to update on a theme > change, so we can't just set-and-forget the icon color. the idea behind my suggestion is that you don't need VectorIconImageUpdater or FindBarViewButtonImageUpdater, which I don't think is scalable to all vector icon owners, or even desirable for FindBarView. The owner of the button (FindBarView in this case) just becomes a ViewObserver and calls SetVectorIconForImageButton() as appropriate (either of the theme change notifications).
On 2017/03/15 22:46:20, Evan Stade wrote: > https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... > File ui/views/controls/button/vector_icon_image_button_observer.h (right): > > https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... > ui/views/controls/button/vector_icon_image_button_observer.h:23: class > VIEWS_EXPORT VectorIconImageButtonObserver : public ViewObserver { > On 2017/03/15 21:41:21, bruthig wrote: > > On 2017/03/15 16:59:25, Evan Stade wrote: > > > On 2017/03/15 15:53:20, bruthig wrote: > > > > On 2017/03/15 15:04:02, Evan Stade wrote: > > > > > with ViewObserver I'm no longer sure this interface is all that helpful. > > It > > > > > doesn't cut down on the number of overrides needed. Maybe the owners of > > the > > > > > vector icon buttons should just be ViewObservers. > > > > > > > > I think the value of this class will grow as it gets enhanced. The > current > > > > value of this class is that it derives a disabled icon color and applies > it > > > > depending on the observed view's state. I don't know why we would want to > > > > duplicate that logic in multiple vector icon owners. > > > > > > > > As for naming, I propose SimpleVectorIconImageUpdater. I imagine we will > > have > > > > fancier image updater use cases in the future, also be based on vector > > icons, > > > > that won't make sense to bloat this class with. > > > > > > I see. I think this is breaking the Chromium inheritance rules against > > multiple > > > inheritance though. Only one base class can have non-trivial/no-op > > > functionality. > > > > The FindBarViewButtonObserver found in find_bar_view.h only inherits from > > VectorIconImageButtonObserver. Not sure where we are violating the multiple > > inheritance restriction. > > That's kind of an elaborate way to get around multiple inheritance. Are all six > current VectorIconButtonDelegate implementers going to create a private class > like this, e.g. DownloadItemViewButtonObserver? > > I guess I'm struggling to see how defining and owning a thunk-like class that > overrides a half-interface, half-class base class which in turn overrides > another related interface is a win over having VectorIconButton. If we really > don't want VectorIconButton, I think we should just be folding the functionality > into ImageButton, i.e. ImageButton::SetVectorIcon(gfx::VectorIcon, SkColor) See earlier comments from me on this thread. I don't want to see more functionality folded into ImageButton that can be trivially implemented outside of it. Folding the logic into ImageButton makes it more complicated than I think it needs to be.
https://codereview.chromium.org/2744463002/diff/90001/ui/views/controls/butto... File ui/views/controls/button/image_button.h (right): https://codereview.chromium.org/2744463002/diff/90001/ui/views/controls/butto... ui/views/controls/button/image_button.h:76: void SetPadding(const gfx::Insets& padding); SetPadding makes it sound like this is setting something in addition to the border, which isn't the case. I would rather callers explicitly set the border. Less confusion as what SetBorder(...) does and less duplicate ways of doing the same thing.
On 2017/03/16 04:12:00, sky wrote: > On 2017/03/15 22:46:20, Evan Stade wrote: > > > https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... > > File ui/views/controls/button/vector_icon_image_button_observer.h (right): > > > > > https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... > > ui/views/controls/button/vector_icon_image_button_observer.h:23: class > > VIEWS_EXPORT VectorIconImageButtonObserver : public ViewObserver { > > On 2017/03/15 21:41:21, bruthig wrote: > > > On 2017/03/15 16:59:25, Evan Stade wrote: > > > > On 2017/03/15 15:53:20, bruthig wrote: > > > > > On 2017/03/15 15:04:02, Evan Stade wrote: > > > > > > with ViewObserver I'm no longer sure this interface is all that > helpful. > > > It > > > > > > doesn't cut down on the number of overrides needed. Maybe the owners > of > > > the > > > > > > vector icon buttons should just be ViewObservers. > > > > > > > > > > I think the value of this class will grow as it gets enhanced. The > > current > > > > > value of this class is that it derives a disabled icon color and applies > > it > > > > > depending on the observed view's state. I don't know why we would want > to > > > > > duplicate that logic in multiple vector icon owners. > > > > > > > > > > As for naming, I propose SimpleVectorIconImageUpdater. I imagine we > will > > > have > > > > > fancier image updater use cases in the future, also be based on vector > > > icons, > > > > > that won't make sense to bloat this class with. > > > > > > > > I see. I think this is breaking the Chromium inheritance rules against > > > multiple > > > > inheritance though. Only one base class can have non-trivial/no-op > > > > functionality. > > > > > > The FindBarViewButtonObserver found in find_bar_view.h only inherits from > > > VectorIconImageButtonObserver. Not sure where we are violating the multiple > > > inheritance restriction. > > > > That's kind of an elaborate way to get around multiple inheritance. Are all > six > > current VectorIconButtonDelegate implementers going to create a private class > > like this, e.g. DownloadItemViewButtonObserver? > > > > I guess I'm struggling to see how defining and owning a thunk-like class that > > overrides a half-interface, half-class base class which in turn overrides > > another related interface is a win over having VectorIconButton. If we really > > don't want VectorIconButton, I think we should just be folding the > functionality > > into ImageButton, i.e. ImageButton::SetVectorIcon(gfx::VectorIcon, SkColor) > > See earlier comments from me on this thread. I don't want to see more > functionality folded into ImageButton that can be trivially implemented outside > of it. Folding the logic into ImageButton makes it more complicated than I think > it needs to be. OK, for the same reason I suggested a utility function instead. That comment, which was a follow up, might have gotten lost. This thread is long and has many conversations, so here is my suggestion in the form of a CL which I think is a lot less elaborate way of accomplishing the goal of deleting VectorIconButton: https://codereview.chromium.org/2753833003/
Yes!!! I would probably put the function in a separate file, but that's a small nit. Thanks for clarifying what you mean with some code. -Scott On Thu, Mar 16, 2017 at 7:12 AM, <estade@chromium.org> wrote: > On 2017/03/16 04:12:00, sky wrote: >> On 2017/03/15 22:46:20, Evan Stade wrote: >> > >> > https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... >> > File ui/views/controls/button/vector_icon_image_button_observer.h >> > (right): >> > >> > >> > https://codereview.chromium.org/2744463002/diff/70001/ui/views/controls/butto... >> > ui/views/controls/button/vector_icon_image_button_observer.h:23: class >> > VIEWS_EXPORT VectorIconImageButtonObserver : public ViewObserver { >> > On 2017/03/15 21:41:21, bruthig wrote: >> > > On 2017/03/15 16:59:25, Evan Stade wrote: >> > > > On 2017/03/15 15:53:20, bruthig wrote: >> > > > > On 2017/03/15 15:04:02, Evan Stade wrote: >> > > > > > with ViewObserver I'm no longer sure this interface is all that >> helpful. >> > > It >> > > > > > doesn't cut down on the number of overrides needed. Maybe the >> > > > > > owners >> of >> > > the >> > > > > > vector icon buttons should just be ViewObservers. >> > > > > >> > > > > I think the value of this class will grow as it gets enhanced. The >> > current >> > > > > value of this class is that it derives a disabled icon color and > applies >> > it >> > > > > depending on the observed view's state. I don't know why we would > want >> to >> > > > > duplicate that logic in multiple vector icon owners. >> > > > > >> > > > > As for naming, I propose SimpleVectorIconImageUpdater. I imagine >> > > > > we >> will >> > > have >> > > > > fancier image updater use cases in the future, also be based on >> > > > > vector >> > > icons, >> > > > > that won't make sense to bloat this class with. >> > > > >> > > > I see. I think this is breaking the Chromium inheritance rules >> > > > against >> > > multiple >> > > > inheritance though. Only one base class can have non-trivial/no-op >> > > > functionality. >> > > >> > > The FindBarViewButtonObserver found in find_bar_view.h only inherits >> > > from >> > > VectorIconImageButtonObserver. Not sure where we are violating the > multiple >> > > inheritance restriction. >> > >> > That's kind of an elaborate way to get around multiple inheritance. Are >> > all >> six >> > current VectorIconButtonDelegate implementers going to create a private > class >> > like this, e.g. DownloadItemViewButtonObserver? >> > >> > I guess I'm struggling to see how defining and owning a thunk-like class > that >> > overrides a half-interface, half-class base class which in turn >> > overrides >> > another related interface is a win over having VectorIconButton. If we > really >> > don't want VectorIconButton, I think we should just be folding the >> functionality >> > into ImageButton, i.e. ImageButton::SetVectorIcon(gfx::VectorIcon, >> > SkColor) >> >> See earlier comments from me on this thread. I don't want to see more >> functionality folded into ImageButton that can be trivially implemented > outside >> of it. Folding the logic into ImageButton makes it more complicated than I > think >> it needs to be. > > OK, for the same reason I suggested a utility function instead. That > comment, > which was a follow up, might have gotten lost. This thread is long and has > many > conversations, so here is my suggestion in the form of a CL which I think is > a > lot less elaborate way of accomplishing the goal of deleting > VectorIconButton: > https://codereview.chromium.org/2753833003/ > > https://codereview.chromium.org/2744463002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Add VectorIconButton functionality to ImageButton. There are only a few users of VectorIconButton, and its hard-coded nature means it is not very flexible. This patch moves its functionality into ImageButton and adds a function that provides a default button with a vector icon for existing users. This patch also removes the button padding in Harmony and fixes the find bar buttons to work with less padding. BUG=691895 ========== to ========== Move VectorIconButton functionality into static utilities. There are only a few users of VectorIconButton, and its hard-coded nature means it is not very flexible. This patch moves its functionality into two static utilities, one which creates an ImageButton with the existing vector button styling, and another that updates the ImageButton's images from an icon. The owner classes are now responsible for updating the styling if the theme changes. BUG=691895 ==========
PTAL at patchset 8, which uses Evan's idea. I removed the harmony-related changes and updated the description. sky@ is the only required reviewer though the rest of you are welcome to review as well.
net negative CL for a big win!! LGTM
lgtm with nits https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:502: UpdateDropdownButton(); this may not be necessary because the color is derived from the browser theme (ThemeProvider) so the updates in OnThemeChanged ought to be sufficient. https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_shelf_view.cc (right): https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_shelf_view.cc:119: UpdateCloseButton(); is this necessary? We do it when added to the view hierarchy. https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_views_util.cc (right): https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_views_util.cc:159: SK_ColorBLACK); nit: black is technically wrong here although in practice it may not matter. It should match nearby text, which is going to be the color of title_label. See comment about defaults in image_button_util.h https://codereview.chromium.org/2744463002/diff/130001/ui/views/animation/ink... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2744463002/diff/130001/ui/views/animation/ink... ui/views/animation/ink_drop_host_view.cc:188: if (ink_drop_mode == ink_drop_mode_) this doesn't seem like a bad thing but could it have unintended consequences? Aside from perhaps tests, it seems like this is only called right after construction. Can we make that a requirement? Either way, I don't think it's necessary for this change any more, so maybe it should be separated. https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... File ui/views/controls/button/image_button_util.cc (right): https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... ui/views/controls/button/image_button_util.cc:26: button->SetBorder(CreateEmptyBorder(gfx::Insets(kButtonExtraTouchSize))); VectorIconButton checks if a different border has been set, do we not want that here? I can't remember if that check was put there for a particular reason. https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... File ui/views/controls/button/image_button_util.h (right): https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... ui/views/controls/button/image_button_util.h:5: #ifndef UI_VIEWS_CONTROLS_BUTTON_IMAGE_BUTTON_UTIL_H_ nit: not sure how many more things will be added to this file. If we're trying to consolidate button classes though, there may be similar things added for other button types. Should we make one button_util file? https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... ui/views/controls/button/image_button_util.h:22: VIEWS_EXPORT ImageButton* CreateImageButtonWithVectorIconStyling( nit: naming is hard, but I'd give this a shorter name, like CreateVectorImageButton https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... ui/views/controls/button/image_button_util.h:29: SkColor base_color); nit: the nomenclature here is probably my fault, but it's weird that "base" color is used in two different senses in this function ("base" for the vector icon is not "base" for the ink drop). We might want to call the parameter adjacent_text_color or something (because it's the color value for nearby/would be nearby text, e.g. for the tab close button it would be tab text). It also bears mentioning in the documentation above what this value means. https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... ui/views/controls/button/image_button_util.h:29: SkColor base_color); nit: I think we should make base_color optional, and if not passed use GetNativeTheme()->GetSystemColor(kColorId_LabelEnabledColor)
Nice cleanup! LGTM https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... File ui/views/controls/button/image_button_util.h (right): https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... ui/views/controls/button/image_button_util.h:5: #ifndef UI_VIEWS_CONTROLS_BUTTON_IMAGE_BUTTON_UTIL_H_ On 2017/03/17 13:54:44, Evan Stade wrote: > nit: not sure how many more things will be added to this file. If we're trying > to consolidate button classes though, there may be similar things added for > other button types. Should we make one button_util file? I prefer not having files that become dumping grounds. I might go with image_button_factory, which isn't entirely true but close. If you prefer the name you have here, stick with it. https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... ui/views/controls/button/image_button_util.h:26: // vector icon and color. optional: as a common pattern is calling this from theme changed it worth documenting an example of that?
https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:502: UpdateDropdownButton(); On 2017/03/17 13:54:44, Evan Stade wrote: > this may not be necessary because the color is derived from the browser theme > (ThemeProvider) so the updates in OnThemeChanged ought to be sufficient. The button is initially red without this because the theme provider isn't plumbed in yet. When it is we get OnNativeThemeChanged. So this is necessary. https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_shelf_view.cc (right): https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_shelf_view.cc:119: UpdateCloseButton(); On 2017/03/17 13:54:44, Evan Stade wrote: > is this necessary? We do it when added to the view hierarchy. This one isn't necessary. Removed. https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/payments/payment_request_views_util.cc (right): https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/payments/payment_request_views_util.cc:159: SK_ColorBLACK); On 2017/03/17 13:54:44, Evan Stade wrote: > nit: black is technically wrong here although in practice it may not matter. It > should match nearby text, which is going to be the color of title_label. See > comment about defaults in image_button_util.h It was using black before, by way of VectorIconButtonDelegate's default, and no one was complaining. This is used by multiple views so anything more complicated would require additional plumbing. https://codereview.chromium.org/2744463002/diff/130001/ui/views/animation/ink... File ui/views/animation/ink_drop_host_view.cc (right): https://codereview.chromium.org/2744463002/diff/130001/ui/views/animation/ink... ui/views/animation/ink_drop_host_view.cc:188: if (ink_drop_mode == ink_drop_mode_) On 2017/03/17 13:54:44, Evan Stade wrote: > this doesn't seem like a bad thing but could it have unintended consequences? > > Aside from perhaps tests, it seems like this is only called right after > construction. Can we make that a requirement? > > Either way, I don't think it's necessary for this change any more, so maybe it > should be separated. I didn't see any trouble with this when I was testing, but reverted for now anyway. https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... File ui/views/controls/button/image_button_util.cc (right): https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... ui/views/controls/button/image_button_util.cc:26: button->SetBorder(CreateEmptyBorder(gfx::Insets(kButtonExtraTouchSize))); On 2017/03/17 13:54:44, Evan Stade wrote: > VectorIconButton checks if a different border has been set, do we not want that > here? I can't remember if that check was put there for a particular reason. It's doing that because its SetBorder call is in SetIcon and it's trying to be idempotent. I'm not really sure why it's in SetIcon in the first place so I moved it to the constructor. https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... File ui/views/controls/button/image_button_util.h (right): https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... ui/views/controls/button/image_button_util.h:5: #ifndef UI_VIEWS_CONTROLS_BUTTON_IMAGE_BUTTON_UTIL_H_ On 2017/03/17 15:38:12, sky wrote: > On 2017/03/17 13:54:44, Evan Stade wrote: > > nit: not sure how many more things will be added to this file. If we're trying > > to consolidate button classes though, there may be similar things added for > > other button types. Should we make one button_util file? > > I prefer not having files that become dumping grounds. I might go with > image_button_factory, which isn't entirely true but close. If you prefer the > name you have here, stick with it. I went with image_button_factory as the more specific name. https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... ui/views/controls/button/image_button_util.h:22: VIEWS_EXPORT ImageButton* CreateImageButtonWithVectorIconStyling( On 2017/03/17 13:54:45, Evan Stade wrote: > nit: naming is hard, but I'd give this a shorter name, like > CreateVectorImageButton This one is pretty tough. I went with your suggestion. https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... ui/views/controls/button/image_button_util.h:26: // vector icon and color. On 2017/03/17 15:38:12, sky wrote: > optional: as a common pattern is calling this from theme changed it worth > documenting an example of that? I put in a sentence about calling this again if the color changes. https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... ui/views/controls/button/image_button_util.h:29: SkColor base_color); On 2017/03/17 13:54:45, Evan Stade wrote: > nit: I think we should make base_color optional, and if not passed use > GetNativeTheme()->GetSystemColor(kColorId_LabelEnabledColor) I updated the names and comment. I'm a bit hesitant to make the color actually optional, since for this patch I'd rather all callers just pass in the color they're using right now.
https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:502: UpdateDropdownButton(); On 2017/03/17 23:41:41, Bret Sepulveda wrote: > On 2017/03/17 13:54:44, Evan Stade wrote: > > this may not be necessary because the color is derived from the browser theme > > (ThemeProvider) so the updates in OnThemeChanged ought to be sufficient. > > The button is initially red without this because the theme provider isn't > plumbed in yet. When it is we get OnNativeThemeChanged. So this is necessary. This may be right but only incidentally so. You'd get OnNativeThemeChanged when added to a widget hierarchy, at which point you'd also have a ThemeProvider. But that doesn't mean OnNativeThemeChanged is the right thing to override. You'd either want to use OnViewHierarchyChanged or make it so OnThemeChanged is called when the theme is first ready, like OnNativeThemeChanged[1] [1] https://cs.chromium.org/chromium/src/ui/views/view.cc?rcl=a5082d6ce45219eba13... https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... File ui/views/controls/button/image_button_util.h (right): https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... ui/views/controls/button/image_button_util.h:29: SkColor base_color); On 2017/03/17 23:41:41, Bret Sepulveda wrote: > On 2017/03/17 13:54:45, Evan Stade wrote: > > nit: I think we should make base_color optional, and if not passed use > > GetNativeTheme()->GetSystemColor(kColorId_LabelEnabledColor) > > I updated the names and comment. I'm a bit hesitant to make the color actually > optional, since for this patch I'd rather all callers just pass in the color > they're using right now. It was already optional. Callers that didn't specify a color would get black. Let's continue making the color optional and default to black. I think the label text color trick won't work until Harmony is on.
I'll submit this when I get in next week if there are no more comments. https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2744463002/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:502: UpdateDropdownButton(); On 2017/03/18 00:27:57, Evan Stade wrote: > On 2017/03/17 23:41:41, Bret Sepulveda wrote: > > On 2017/03/17 13:54:44, Evan Stade wrote: > > > this may not be necessary because the color is derived from the browser > theme > > > (ThemeProvider) so the updates in OnThemeChanged ought to be sufficient. > > > > The button is initially red without this because the theme provider isn't > > plumbed in yet. When it is we get OnNativeThemeChanged. So this is necessary. > > This may be right but only incidentally so. You'd get OnNativeThemeChanged when > added to a widget hierarchy, at which point you'd also have a ThemeProvider. But > that doesn't mean OnNativeThemeChanged is the right thing to override. You'd > either want to use OnViewHierarchyChanged or make it so OnThemeChanged is called > when the theme is first ready, like OnNativeThemeChanged[1] > > [1] > https://cs.chromium.org/chromium/src/ui/views/view.cc?rcl=a5082d6ce45219eba13... Okay. Changed to a ViewHierachyChanged override. I think in principle we should call OnThemeChanged when the theme provider changes in that case, but it seems like ToolbarView at least doesn't expect to have that called right away, so I'm going to let that refactor happen in another patch. https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... File ui/views/controls/button/image_button_util.h (right): https://codereview.chromium.org/2744463002/diff/130001/ui/views/controls/butt... ui/views/controls/button/image_button_util.h:29: SkColor base_color); On 2017/03/18 00:27:58, Evan Stade wrote: > On 2017/03/17 23:41:41, Bret Sepulveda wrote: > > On 2017/03/17 13:54:45, Evan Stade wrote: > > > nit: I think we should make base_color optional, and if not passed use > > > GetNativeTheme()->GetSystemColor(kColorId_LabelEnabledColor) > > > > I updated the names and comment. I'm a bit hesitant to make the color actually > > optional, since for this patch I'd rather all callers just pass in the color > > they're using right now. > > It was already optional. Callers that didn't specify a color would get black. > Let's continue making the color optional and default to black. I think the label > text color trick won't work until Harmony is on. Okay, done.
The CQ bit was checked by bsep@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 bsep@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/2744463002/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2744463002/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:505: UpdateDropdownButton(); nit: you should check the details, e.g. https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_view.cc?rcl=34fb3...
https://codereview.chromium.org/2744463002/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/2744463002/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:505: UpdateDropdownButton(); On 2017/03/20 16:34:46, Evan Stade wrote: > nit: you should check the details, e.g. > > https://cs.chromium.org/chromium/src/ash/common/shelf/shelf_view.cc?rcl=34fb3... Done.
The CQ bit was checked by bsep@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org, sky@chromium.org, bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2744463002/#ps230001 (title: "check details")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 230001, "attempt_start_ts": 1490134476748480,
"parent_rev": "2e8294f8c416b5ec2ad67ae06ff40462aa6020d4", "commit_rev":
"b7f329f1bcb344b270da73f0ef23650198886a0f"}
Message was sent while issue was closed.
Description was changed from ========== Move VectorIconButton functionality into static utilities. There are only a few users of VectorIconButton, and its hard-coded nature means it is not very flexible. This patch moves its functionality into two static utilities, one which creates an ImageButton with the existing vector button styling, and another that updates the ImageButton's images from an icon. The owner classes are now responsible for updating the styling if the theme changes. BUG=691895 ========== to ========== Move VectorIconButton functionality into static utilities. There are only a few users of VectorIconButton, and its hard-coded nature means it is not very flexible. This patch moves its functionality into two static utilities, one which creates an ImageButton with the existing vector button styling, and another that updates the ImageButton's images from an icon. The owner classes are now responsible for updating the styling if the theme changes. BUG=691895 Review-Url: https://codereview.chromium.org/2744463002 Cr-Commit-Position: refs/heads/master@{#458601} Committed: https://chromium.googlesource.com/chromium/src/+/b7f329f1bcb344b270da73f0ef23... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:230001) as https://chromium.googlesource.com/chromium/src/+/b7f329f1bcb344b270da73f0ef23... |
