[OSX, OmniTheatre] Handle OriginChip click properly.

[OSX, OmniTheatre] Handle OriginChip click properly.

Clicking on the OriginChip requires the chip to stay visible until the
determination has been made that the chip was clicked and its callback
is executed.

Previous code disabled the chip in OnSetFocus, so that the following
mouseDown: did not get routed to the OriginChip decoration any more.

This change postpones calling OnSetFocus until immediately before the
ButtonPressed callback is invoked - i.e. mouseDown: has already
traversed all decorations and is not affected by possible visibility
changes caused as a reaction to the click.

For a longer discussion of potential approaches to the problem, see
also https://codereview.chromium.org/165853002/

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

[groby-ooo-7-16, 2014-02-22 04:26:13]

PTAL.

I'm not entirely happy with the method names, and it bugs me that the callback
handling moved into the cell - but we really need to postpone the OnSetFocus
call until _right_ before we call OnMousePressed() for the decoration, or it
will not be found because OnSetFocus caused it to go invisible.

Other approaches that didn't work:

* Override hitTest: - can't do that, since it will not even send the mouseDown
event to the view.
* reject firstResponder if it's due to a click on a button - can't do that
because we still expect the field to have a field editor after clicking on the
button.
* validateProposedFirstResponder:forEvent: - again, we need to always become
firstResponder
* played around with moving the chip into the field, instead of a cell
decoration, but that's so different from the Views implementation that a lot of
control flows break.
----

macourteau: I expect even more fun once you have clicking on the icon part of
the chip, which might need the field to not become firstResponder at all. Let me
know how things on your side go.

Also, heads up that this is for _all_ ButtonDecorations, which is also the
SearchButtonDecoration. I believe this should work fine, but just in case... :)

[Scott Hess - ex-Googler, 2014-02-25 21:37:36]

Wow.  I have to think about this a little.  I'll try to revisit it a couple
times this afternoon in hopes of driving that.

The initial things which I'm going to think on are the control/cell breakdown,
and whether there are ways that it could get focus which evade this detection.

Sorry about no immediate response.  Your email fell below the fold.

[groby-ooo-7-16, 2014-02-26 04:24:55]

Long run, I really think we should abandon the cell breakdown for OT - it's not
like we're running on a 68030 any more ;) (Or at least do the right and
implement the equivalent to a one-row NSTableView, or something). So I'd love to
hear your thoughts on the View/Cell breakdown - I think what OmniboxViewMac does
right now is the result of slowly adding features, and might need a bit of a
rethink.

But this CL is more about a short-term effort to do the right thing for the
OriginChip - ideally, I'd like to not refactor the entire view for something
that's still more or less experimental. Maybe the behavior should be hidden
behind the flag, too?

[Scott Hess - ex-Googler, 2014-02-26 18:59:11]

On 2014/02/26 04:24:55, groby wrote:
> Long run, I really think we should abandon the cell breakdown for OT - it's
not
> like we're running on a 68030 any more ;) (Or at least do the right and
> implement the equivalent to a one-row NSTableView, or something). So I'd love
to
> hear your thoughts on the View/Cell breakdown - I think what OmniboxViewMac
does
> right now is the result of slowly adding features, and might need a bit of a
> rethink.
> 
> But this CL is more about a short-term effort to do the right thing for the
> OriginChip - ideally, I'd like to not refactor the entire view for something
> that's still more or less experimental. Maybe the behavior should be hidden
> behind the flag, too?

OK, so I'm overall happy with your approach, but I'm not clear why the code is
in the cell.  And I have no idea what a one-row NSTableView would do for any of
this.  Why I brought it up is because the control is focussed, not the cell, so
I think it makes sense to have the control dealing with the focus notifications.

I don't think it needs a flag.  I assume the overall origin chip stuff is behind
a flag.

What I mean is that the cell really only needs a bool-return method on the order
of "Is this mouse-down one which needs deferred focus?"  Basically
-handleFocusEvent:ofView:, except rephrased as a question.  Then the control's
-mouseDown: can always check this and set focus if true, without worrying about
whether it's a button decoration, because if it's not a button decoration then
it wouldn't have been set.

