Use gfx::NativeView rather than gfx::NativeWindow to identify web contents modal dialogs

Use gfx::NativeView rather than gfx::NativeWindow to identify web contents modal dialogs

The API suggested in the associated bug uses gfx::NativeView.  This also enables the Cocoa implementation to be refactored to the interfaces associated with the new API.

This is an intermediate step to get to where we can use the same WebContentsModalDialogManager implementation across all platforms.

BUG=157161

[Mike Wittman, 2013-02-06 01:24:28]

Ben, please review the non-Cocoa changes.
Sailesh, please review chrome/browser/ui/web_contents_modal_dialog_manager* and
the Cocoa changes.

These changes are relative to https://codereview.chromium.org/12045037, which I
hope to submit soon.

[Ben Goodger (Google), 2013-02-06 01:30:21]

Might be splitting hairs but can you explain in your cl desc why you're doing
this.

Splitting hairs because on win32/aura it makes no difference which type you use
since they're both equivalent... is this because the constrained window is
implemented as an overlay NSView on Mac vs. a NSWindow?

LGTM anyway.

https://codereview.chromium.org/12224020/diff/1/chrome/browser/ui/native_web_...
File chrome/browser/ui/native_web_contents_modal_dialog_manager.h (right):

https://codereview.chromium.org/12224020/diff/1/chrome/browser/ui/native_web_...
chrome/browser/ui/native_web_contents_modal_dialog_manager.h:14:
NativeWebContentsModalDialogManager() {}
since this is an interface it shouldn't have a ctor.

[Mike Wittman, 2013-02-06 03:08:42]

On 2013/02/06 01:30:21, Ben Goodger (Google) wrote:
> Might be splitting hairs but can you explain in your cl desc why you're doing
> this.
> 
> Splitting hairs because on win32/aura it makes no difference which type you
use
> since they're both equivalent... is this because the constrained window is
> implemented as an overlay NSView on Mac vs. a NSWindow?

My understanding based on discussion with Hoa Dinh was that NSView was a more
natural identifier for the WCMD on Mac.  Hoa and Sailesh, please comment on
whether this is indeed the preferred type to use as a platform-independent
opaque ID for this dialog.

As far as the WCMDM is concerned it doesn't matter what type we use here as long
as it's unique to each WCMD and the native web contents modal dialog manager
subclass has a mechanism to look up the actual dialog type of interest from it.

I'll update the CL with the clarification we get from the Mac guys.

[dvh-g, 2013-02-06 03:24:38]

-- 
Hoa V. Dinh


On Tuesday, February 5, 2013 at 7:08 PM, wittman@chromium.org wrote:

> On 2013/02/06 01:30:21, Ben Goodger (Google) wrote:
> > Might be splitting hairs but can you explain in your cl desc why you're 
> > doing
> > this.
> > 
> 
> 
> > Splitting hairs because on win32/aura it makes no difference which type 
> > you
> > 
> 
> use
> > since they're both equivalent... is this because the constrained window is
> > implemented as an overlay NSView on Mac vs. a NSWindow?
> > 
> 
> 
> My understanding based on discussion with Hoa Dinh was that NSView was a 
> more
> natural identifier for the WCMD on Mac. Hoa and Sailesh, please comment on
> whether this is indeed the preferred type to use as a platform-independent
> opaque ID for this dialog.
> 
> 

If it's a NSWindow, the dialog will come with the look of the given window.
If it's a NSView, we can embed the NSView in a consistent customized NSWindow.

What the choice on other platforms?
> 
> As far as the WCMDM is concerned it doesn't matter what type we use here as 
> long
> as it's unique to each WCMD and the native web contents modal dialog manager
> subclass has a mechanism to look up the actual dialog type of interest from 
> it.
> 
> I'll update the CL with the clarification we get from the Mac guys.
> 
> https://codereview.chromium.org/12224020/ 

[Mike Wittman, 2013-02-06 16:43:34]

On 2013/02/06 03:24:38, dvh wrote:
> If it's a NSWindow, the dialog will come with the look of the given window.
> If it's a NSView, we can embed the NSView in a consistent customized NSWindow.
> 
> What the choice on other platforms?

