Use a uniform opaque type to represent web contents modal dialogs

Use a uniform opaque type to represent web contents modal dialogs

Since the WebContentsModalDialog interface is going away, the
platform-independent code in WebContentsModalDialogManager requires an
opaque type with which to identify dialogs.

In a first attempt at this under https://codereview.chromium.org/12224020,
comments indicated that the Mac implementation of the web contents
modal dialog does not fit any of the abstracted native types provided
by ui/gfx/native_widget_types.h.  So, define a special typedef
NativeWebContentsModalDialog that uses the existing gfx::NativeView
abstraction on platforms where supported, mainly to document the expected
type. On Mac, we use a void* which is the preferred type for this, per
Sailesh in comments in this review.

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

[Mike Wittman, 2013-02-07 00:34:41]

Second try on the the WCMDM opaque ID after feedback from the earlier review.
Note that this type will eventually appear in the WCMDM ShowDialog interface
function as specified in
http://code.google.com/p/chromium/issues/detail?id=157161#c5:

void ShowDialog(NativeWebContentsModalDialog dialog);

I've kept the platform-specific ifdef approach rather than a uniform typedef to
void*, mainly to document the expected types on each platform.

Ben, please review all but Cocoa.

Sailesh, please review chrome/browser/ui/native_web_contents_modal_dialog*,
chrome/browser/ui/web_contents_modal_dialog_manager* and the Cocoa changes.

[sail, 2013-02-07 00:46:08]

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

https://codereview.chromium.org/12221055/diff/1/chrome/browser/ui/native_web_...
chrome/browser/ui/native_web_contents_modal_dialog.h:13: @protocol
ConstrainedWindowSheet;
I don't think this would compile if it was included from a .cc file.

You could fix it by doing something like this:
#ifdef __OBJC__
typedef id<ConstrainedWindowSheet> NativeWebContentsModalDialog;
#else
class ConstrainedWindowSheet;
typedef ConstrainedWindowSheet* NativeWebContentsModalDialog;
#endif

Here's an example:
https://code.google.com/p/chromium/codesearch#chrome/src/ui/gfx/image/image_s...

If this class is never included from a .cc file on Mac then you could just do
something like this instead:

#if defined(OS_MACOSX)
#ifdef __OBJC__
...
#else
#error This header should be included from a .mm file.
#endif

[Mike Wittman, 2013-02-11 22:20:30]

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

https://codereview.chromium.org/12221055/diff/1/chrome/browser/ui/native_web_...
chrome/browser/ui/native_web_contents_modal_dialog.h:13: @protocol
ConstrainedWindowSheet;
I've updated the ifdefs based on the code in image_skia_util_mac.h.  I'm not
certain this will never be included in a .cc file on Mac.

[sail, 2013-02-11 22:26:21]

lgtm

[Mike Wittman, 2013-02-12 00:16:43]

Sailesh - it turns out the change in patch set 2 does not work as we're not able
to produce a ConstrainedWindowSheet* from an id<ConstrainedWindowSheet>.

Also, I verified that the header is included from .cc files on Mac, so we do
need to handle the !defined(__OBJC__) case.

Since this change is rapidly moving beyond my abilities in Objective C++, how
about we use a void* typedef on Mac for this CL (per latest patch), and Hoa can
update this to a more specific type in a future CL?

Sailesh and Hoa, are you OK with that?

[sail, 2013-02-12 00:20:58]

On 2013/02/12 00:16:43, Mike Wittman wrote:
> Sailesh - it turns out the change in patch set 2 does not work as we're not
able
> to produce a ConstrainedWindowSheet* from an id<ConstrainedWindowSheet>.
> 
> Also, I verified that the header is included from .cc files on Mac, so we do
> need to handle the !defined(__OBJC__) case.
> 
> Since this change is rapidly moving beyond my abilities in Objective C++, how
> about we use a void* typedef on Mac for this CL (per latest patch), and Hoa
can
> update this to a more specific type in a future CL?
> 
> Sailesh and Hoa, are you OK with that?

I actually prefer using void* actually! So new patch LGTM!

[Mike Wittman, 2013-02-13 18:59:21]

Hi Sailesh - can you take one more look at this? :)

Temporarily, I'd like to use the ConstrainedWindowMac pointer for the opaque
type on Mac.  The sheet pointer is the right thing to do in the long term, but
moving to that now will cause the platform-independent refactoring to be blocked
on Mac-specific changes.

Once we have the CL in, Hoa should be able to update this typedef on Mac  when
it makes sense, without affecting any other platforms.

[sail, 2013-02-13 19:01:41]

On 2013/02/13 18:59:21, Mike Wittman wrote:
> Hi Sailesh - can you take one more look at this? :)
> 
> Temporarily, I'd like to use the ConstrainedWindowMac pointer for the opaque
> type on Mac.  The sheet pointer is the right thing to do in the long term, but
> moving to that now will cause the platform-independent refactoring to be
blocked
> on Mac-specific changes.
> 
> Once we have the CL in, Hoa should be able to update this typedef on Mac  when
> it makes sense, without affecting any other platforms.

LGTM, maybe add a TODO?

[Ben Goodger (Google), 2013-02-13 23:27:01]

Hey Mike, one thing that isn't clear from your cl desc is why it's
necessary to use void* just for Mac. ?


On Wed, Feb 13, 2013 at 11:01 AM, <sail@chromium.org> wrote:

> On 2013/02/13 18:59:21, Mike Wittman wrote:
>
>> Hi Sailesh - can you take one more look at this? :)
>>
>
>  Temporarily, I'd like to use the ConstrainedWindowMac pointer for the
>> opaque
>> type on Mac.  The sheet pointer is the right thing to do in the long
>> term, but
>> moving to that now will cause the platform-independent refactoring to be
>>
> blocked
>
>> on Mac-specific changes.
>>
>
>  Once we have the CL in, Hoa should be able to update this typedef on Mac
>>  when
>> it makes sense, without affecting any other platforms.
>>
>
> LGTM, maybe add a TODO?
>
>
https://codereview.chromium.**org/12221055/<https://codereview.chromium.org/1...
>

[Mike Wittman, 2013-02-13 23:46:25]

On 2013/02/13 23:27:01, Ben Goodger (Google) wrote:
> Hey Mike, one thing that isn't clear from your cl desc is why it's
> necessary to use void* just for Mac. ?

I updated the description to hopefully clarify this question.

Also, subsequent development shows that we need to use gfx::NativeView on Gtk,
since the dialog is not contained in a gfx::NativeWindow on that platform.  I've
added another patch to go back to gfx::NativeView on non-Mac platforms.

[Ben Goodger (Google), 2013-02-14 23:24:32]

lgtm

https://codereview.chromium.org/12221055/diff/16001/chrome/browser/ui/native_...
File chrome/browser/ui/native_web_contents_modal_dialog.h (right):

https://codereview.chromium.org/12221055/diff/16001/chrome/browser/ui/native_...
chrome/browser/ui/native_web_contents_modal_dialog.h:11: // Use a void* since
none of the gfx::Native* types are suitable for
really? not even NativeView? why not?

[Ben Goodger (Google), 2013-02-14 23:25:19]

ignore the comments on that last part they must have been left in a cookie
from a previous session


On Thu, Feb 14, 2013 at 3:24 PM, <ben@chromium.org> wrote:

> lgtm
>
>
>
>
> https://codereview.chromium.**org/12221055/diff/16001/**
>
chrome/browser/ui/native_web_**contents_modal_dialog.h<https://codereview.chromium.org/12221055/diff/16001/chrome/browser/ui/native_web_contents_modal_dialog.h>
> File chrome/browser/ui/native_web_**contents_modal_dialog.h (right):
>
> https://codereview.chromium.**org/12221055/diff/16001/**
>
chrome/browser/ui/native_web_**contents_modal_dialog.h#**newcode11<https://codereview.chromium.org/12221055/diff/16001/chrome/browser/ui/native_web_contents_modal_dialog.h#newcode11>
> chrome/browser/ui/native_web_**contents_modal_dialog.h:11: // Use a void*
>
> since none of the gfx::Native* types are suitable for
> really? not even NativeView? why not?
>
>
https://codereview.chromium.**org/12221055/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-02-15 00:23:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/12221055/15003

[commit-bot: I haz the power, 2013-02-15 01:02:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Mike Wittman, 2013-02-15 20:03:16]

Hi Elliot, can you review patch set 9 on this CL?

The background is that I've made changes to WebContentsModalDialog to identify
the dialog by a gfx::NativeView rather than a gfx::NativeWindow.  This results
in a compilation error in the WebUI CertificateViewerDialog, which is compiled
on Linux but not used since Gtk has its own native CertificateViewerGtk.

I don't see a reason to use CertificateViewerDialog under Gtk before we move to
Views on Linux, so I'd like to just disable this in the build.

[Elliot Glaysher, 2013-02-15 22:01:43]

On 2013/02/15 20:03:16, Mike Wittman wrote:
> I don't see a reason to use CertificateViewerDialog under Gtk before we move
to
> Views on Linux, so I'd like to just disable this in the build.

I'm still confused about what's going on here. If there's an existing native gtk
dialog to show certificates, disabling whatever webui version we don't use is
fine. Am I correct that that's what you want to do?

Also, are there plans on porting embedded webui to views?

[Mike Wittman, 2013-02-15 22:24:38]

On 2013/02/15 22:01:43, Elliot Glaysher wrote:
> On 2013/02/15 20:03:16, Mike Wittman wrote:
> > I don't see a reason to use CertificateViewerDialog under Gtk before we move
> to
> > Views on Linux, so I'd like to just disable this in the build.
> 
> I'm still confused about what's going on here. If there's an existing native
gtk
> dialog to show certificates, disabling whatever webui version we don't use is
> fine. Am I correct that that's what you want to do?

Yes, that's correct.

> Also, are there plans on porting embedded webui to views?

I believe the WebUI dialog for this is already being used under Views on
ChromeOS.  (On Windows, Views uses a system-provided dialog.)

[Elliot Glaysher, 2013-02-15 22:29:44]

lgtm

On 2013/02/15 22:24:38, Mike Wittman wrote:
> On 2013/02/15 22:01:43, Elliot Glaysher wrote:
> > Also, are there plans on porting embedded webui to views?
> 
> I believe the WebUI dialog for this is already being used under Views on
> ChromeOS.  (On Windows, Views uses a system-provided dialog.)

What about desktop windows aura/desktop linux aura?

[Mike Wittman, 2013-02-15 23:18:43]

On 2013/02/15 22:29:44, Elliot Glaysher wrote:
> lgtm
> 
> On 2013/02/15 22:24:38, Mike Wittman wrote:
> > On 2013/02/15 22:01:43, Elliot Glaysher wrote:
> > > Also, are there plans on porting embedded webui to views?
> > 
> > I believe the WebUI dialog for this is already being used under Views on
> > ChromeOS.  (On Windows, Views uses a system-provided dialog.)
> 
> What about desktop windows aura/desktop linux aura?

I assume we will move to the WebUI dialog on Linux/Aura.  I'm not sure if we
will do the same for Windows/Aura or if we'll continue to use the Windows
dialog.

[commit-bot: I haz the power, 2013-02-15 23:26:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/12221055/26001

[commit-bot: I haz the power, 2013-02-16 00:56:35]

Change committed as 182878