Another option to retaining the event would be to use base::bind() to create a
callback that will be called at the top of -mouseDown:, if set (I mean like if
(!callback.is_null()) { callback.Run(); callback.reset(); }).

I'm not sure what it would mean to get a mouse-down event which was not the
mouse-down event that caused things to get focus - AFAICT, it means that maybe
focus notification is never sent?  That seems wrong.  I think it would be
reasonable to _always_ send the notification on -mouseDown: unless we have a
specific case which evades that.

[groby-ooo-7-16, 2014-02-26 20:46:22]

It's in the cell because the 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), and it
needs to happen after any potential drag handling.

Both of these happen in the cell, so there's not much of a choice there. And I
can't move the drag handling out of it - that's what NSTextField expects. And
since OnMousePressed depends on the drag finishing inside the field, it makes
sense to call OnMousePressed there, too...

As for  mouseDown's which are not causing the focus - click on the decoration of
an NSTextField instead of the editor (it's a small 2-pixel frame on the outside
of the main text field), and you'll get a mouseDown, even if the text field is
firstResponder.

Did I mention I do not particularly like NSTextField? ;)
---- Long term things ---
I brought up NSMatrix/NSTableView as examples of multi-cell views, which this
essentially is. It's just not really acknowledging that, and causes a few grey
hairs along the way. But I'd prefer if we abandoned the idea of doing this all
in cells, and instead just used an NSControl for each decoration. The NSView
storage overhead won't kill us :)

[Scott Hess - ex-Googler, 2014-02-26 21:08:39]

On Wed, Feb 26, 2014 at 12:46 PM, <groby@chromium.org> wrote:

> It's in the cell because the 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), and
> it
> needs to happen after any potential drag handling.
>
> Both of these happen in the cell, so there's not much of a choice there.
> And I
> can't move the drag handling out of it - that's what NSTextField expects.
> And
> since OnMousePressed depends on the drag finishing inside the field, it
> makes
> sense to call OnMousePressed there, too...
>

ButtonDecoration is not draggable, though?  So as long as the control
-mouseDown: handler does the focus thing before calling into the cell, it
should be fine, since that is how it has always happened?


> As for  mouseDown's which are not causing the focus - click on the
> decoration of
> an NSTextField instead of the editor (it's a small 2-pixel frame on the
> outside
> of the main text field), and you'll get a mouseDown, even if the text
> field is
> firstResponder.
>

But in that case, the saved focus event should be the same as the event
passed to mouse down.  I'm wondering about cases where the mouse down could
receive a different event than the mouse down event which was current when
-becomeFirstResponder was called.


> Did I mention I do not particularly like NSTextField? ;)
>

It is complicated.

---- Long term things ---
> I brought up NSMatrix/NSTableView as examples of multi-cell views, which
> this
> essentially is. It's just not really acknowledging that, and causes a few
> grey
> hairs along the way. But I'd prefer if we abandoned the idea of doing this
> all
> in cells, and instead just used an NSControl for each decoration. The
> NSView
> storage overhead won't kill us :)
>

In this case, though, the cells are not homogenous in any way.  Doing it as
a federation of NSView objects will work BUT to a certain extent it just
flips the issues which need to be dealt with to some other area.  The views
will have to be laid out manually, the right (not the default) focus
behavior will have to be implemented, etc.  Maybe the decoration stuff will
still be there (I originally did it because every time a C++ Chromium
developer came into the cocoa code, they built Yet Another C++ Abstraction
Which Wasn't Cocoa Nor Chromium rather than learn any Objective-C).  I
don't even want to think about whether the field editor stuff has hidden
dependencies from the editor to the control.

IOW, I'm not completely convinced either way.  It's probably a 40%/60%
choice, I just am not sure which is the 40% and which is the 60% :-).

[Unfortunately, I am 100% convinced that there is no way to put the Cocoa
stuff over here, and the Chromium stuff over there, with a sane interface
between.]