On Views and Gk, it doesn't make a difference whether this is gfx::NativeWindow
or gfx::NativeView.

[sail, 2013-02-06 18:42:57]

Hi Mike.

Currently I have three ways to lookup or identify an instance of a constrained
window:
  - pointer to a content::WebContents that the dialog is attached to
  - pointer to a WebContentsModalDialog
  - pointer to a ConstrainedWindowSheet (a custom Objective-C class)

Using a gfx::NativeView probably won't work. In many cases we don't have a
unique view reference to track the dialog with.

Using a gfx::NativeWindow may work. One problem is that we may not have a window
reference until the dialog is shown.

Taking a step back, do you actually need this at all? Could you just use
content::WebContents and WebContentsModalDialog to track the dialog instead?

[dvh-g, 2013-02-06 19:10:13]

> Using a gfx::NativeView probably won't work. In many cases we don't have a
> unique view reference to track the dialog with.

Can't we embed every views of a dialog in a unique NativeView?

Though, I don't know what WebContents / WebContentsModalDialog are about.

[sail, 2013-02-06 19:14:22]

> Can't we embed every views of a dialog in a unique NativeView?

Not in all cases. For example, the SSLClientCertificateSelectorCocoa class uses
the tab modal dialog API to display a system certificate selector.

The system API just gives you an NSPanel. It turns out that the NSPanel isn't
even the panel that gets displayed when you do beginSheet:...

> 
> Though, I don't know what WebContents / WebContentsModalDialog are about.

content::WebContents is the class that manages the web contents for each tab.
When you create a tab modal dialog you attach it to a content::WebContents. Each
content::WebContents has a unique NSView that it draws too.

WebContentsModalDialog is the tab modal dialog API. It's an instance of a tab
modal dialog.

[Mike Wittman, 2013-02-06 19:31:04]

On 2013/02/06 18:42:57, sail wrote:
> Hi Mike.
> 
> Currently I have three ways to lookup or identify an instance of a constrained
> window:
>   - pointer to a content::WebContents that the dialog is attached to
>   - pointer to a WebContentsModalDialog
>   - pointer to a ConstrainedWindowSheet (a custom Objective-C class)
> 
> Using a gfx::NativeView probably won't work. In many cases we don't have a
> unique view reference to track the dialog with.
> 
> Using a gfx::NativeWindow may work. One problem is that we may not have a
window
> reference until the dialog is shown.
> 
> Taking a step back, do you actually need this at all? Could you just use
> content::WebContents and WebContentsModalDialog to track the dialog instead?

Hi Sailesh,

The WebContentsModalDialog interface is going away as part of the API refactor,
so I can't use it to reference the dialogs.  As I understand, the goal of the
refactoring is to be able to present arbitrary UI views in either a tab-modal or
non-tab-modal fashion, so we won't have any special interface for the dialogs. 
(Does this new approach change the way you think about the web contents modal
dialog display on Mac at all?)

We support multiple modal dialogs on the same WebContents at the same time, so
the WebContents pointer isn't suitable as a unique identifier.

To satisfy the API that Ben outlined in
http://code.google.com/p/chromium/issues/detail?id=157161#c5, we'll need the
identifier to be available before the dialog is shown, so it sounds like that
rules out gfx::NativeWindow.

The remaining options I'm aware of are:
 - use a dummy gfx::NativeWindow or gfx::NativeView in Cocoa, as in this CL
 - create a new opaque gfx native typedef that can represent a
ConstrainedWindowSheet and something else analogous on all the other platforms

What do you think?  Can you think of any other options?  I'm loathe to introduce
a new gfx typedef unless we're likely to need to reference types like this on a
widespread basis.

[sail, 2013-02-06 20:12:11]

