Shows the info bubble when the location bar icon is clicked in the origin chip.

Shows the info bubble when the location bar icon is clicked in the origin chip.

BUG=349065
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269671

[groby-ooo-7-16, 2014-04-10 21:26:09]

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(left):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:510: [self
focusNotificationFor:focusEvent ofView:controlView];
The focus notification should always happen before we do the click event. (I
wish I recalled exactly why, but there were issues around that)

It also should only happen if the button is clicked, not if there's no click.

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:497:
[controlView convertPoint:[[NSApp currentEvent] locationInWindow]
Why not use the current event that you already have? Also, why use the mousedown
location? Standard behavior is mouseUp triggers clicks.

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm (right):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm:101: if
((chrome::ShouldDisplayOriginChipV2() &&
Why the separate branch for ShouldDisplayOriginChipV2?

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.mm (right):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.mm:70:
LocationIconDecoration* location_icon)
I'm really queasy about the idea of decorations depending on each other. Should
we instead hoist the code for showing the settings into LocationBarViewMac?

(Ultimately I think they can be pulled out even further, given that there's
similar code in page_info_helper.cc,  location_icon_decoration.mm, and 2
instances in render_view_context_menu.cc. But that's probably beyond scope for
this CL)

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.mm:204: if
(!NSEqualRects(icon_rect_, NSZeroRect) &&
Is NSPointInRect not enough? (It's not if the decoration could feasibly include
(0,0) as a point - otherwise, it should be)

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.mm:206:
owner_->GetOmniboxView()->model()->set_focused_via_location_icon();
Technically, the delayed focus should still set focus?

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.mm:241: bool
OriginChipDecoration::ShouldShow() const {
That code is identical to views. Does it make sense to have ShouldShowOriginChip
on the ToolbarModel?

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_view.cc:96:
!model()->focused_via_location_icon()) {
This feels very fragile - would it make more sense to check if the page info
bubble is showing, instead?

Also - why does the location icon decoration not need to set the focused_via
indicator?

[macourteau, 2014-04-28 19:19:24]

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(left):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:510: [self
focusNotificationFor:focusEvent ofView:controlView];
On 2014/04/10 21:26:09, groby wrote:
> The focus notification should always happen before we do the click event. (I
> wish I recalled exactly why, but there were issues around that)
> 
> It also should only happen if the button is clicked, not if there's no click.

Hmm, the problem I'm having is that when the chip is set to hide-on-click, I
have no way to figure out if the click was on the location icon or on the rest
of the chip, and sending the focus notification ends up here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
which hides the chip because the mode is hide-on-click.

Suggestions?

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:497:
[controlView convertPoint:[[NSApp currentEvent] locationInWindow]
On 2014/04/10 21:26:09, groby wrote:
> Why not use the current event that you already have? Also, why use the
mousedown
> location? Standard behavior is mouseUp triggers clicks.

Done and Done. :)

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm (right):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm:101: if
((chrome::ShouldDisplayOriginChipV2() &&
On 2014/04/10 21:26:09, groby wrote:
> Why the separate branch for ShouldDisplayOriginChipV2?

I call this method to display the page info bubble (same as what
EVBubbleDecoration does). If the origin chip is enabled (i.e.
ShouldDisplayOriginChipV2 returns true), the omnibox will be empty so the
original code would just return true and not display the page info bubble (hence
the first branch).

If the origin chip isn't enabled though, we don't want to show the page info
bubble if the omnibox is empty so the second branch of the condition retains
that behavior.

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.mm (right):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.mm:70:
LocationIconDecoration* location_icon)
On 2014/04/10 21:26:09, groby wrote:
> I'm really queasy about the idea of decorations depending on each other.
Should
> we instead hoist the code for showing the settings into LocationBarViewMac?
> 
> (Ultimately I think they can be pulled out even further, given that there's
> similar code in page_info_helper.cc,  location_icon_decoration.mm, and 2
> instances in render_view_context_menu.cc. But that's probably beyond scope for
> this CL)

That's probably a good idea (though as you say, it might be better to do that in
a separate CL). The EVBubbleDecoration also depends on the
LocationIconDecoration (that's what it uses to display the page info bubble as
well).

I'll add a TODO in the LocationIconDecoration class.

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.mm:204: if
(!NSEqualRects(icon_rect_, NSZeroRect) &&
On 2014/04/10 21:26:09, groby wrote:
> Is NSPointInRect not enough? (It's not if the decoration could feasibly
include
> (0,0) as a point - otherwise, it should be)

Done.

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.mm:206:
owner_->GetOmniboxView()->model()->set_focused_via_location_icon();
On 2014/04/10 21:26:09, groby wrote:
> Technically, the delayed focus should still set focus?

I'm not sure I get the question? The omnibox does get focus when the origin chip
is clicked.

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.mm:241: bool
OriginChipDecoration::ShouldShow() const {
On 2014/04/10 21:26:09, groby wrote:
> That code is identical to views. Does it make sense to have
ShouldShowOriginChip
> on the ToolbarModel?

Done.

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_view.cc:96:
!model()->focused_via_location_icon()) {
On 2014/04/10 21:26:09, groby wrote:
> This feels very fragile - would it make more sense to check if the page info
> bubble is showing, instead?

Would the bubble already be showing by the time we get here? If yes, how can I
test to see if it's showing?

> 
> Also - why does the location icon decoration not need to set the focused_via
> indicator?

The location icon decoration is not showing when the origin chip is showing (the
icon is drawn as part of the chip). If it's showing, that means that the origin
chip is not showing, and we don't want to change the behavior in that case.

[groby-ooo-7-16, 2014-04-30 21:55:39]

Hopefully these comments make sense. The focus handling situation is tricky, but
I believe 

a) leaving the notification exactly where it happens, and

b) allowing buttons to prevent focus notifications

would solve the issue. It's still a hack, because the view _did_ get focus for a
while before it loses it to the location bar icon, but the observable effects
will be correct.

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(left):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:510: [self
focusNotificationFor:focusEvent ofView:controlView];
On 2014/04/28 19:19:24, macourteau wrote:
> On 2014/04/10 21:26:09, groby wrote:
> > The focus notification should always happen before we do the click event. (I
> > wish I recalled exactly why, but there were issues around that)
> > 
> > It also should only happen if the button is clicked, not if there's no
click.
> 
> Hmm, the problem I'm having is that when the chip is set to hide-on-click, I
> have no way to figure out if the click was on the location icon or on the rest
> of the chip, and sending the focus notification ends up here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> which hides the chip because the mode is hide-on-click.
> 
> Suggestions?

All suggestions are going to amount to evil hackery. We're doing things the
location icons weren't built for.

To quote from the original code review[1]: "...focus event needs to happen
before the ButtonDecoration gets the OnMousePressed (otherwise, the order of
events is wrong, and the decoration would get OnSetFocus after
OnMousePressed)..."

If you move it after the OnMousePressed, this is exactly what we'll encounter.

The least convoluted solution is probably checking inside the button's
OnSetFocus if it's the location icon, and not treat it as a hide-on-click in
that case.

[1] https://codereview.chromium.org/163913011

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm (right):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm:101: if
((chrome::ShouldDisplayOriginChipV2() &&
On 2014/04/28 19:19:24, macourteau wrote:
> On 2014/04/10 21:26:09, groby wrote:
> > Why the separate branch for ShouldDisplayOriginChipV2?
> 
> I call this method to display the page info bubble (same as what
> EVBubbleDecoration does). If the origin chip is enabled (i.e.
> ShouldDisplayOriginChipV2 returns true), the omnibox will be empty so the
> original code would just return true and not display the page info bubble
(hence
> the first branch).
> 
> If the origin chip isn't enabled though, we don't want to show the page info
> bubble if the omnibox is empty so the second branch of the condition retains
> that behavior.

Yuck :)

You probably want to at least leave a comment what this statement is doing. (I
wonder if IsEditingOrEmpty needs to change to accommodate the OriginChip)

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.mm (right):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.mm:206:
owner_->GetOmniboxView()->model()->set_focused_via_location_icon();
On 2014/04/28 19:19:24, macourteau wrote:
> On 2014/04/10 21:26:09, groby wrote:
> > Technically, the delayed focus should still set focus?
> 
> I'm not sure I get the question? The omnibox does get focus when the origin
chip
> is clicked.

Yes, but that should be handled via the cell's focus machinery. We shouldn't
need to manually set focus.

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_view.cc:96:
!model()->focused_via_location_icon()) {
On 2014/04/28 19:19:24, macourteau wrote:
> On 2014/04/10 21:26:09, groby wrote:
> > This feels very fragile - would it make more sense to check if the page info
> > bubble is showing, instead?
> 
> Would the bubble already be showing by the time we get here? If yes, how can I
> test to see if it's showing?
> 

I think if you don't invoke HandleOriginChipMouseRelease when the focus came
from the location icon, you should be fine. 

I'm currently thinking that ButtonDecoration needs a PreventFocus() method. Pass
the current mouse position, have OriginChip prevent focus if it's on the
location icon.

That will also break the dependency on focused_via_location_icon state.

> > 
> > Also - why does the location icon decoration not need to set the focused_via
> > indicator?
> 
> The location icon decoration is not showing when the origin chip is showing
(the
> icon is drawn as part of the chip). If it's showing, that means that the
origin
> chip is not showing, and we don't want to change the behavior in that case.

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_view.cc:96:
!model()->focused_via_location_icon()) {
On 2014/04/28 19:19:24, macourteau wrote:
> On 2014/04/10 21:26:09, groby wrote:
> > This feels very fragile - would it make more sense to check if the page info
> > bubble is showing, instead?
> 
> Would the bubble already be showing by the time we get here? If yes, how can I
> test to see if it's showing?
> 
> > 
> > Also - why does the location icon decoration not need to set the focused_via
> > indicator?
> 
> The location icon decoration is not showing when the origin chip is showing
(the
> icon is drawn as part of the chip). If it's showing, that means that the
origin
> chip is not showing, and we don't want to change the behavior in that case.

Keeping location icon behavior the same if there's no origin chip would be a
side effect of the PreventFocus() implementation :)

[macourteau, 2014-05-01 19:04:48]

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(left):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:510: [self
focusNotificationFor:focusEvent ofView:controlView];
On 2014/04/30 21:55:39, groby wrote:
> On 2014/04/28 19:19:24, macourteau wrote:
> > On 2014/04/10 21:26:09, groby wrote:
> > > The focus notification should always happen before we do the click event.
(I
> > > wish I recalled exactly why, but there were issues around that)
> > > 
> > > It also should only happen if the button is clicked, not if there's no
> click.
> > 
> > Hmm, the problem I'm having is that when the chip is set to hide-on-click, I
> > have no way to figure out if the click was on the location icon or on the
rest
> > of the chip, and sending the focus notification ends up here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > which hides the chip because the mode is hide-on-click.
> > 
> > Suggestions?
> 
> All suggestions are going to amount to evil hackery. We're doing things the
> location icons weren't built for.
> 
> To quote from the original code review[1]: "...focus event needs to happen
> before the ButtonDecoration gets the OnMousePressed (otherwise, the order of
> events is wrong, and the decoration would get OnSetFocus after
> OnMousePressed)..."
> 
> If you move it after the OnMousePressed, this is exactly what we'll encounter.
> 
> The least convoluted solution is probably checking inside the button's
> OnSetFocus if it's the location icon, and not treat it as a hide-on-click in
> that case.
> 
> [1] https://codereview.chromium.org/163913011
> 

The "PreventFocus" trick seems to work. The only potentially confusing
side-effect (which wasn't the case with the set_focused_... implementation) is
if you click the icon to show the bubble, then close the bubble but leave the
omnibox focused (chip still shows), then switch to another tab and switch back
to the previous tab. In that case, the omnibox will be focused but the chip is
gone (since the OmniboxEditModel doesn't know how the omnibox was focused, so it
hides the chip). Not sure if that's really an issue though :)

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm (right):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/location_icon_decoration.mm:101: if
((chrome::ShouldDisplayOriginChipV2() &&
On 2014/04/30 21:55:39, groby wrote:
> On 2014/04/28 19:19:24, macourteau wrote:
> > On 2014/04/10 21:26:09, groby wrote:
> > > Why the separate branch for ShouldDisplayOriginChipV2?
> > 
> > I call this method to display the page info bubble (same as what
> > EVBubbleDecoration does). If the origin chip is enabled (i.e.
> > ShouldDisplayOriginChipV2 returns true), the omnibox will be empty so the
> > original code would just return true and not display the page info bubble
> (hence
> > the first branch).
> > 
> > If the origin chip isn't enabled though, we don't want to show the page info
> > bubble if the omnibox is empty so the second branch of the condition retains
> > that behavior.
> 
> Yuck :)
> 
> You probably want to at least leave a comment what this statement is doing. (I
> wonder if IsEditingOrEmpty needs to change to accommodate the OriginChip)

Done.

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.mm (right):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.mm:206:
owner_->GetOmniboxView()->model()->set_focused_via_location_icon();
On 2014/04/30 21:55:39, groby wrote:
> On 2014/04/28 19:19:24, macourteau wrote:
> > On 2014/04/10 21:26:09, groby wrote:
> > > Technically, the delayed focus should still set focus?
> > 
> > I'm not sure I get the question? The omnibox does get focus when the origin
> chip
> > is clicked.
> 
> Yes, but that should be handled via the cell's focus machinery. We shouldn't
> need to manually set focus.
> 

This wasn't actually setting focus - the set_focused_via_location_icon was just
setting state meaning that focus had occurred through the location icon (instead
of the omnibox). I've removed the set_focused_... method anyway :)

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_view.cc (right):

https://codereview.chromium.org/233623002/diff/20001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_view.cc:96:
!model()->focused_via_location_icon()) {
On 2014/04/30 21:55:39, groby wrote:
> On 2014/04/28 19:19:24, macourteau wrote:
> > On 2014/04/10 21:26:09, groby wrote:
> > > This feels very fragile - would it make more sense to check if the page
info
> > > bubble is showing, instead?
> > 
> > Would the bubble already be showing by the time we get here? If yes, how can
I
> > test to see if it's showing?
> > 
> > > 
> > > Also - why does the location icon decoration not need to set the
focused_via
> > > indicator?
> > 
> > The location icon decoration is not showing when the origin chip is showing
> (the
> > icon is drawn as part of the chip). If it's showing, that means that the
> origin
> > chip is not showing, and we don't want to change the behavior in that case.
> 
> Keeping location icon behavior the same if there's no origin chip would be a
> side effect of the PreventFocus() implementation :)

Yep, fixed.

[groby-ooo-7-16, 2014-05-02 01:31:11]

lgtm

https://codereview.chromium.org/233623002/diff/90001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/233623002/diff/90001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:521:
handled = decoration->AsButtonDecoration()->OnMousePressed(
You can remove "AsButtonDecoration" - all decorations support OnMousePressed.

https://codereview.chromium.org/233623002/diff/90001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:522: [self
frameForDecoration:decoration inFrame:cellFrame],
Shouldn't this be decorationRect?

[macourteau, 2014-05-02 14:35:08]

https://codereview.chromium.org/233623002/diff/90001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/233623002/diff/90001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:521:
handled = decoration->AsButtonDecoration()->OnMousePressed(
On 2014/05/02 01:31:11, groby wrote:
> You can remove "AsButtonDecoration" - all decorations support OnMousePressed.

Done.

https://codereview.chromium.org/233623002/diff/90001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:522: [self
frameForDecoration:decoration inFrame:cellFrame],
On 2014/05/02 01:31:11, groby wrote:
> Shouldn't this be decorationRect?

Done (and changed above for trackMouse as well, even though that doesn't change
anything since untilMouseUp is YES).

[macourteau, 2014-05-02 14:39:53]

Hi Scott & Peter,

Could you take a look at this?
shess for c/b/u/cocoa/location_bar/*
pkasting for c/b/u/toolbar/* and c/b/u/views/location_bar/origin_chip_view.cc

Thanks!

[Peter Kasting, 2014-05-05 22:39:03]

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model.h:102: virtual bool
ShouldShowOriginChip() const = 0;
Make this non-virtual and eliminate the test_toolbar_model.cc setter and
override unless you actually have test code using them.

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:194: bool
ToolbarModelImpl::WouldOmitURLDueToOriginChip() const {
Please move both ShouldDisplayURL() and this function down below
GetEVCertName(), to match the .h order better.

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:229: return
(chrome::ShouldDisplayOriginChipV2() &&
Nit: No ()

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:283: base::string16
ToolbarModelImpl::GetEVCertName(
While here, move this up to below GetSecurityLevelForWebContents().

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/origin_chip_view.cc (right):

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/origin_chip_view.cc:173: return
location_bar_view_->GetToolbarModel()->ShouldShowOriginChip();
Let's eliminate ShouldShow() entirely and have callers just call this directly.

[macourteau, 2014-05-08 21:35:55]

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model.h:102: virtual bool
ShouldShowOriginChip() const = 0;
On 2014/05/05 22:39:04, Peter Kasting wrote:
> Make this non-virtual and eliminate the test_toolbar_model.cc setter and
> override unless you actually have test code using them.

Done.

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:194: bool
ToolbarModelImpl::WouldOmitURLDueToOriginChip() const {
On 2014/05/05 22:39:04, Peter Kasting wrote:
> Please move both ShouldDisplayURL() and this function down below
> GetEVCertName(), to match the .h order better.

Done.

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:229: return
(chrome::ShouldDisplayOriginChipV2() &&
On 2014/05/05 22:39:04, Peter Kasting wrote:
> Nit: No ()

Done.

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:283: base::string16
ToolbarModelImpl::GetEVCertName(
On 2014/05/05 22:39:04, Peter Kasting wrote:
> While here, move this up to below GetSecurityLevelForWebContents().

Done.

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/origin_chip_view.cc (right):

https://codereview.chromium.org/233623002/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/origin_chip_view.cc:173: return
location_bar_view_->GetToolbarModel()->ShouldShowOriginChip();
On 2014/05/05 22:39:04, Peter Kasting wrote:
> Let's eliminate ShouldShow() entirely and have callers just call this
directly.

Done.

[Scott Hess - ex-Googler, 2014-05-08 23:00:46]

lgtm.  Just double-check the reset() point.

Sorry for the delay.  The CL was intimidating me, I ended up pulling it local to
break it into mostly light refactors.

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:513: [self
focusNotificationFor:focusEvent ofView:controlView];
I don't recall who originally wrote this code, but I'm not sure that this should
simply skip the test.  If !PreventFocus(point), someone should probably
focusEvent.reset().  That could be outside the if(), probably.

I don't _think_ the current code will mess anything up, I just worry about
retaining state which isn't correct, in case someone later adds additional
dependencies on that state.

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.h (right):

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.h:76:
LocationIconDecoration* location_icon_;
This dependency ever-so-slightly concerns me.  I can see why it is, and why
EvBubbleDecoration has it, but now it's in two places and I start to wonder if
it should be some other way entirely.  This class has owner_, the ev bubble
doesn't, but maybe owner_ dependency is more sane than depending directly on the
innards of a different decoration?  Or maybe it's time to lift the shared code
some other way?

Probably outside the scope of this CL, though.

[groby-ooo-7-16, 2014-05-08 23:14:07]

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:513: [self
focusNotificationFor:focusEvent ofView:controlView];
On 2014/05/08 23:00:47, shess wrote:
> I don't recall who originally wrote this code, but I'm not sure that this
should
> simply skip the test.  If !PreventFocus(point), someone should probably
> focusEvent.reset().  That could be outside the if(), probably.
> 
> I don't _think_ the current code will mess anything up, I just worry about
> retaining state which isn't correct, in case someone later adds additional
> dependencies on that state.

The original writer would be me :)

focusEvent is function-scope, so it will be reset at exit time anyways.
mouseDown takes ownership of the cell's focusEvent_ when it's invoked, by
copying it into the local focusEvent. (Note underscores)

In terms of code hygiene, it's probably still nice to call reset() right after
the if()

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.h (right):

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.h:76:
LocationIconDecoration* location_icon_;
On 2014/05/08 23:00:47, shess wrote:
> This dependency ever-so-slightly concerns me.  I can see why it is, and why
> EvBubbleDecoration has it, but now it's in two places and I start to wonder if
> it should be some other way entirely.  This class has owner_, the ev bubble
> doesn't, but maybe owner_ dependency is more sane than depending directly on
the
> innards of a different decoration?  Or maybe it's time to lift the shared code
> some other way?
> 
> Probably outside the scope of this CL, though.

I made the same point further up - search for "I'm really queasy about the idea
of decorations depending on each other."

I'd suggest definitely doing this, in a follow-up CL.

[Scott Hess - ex-Googler, 2014-05-08 23:21:35]

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:513: [self
focusNotificationFor:focusEvent ofView:controlView];
On 2014/05/08 23:14:07, groby wrote:
> On 2014/05/08 23:00:47, shess wrote:
> > I don't recall who originally wrote this code, but I'm not sure that this
> should
> > simply skip the test.  If !PreventFocus(point), someone should probably
> > focusEvent.reset().  That could be outside the if(), probably.
> > 
> > I don't _think_ the current code will mess anything up, I just worry about
> > retaining state which isn't correct, in case someone later adds additional
> > dependencies on that state.
> 
> The original writer would be me :)
> 
> focusEvent is function-scope, so it will be reset at exit time anyways.
> mouseDown takes ownership of the cell's focusEvent_ when it's invoked, by
> copying it into the local focusEvent. (Note underscores)
> 
> In terms of code hygiene, it's probably still nice to call reset() right after
> the if()

Apologies, did not notice the missing _.  In that case I think it's probably
pretty safe, I was more worried about some random other call keying off of it. 
Any changes local to this function would (hopefully) review the uses of the
local |focusEvent|.

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.h (right):

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.h:76:
LocationIconDecoration* location_icon_;
On 2014/05/08 23:14:07, groby wrote:
> On 2014/05/08 23:00:47, shess wrote:
> > This dependency ever-so-slightly concerns me.  I can see why it is, and why
> > EvBubbleDecoration has it, but now it's in two places and I start to wonder
if
> > it should be some other way entirely.  This class has owner_, the ev bubble
> > doesn't, but maybe owner_ dependency is more sane than depending directly on
> the
> > innards of a different decoration?  Or maybe it's time to lift the shared
code
> > some other way?
> > 
> > Probably outside the scope of this CL, though.
> 
> I made the same point further up - search for "I'm really queasy about the
idea
> of decorations depending on each other."
> 
> I'd suggest definitely doing this, in a follow-up CL.

I think my overall goal would be to make sure things stay pretty modular, and
not encourage everything to start relying on everything else.  I would guess
that's why I had the ev bubble depends on the decoration, to discourage people
from reaching through a juicy owner_ for additional state.

In the long term, it's plausible that this will simply display the ev bubble and
inline the location icon code entirely, in which case this concern will fall
away naturally.  In that case, I would actually prefer to not lard up
location_bar_view_mac.mm any further than it already is.

[All this said - mostly I just have opinions over here, so I'm also open to the
notion of having other opinionated people adding to a consensus to spread the
joy.]

[macourteau, 2014-05-09 14:00:26]

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:513: [self
focusNotificationFor:focusEvent ofView:controlView];
On 2014/05/08 23:14:07, groby wrote:
> On 2014/05/08 23:00:47, shess wrote:
> > I don't recall who originally wrote this code, but I'm not sure that this
> should
> > simply skip the test.  If !PreventFocus(point), someone should probably
> > focusEvent.reset().  That could be outside the if(), probably.
> > 
> > I don't _think_ the current code will mess anything up, I just worry about
> > retaining state which isn't correct, in case someone later adds additional
> > dependencies on that state.
> 
> The original writer would be me :)
> 
> focusEvent is function-scope, so it will be reset at exit time anyways.
> mouseDown takes ownership of the cell's focusEvent_ when it's invoked, by
> copying it into the local focusEvent. (Note underscores)
> 
> In terms of code hygiene, it's probably still nice to call reset() right after
> the if()

Done.

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.h (right):

https://codereview.chromium.org/233623002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/origin_chip_decoration.h:76:
LocationIconDecoration* location_icon_;
On 2014/05/08 23:21:36, shess wrote:
> On 2014/05/08 23:14:07, groby wrote:
> > On 2014/05/08 23:00:47, shess wrote:
> > > This dependency ever-so-slightly concerns me.  I can see why it is, and
why
> > > EvBubbleDecoration has it, but now it's in two places and I start to
wonder
> if
> > > it should be some other way entirely.  This class has owner_, the ev
bubble
> > > doesn't, but maybe owner_ dependency is more sane than depending directly
on
> > the
> > > innards of a different decoration?  Or maybe it's time to lift the shared
> code
> > > some other way?
> > > 
> > > Probably outside the scope of this CL, though.
> > 
> > I made the same point further up - search for "I'm really queasy about the
> idea
> > of decorations depending on each other."
> > 
> > I'd suggest definitely doing this, in a follow-up CL.
> 
> I think my overall goal would be to make sure things stay pretty modular, and
> not encourage everything to start relying on everything else.  I would guess
> that's why I had the ev bubble depends on the decoration, to discourage people
> from reaching through a juicy owner_ for additional state.
> 
> In the long term, it's plausible that this will simply display the ev bubble
and
> inline the location icon code entirely, in which case this concern will fall
> away naturally.  In that case, I would actually prefer to not lard up
> location_bar_view_mac.mm any further than it already is.
> 
> [All this said - mostly I just have opinions over here, so I'm also open to
the
> notion of having other opinionated people adding to a consensus to spread the
> joy.]

Yes, I agree. There's a TODO in LocationIconDecoration to address this:

  // TODO(macourteau): this code (for displaying the page info bubble) should be
  // pulled out into LocationBarViewMac (or maybe even further), as other
  // decorations currently depend on this decoration only to show the page info
  // bubble.

[Scott Hess - ex-Googler, 2014-05-09 14:23:40]

still lgtm!

[macourteau, 2014-05-09 15:39:58]

On 2014/05/09 14:23:40, shess wrote:
> still lgtm!

Thanks! Peter?

[Peter Kasting, 2014-05-09 20:35:40]

LGTM

I realize you're just moving the code below around, but while you're touching it
anyway, two nits.

https://codereview.chromium.org/233623002/diff/160001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/233623002/diff/160001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:104: NOTREACHED();
Nit: This violates the Chrome style guide rule about not handling DCHECK
failure.  This should just be:

  DCHECK(!cert.subject().organization_names.empty());
  DCHECK(!cert.subject().country_name.empty());

(Unless for some reason these really can be empty, in which case we should
remove the NOTREACHED() instead.  I wouldn't do that unless you know you need
to.)

https://codereview.chromium.org/233623002/diff/160001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:253: if
(chrome::IsInstantNTP(delegate_->GetActiveWebContents()))
Nit: Just:

  return !chrome::IsInstantNTP(delegate_->GetActiveWebContents());

[macourteau, 2014-05-09 20:53:22]

Just one more question re:one of the nits. Thanks!

https://codereview.chromium.org/233623002/diff/160001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/233623002/diff/160001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:104: NOTREACHED();
On 2014/05/09 20:35:40, Peter Kasting wrote:
> Nit: This violates the Chrome style guide rule about not handling DCHECK
> failure.  This should just be:
> 
>   DCHECK(!cert.subject().organization_names.empty());
>   DCHECK(!cert.subject().country_name.empty());
> 
> (Unless for some reason these really can be empty, in which case we should
> remove the NOTREACHED() instead.  I wouldn't do that unless you know you need
> to.)

Isn't the behavior of the two DCHECK's different though? I'm not the author of
this code, but what I see here is that in debug, it will trigger the NOTREACHED,
but in release it will log an error and return an empty string.

With the two DCHECK's, on the other hand, the debug behavior would be the same,
but control would reach the GetStringFUTF16 call in release, which is different
from returning an empty string (and might crash due to accessing index 0 of
organization_name, if it's empty).

https://codereview.chromium.org/233623002/diff/160001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:253: if
(chrome::IsInstantNTP(delegate_->GetActiveWebContents()))
On 2014/05/09 20:35:40, Peter Kasting wrote:
> Nit: Just:
> 
>   return !chrome::IsInstantNTP(delegate_->GetActiveWebContents());

Done.

[Peter Kasting, 2014-05-09 21:15:46]

https://codereview.chromium.org/233623002/diff/160001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/233623002/diff/160001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:104: NOTREACHED();
On 2014/05/09 20:53:23, macourteau wrote:
> On 2014/05/09 20:35:40, Peter Kasting wrote:
> > Nit: This violates the Chrome style guide rule about not handling DCHECK
> > failure.  This should just be:
> > 
> >   DCHECK(!cert.subject().organization_names.empty());
> >   DCHECK(!cert.subject().country_name.empty());
> > 
> > (Unless for some reason these really can be empty, in which case we should
> > remove the NOTREACHED() instead.  I wouldn't do that unless you know you
need
> > to.)
> 
> Isn't the behavior of the two DCHECK's different though? I'm not the author of
> this code, but what I see here is that in debug, it will trigger the
NOTREACHED,
> but in release it will log an error and return an empty string.
> 
> With the two DCHECK's, on the other hand, the debug behavior would be the
same,
> but control would reach the GetStringFUTF16 call in release, which is
different
> from returning an empty string (and might crash due to accessing index 0 of
> organization_name, if it's empty).

NOTREACHED/DCHECK is an assertion that the check in question cannot possibly
fail, ever, in any mode, even for unusual reasons (e.g. disk corruption or
out-of-memory should still not trigger DCHECK failures).

Code is not supposed to speculate about what might happen if the assertion is
violated, and how to handle that.  In other words, and code that says, "but in
release mode, if this fails, then" is wrong, because the entire point of a
NOTREACHED/DCHECK is to say that the conditional literally never fails.

[macourteau, 2014-05-09 22:23:51]

https://codereview.chromium.org/233623002/diff/160001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/233623002/diff/160001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:104: NOTREACHED();
On 2014/05/09 21:15:47, Peter Kasting wrote:
> On 2014/05/09 20:53:23, macourteau wrote:
> > On 2014/05/09 20:35:40, Peter Kasting wrote:
> > > Nit: This violates the Chrome style guide rule about not handling DCHECK
> > > failure.  This should just be:
> > > 
> > >   DCHECK(!cert.subject().organization_names.empty());
> > >   DCHECK(!cert.subject().country_name.empty());
> > > 
> > > (Unless for some reason these really can be empty, in which case we should
> > > remove the NOTREACHED() instead.  I wouldn't do that unless you know you
> need
> > > to.)
> > 
> > Isn't the behavior of the two DCHECK's different though? I'm not the author
of
> > this code, but what I see here is that in debug, it will trigger the
> NOTREACHED,
> > but in release it will log an error and return an empty string.
> > 
> > With the two DCHECK's, on the other hand, the debug behavior would be the
> same,
> > but control would reach the GetStringFUTF16 call in release, which is
> different
> > from returning an empty string (and might crash due to accessing index 0 of
> > organization_name, if it's empty).
> 
> NOTREACHED/DCHECK is an assertion that the check in question cannot possibly
> fail, ever, in any mode, even for unusual reasons (e.g. disk corruption or
> out-of-memory should still not trigger DCHECK failures).
> 
> Code is not supposed to speculate about what might happen if the assertion is
> violated, and how to handle that.  In other words, and code that says, "but in
> release mode, if this fails, then" is wrong, because the entire point of a
> NOTREACHED/DCHECK is to say that the conditional literally never fails.

SGTM. Done.

[macourteau, 2014-05-09 22:24:00]

The CQ bit was checked by macourteau@chromium.org

[commit-bot: I haz the power, 2014-05-09 22:25:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/macourteau@chromium.org/233623002/200001

[commit-bot: I haz the power, 2014-05-09 23:38:50]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-09 23:43:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-09 23:43:18]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[macourteau, 2014-05-10 02:14:22]

The CQ bit was checked by macourteau@chromium.org

[commit-bot: I haz the power, 2014-05-10 02:17:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/macourteau@chromium.org/233623002/220001

[commit-bot: I haz the power, 2014-05-10 05:42:43]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-10 05:49:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-10 05:49:12]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[macourteau, 2014-05-10 13:16:27]

The CQ bit was checked by macourteau@chromium.org

[commit-bot: I haz the power, 2014-05-10 13:18:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/macourteau@chromium.org/233623002/220001

[commit-bot: I haz the power, 2014-05-11 01:16:32]

Change committed as 269671