https://codereview.chromium.org/163913011/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[groby-ooo-7-16, 2014-02-26 22:30:00]

On 2014/02/26 21:08:39, shess wrote:
> On Wed, Feb 26, 2014 at 12:46 PM, <mailto:groby@chromium.org> wrote:
> 
> > It's in the cell because the 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), and
> > it
> > needs to happen after any potential drag handling.
> >
> > Both of these happen in the cell, so there's not much of a choice there.
> > And I
> > can't move the drag handling out of it - that's what NSTextField expects.
> > And
> > since OnMousePressed depends on the drag finishing inside the field, it
> > makes
> > sense to call OnMousePressed there, too...
> >
> 
> ButtonDecoration is not draggable, though?  So as long as the control
> -mouseDown: handler does the focus thing before calling into the cell, it
> should be fine, since that is how it has always happened?

Yes and no. If I click on a button and then drag off before releasing, it's a
drag event. (With two different results - dragged entirely off the button, or
dragged back onto the button before mouseUp). That needs to be handled by
-trackMouse:, and that lives in the cell.

We _could_ move that entire logic up to the control, but I don't see a way to
just handle it via a return value from mouseDown.

> > As for  mouseDown's which are not causing the focus - click on the
> > decoration of
> > an NSTextField instead of the editor (it's a small 2-pixel frame on the
> > outside
> > of the main text field), and you'll get a mouseDown, even if the text
> > field is
> > firstResponder.
> >
> 
> But in that case, the saved focus event should be the same as the event
> passed to mouse down.  I'm wondering about cases where the mouse down could
> receive a different event than the mouse down event which was current when
> -becomeFirstResponder was called.

Not necessarily. Here's a sequence of events:

* First click on NSTextField, _inside_ the editor frame -- Event EV1
  -> becomeFirstResponder
  -> focusEvent is EV1
  -> The control's mouseDown forwards the event to the editor, without letting
the cell do its thing. See line 110ff in autocomplete_text_field.mm

* Second click, on an Omnibox decoration -- EV2
  -> Already first responder, so no new focusEvent
  -> control's mouseDown is called, and since it's on a decoration, it's outside
the textframe -> call the cell's mouseDown
  -> cell's mouseDown is called with EV2. It's not the mouse event that caused
us to focus (EV1), so do nothing.

Technically, focusEvent_ will be NULL for the second click, so I _could_ just
check if focusEvent_ is set. isEqual: feels like a better choice, though, given
the complexity of interactions. Or maybe I'm just too cautious. (But without the
isEqual, I'm relying on the next person coming in to remember to update
mouseDown when they update handleFocusEvent for other focus events. It's a bit
more brittle)

If it makes you happier, I could replace it with a DCHECK(!focusEvent_ ||
[focusEvent_ isEqual:theEvent])

> 
> 
> > Did I mention I do not particularly like NSTextField? ;)
> >
> 
> It is complicated.
> 
> ---- Long term things ---
> > I brought up NSMatrix/NSTableView as examples of multi-cell views, which
> > this
> > essentially is. It's just not really acknowledging that, and causes a few
> > grey
> > hairs along the way. But I'd prefer if we abandoned the idea of doing this
> > all
> > in cells, and instead just used an NSControl for each decoration. The
> > NSView
> > storage overhead won't kill us :)
> >
> 
> In this case, though, the cells are not homogenous in any way. 

They don't really need to, if we do this as a multi-cell view. We're already
mostly there, I think. Making each decoration be represented by a separate cell
might simplify some things. I don't know - I haven't spent enough time thinking
that through yet. (Ask me what I think about drag events across multiple cells
:)