On 2013/02/06 19:31:04, Mike Wittman wrote:
> On 2013/02/06 18:42:57, sail wrote:
> > Hi Mike.
> > 
> > Currently I have three ways to lookup or identify an instance of a
constrained
> > window:
> >   - pointer to a content::WebContents that the dialog is attached to
> >   - pointer to a WebContentsModalDialog
> >   - pointer to a ConstrainedWindowSheet (a custom Objective-C class)
> > 
> > Using a gfx::NativeView probably won't work. In many cases we don't have a
> > unique view reference to track the dialog with.
> > 
> > Using a gfx::NativeWindow may work. One problem is that we may not have a
> window
> > reference until the dialog is shown.
> > 
> > Taking a step back, do you actually need this at all? Could you just use
> > content::WebContents and WebContentsModalDialog to track the dialog instead?
> 
> Hi Sailesh,
> 
> The WebContentsModalDialog interface is going away as part of the API
refactor,
> so I can't use it to reference the dialogs.  As I understand, the goal of the
> refactoring is to be able to present arbitrary UI views in either a tab-modal
or
> non-tab-modal fashion, so we won't have any special interface for the dialogs.

> (Does this new approach change the way you think about the web contents modal
> dialog display on Mac at all?)
> 
> We support multiple modal dialogs on the same WebContents at the same time, so
> the WebContents pointer isn't suitable as a unique identifier.
> 
> To satisfy the API that Ben outlined in
> http://code.google.com/p/chromium/issues/detail?id=157161#c5, we'll need the
> identifier to be available before the dialog is shown, so it sounds like that
> rules out gfx::NativeWindow.
> 
> The remaining options I'm aware of are:
>  - use a dummy gfx::NativeWindow or gfx::NativeView in Cocoa, as in this CL
>  - create a new opaque gfx native typedef that can represent a
> ConstrainedWindowSheet and something else analogous on all the other platforms
> 
> What do you think?  Can you think of any other options?  I'm loathe to
introduce
> a new gfx typedef unless we're likely to need to reference types like this on
a
> widespread basis.

Hi Mike. How about just a void*? On Windows you can return a gfx::NativeWinodw.
On Mac we can return a ConstrainedWindowSheet instance.

[Mike Wittman, 2013-02-06 21:37:56]

On 2013/02/06 20:12:11, sail wrote:
> On 2013/02/06 19:31:04, Mike Wittman wrote:
> > On 2013/02/06 18:42:57, sail wrote:
> > > Hi Mike.
> > > 
> > > Currently I have three ways to lookup or identify an instance of a
> constrained
> > > window:
> > >   - pointer to a content::WebContents that the dialog is attached to
> > >   - pointer to a WebContentsModalDialog
> > >   - pointer to a ConstrainedWindowSheet (a custom Objective-C class)
> > > 
> > > Using a gfx::NativeView probably won't work. In many cases we don't have a
> > > unique view reference to track the dialog with.
> > > 
> > > Using a gfx::NativeWindow may work. One problem is that we may not have a
> > window
> > > reference until the dialog is shown.
> > > 
> > > Taking a step back, do you actually need this at all? Could you just use
> > > content::WebContents and WebContentsModalDialog to track the dialog
instead?
> > 
> > Hi Sailesh,
> > 
> > The WebContentsModalDialog interface is going away as part of the API
> refactor,
> > so I can't use it to reference the dialogs.  As I understand, the goal of
the
> > refactoring is to be able to present arbitrary UI views in either a
tab-modal
> or
> > non-tab-modal fashion, so we won't have any special interface for the
dialogs.
> 
> > (Does this new approach change the way you think about the web contents
modal
> > dialog display on Mac at all?)
> > 
> > We support multiple modal dialogs on the same WebContents at the same time,
so
> > the WebContents pointer isn't suitable as a unique identifier.
> > 
> > To satisfy the API that Ben outlined in
> > http://code.google.com/p/chromium/issues/detail?id=157161#c5, we'll need the
> > identifier to be available before the dialog is shown, so it sounds like
that
> > rules out gfx::NativeWindow.
> > 
> > The remaining options I'm aware of are:
> >  - use a dummy gfx::NativeWindow or gfx::NativeView in Cocoa, as in this CL
> >  - create a new opaque gfx native typedef that can represent a
> > ConstrainedWindowSheet and something else analogous on all the other
platforms
> > 
> > What do you think?  Can you think of any other options?  I'm loathe to
> introduce
> > a new gfx typedef unless we're likely to need to reference types like this
on
> a
> > widespread basis.
> 
> Hi Mike. How about just a void*? On Windows you can return a
gfx::NativeWinodw.
> On Mac we can return a ConstrainedWindowSheet instance.

