Convert location bar bubble delegates to bubble dialog delegates

Convert location bar bubble delegates to bubble dialog delegates

BUG=585312
Committed: https://crrev.com/6c27fa4ddfb8efa64df7c16b783b8252b81834dd
Cr-Commit-Position: refs/heads/master@{#380411}

[Evan Stade, 2016-03-02 02:15:06]

Description was changed from

==========
WIP - convert location bar bubble delegates to bubble dialog delegates

BUG=
==========

to

==========
Convert location bar bubble delegates to bubble dialog delegates

BUG=585312
==========

[Evan Stade, 2016-03-02 02:15:52]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-02 02:16:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759453002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759453002/40001

[commit-bot: I haz the power, 2016-03-02 02:35:53]

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

[commit-bot: I haz the power, 2016-03-02 02:35:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2016-03-03 04:12:17]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-03 04:12:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759453002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759453002/60001

[commit-bot: I haz the power, 2016-03-03 04:33:19]

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

[commit-bot: I haz the power, 2016-03-03 04:33:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2016-03-03 21:57:38]

estade@chromium.org changed reviewers:
+ msw@chromium.org

[Evan Stade, 2016-03-03 22:01:50]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-03 22:02:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759453002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759453002/80001

[commit-bot: I haz the power, 2016-03-03 22:19:09]

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

[commit-bot: I haz the power, 2016-03-03 22:19:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2016-03-03 23:01:40]

Please avoid the new View function if possible.

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:98: return true;
nit: maybe return LocationBarBubbleDelegateView::Accept here and elsewhere?

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:103: return true;
nit: maybe return LocationBarBubbleDelegateView::Cancel here and elsewhere?

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right):

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:12: #include
"ui/views/controls/button/button.h"
nit: remove

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:21: class LabelButton;
nit: remove

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/translate/translate_bubble_view.cc:134:
translate_bubble_view_->CloseBubble();
optional nit: invert conditional and nest, like other impls.

https://codereview.chromium.org/1759453002/diff/80001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1759453002/diff/80001/ui/views/view.cc#newcod...
ui/views/view.cc:1369: return accelerators_ &&
nit: should this check CanHandleAccelerators() too?

https://codereview.chromium.org/1759453002/diff/80001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1759453002/diff/80001/ui/views/view.h#newcode...
ui/views/view.h:1005: bool HandlesAccelerator(const ui::Accelerator&
accelerator);
nit: group with others on line 736 if needed (reorder impl to match).

https://codereview.chromium.org/1759453002/diff/80001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/1759453002/diff/80001/ui/views/window/dialog_...
ui/views/window/dialog_client_view.cc:219:
contents_view()->AcceleratorPressed(accelerator)) {
Why not just call AcceleratorPressed and avoid the new View api?

[Evan Stade, 2016-03-04 04:12:08]

https://codereview.chromium.org/1759453002/diff/80001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/1759453002/diff/80001/ui/views/window/dialog_...
ui/views/window/dialog_client_view.cc:219:
contents_view()->AcceleratorPressed(accelerator)) {
On 2016/03/03 23:01:40, msw wrote:
> Why not just call AcceleratorPressed and avoid the new View api?

because many views do something like

AcceleratorPressed(accelerator) {
  DCHECK_EQ(VKEY_RETURN, accelerator);
  // do stuff for return
}

Even if there were no DCHECKs I still think it would be dumb to make all views
write this fn as

if (accelerator == VKEY_RETURN) {
  do something;
}

So, in summary, I don't think AcceleratorPressed should be (and presently isn't)
called for accelerators not registered for.

[msw, 2016-03-04 18:21:09]