> Doing it as
> a federation of NSView objects will work BUT to a certain extent it just
> flips the issues which need to be dealt with to some other area.  The views
> will have to be laid out manually, the right (not the default) focus
> behavior will have to be implemented, etc.  Maybe the decoration stuff will
> still be there (I originally did it because every time a C++ Chromium
> developer came into the cocoa code, they built Yet Another C++ Abstraction
> Which Wasn't Cocoa Nor Chromium rather than learn any Objective-C).  I
> don't even want to think about whether the field editor stuff has hidden
> dependencies from the editor to the control.
> 
> IOW, I'm not completely convinced either way.  It's probably a 40%/60%
> choice, I just am not sure which is the 40% and which is the 60% :-).

I'm not sure either. That's why I'm not too keen on refactoring and instead
wrote this. It's not happy-making, but it's also a lot less painful.
> 
> [Unfortunately, I am 100% convinced that there is no way to put the Cocoa
> stuff over here, and the Chromium stuff over there, with a sane interface
> between.]

I have a 5% hope there ;)

> 
> https://codereview.chromium.org/163913011/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Scott Hess - ex-Googler, 2014-02-27 02:21:31]

On 2014/02/26 22:30:00, groby wrote:
> On 2014/02/26 21:08:39, shess wrote:
> > On Wed, Feb 26, 2014 at 12:46 PM, <mailto:groby@chromium.org> wrote:
> > > It's in the cell because the 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), and
> > > it
> > > needs to happen after any potential drag handling.
> > >
> > > Both of these happen in the cell, so there's not much of a choice there.
> > > And I
> > > can't move the drag handling out of it - that's what NSTextField expects.
> > > And
> > > since OnMousePressed depends on the drag finishing inside the field, it
> > > makes
> > > sense to call OnMousePressed there, too...
> > >
> > 
> > ButtonDecoration is not draggable, though?  So as long as the control
> > -mouseDown: handler does the focus thing before calling into the cell, it
> > should be fine, since that is how it has always happened?
> 
> Yes and no. If I click on a button and then drag off before releasing, it's a
> drag event. (With two different results - dragged entirely off the button, or
> dragged back onto the button before mouseUp). That needs to be handled by
> -trackMouse:, and that lives in the cell.
> 
> We _could_ move that entire logic up to the control, but I don't see a way to
> just handle it via a return value from mouseDown.

It couldn't be handled with a return value from mousedown (well, not from _this_
mousedown).

AFAICT, the existing -trackMouse:* bit must be attempting to implement
Cocoa-style button clicking on mouse-up instead of mouse-down.  Given your
current code, actual focus has been changed, so if the user moves off the field
during the click, then OnSetFocus() will never be sent, I think, though the
field does have focus.  So maybe the original focus should be passed through in
that case?  [I suspect that convincing the control not to have stolen focus will
be too challenging for this approach, it would need a view sitting above to
intercept, or some other sort of magic.]

> > > As for  mouseDown's which are not causing the focus - click on the
> > > decoration of
> > > an NSTextField instead of the editor (it's a small 2-pixel frame on the
> > > outside
> > > of the main text field), and you'll get a mouseDown, even if the text
> > > field is
> > > firstResponder.
> > >
> > 
> > But in that case, the saved focus event should be the same as the event
> > passed to mouse down.  I'm wondering about cases where the mouse down could
> > receive a different event than the mouse down event which was current when
> > -becomeFirstResponder was called.
> 
> Not necessarily. Here's a sequence of events:
> 
> * First click on NSTextField, _inside_ the editor frame -- Event EV1
>   -> becomeFirstResponder
>   -> focusEvent is EV1
>   -> The control's mouseDown forwards the event to the editor, without letting
> the cell do its thing. See line 110ff in autocomplete_text_field.mm
> 
> * Second click, on an Omnibox decoration -- EV2
>   -> Already first responder, so no new focusEvent
>   -> control's mouseDown is called, and since it's on a decoration, it's
outside
> the textframe -> call the cell's mouseDown
>   -> cell's mouseDown is called with EV2. It's not the mouse event that caused
> us to focus (EV1), so do nothing.

OK, so if the click is in the text area, then this code will forward the
OnSetFocus() immediately, so good, and the mouseDown doesn't have to do
anything.