I guess this would be equivalent to a custom typedef for the web contents modal
dialog?  Something like:

#if defined(OS_MACOSX)
typedef void* NativeWebContentsModalDialog;
#else
typedef gfx::NativeWindow NativeWebContentsModalDialog;
#endif

with interfaces like:

// WebContentsModalDialogManager:
void ShowDialog(NativeWebContentsModalDialog dialog);

It doesn't get rid of the custom modal dialog type, but at least reduces it to a
minimal abstraction.

Ben, what do you think?

[sail, 2013-02-06 21:43:30]

On 2013/02/06 21:37:56, Mike Wittman wrote:
> On 2013/02/06 20:12:11, sail wrote:
> > On 2013/02/06 19:31:04, Mike Wittman wrote:
> > > On 2013/02/06 18:42:57, sail wrote:
> > > > Hi Mike.
> > > > 
> > > > Currently I have three ways to lookup or identify an instance of a
> > constrained
> > > > window:
> > > >   - pointer to a content::WebContents that the dialog is attached to
> > > >   - pointer to a WebContentsModalDialog
> > > >   - pointer to a ConstrainedWindowSheet (a custom Objective-C class)
> > > > 
> > > > Using a gfx::NativeView probably won't work. In many cases we don't have
a
> > > > unique view reference to track the dialog with.
> > > > 
> > > > Using a gfx::NativeWindow may work. One problem is that we may not have
a
> > > window
> > > > reference until the dialog is shown.
> > > > 
> > > > Taking a step back, do you actually need this at all? Could you just use
> > > > content::WebContents and WebContentsModalDialog to track the dialog
> instead?
> > > 
> > > Hi Sailesh,
> > > 
> > > The WebContentsModalDialog interface is going away as part of the API
> > refactor,
> > > so I can't use it to reference the dialogs.  As I understand, the goal of
> the
> > > refactoring is to be able to present arbitrary UI views in either a
> tab-modal
> > or
> > > non-tab-modal fashion, so we won't have any special interface for the
> dialogs.
> > 
> > > (Does this new approach change the way you think about the web contents
> modal
> > > dialog display on Mac at all?)
> > > 
> > > We support multiple modal dialogs on the same WebContents at the same
time,
> so
> > > the WebContents pointer isn't suitable as a unique identifier.
> > > 
> > > To satisfy the API that Ben outlined in
> > > http://code.google.com/p/chromium/issues/detail?id=157161#c5, we'll need
the
> > > identifier to be available before the dialog is shown, so it sounds like
> that
> > > rules out gfx::NativeWindow.
> > > 
> > > The remaining options I'm aware of are:
> > >  - use a dummy gfx::NativeWindow or gfx::NativeView in Cocoa, as in this
CL
> > >  - create a new opaque gfx native typedef that can represent a
> > > ConstrainedWindowSheet and something else analogous on all the other
> platforms
> > > 
> > > What do you think?  Can you think of any other options?  I'm loathe to
> > introduce
> > > a new gfx typedef unless we're likely to need to reference types like this
> on
> > a
> > > widespread basis.
> > 
> > Hi Mike. How about just a void*? On Windows you can return a
> gfx::NativeWinodw.
> > On Mac we can return a ConstrainedWindowSheet instance.
> 
> I guess this would be equivalent to a custom typedef for the web contents
modal
> dialog?  Something like:
> 
> #if defined(OS_MACOSX)
> typedef void* NativeWebContentsModalDialog;
> #else
> typedef gfx::NativeWindow NativeWebContentsModalDialog;
> #endif
> 
> with interfaces like:
> 
> // WebContentsModalDialogManager:
> void ShowDialog(NativeWebContentsModalDialog dialog);
> 
> It doesn't get rid of the custom modal dialog type, but at least reduces it to
a
> minimal abstraction.
> 
> Ben, what do you think?

How about getting rid of the ifdef and just doing:
  typedef void* NativeWebContentsModalDialog;

[Mike Wittman, 2013-02-06 21:56:02]

