Changed AppListDialogContentsView to extend DialogDelegate

Changed AppListDialogContentsView to extend DialogDelegate

Originally, AppListDialogContentsView extended WidgetDelegate directly,
and was not being detected as a dialog correctly on some platforms.
Updated AppListDialogContentsView to now be a DialogDelegate.

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

[tapted, 2014-07-21 00:34:07]

ffff

https://codereview.chromium.org/405873003/diff/1/chrome/browser/ui/views/app_...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc (right):

https://codereview.chromium.org/405873003/diff/1/chrome/browser/ui/views/app_...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:158: return
true;
It looks like you'll also pick up "WS_EX_DLGMODALFRAME", documented as "The
window has a double border" - does that change the visual style?

https://codereview.chromium.org/405873003/diff/1/ui/views/widget/widget_deleg...
File ui/views/widget/widget_delegate.cc (right):

https://codereview.chromium.org/405873003/diff/1/ui/views/widget/widget_deleg...
ui/views/widget/widget_delegate.cc:63: // TODO(sashab): Return false by default
here, and add an override to all
I think this CL is small enough to do this now  - pretty sure just
dialog_delegate.h/cc needs to change.

https://codereview.chromium.org/405873003/diff/1/ui/views/widget/widget_deleg...
File ui/views/widget/widget_delegate.h (right):

https://codereview.chromium.org/405873003/diff/1/ui/views/widget/widget_deleg...
ui/views/widget/widget_delegate.h:70: virtual bool IsDialogBox();
nit: const

[tapted, 2014-07-21 00:35:21]

On 2014/07/21 00:34:07, tapted wrote:
> ffff

I have *no idea* where those 'f's came from. They were waiting for me in the box
when I sent comments. I think my Rietveld is haunted.

[sashab, 2014-07-21 03:36:21]

https://codereview.chromium.org/405873003/diff/1/chrome/browser/ui/views/app_...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc (right):

https://codereview.chromium.org/405873003/diff/1/chrome/browser/ui/views/app_...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.cc:158: return
true;
On 2014/07/21 00:34:07, tapted wrote:
> It looks like you'll also pick up "WS_EX_DLGMODALFRAME", documented as "The
> window has a double border" - does that change the visual style?
> 

Not at all. Enabling it doesn't seem to do anything either.

I suspect this border is hidden elsewhere, though, considering this is a
borderless window.

https://codereview.chromium.org/405873003/diff/1/ui/views/widget/widget_deleg...
File ui/views/widget/widget_delegate.cc (right):

https://codereview.chromium.org/405873003/diff/1/ui/views/widget/widget_deleg...
ui/views/widget/widget_delegate.cc:63: // TODO(sashab): Return false by default
here, and add an override to all
On 2014/07/21 00:34:07, tapted wrote:
> I think this CL is small enough to do this now  - pretty sure just
> dialog_delegate.h/cc needs to change.

Oh, I didn't think of that, for some reason :) Done.

https://codereview.chromium.org/405873003/diff/1/ui/views/widget/widget_deleg...
File ui/views/widget/widget_delegate.h (right):

https://codereview.chromium.org/405873003/diff/1/ui/views/widget/widget_deleg...
ui/views/widget/widget_delegate.h:70: virtual bool IsDialogBox();
On 2014/07/21 00:34:07, tapted wrote:
> nit: const

Originally, it actually couldn't be const, because AsDialogDelegate is not const
:) So just as well I did them both in this CL. Done.

[tapted, 2014-07-21 03:49:08]

lgtm

There's maybe a better name now than `IsDialogBox` (IsPopupWindow maybe), but
it'll change a few callsites that maybe care more about it being a DialogBox, so
is probably orthogonal (OWNERS might still suggest a rename though :).

[sashab, 2014-07-21 04:29:59]

sky@chromium.org: Please review changes in ui/views :)

[sky, 2014-07-21 16:12:10]

Why is this better than subclassing DialogDelegate? Seems a lot more
straightforward, less error prone and we don't get another confusing method.

[sashab, 2014-07-22 00:36:50]

Hmm, good idea. Although I still think the RTTI in such an important method is
not such a good idea... Maybe we should still move that method to
WidgetDelegate, or remove it altogether and just use IsModal?

https://codereview.chromium.org/405873003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h (left):

https://codereview.chromium.org/405873003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h:51: virtual
ui::AXRole GetAccessibleWindowRole() const OVERRIDE;
DialogDelegate's overwrite of this method is fine now.

[sky, 2014-07-22 04:13:04]

The nice part of AsDialogDelegate is that it's pretty clear what it
means. IsModal isn't so clear. I prefer what we have.

  -Scott

On Mon, Jul 21, 2014 at 5:36 PM,  <sashab@chromium.org> wrote:
> Hmm, good idea. Although I still think the RTTI in such an important method
> is
> not such a good idea... Maybe we should still move that method to
> WidgetDelegate, or remove it altogether and just use IsModal?
>
>
>
https://codereview.chromium.org/405873003/diff/40001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h
> (left):
>
>
https://codereview.chromium.org/405873003/diff/40001/chrome/browser/ui/views/...
> chrome/browser/ui/views/app_list/app_list_dialog_contents_view.h:51:
>
> virtual ui::AXRole GetAccessibleWindowRole() const OVERRIDE;
> DialogDelegate's overwrite of this method is fine now.
>
> https://codereview.chromium.org/405873003/

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

[sashab, 2014-07-22 04:49:12]

Thanks Scott, glad I got your input, this is definitely much nicer (and possibly
mergeable now!) :)

tapted, thoughts? :)

[tapted, 2014-07-22 05:04:47]

On 2014/07/22 04:49:12, sasha_b wrote:
> Thanks Scott, glad I got your input, this is definitely much nicer (and
possibly
> mergeable now!) :)
> 
> tapted, thoughts? :)

It's nice! lgtm if there are no regressions in terms of behaviour or appearance
-- i.e. non-client-frame-views / content padding / initially focussed views /
etc. (but AppListDialogContentsView already overrides CreateNonClientFrameView,
so it might be fine).

[sashab, 2014-07-22 05:12:26]

The CQ bit was checked by sashab@chromium.org

[sashab, 2014-07-22 05:13:01]

Tested on Linux, Win8 and Linux ChromeOS and it seems to look and behave
identically.

[commit-bot: I haz the power, 2014-07-22 05:13:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sashab@chromium.org/405873003/40001

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

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-22 08:43:18]

Change committed as 284648