Refactor the content setting view to allow a subclass for the web intents button.

Refactor the content setting view to allow a subclass for the web intents button.

R=bauerb@chromium.org
BUG=139028
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150445

[Bernhard Bauer, 2012-07-31 23:31:18]

http://codereview.chromium.org/10821114/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right):

http://codereview.chromium.org/10821114/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:72: if
(tab_contents) {
Could we do an early-ish return if tab_content is NULL?

[Bernhard Bauer, 2012-07-31 23:31:48]

Could you a add a bug ID to the CL description?

[Greg Billock, 2012-08-06 22:49:47]

http://codereview.chromium.org/10821114/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right):

http://codereview.chromium.org/10821114/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:72: if
(tab_contents) {
On 2012/07/31 23:31:18, Bernhard Bauer wrote:
> Could we do an early-ish return if tab_content is NULL?

We could. That slightly changes the logic -- it'll currently make sure to set
visibility to the current state, which is un-updated if there's a null
TabContents. I think in practice this ends up being always satisfied, but I'm
not totally sure.

[Bernhard Bauer, 2012-08-07 00:08:37]

http://codereview.chromium.org/10821114/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right):

http://codereview.chromium.org/10821114/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:72: if
(tab_contents) {
On 2012/08/06 22:49:47, Greg Billock wrote:
> On 2012/07/31 23:31:18, Bernhard Bauer wrote:
> > Could we do an early-ish return if tab_content is NULL?
> 
> We could. That slightly changes the logic -- it'll currently make sure to set
> visibility to the current state, which is un-updated if there's a null
> TabContents. I think in practice this ends up being always satisfied, but I'm
> not totally sure.

Yeah... It's fine with me if you want to leave it as it is.

[Bernhard Bauer, 2012-08-07 00:09:28]

...and LGTM (Rietveld is acting weird).

[Greg Billock, 2012-08-07 00:30:45]

On 2012/08/07 00:09:28, Bernhard Bauer wrote:
> ...and LGTM (Rietveld is acting weird).

Thanks. sky, can I get an owners approval?

[sky, 2012-08-07 03:33:00]

http://codereview.chromium.org/10821114/diff/6001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right):

http://codereview.chromium.org/10821114/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:169: void
ContentSettingImageView::OnMouseReleased(const views::MouseEvent& event) {
This class needs to override OnGestureEvent and treat ET_GESTURE_TAP as a click
too.

http://codereview.chromium.org/10821114/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:279: SkColor
ContentSettingImageView::button_border_color() const {
Order doesn't match header.

http://codereview.chromium.org/10821114/diff/6001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right):

http://codereview.chromium.org/10821114/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/content_setting_image_view.h:63: virtual
SkColor button_border_color() const;
virtual methods shouldn't be named named with unix_hacker_style. Also,
descriptions should be added for at least OnClick().

[Greg Billock, 2012-08-07 19:29:04]

http://codereview.chromium.org/10821114/diff/6001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right):

http://codereview.chromium.org/10821114/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:169: void
ContentSettingImageView::OnMouseReleased(const views::MouseEvent& event) {
On 2012/08/07 03:33:00, sky wrote:
> This class needs to override OnGestureEvent and treat ET_GESTURE_TAP as a
click
> too. 

Done.

http://codereview.chromium.org/10821114/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:279: SkColor
ContentSettingImageView::button_border_color() const {
On 2012/08/07 03:33:00, sky wrote:
> Order doesn't match header.

These are right under GetBuiltInHorizontalPadding in the header.

I left OnClick in-place in the .cc file since it made the diff a lot easier to
read. I can copy-paste it to the bottom here, but the private methods are out of
order as it is, so I'm not sure it'd help predictability much.

http://codereview.chromium.org/10821114/diff/6001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/content_setting_image_view.h (right):

http://codereview.chromium.org/10821114/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/content_setting_image_view.h:63: virtual
SkColor button_border_color() const;
On 2012/08/07 03:33:00, sky wrote:
> virtual methods shouldn't be named named with unix_hacker_style. Also,
> descriptions should be added for at least OnClick().

Done.

[sky, 2012-08-07 20:46:19]

LGTM with the following change.

As to order, someone should do a followup patch to straighten out the order of
the file.

http://codereview.chromium.org/10821114/diff/12001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right):

http://codereview.chromium.org/10821114/diff/12001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:136: return
ui::GESTURE_STATUS_CONSUMED;
You need to return CONSUMED for event.type() == ui::ET_GESTURE_TAP_DOWN too.

[Greg Billock, 2012-08-07 22:11:41]

http://codereview.chromium.org/10821114/diff/12001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right):

http://codereview.chromium.org/10821114/diff/12001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:136: return
ui::GESTURE_STATUS_CONSUMED;
On 2012/08/07 20:46:19, sky wrote:
> You need to return CONSUMED for event.type() == ui::ET_GESTURE_TAP_DOWN too.

Done.

[Greg Billock, 2012-08-07 22:12:26]

On 2012/08/07 20:46:19, sky wrote:
> LGTM with the following change.
> 
> As to order, someone should do a followup patch to straighten out the order of
> the file.

Yeah, I can do that. I'm going to be factoring out a bunch of implementation in
the follow-on to add the web intents button class.

>
http://codereview.chromium.org/10821114/diff/12001/chrome/browser/ui/views/lo...
> File chrome/browser/ui/views/location_bar/content_setting_image_view.cc
(right):
> 
>
http://codereview.chromium.org/10821114/diff/12001/chrome/browser/ui/views/lo...
> chrome/browser/ui/views/location_bar/content_setting_image_view.cc:136: return
> ui::GESTURE_STATUS_CONSUMED;
> You need to return CONSUMED for event.type() == ui::ET_GESTURE_TAP_DOWN too.

[commit-bot: I haz the power, 2012-08-07 22:12:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10821114/8002

[commit-bot: I haz the power, 2012-08-07 23:44:11]

Change committed as 150445