[Mac] Fix for security indicator drag&drop navigation

[Mac] Fix for security indicator drag&drop navigation

Reject drops that land on the omnibox decorations

BUG=588381
Committed: https://crrev.com/441287d182ed3eef1fa522d682ad705e659023bc
Cr-Commit-Position: refs/heads/master@{#420768}

[spqchan, 2016-09-07 22:30:55]

spqchan@chromium.org changed reviewers:
+ shrike@chromium.org

[spqchan, 2016-09-07 22:30:56]

PTAL

[shrike, 2016-09-07 23:49:27]

https://codereview.chromium.org/2324593002/diff/1/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right):

https://codereview.chromium.org/2324593002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:522: [[self
cell] decorationForEvent:[NSApp currentEvent]
Relying on [NSApp currentEvent] for the drag event makes me nervous. It's
probably better to call [sender draggingLocation] to get the mouse location, and
to pass that to a version of decorationForEvent that takes a mouse point.

[spqchan, 2016-09-08 17:41:43]

PTAL

https://codereview.chromium.org/2324593002/diff/1/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right):

https://codereview.chromium.org/2324593002/diff/1/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:522: [[self
cell] decorationForEvent:[NSApp currentEvent]
On 2016/09/07 23:49:27, shrike wrote:
> Relying on [NSApp currentEvent] for the drag event makes me nervous. It's
> probably better to call [sender draggingLocation] to get the mouse location,
and
> to pass that to a version of decorationForEvent that takes a mouse point.

Done.

[shrike, 2016-09-08 18:01:18]

On 2016/09/08 17:41:43, spqchan wrote:
> Done.

Thank you. I think that's safer.

One more thing I noticed: if I drag the URL far to the left, before I get to the
start of the Omnibox I get a green plus cursor. I believe that's because the
decoration starts a few pixels to the right of the Omnibox's left edge. I'm
thinking the drag code should not accept anything dropped to the right of the
EV-origin decoration (not sure what it's called).

[spqchan, 2016-09-08 18:09:17]

On 2016/09/08 18:01:18, shrike wrote:
> On 2016/09/08 17:41:43, spqchan wrote:
> > Done.
> 
> Thank you. I think that's safer.
> 
> One more thing I noticed: if I drag the URL far to the left, before I get to
the
> start of the Omnibox I get a green plus cursor. I believe that's because the
> decoration starts a few pixels to the right of the Omnibox's left edge. I'm
> thinking the drag code should not accept anything dropped to the right of the
> EV-origin decoration (not sure what it's called).

Haha funny thing, I was actually making sure that part still accept the drag.

[spqchan, 2016-09-15 00:33:10]

PTAL

[shrike, 2016-09-16 18:28:41]

https://codereview.chromium.org/2324593002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right):

https://codereview.chromium.org/2324593002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:524: return
canDropAtLocation ? [dropHandler_ draggingUpdated:sender]
I think you need to add canDropAtLocation logic to draggingEntered: as well? I
noticed that if I drag to the left out of the Omnibox, and then slowly drag back
to the right, when I reach the left edge of the Omnibox I get a green plus
cursor for a moment, and then an arrow cursor. I suspect draggingEntered: is
returning YES, and then the Appkit is immediately follows up with a call to
draggingUpdated:, which returns NO.

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

https://codereview.chromium.org/2324593002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:438: if
(NSMouseInRect(locationInView, leftPaddingFrame, flipped))
You could also just say if (locationInView.x < LeftDecorationXOffset())...

https://codereview.chromium.org/2324593002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:442: if
(!rightDecorations_.empty()) {
Do you think we really need this check? Seems like as long as locationInView.x
is beyond the first left decoration, it's OK to drop the URL?

[spqchan, 2016-09-20 21:00:00]

PTAL

https://codereview.chromium.org/2324593002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right):

https://codereview.chromium.org/2324593002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:524: return
canDropAtLocation ? [dropHandler_ draggingUpdated:sender]
On 2016/09/16 18:28:41, shrike wrote:
> I think you need to add canDropAtLocation logic to draggingEntered: as well? I
> noticed that if I drag to the left out of the Omnibox, and then slowly drag
back
> to the right, when I reach the left edge of the Omnibox I get a green plus
> cursor for a moment, and then an arrow cursor. I suspect draggingEntered: is
> returning YES, and then the Appkit is immediately follows up with a call to
> draggingUpdated:, which returns NO.

Done.

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

https://codereview.chromium.org/2324593002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:438: if
(NSMouseInRect(locationInView, leftPaddingFrame, flipped))
On 2016/09/16 18:28:41, shrike wrote:
> You could also just say if (locationInView.x < LeftDecorationXOffset())...

Done.

https://codereview.chromium.org/2324593002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:442: if
(!rightDecorations_.empty()) {
On 2016/09/16 18:28:41, shrike wrote:
> Do you think we really need this check? Seems like as long as locationInView.x
> is beyond the first left decoration, it's OK to drop the URL?

I think it's better to be consistent with the left and right sides

[shrike, 2016-09-20 23:10:47]

lgtm

On 2016/09/20 21:00:00, spqchan wrote:
> On 2016/09/16 18:28:41, shrike wrote:
> > Do you think we really need this check? Seems like as long as
locationInView.x
> > is beyond the first left decoration, it's OK to drop the URL?
> 
> I think it's better to be consistent with the left and right sides

I agree.

[spqchan, 2016-09-20 23:12:09]

spqchan@chromium.org changed reviewers:
+ avi@chromium.org

[spqchan, 2016-09-20 23:12:09]

Thanks!

+avi for owner

[Avi (use Gerrit), 2016-09-21 02:44:42]

lgtm

[lgarron, 2016-09-23 22:42:42]

On 2016/09/21 at 02:44:42, avi wrote:
> lgtm

Works well on my computer. Safe to press the commit button? :-)

[spqchan, 2016-09-23 22:45:50]

On 2016/09/23 22:42:42, lgarron wrote:
> On 2016/09/21 at 02:44:42, avi wrote:
> > lgtm
> 
> Works well on my computer. Safe to press the commit button? :-)

whoops. Thanks for the ping!

[spqchan, 2016-09-23 22:45:54]

The CQ bit was checked by spqchan@chromium.org

[commit-bot: I haz the power, 2016-09-23 22:46:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 23:04:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-23 23:06:32]

Description was changed from

==========
[Mac] Fix for security indicator drag&drop navigation

Reject drops that land on the omnibox decorations

BUG=588381
==========

to

==========
[Mac] Fix for security indicator drag&drop navigation

Reject drops that land on the omnibox decorations

BUG=588381

Committed: https://crrev.com/441287d182ed3eef1fa522d682ad705e659023bc
Cr-Commit-Position: refs/heads/master@{#420768}
==========

[commit-bot: I haz the power, 2016-09-23 23:06:33]

Patchset 5 (id:??) landed as
https://crrev.com/441287d182ed3eef1fa522d682ad705e659023bc
Cr-Commit-Position: refs/heads/master@{#420768}