For the second click, the field itself is not first responder, so it will
receive first responder again, so it should go through the first-click sequence
for the field.  But OnSetFocus() already worked in that case, so sending it
defered should still work in this case.

> Technically, focusEvent_ will be NULL for the second click, so I _could_ just
> check if focusEvent_ is set. isEqual: feels like a better choice, though,
given
> the complexity of interactions. Or maybe I'm just too cautious. (But without
the
> isEqual, I'm relying on the next person coming in to remember to update
> mouseDown when they update handleFocusEvent for other focus events. It's a bit
> more brittle)

Maybe it could always steal the focusEvent_, but then throw it away if it's not
the current event.  That way it always ends up cleared after mousedown.

I _think_ it must always work out that it will always end up clear after the
control's mousedown, because it would only be set if in the button decoration,
which would only be handled by the cell, which would clear it.  Except for the
case I mentioned above where it tracked the mouse back out of the cell, which
should be fixed.  Right?

[groby-ooo-7-16, 2014-02-27 02:31:40]

On 2014/02/27 02:21:31, shess wrote:
> On 2014/02/26 22:30:00, groby wrote:
> > On 2014/02/26 21:08:39, shess wrote:
> > > On Wed, Feb 26, 2014 at 12:46 PM, <mailto:groby@chromium.org> wrote:
> > > > It's in the cell because the 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),
and
> > > > it
> > > > needs to happen after any potential drag handling.
> > > >
> > > > Both of these happen in the cell, so there's not much of a choice there.
> > > > And I
> > > > can't move the drag handling out of it - that's what NSTextField
expects.
> > > > And
> > > > since OnMousePressed depends on the drag finishing inside the field, it
> > > > makes
> > > > sense to call OnMousePressed there, too...
> > > >
> > > 
> > > ButtonDecoration is not draggable, though?  So as long as the control
> > > -mouseDown: handler does the focus thing before calling into the cell, it
> > > should be fine, since that is how it has always happened?
> > 
> > Yes and no. If I click on a button and then drag off before releasing, it's
a
> > drag event. (With two different results - dragged entirely off the button,
or
> > dragged back onto the button before mouseUp). That needs to be handled by
> > -trackMouse:, and that lives in the cell.
> > 
> > We _could_ move that entire logic up to the control, but I don't see a way
to
> > just handle it via a return value from mouseDown.
> 
> It couldn't be handled with a return value from mousedown (well, not from
_this_
> mousedown).
> 
> AFAICT, the existing -trackMouse:* bit must be attempting to implement
> Cocoa-style button clicking on mouse-up instead of mouse-down.  Given your
> current code, actual focus has been changed, so if the user moves off the
field
> during the click, then OnSetFocus() will never be sent, I think, though the
> field does have focus.  So maybe the original focus should be passed through
in
> that case?  [I suspect that convincing the control not to have stolen focus
will
> be too challenging for this approach, it would need a view sitting above to
> intercept, or some other sort of magic.]
> 
> > > > As for  mouseDown's which are not causing the focus - click on the
> > > > decoration of
> > > > an NSTextField instead of the editor (it's a small 2-pixel frame on the
> > > > outside
> > > > of the main text field), and you'll get a mouseDown, even if the text
> > > > field is
> > > > firstResponder.
> > > >
> > > 
> > > But in that case, the saved focus event should be the same as the event
> > > passed to mouse down.  I'm wondering about cases where the mouse down
could
> > > receive a different event than the mouse down event which was current when
> > > -becomeFirstResponder was called.
> > 
> > Not necessarily. Here's a sequence of events:
> > 
> > * First click on NSTextField, _inside_ the editor frame -- Event EV1
> >   -> becomeFirstResponder
> >   -> focusEvent is EV1
> >   -> The control's mouseDown forwards the event to the editor, without
letting
> > the cell do its thing. See line 110ff in autocomplete_text_field.mm
> > 
> > * Second click, on an Omnibox decoration -- EV2
> >   -> Already first responder, so no new focusEvent
> >   -> control's mouseDown is called, and since it's on a decoration, it's
> outside
> > the textframe -> call the cell's mouseDown
> >   -> cell's mouseDown is called with EV2. It's not the mouse event that
caused
> > us to focus (EV1), so do nothing.
> 
> OK, so if the click is in the text area, then this code will forward the
> OnSetFocus() immediately, so good, and the mouseDown doesn't have to do
> anything.
> 
> For the second click, the field itself is not first responder, so it will
> receive first responder again, so it should go through the first-click
sequence
> for the field.  But OnSetFocus() already worked in that case, so sending it
> defered should still work in this case.
> 
> > Technically, focusEvent_ will be NULL for the second click, so I _could_
just
> > check if focusEvent_ is set. isEqual: feels like a better choice, though,
> given
> > the complexity of interactions. Or maybe I'm just too cautious. (But without
> the
> > isEqual, I'm relying on the next person coming in to remember to update
> > mouseDown when they update handleFocusEvent for other focus events. It's a
bit
> > more brittle)
> 
> Maybe it could always steal the focusEvent_, but then throw it away if it's
not
> the current event.  That way it always ends up cleared after mousedown.