https://codereview.chromium.org/1759453002/diff/80001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/1759453002/diff/80001/ui/views/window/dialog_...
ui/views/window/dialog_client_view.cc:219:
contents_view()->AcceleratorPressed(accelerator)) {
On 2016/03/04 04:12:08, Evan Stade wrote:
> On 2016/03/03 23:01:40, msw wrote:
> > Why not just call AcceleratorPressed and avoid the new View api?
> 
> because many views do something like
> 
> AcceleratorPressed(accelerator) {
>   DCHECK_EQ(VKEY_RETURN, accelerator);
>   // do stuff for return
> }
> 
> Even if there were no DCHECKs I still think it would be dumb to make all views
> write this fn as
> 
> if (accelerator == VKEY_RETURN) {
>   do something;
> }
> 
> So, in summary, I don't think AcceleratorPressed should be (and presently
isn't)
> called for accelerators not registered for.

Okay, good argument, I suppose I'm in favor of adding HandlesAccelerator.

[msw, 2016-03-04 18:21:51]

> Okay, good argument, I suppose I'm in favor of adding HandlesAccelerator.
Maybe add some unit testing of the new function?

[Evan Stade, 2016-03-07 18:36:37]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[Evan Stade, 2016-03-07 18:36:38]

+sky as msw is ooo

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:98: return true;
On 2016/03/03 23:01:39, out mon and wed-fri wrote:
> nit: maybe return LocationBarBubbleDelegateView::Accept here and elsewhere?

I don't think that's right. This seems like a situation where you want to
override default (non-) behavior, not tack on extra behavior.

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:103: return true;
On 2016/03/03 23:01:39, out mon and wed-fri wrote:
> nit: maybe return LocationBarBubbleDelegateView::Cancel here and elsewhere?

ditto

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right):

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:12: #include
"ui/views/controls/button/button.h"
On 2016/03/03 23:01:39, out mon and wed-fri wrote:
> nit: remove

Done.

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/save_card_bubble_views.h:21: class LabelButton;
On 2016/03/03 23:01:39, out mon and wed-fri wrote:
> nit: remove

Done.

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/1759453002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/translate/translate_bubble_view.cc:134:
translate_bubble_view_->CloseBubble();
On 2016/03/03 23:01:40, out mon and wed-fri wrote:
> optional nit: invert conditional and nest, like other impls.

Done.

https://codereview.chromium.org/1759453002/diff/80001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1759453002/diff/80001/ui/views/view.cc#newcod...
ui/views/view.cc:1369: return accelerators_ &&
On 2016/03/03 23:01:40, out mon and wed-fri wrote:
> nit: should this check CanHandleAccelerators() too?

hm, maybe. Done.

https://codereview.chromium.org/1759453002/diff/80001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/1759453002/diff/80001/ui/views/view.h#newcode...
ui/views/view.h:1005: bool HandlesAccelerator(const ui::Accelerator&
accelerator);
On 2016/03/03 23:01:40, out mon and wed-fri wrote:
> nit: group with others on line 736 if needed (reorder impl to match).

Done.

[sky, 2016-03-07 21:14:31]