On 2013/02/06 21:43:30, sail wrote:
> On 2013/02/06 21:37:56, Mike Wittman wrote:
> > On 2013/02/06 20:12:11, sail wrote:
> > > On 2013/02/06 19:31:04, Mike Wittman wrote:
> > > > On 2013/02/06 18:42:57, sail wrote:
> > > > > Hi Mike.
> > > > > 
> > > > > Currently I have three ways to lookup or identify an instance of a
> > > constrained
> > > > > window:
> > > > >   - pointer to a content::WebContents that the dialog is attached to
> > > > >   - pointer to a WebContentsModalDialog
> > > > >   - pointer to a ConstrainedWindowSheet (a custom Objective-C class)
> > > > > 
> > > > > Using a gfx::NativeView probably won't work. In many cases we don't
have
> a
> > > > > unique view reference to track the dialog with.
> > > > > 
> > > > > Using a gfx::NativeWindow may work. One problem is that we may not
have
> a
> > > > window
> > > > > reference until the dialog is shown.
> > > > > 
> > > > > Taking a step back, do you actually need this at all? Could you just
use
> > > > > content::WebContents and WebContentsModalDialog to track the dialog
> > instead?
> > > > 
> > > > Hi Sailesh,
> > > > 
> > > > The WebContentsModalDialog interface is going away as part of the API
> > > refactor,
> > > > so I can't use it to reference the dialogs.  As I understand, the goal
of
> > the
> > > > refactoring is to be able to present arbitrary UI views in either a
> > tab-modal
> > > or
> > > > non-tab-modal fashion, so we won't have any special interface for the
> > dialogs.
> > > 
> > > > (Does this new approach change the way you think about the web contents
> > modal
> > > > dialog display on Mac at all?)
> > > > 
> > > > We support multiple modal dialogs on the same WebContents at the same
> time,
> > so
> > > > the WebContents pointer isn't suitable as a unique identifier.
> > > > 
> > > > To satisfy the API that Ben outlined in
> > > > http://code.google.com/p/chromium/issues/detail?id=157161#c5, we'll need
> the
> > > > identifier to be available before the dialog is shown, so it sounds like
> > that
> > > > rules out gfx::NativeWindow.
> > > > 
> > > > The remaining options I'm aware of are:
> > > >  - use a dummy gfx::NativeWindow or gfx::NativeView in Cocoa, as in this
> CL
> > > >  - create a new opaque gfx native typedef that can represent a
> > > > ConstrainedWindowSheet and something else analogous on all the other
> > platforms
> > > > 
> > > > What do you think?  Can you think of any other options?  I'm loathe to
> > > introduce
> > > > a new gfx typedef unless we're likely to need to reference types like
this
> > on
> > > a
> > > > widespread basis.
> > > 
> > > Hi Mike. How about just a void*? On Windows you can return a
> > gfx::NativeWinodw.
> > > On Mac we can return a ConstrainedWindowSheet instance.
> > 
> > I guess this would be equivalent to a custom typedef for the web contents
> modal
> > dialog?  Something like:
> > 
> > #if defined(OS_MACOSX)
> > typedef void* NativeWebContentsModalDialog;
> > #else
> > typedef gfx::NativeWindow NativeWebContentsModalDialog;
> > #endif
> > 
> > with interfaces like:
> > 
> > // WebContentsModalDialogManager:
> > void ShowDialog(NativeWebContentsModalDialog dialog);
> > 
> > It doesn't get rid of the custom modal dialog type, but at least reduces it
to
> a
> > minimal abstraction.
> > 
> > Ben, what do you think?
> 
> How about getting rid of the ifdef and just doing:
>   typedef void* NativeWebContentsModalDialog;

Either would work.  The ifdef approach has the benefit providing a more natural
type on non-Mac platforms and compile-time type checking, at the expense of
being ugly.

[sail, 2013-02-06 22:02:13]