That's what focusEvent.swap(focusEvent_) does. The local - focusEvent - is
always nil before this. So afterwards, the deferred event is only referenced in
the local scoped_nsobject, and automatically gets released however we leave
scope.

> 
> I _think_ it must always work out that it will always end up clear after the
> control's mousedown, because it would only be set if in the button decoration,
> which would only be handled by the cell, which would clear it.  Except for the
> case I mentioned above where it tracked the mouse back out of the cell, which
> should be fixed.  Right?

Yep. Will fix tomorrow. LMK if there's anything else you'd like addressed.

[Scott Hess - ex-Googler, 2014-02-27 02:34:50]

On 2014/02/27 02:31:40, groby wrote:
> On 2014/02/27 02:21:31, shess wrote:
> > Maybe it could always steal the focusEvent_, but then throw it away if it's
> not
> > the current event.  That way it always ends up cleared after mousedown.
> 
> That's what focusEvent.swap(focusEvent_) does. The local - focusEvent - is
> always nil before this. So afterwards, the deferred event is only referenced
in
> the local scoped_nsobject, and automatically gets released however we leave
> scope.

It does the reverse.  It steals it only if equal, but otherwise leaves it in
peace.

> Yep. Will fix tomorrow. LMK if there's anything else you'd like addressed.

I think I'm happy enough with it.  Hopefully if there are chinks, someone will
log bugs.

-scott

[groby-ooo-7-16, 2014-02-27 20:55:31]

PTAL

[Scott Hess - ex-Googler, 2014-02-27 21:09:36]

LGTM, with the first comment's error fixed.  The later logic change is your
call.

I'm not going to block the review on this, but it may be worthwhile thinking on
whether you can unit-test this.  It seems like the kind of thing which might be
broken by accident.

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

https://codereview.chromium.org/163913011/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:423: if
(![theEvent isEqual:focusEvent_])
focusEvent (no _), focusEvent_ is nil.

https://codereview.chromium.org/163913011/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:497: [self
focusNotificationFor:focusEvent ofView:controlView];
Yeah, I think so.  Nothing has occurred to me about clean ways to not focus in
the current design, probably it would have to just integrate a real button
somehow.  Or, possibly, a multi-purpose sub-view which exists only to intercept
specific regions.

https://codereview.chromium.org/163913011/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:831: }
I can't tell if the following is clearer or not, so I'm just throwing it out
there to potentially be ignored.  I certainly don't feel strongly about it (I
suspect this all is six-or-half-a-dozen in terms of subtleness).

If the code at the top of -mouseDown: were removed, then event here would always
be non-nil.  So it could be:

if ([controlView observer] && (!focusEvent_ || [focusEvent_ isEqual:event])) {
  // OnSetFocus().
}
focusEvent_.reset();

Then these two methods which are close to each other are the sole source of this
knowledge.

[groby-ooo-7-16, 2014-02-27 23:15:27]