https://codereview.chromium.org/1759453002/diff/100001/ui/views/window/dialog...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/1759453002/diff/100001/ui/views/window/dialog...
ui/views/window/dialog_client_view.cc:218: if
(contents_view()->HandlesAccelerator(accelerator) &&
I dislike this because I think it's possible for the view to be called for
AcceleratorPressed twice. I say that because the ordering of targets registered
with the same accelerator depends upon the order the accelerators are added. If
the view is registered first, then it means HandleAccelerator() is called twice
for the view. It won't be detrimental, but it's a bit weird. How about a
delegate method as to whether escape should close?

[Evan Stade, 2016-03-08 01:34:44]

https://codereview.chromium.org/1759453002/diff/100001/ui/views/window/dialog...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/1759453002/diff/100001/ui/views/window/dialog...
ui/views/window/dialog_client_view.cc:218: if
(contents_view()->HandlesAccelerator(accelerator) &&
On 2016/03/07 21:14:31, sky wrote:
> I dislike this because I think it's possible for the view to be called for
> AcceleratorPressed twice. I say that because the ordering of targets
registered
> with the same accelerator depends upon the order the accelerators are added.
If
> the view is registered first, then it means HandleAccelerator() is called
twice
> for the view. It won't be detrimental, but it's a bit weird. How about a
> delegate method as to whether escape should close?

How about this: remove escape handling from DialogClientView and add it to
DialogDelegateView. This way subclass dialogs can override it.

See also patchset 7 for an alternative which I decided was less sensible.

[Evan Stade, 2016-03-08 01:34:54]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-08 01:35:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759453002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759453002/140001

[commit-bot: I haz the power, 2016-03-08 02:06:05]

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

[commit-bot: I haz the power, 2016-03-08 02:06:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-03-08 16:27:05]

I agree with patchset 7 being less sensible.

Moving to DialogDelegateView only works if there is no subclasses of
just DialogDelegate. I bet there are. I'm still liking something like
ShouldEscapeClose() on the delegate that DialogClientView calls to
determine if it should register escape.

On Mon, Mar 7, 2016 at 5:34 PM,  <estade@chromium.org> wrote:
>
>
https://codereview.chromium.org/1759453002/diff/100001/ui/views/window/dialog...
> File ui/views/window/dialog_client_view.cc (right):
>
>
https://codereview.chromium.org/1759453002/diff/100001/ui/views/window/dialog...
> ui/views/window/dialog_client_view.cc:218: if
> (contents_view()->HandlesAccelerator(accelerator) &&
> On 2016/03/07 21:14:31, sky wrote:
>> I dislike this because I think it's possible for the view to be called
> for
>> AcceleratorPressed twice. I say that because the ordering of targets
> registered
>> with the same accelerator depends upon the order the accelerators are
> added. If
>> the view is registered first, then it means HandleAccelerator() is
> called twice
>> for the view. It won't be detrimental, but it's a bit weird. How about
> a
>> delegate method as to whether escape should close?
>
> How about this: remove escape handling from DialogClientView and add it
> to DialogDelegateView. This way subclass dialogs can override it.
>
> See also patchset 7 for an alternative which I decided was less
> sensible.
>
> https://codereview.chromium.org/1759453002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2016-03-08 22:12:20]

On 2016/03/08 16:27:05, sky wrote:
> I agree with patchset 7 being less sensible.
> 
> Moving to DialogDelegateView only works if there is no subclasses of
> just DialogDelegate. I bet there are. 

Ah good call, there are. Some of them look like they could/should be
DialogDelegateViews but not all.

> I'm still liking something like
> ShouldEscapeClose() on the delegate that DialogClientView calls to
> determine if it should register escape.

So basically patchset 7 but with an extra conditional? I kind of like relying on
the explicit API promise in FocusManager:

  // Register a keyboard accelerator for the specified target. If multiple
  // targets are registered for an accelerator, a target registered later has
  // higher priority.

The reason I am looking around for alternatives is that I think this will look
weird:

bool BookmarkBubbleView::ShouldEscapeClose() {
  return false;
}

because it actually does close on escape. I also like the idea of keeping the
delegate interface as small as possible.

> 
> On Mon, Mar 7, 2016 at 5:34 PM,  <mailto:estade@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/1759453002/diff/100001/ui/views/window/dialog...
> > File ui/views/window/dialog_client_view.cc (right):
> >
> >
>
https://codereview.chromium.org/1759453002/diff/100001/ui/views/window/dialog...
> > ui/views/window/dialog_client_view.cc:218: if
> > (contents_view()->HandlesAccelerator(accelerator) &&
> > On 2016/03/07 21:14:31, sky wrote:
> >> I dislike this because I think it's possible for the view to be called
> > for
> >> AcceleratorPressed twice. I say that because the ordering of targets
> > registered
> >> with the same accelerator depends upon the order the accelerators are
> > added. If
> >> the view is registered first, then it means HandleAccelerator() is
> > called twice
> >> for the view. It won't be detrimental, but it's a bit weird. How about
> > a
> >> delegate method as to whether escape should close?
> >
> > How about this: remove escape handling from DialogClientView and add it
> > to DialogDelegateView. This way subclass dialogs can override it.
> >
> > See also patchset 7 for an alternative which I decided was less
> > sensible.
> >
> > https://codereview.chromium.org/1759453002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[sky, 2016-03-08 23:07:58]

On Tue, Mar 8, 2016 at 2:12 PM,  <estade@chromium.org> wrote:
> On 2016/03/08 16:27:05, sky wrote:
>> I agree with patchset 7 being less sensible.
>>
>> Moving to DialogDelegateView only works if there is no subclasses of
>> just DialogDelegate. I bet there are.
>
> Ah good call, there are. Some of them look like they could/should be
> DialogDelegateViews but not all.
>
>> I'm still liking something like
>> ShouldEscapeClose() on the delegate that DialogClientView calls to
>> determine if it should register escape.
>
> So basically patchset 7 but with an extra conditional? I kind of like
> relying on
> the explicit API promise in FocusManager:
>
> // Register a keyboard accelerator for the specified target. If multiple
> // targets are registered for an accelerator, a target registered later has
> // higher priority.

The problem in how do you know who is registering first? When
DialogClientView registers accelerators is rather implementation
dependent. By which I mean could easily change. That's why I like the
delegate interface.

> The reason I am looking around for alternatives is that I think this will
> look
> weird:
>
> bool BookmarkBubbleView::ShouldEscapeClose() {
> return false;
> }
>
> because it actually does close on escape. I also like the idea of keeping
> the
> delegate interface as small as possible.

That's all in the naming. You could make the name more explicit,
ShouldRegisterAcceleratorForEscape(). An alternative is an API to
remove the escape accelerator, perhaps
DialogClientView::RemoveEscapeAccelerator(). That has the advantage of
not adding to the delegate interface (which I agree, is nice).

  -Scott


>
>>
>> On Mon, Mar 7, 2016 at 5:34 PM, <mailto:estade@chromium.org> wrote:
>> >
>> >
>>
>
https://codereview.chromium.org/1759453002/diff/100001/ui/views/window/dialog...
>> > File ui/views/window/dialog_client_view.cc (right):
>> >
>> >
>>
>
https://codereview.chromium.org/1759453002/diff/100001/ui/views/window/dialog...
>> > ui/views/window/dialog_client_view.cc:218: if
>> > (contents_view()->HandlesAccelerator(accelerator) &&
>> > On 2016/03/07 21:14:31, sky wrote:
>> >> I dislike this because I think it's possible for the view to be called
>> > for
>> >> AcceleratorPressed twice. I say that because the ordering of targets
>> > registered
>> >> with the same accelerator depends upon the order the accelerators are
>> > added. If
>> >> the view is registered first, then it means HandleAccelerator() is
>> > called twice
>> >> for the view. It won't be detrimental, but it's a bit weird. How about
>> > a
>> >> delegate method as to whether escape should close?
>> >
>> > How about this: remove escape handling from DialogClientView and add it
>> > to DialogDelegateView. This way subclass dialogs can override it.
>> >
>> > See also patchset 7 for an alternative which I decided was less
>> > sensible.
>> >
>> > https://codereview.chromium.org/1759453002/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/1759453002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2016-03-09 00:39:03]

On 2016/03/08 23:07:58, sky wrote:
> On Tue, Mar 8, 2016 at 2:12 PM,  <mailto:estade@chromium.org> wrote:
> > On 2016/03/08 16:27:05, sky wrote:
> >> I agree with patchset 7 being less sensible.
> >>
> >> Moving to DialogDelegateView only works if there is no subclasses of
> >> just DialogDelegate. I bet there are.
> >
> > Ah good call, there are. Some of them look like they could/should be
> > DialogDelegateViews but not all.
> >
> >> I'm still liking something like
> >> ShouldEscapeClose() on the delegate that DialogClientView calls to
> >> determine if it should register escape.
> >
> > So basically patchset 7 but with an extra conditional? I kind of like
> > relying on
> > the explicit API promise in FocusManager:
> >
> > // Register a keyboard accelerator for the specified target. If multiple
> > // targets are registered for an accelerator, a target registered later has
> > // higher priority.
> 
> The problem in how do you know who is registering first? When
> DialogClientView registers accelerators is rather implementation
> dependent. By which I mean could easily change. That's why I like the
> delegate interface.

Well, the contents view always gets added to the widget after the client view
gets added to the widget:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/window/cl...

So we know its accelerators will be registered first (and therefore be
overrideable). Plus from reading view.cc it seems that parents will always
register pending accelerators before children do.

> 
> > The reason I am looking around for alternatives is that I think this will
> > look
> > weird:
> >
> > bool BookmarkBubbleView::ShouldEscapeClose() {
> > return false;
> > }
> >
> > because it actually does close on escape. I also like the idea of keeping
> > the
> > delegate interface as small as possible.
> 
> That's all in the naming. You could make the name more explicit,
> ShouldRegisterAcceleratorForEscape(). An alternative is an API to
> remove the escape accelerator, perhaps
> DialogClientView::RemoveEscapeAccelerator(). That has the advantage of
> not adding to the delegate interface (which I agree, is nice).

We can do that with GetDialogClientView()->RemoveAccelerator(escape) as I have
done in the latest patch. However this seems just as reliant on the current
order of adding things to the widget hierarchy as ps7 does. And if DCV tried to
add an accelerator dynamically (like it did before this patch) this wouldn't
work.

> 
>   -Scott
> 
> 
> >
> >>
> >> On Mon, Mar 7, 2016 at 5:34 PM, <mailto:estade@chromium.org> wrote:
> >> >
> >> >
> >>
> >
>
https://codereview.chromium.org/1759453002/diff/100001/ui/views/window/dialog...
> >> > File ui/views/window/dialog_client_view.cc (right):
> >> >
> >> >
> >>
> >
>
https://codereview.chromium.org/1759453002/diff/100001/ui/views/window/dialog...
> >> > ui/views/window/dialog_client_view.cc:218: if
> >> > (contents_view()->HandlesAccelerator(accelerator) &&
> >> > On 2016/03/07 21:14:31, sky wrote:
> >> >> I dislike this because I think it's possible for the view to be called
> >> > for
> >> >> AcceleratorPressed twice. I say that because the ordering of targets
> >> > registered
> >> >> with the same accelerator depends upon the order the accelerators are
> >> > added. If
> >> >> the view is registered first, then it means HandleAccelerator() is
> >> > called twice
> >> >> for the view. It won't be detrimental, but it's a bit weird. How about
> >> > a
> >> >> delegate method as to whether escape should close?
> >> >
> >> > How about this: remove escape handling from DialogClientView and add it
> >> > to DialogDelegateView. This way subclass dialogs can override it.
> >> >
> >> > See also patchset 7 for an alternative which I decided was less
> >> > sensible.
> >> >
> >> > https://codereview.chromium.org/1759453002/
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups
> >> "Chromium-reviews" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an
> > email
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> >
> > https://codereview.chromium.org/1759453002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[Evan Stade, 2016-03-09 00:39:58]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-09 00:40:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759453002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759453002/160001

[sky, 2016-03-09 00:57:10]

On Tue, Mar 8, 2016 at 4:39 PM,  <estade@chromium.org> wrote:
> On 2016/03/08 23:07:58, sky wrote:
>> On Tue, Mar 8, 2016 at 2:12 PM, <mailto:estade@chromium.org> wrote:
>> > On 2016/03/08 16:27:05, sky wrote:
>> >> I agree with patchset 7 being less sensible.
>> >>
>> >> Moving to DialogDelegateView only works if there is no subclasses of
>> >> just DialogDelegate. I bet there are.
>> >
>> > Ah good call, there are. Some of them look like they could/should be
>> > DialogDelegateViews but not all.
>> >
>> >> I'm still liking something like
>> >> ShouldEscapeClose() on the delegate that DialogClientView calls to
>> >> determine if it should register escape.
>> >
>> > So basically patchset 7 but with an extra conditional? I kind of like
>> > relying on
>> > the explicit API promise in FocusManager:
>> >
>> > // Register a keyboard accelerator for the specified target. If multiple
>> > // targets are registered for an accelerator, a target registered later
>> > has
>> > // higher priority.
>>
>> The problem in how do you know who is registering first? When
>> DialogClientView registers accelerators is rather implementation
>> dependent. By which I mean could easily change. That's why I like the
>> delegate interface.
>
> Well, the contents view always gets added to the widget after the client
> view
> gets added to the widget:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/window/cl...
>
> So we know its accelerators will be registered first (and therefore be
> overrideable). Plus from reading view.cc it seems that parents will always
> register pending accelerators before children do.

Ok, you convinced me. Just add a test to make sure we don't regress
this, and I'm ok. And by test I mean one explicitly using
dialogclientview.

  -Scott

>
>>
>> > The reason I am looking around for alternatives is that I think this
>> > will
>> > look
>> > weird:
>> >
>> > bool BookmarkBubbleView::ShouldEscapeClose() {
>> > return false;
>> > }
>> >
>> > because it actually does close on escape. I also like the idea of
>> > keeping
>> > the
>> > delegate interface as small as possible.
>>
>> That's all in the naming. You could make the name more explicit,
>> ShouldRegisterAcceleratorForEscape(). An alternative is an API to
>> remove the escape accelerator, perhaps
>> DialogClientView::RemoveEscapeAccelerator(). That has the advantage of
>> not adding to the delegate interface (which I agree, is nice).
>
> We can do that with GetDialogClientView()->RemoveAccelerator(escape) as I
> have
> done in the latest patch. However this seems just as reliant on the current
> order of adding things to the widget hierarchy as ps7 does. And if DCV tried
> to
> add an accelerator dynamically (like it did before this patch) this wouldn't
> work.
>
>
>>
>> -Scott
>>
>>
>> >
>> >>
>> >> On Mon, Mar 7, 2016 at 5:34 PM, <mailto:estade@chromium.org> wrote:
>> >> >
>> >> >
>> >>
>> >
>>
>
https://codereview.chromium.org/1759453002/diff/100001/ui/views/window/dialog...
>> >> > File ui/views/window/dialog_client_view.cc (right):
>> >> >
>> >> >
>> >>
>> >
>>
>
https://codereview.chromium.org/1759453002/diff/100001/ui/views/window/dialog...
>> >> > ui/views/window/dialog_client_view.cc:218: if
>> >> > (contents_view()->HandlesAccelerator(accelerator) &&
>> >> > On 2016/03/07 21:14:31, sky wrote:
>> >> >> I dislike this because I think it's possible for the view to be
>> >> >> called
>> >> > for
>> >> >> AcceleratorPressed twice. I say that because the ordering of targets
>> >> > registered
>> >> >> with the same accelerator depends upon the order the accelerators
>> >> >> are
>> >> > added. If
>> >> >> the view is registered first, then it means HandleAccelerator() is
>> >> > called twice
>> >> >> for the view. It won't be detrimental, but it's a bit weird. How
>> >> >> about
>> >> > a
>> >> >> delegate method as to whether escape should close?
>> >> >
>> >> > How about this: remove escape handling from DialogClientView and add
>> >> > it
>> >> > to DialogDelegateView. This way subclass dialogs can override it.
>> >> >
>> >> > See also patchset 7 for an alternative which I decided was less
>> >> > sensible.
>> >> >
>> >> > https://codereview.chromium.org/1759453002/
>> >>
>> >> --
>> >> You received this message because you are subscribed to the Google
>> >> Groups
>> >> "Chromium-reviews" group.
>> >> To unsubscribe from this group and stop receiving emails from it, send
>> >> an
>> > email
>> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
>> >>
>> >
>> >
>> > https://codereview.chromium.org/1759453002/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
> https://codereview.chromium.org/1759453002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-03-09 01:15:54]

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

[commit-bot: I haz the power, 2016-03-09 01:15:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2016-03-09 01:52:37]

https://codereview.chromium.org/1759453002/diff/180001/ui/views/window/dialog...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/1759453002/diff/180001/ui/views/window/dialog...
ui/views/window/dialog_client_view.cc:216: return
cancel_button_->AcceleratorPressed(accelerator);
note this additional change from previous. This way even dialogs with cancel
buttons can override the accelerator (otherwise UpdateDialogButtons registers
the accelerator after the contents likely does).

https://codereview.chromium.org/1759453002/diff/180001/ui/views/window/dialog...
File ui/views/window/dialog_delegate_unittest.cc (right):

https://codereview.chromium.org/1759453002/diff/180001/ui/views/window/dialog...
ui/views/window/dialog_delegate_unittest.cc:183:
dialog()->set_should_handle_escape(true);
there's a flag instead of just AddAccelerator here because I wanted to test
adding the accelerator to the contents before creating the DCV.

[Evan Stade, 2016-03-09 02:09:07]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-09 02:09:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759453002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759453002/180001

[commit-bot: I haz the power, 2016-03-09 02:50:10]

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

[commit-bot: I haz the power, 2016-03-09 02:50:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-03-09 16:18:38]

https://codereview.chromium.org/1759453002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1759453002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:71: if
(!learn_more_link_) {
DCHECK? CreateExtraView is only called once.

https://codereview.chromium.org/1759453002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right):

https://codereview.chromium.org/1759453002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/bubble_icon_view.cc:38: return
!!GetBubble();
IMO != nullptr is way more readable. Since you don't need to change this, and
there is no style for this, leave it as it was (changing to nullptr if you
want).

https://codereview.chromium.org/1759453002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h
(right):

https://codereview.chromium.org/1759453002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h:53:
virtual void CloseBubble();
Why do you need CloseBubble()? Can't you use GetWidget()->Close()?

[Evan Stade, 2016-03-09 22:37:50]

https://codereview.chromium.org/1759453002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right):

https://codereview.chromium.org/1759453002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/save_card_bubble_views.cc:71: if
(!learn_more_link_) {
On 2016/03/09 16:18:38, sky wrote:
> DCHECK? CreateExtraView is only called once.

Done.

https://codereview.chromium.org/1759453002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right):

https://codereview.chromium.org/1759453002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/bubble_icon_view.cc:38: return
!!GetBubble();
On 2016/03/09 16:18:38, sky wrote:
> IMO != nullptr is way more readable. Since you don't need to change this, and
> there is no style for this, leave it as it was (changing to nullptr if you
> want).

OK

https://codereview.chromium.org/1759453002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h
(right):

https://codereview.chromium.org/1759453002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h:53:
virtual void CloseBubble();
On 2016/03/09 16:18:38, sky wrote:
> Why do you need CloseBubble()? Can't you use GetWidget()->Close()?

There are some overrides, like ZoomBubbleView::CloseBubble(), which seem to
state that WidgetDelegate::OnClosing() won't work. I didn't really investigate
changing this.

[Evan Stade, 2016-03-09 22:38:26]

there are some test failures - please hold off on review till I sort those out.

[Evan Stade, 2016-03-10 02:17:20]

ImmersiveFullscreenController proved to be a headache but tests seem to pass
now.

https://codereview.chromium.org/1759453002/diff/240001/ash/wm/immersive_fulls...
File ash/wm/immersive_fullscreen_controller.h (right):

https://codereview.chromium.org/1759453002/diff/240001/ash/wm/immersive_fulls...
ash/wm/immersive_fullscreen_controller.h:291: class BubbleObserver;
There's already a different class called BubbleManager

[Evan Stade, 2016-03-10 02:17:29]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-10 02:18:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759453002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759453002/240001

[commit-bot: I haz the power, 2016-03-10 03:28:17]

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

[commit-bot: I haz the power, 2016-03-10 03:28:19]

Dry run: This issue passed the CQ dry run.

[sky, 2016-03-10 04:45:52]

LGTM

[Evan Stade, 2016-03-10 15:23:22]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-03-10 15:23:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759453002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759453002/240001

[commit-bot: I haz the power, 2016-03-10 15:29:20]

Description was changed from

==========
Convert location bar bubble delegates to bubble dialog delegates

BUG=585312
==========

to

==========
Convert location bar bubble delegates to bubble dialog delegates

BUG=585312
==========

[commit-bot: I haz the power, 2016-03-10 15:29:22]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-03-10 15:30:49]

Description was changed from

==========
Convert location bar bubble delegates to bubble dialog delegates

BUG=585312
==========

to

==========
Convert location bar bubble delegates to bubble dialog delegates

BUG=585312

Committed: https://crrev.com/6c27fa4ddfb8efa64df7c16b783b8252b81834dd
Cr-Commit-Position: refs/heads/master@{#380411}
==========

[commit-bot: I haz the power, 2016-03-10 15:30:51]

Patchset 13 (id:??) landed as
https://crrev.com/6c27fa4ddfb8efa64df7c16b783b8252b81834dd
Cr-Commit-Position: refs/heads/master@{#380411}