On 2013/02/06 21:56:02, Mike Wittman wrote:
> On 2013/02/06 21:43:30, sail wrote:
> > On 2013/02/06 21:37:56, Mike Wittman wrote:
> > > On 2013/02/06 20:12:11, sail wrote:
> > > > On 2013/02/06 19:31:04, Mike Wittman wrote:
> > > > > On 2013/02/06 18:42:57, sail wrote:
> > > > > > Hi Mike.
> > > > > > 
> > > > > > Currently I have three ways to lookup or identify an instance of a
> > > > constrained
> > > > > > window:
> > > > > >   - pointer to a content::WebContents that the dialog is attached to
> > > > > >   - pointer to a WebContentsModalDialog
> > > > > >   - pointer to a ConstrainedWindowSheet (a custom Objective-C class)
> > > > > > 
> > > > > > Using a gfx::NativeView probably won't work. In many cases we don't
> have
> > a
> > > > > > unique view reference to track the dialog with.
> > > > > > 
> > > > > > Using a gfx::NativeWindow may work. One problem is that we may not
> have
> > a
> > > > > window
> > > > > > reference until the dialog is shown.
> > > > > > 
> > > > > > Taking a step back, do you actually need this at all? Could you just
> use
> > > > > > content::WebContents and WebContentsModalDialog to track the dialog
> > > instead?
> > > > > 
> > > > > Hi Sailesh,
> > > > > 
> > > > > The WebContentsModalDialog interface is going away as part of the API
> > > > refactor,
> > > > > so I can't use it to reference the dialogs.  As I understand, the goal
> of
> > > the
> > > > > refactoring is to be able to present arbitrary UI views in either a
> > > tab-modal
> > > > or
> > > > > non-tab-modal fashion, so we won't have any special interface for the
> > > dialogs.
> > > > 
> > > > > (Does this new approach change the way you think about the web
contents
> > > modal
> > > > > dialog display on Mac at all?)
> > > > > 
> > > > > We support multiple modal dialogs on the same WebContents at the same
> > time,
> > > so
> > > > > the WebContents pointer isn't suitable as a unique identifier.
> > > > > 
> > > > > To satisfy the API that Ben outlined in
> > > > > http://code.google.com/p/chromium/issues/detail?id=157161#c5, we'll
need
> > the
> > > > > identifier to be available before the dialog is shown, so it sounds
like
> > > that
> > > > > rules out gfx::NativeWindow.
> > > > > 
> > > > > The remaining options I'm aware of are:
> > > > >  - use a dummy gfx::NativeWindow or gfx::NativeView in Cocoa, as in
this
> > CL
> > > > >  - create a new opaque gfx native typedef that can represent a
> > > > > ConstrainedWindowSheet and something else analogous on all the other
> > > platforms
> > > > > 
> > > > > What do you think?  Can you think of any other options?  I'm loathe to
> > > > introduce
> > > > > a new gfx typedef unless we're likely to need to reference types like
> this
> > > on
> > > > a
> > > > > widespread basis.
> > > > 
> > > > Hi Mike. How about just a void*? On Windows you can return a
> > > gfx::NativeWinodw.
> > > > On Mac we can return a ConstrainedWindowSheet instance.
> > > 
> > > I guess this would be equivalent to a custom typedef for the web contents
> > modal
> > > dialog?  Something like:
> > > 
> > > #if defined(OS_MACOSX)
> > > typedef void* NativeWebContentsModalDialog;
> > > #else
> > > typedef gfx::NativeWindow NativeWebContentsModalDialog;
> > > #endif
> > > 
> > > with interfaces like:
> > > 
> > > // WebContentsModalDialogManager:
> > > void ShowDialog(NativeWebContentsModalDialog dialog);
> > > 
> > > It doesn't get rid of the custom modal dialog type, but at least reduces
it
> to
> > a
> > > minimal abstraction.
> > > 
> > > Ben, what do you think?
> > 
> > How about getting rid of the ifdef and just doing:
> >   typedef void* NativeWebContentsModalDialog;
> 
> Either would work.  The ifdef approach has the benefit providing a more
natural
> type on non-Mac platforms and compile-time type checking, at the expense of
> being ugly.

I don't think the benefit of type checking is worth it. It's basically a opaque
type.

[Mike Wittman, 2013-02-07 00:36:17]

I've posted a new review for this based on the approach we've discussed in the
comments: https://codereview.chromium.org/12221055