I'm thinking on the unit test - it'll be interesting, to say the least. If
you've got any ideas how to test around the trackMouse call, I'd appreciate
them. (Currently tempted to override the cell's -trackMouse: for testing
purposes.)

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

https://codereview.chromium.org/163913011/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:497: [self
focusNotificationFor:focusEvent ofView:controlView];
On 2014/02/27 21:09:36, shess wrote:
> Yeah, I think so.  Nothing has occurred to me about clean ways to not focus in
> the current design, probably it would have to just integrate a real button
> somehow.  Or, possibly, a multi-purpose sub-view which exists only to
intercept
> specific regions.

Well, we need to accept firstResponder for the top level view, since we _do_
want mouseDown's for button clicks. The bigger issue is probably that upstream
code wants to hide buttons in OnFocus(). Breaking up in OnFocus/OnReadyToEdit
might be another way to tackle that, but I don't want to bloat the top-level API
either.

https://codereview.chromium.org/163913011/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:831: }
On 2014/02/27 21:09:36, shess wrote:
> I can't tell if the following is clearer or not, so I'm just throwing it out
> there to potentially be ignored.  I certainly don't feel strongly about it (I
> suspect this all is six-or-half-a-dozen in terms of subtleness).
> 
> If the code at the top of -mouseDown: were removed, then event here would
always
> be non-nil.  So it could be:
> 
> if ([controlView observer] && (!focusEvent_ || [focusEvent_ isEqual:event])) {
>   // OnSetFocus().
> }
> focusEvent_.reset();
> 
> Then these two methods which are close to each other are the sole source of
this
> knowledge.

Except this is only called from button decorations.  I could call it from a few
other places to take care of non-button-decorations, but that's equally
confusing. I suppose what I really want is factor out mouseDown, to

target = [self decorationForEvent:event];
[self postDelayedFocusNotificationForEvent:event];
[self mouseDownForDecoration:target];

I'll leave a TODO() in there - it seems the "done thing" in Omnibox ;)

[groby-ooo-7-16, 2014-02-27 23:18:02]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2014-02-27 23:19:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/163913011/70001

[Scott Hess - ex-Googler, 2014-02-27 23:21:23]

LGTM.

On 2014/02/27 23:15:27, groby wrote:
> I'm thinking on the unit test - it'll be interesting, to say the least. If
> you've got any ideas how to test around the trackMouse call, I'd appreciate
> them. (Currently tempted to override the cell's -trackMouse: for testing
> purposes.)

I believe you can post a series of events to the end of the queue, so you can
"just" construct events and arrange them in order.  There are a few tests that
do mouse-oriented things, though they may be badly bit-rotted.

Mostly, though, I think getting the baseline "OnSetFocus() not called until the
mousedown" is probably most important.  Getting subtle tracking things is a
bonus, of course, but most people either click or don't click.

> I'll leave a TODO() in there - it seems the "done thing" in Omnibox ;)

I am a rampant TODO writer.  Some people think they obscure the code, I
generally think of them as "This is where I wanted to go, but not going to get
there right now, so keep it in mind when wondering what kind of moron wrote
this."

[commit-bot: I haz the power, 2014-02-28 01:13:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/163913011/70001

[groby-ooo-7-16, 2014-02-28 02:59:33]

Just a heads-up: Teensy fix. Needed to account for the fact that there's not
necessarily a currentEvent in becomeFirstResponder.

[groby-ooo-7-16, 2014-02-28 02:59:38]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2014-02-28 03:00:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/163913011/90001

[commit-bot: I haz the power, 2014-02-28 04:13:55]

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

[commit-bot: I haz the power, 2014-02-28 04:13:56]

Retried try job too often on win_x64_rel for step(s) base_unittests,
chrome_elf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[Scott Hess - ex-Googler, 2014-02-28 04:20:47]

change lgtm

[groby-ooo-7-16, 2014-02-28 07:22:16]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2014-02-28 07:22:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/163913011/90001

[commit-bot: I haz the power, 2014-02-28 14:41:08]

Change committed as 254106