MacViews: Enable views based External Protocol dialog behind secondary-ui-md flag.

MacViews: Enable views based External Protocol dialog behind secondary-ui-md flag.

This CL puts the views based External Protocol dialog behind the secondary-ui-md
flag. As a result, the views based version is shown with the flag "secondary-ui-
md" enabled and the Cocoa version is shown without it.

BUG=662128, 652508
Review-Url: https://codereview.chromium.org/2632653002
Cr-Commit-Position: refs/heads/master@{#445688}
Committed: https://chromium.googlesource.com/chromium/src/+/c8473e8aa7a2b5b140bd5e6c77f8abab98adb539

[karandeepb, 2017-01-16 09:38:09]

Patchset #1 (id:1) has been deleted

[karandeepb, 2017-01-16 09:40:10]

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

[commit-bot: I haz the power, 2017-01-16 09:40:19]

Dry run: 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, 2017-01-16 09:45:49]

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

[commit-bot: I haz the power, 2017-01-16 09:45:49]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[karandeepb, 2017-01-16 10:30:50]

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

[commit-bot: I haz the power, 2017-01-16 10:31:01]

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

[karandeepb, 2017-01-16 10:51:13]

Description was changed from

==========
MacViews: Hook up external protocol request dialog.
==========

to

==========
MacViews: Enable views based External Protocol dialog behind secondary-ui-md
flag.

This CL puts the views based External Protocol Dialog behind the secondary-ui-md
flag. As a result, the views based version is shown with the flag "secondary-ui-
md" enabled and the Cocoa version is shown without it.

BUG=662128
==========

[karandeepb, 2017-01-16 11:13:10]

karandeepb@chromium.org changed reviewers:
+ tapted@chromium.org

[karandeepb, 2017-01-16 11:13:11]

PTAL Trent. Tested cocoa browser with and w/o the flag and the
mac_views_browser. The dialog seemed to run fine. Any other things to test?

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/external_protocol_dialog.cc (right):

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/external_protocol_dialog.cc:30: void
RunExternalProtocolDialogViews(const GURL& url,
I am assuming it's fine to name the chromeos version with the suffix "Views".

[commit-bot: I haz the power, 2017-01-16 11:22:21]

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

[commit-bot: I haz the power, 2017-01-16 11:22:22]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-01-17 21:17:13]

Can you add a screenshot somewhere? http://crbug.com/652508 perhaps (and link
that bug).

Also, BUILDFLAG(MAC_VIEWS_BROWSER) can make some of this stuff neater now. i.e.
we need to consider the churn when we "unwind" changes like this since
MAC_VIEWS_BROWSER has become default -- see suggestions below

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/external_protocol_dialog.cc (right):

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/external_protocol_dialog.cc:30: void
RunExternalProtocolDialogViews(const GURL& url,
On 2017/01/16 11:13:10, karandeepb wrote:
> I am assuming it's fine to name the chromeos version with the suffix "Views".

seems reasonable.. but I think we can avoid this change with
BUILDFLAG(MAC_VIEWS_BROWSER) - see below

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:1551: "views/external_protocol_dialog.cc",
Can external_protocol_dialog_browsertest be switched on in chrome/test/BUILD.gn?

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:1555: if (!is_mac || mac_views_browser) {
nesting this also with toolkit_views might be a bit much (and, e.g., there are
two levels checking platform - is_chromeos, is_mac). Perhaps

if (!is_chromeos && (!is_mac || mac_views_browser)) ?

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/brows...
chrome/browser/ui/browser_dialogs.h:155: void
RunExternalProtocolDialogViews(const GURL& url,
Does this need to be here, or can it be a data member of ExternalProtocolHandler
as well?

(maybe BUILDFLAG(MAC_VIEWS_BROWSER) will help do this now)

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/external_protocol_dialog_cocoa.mm (right):

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/external_protocol_dialog_cocoa.mm:24: void
ExternalProtocolHandler::RunExternalProtocolDialog(
I think we can move this definition to a new file
external_protocol_dialog_views_mac.mm - that way we don't have to bounce off
browser_dialogs

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/external_protocol_dialog.cc (right):

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog.cc:32: void
RunExternalProtocolDialogViews(const GURL& url,
consider wrapping this entire method in

#if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER)

then duplicating what's needed in external_protocol_dialog_views_mac.mm (new
file)

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog.cc:40: if
(delegate->program_name().empty()) {
does this mean the same thing on Mac? (i.e. is this check needed -- how does Mac
do this same check currently?

[karandeepb, 2017-01-18 03:30:54]

Description was changed from

==========
MacViews: Enable views based External Protocol dialog behind secondary-ui-md
flag.

This CL puts the views based External Protocol Dialog behind the secondary-ui-md
flag. As a result, the views based version is shown with the flag "secondary-ui-
md" enabled and the Cocoa version is shown without it.

BUG=662128
==========

to

==========
MacViews: Enable views based External Protocol dialog behind secondary-ui-md
flag.

This CL puts the views based External Protocol Dialog behind the secondary-ui-md
flag. As a result, the views based version is shown with the flag "secondary-ui-
md" enabled and the Cocoa version is shown without it.

BUG=662128, 652508
==========

[karandeepb, 2017-01-19 03:59:43]

Before I make the change, seeing the existing code, I think we wanted to house
all the dialog launching functions in browser_dialogs.h. 
This also allows us to avoid repeating the #if!defined(OS_MACOSX) ||
BUILDFLAG(MAC_VIEWS_BROWSER) in code.

So for adding new dialogs to secondary-ui-md, what approach do we want to use?

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/brows...
chrome/browser/ui/browser_dialogs.h:155: void
RunExternalProtocolDialogViews(const GURL& url,
On 2017/01/17 21:17:13, tapted wrote:
> Does this need to be here, or can it be a data member of
ExternalProtocolHandler
> as well?
> 
> (maybe BUILDFLAG(MAC_VIEWS_BROWSER) will help do this now)

It can be a static member function. Don't think it should be public though,
since it's an implementation detail. Maybe a private static function. Although
this is similar to how we do this for other dialogs.

[tapted, 2017-01-20 05:53:44]

On 2017/01/19 03:59:43, karandeepb wrote:
> Before I make the change, seeing the existing code, I think we wanted to house
> all the dialog launching functions in browser_dialogs.h. 
> This also allows us to avoid repeating the #if!defined(OS_MACOSX) ||
> BUILDFLAG(MAC_VIEWS_BROWSER) in code.
> 
> So for adding new dialogs to secondary-ui-md, what approach do we want to use?

We should use whatever makes the minimal "churn" that we later need to undo. See
https://codereview.chromium.org/2586373003 for an example. Nearly all the
changes are under c/b/ui/cocoa, which we can just delete rather than having to
undo changes.

> 
>
https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/brows...
> File chrome/browser/ui/browser_dialogs.h (right):
> 
>
https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/brows...
> chrome/browser/ui/browser_dialogs.h:155: void
> RunExternalProtocolDialogViews(const GURL& url,
> On 2017/01/17 21:17:13, tapted wrote:
> > Does this need to be here, or can it be a data member of
> ExternalProtocolHandler
> > as well?
> > 
> > (maybe BUILDFLAG(MAC_VIEWS_BROWSER) will help do this now)
> 
> It can be a static member function. Don't think it should be public though,
> since it's an implementation detail. Maybe a private static function. Although
> this is similar to how we do this for other dialogs.

[tapted, 2017-01-20 05:53:52]

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/brows...
chrome/browser/ui/browser_dialogs.h:155: void
RunExternalProtocolDialogViews(const GURL& url,
On 2017/01/19 03:59:43, karandeepb wrote:
> On 2017/01/17 21:17:13, tapted wrote:
> > Does this need to be here, or can it be a data member of
> ExternalProtocolHandler
> > as well?
> > 
> > (maybe BUILDFLAG(MAC_VIEWS_BROWSER) will help do this now)
> 
> It can be a static member function. Don't think it should be public though,
> since it's an implementation detail. Maybe a private static function. Although
> this is similar to how we do this for other dialogs.

ExternalProtocolDialog currently uses static class functions - we should stick
with that

[karandeepb, 2017-01-20 08:09:40]

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

[commit-bot: I haz the power, 2017-01-20 08:09:56]

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

[karandeepb, 2017-01-23 00:54:38]

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

[commit-bot: I haz the power, 2017-01-23 00:54:50]

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

[karandeepb, 2017-01-23 00:59:31]

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

[commit-bot: I haz the power, 2017-01-23 00:59:47]

Dry run: 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, 2017-01-23 01:17:38]

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

[commit-bot: I haz the power, 2017-01-23 01:17:39]

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

[karandeepb, 2017-01-23 02:20:04]

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

[commit-bot: I haz the power, 2017-01-23 02:20:17]

Dry run: 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, 2017-01-23 02:32:57]

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

[commit-bot: I haz the power, 2017-01-23 02:32:57]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[karandeepb, 2017-01-23 02:54:12]

Description was changed from

==========
MacViews: Enable views based External Protocol dialog behind secondary-ui-md
flag.

This CL puts the views based External Protocol Dialog behind the secondary-ui-md
flag. As a result, the views based version is shown with the flag "secondary-ui-
md" enabled and the Cocoa version is shown without it.

BUG=662128, 652508
==========

to

==========
MacViews: Enable views based Download Recovery dialog behind secondary-ui-md
flag.

This CL puts the views based Download Recovery dialog behind the secondary-ui-md
flag. As a result, the views based version is shown with the flag "secondary-ui-
md" enabled and the Cocoa version is shown without it.

BUG=662128, 654142
==========

[karandeepb, 2017-01-23 02:55:37]

Description was changed from

==========
MacViews: Enable views based Download Recovery dialog behind secondary-ui-md
flag.

This CL puts the views based Download Recovery dialog behind the secondary-ui-md
flag. As a result, the views based version is shown with the flag "secondary-ui-
md" enabled and the Cocoa version is shown without it.

BUG=662128, 654142
==========

to

==========
MacViews: Enable views based External Protocol dialog behind secondary-ui-md
flag.

This CL puts the views based External Protocol dialog behind the secondary-ui-md
flag. As a result, the views based version is shown with the flag "secondary-ui-
md" enabled and the Cocoa version is shown without it.

BUG=662128, 652508
==========

[karandeepb, 2017-01-23 03:07:30]

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

[commit-bot: I haz the power, 2017-01-23 03:07:38]

Dry run: 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, 2017-01-23 03:22:52]

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

[commit-bot: I haz the power, 2017-01-23 03:22:53]

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

[karandeepb, 2017-01-23 03:45:15]

Patchset #6 (id:120001) has been deleted

[karandeepb, 2017-01-23 03:45:23]

Patchset #6 (id:140001) has been deleted

[karandeepb, 2017-01-23 03:45:31]

Patchset #7 (id:180001) has been deleted

[karandeepb, 2017-01-23 03:45:38]

Patchset #7 (id:200001) has been deleted

[karandeepb, 2017-01-23 03:45:45]

Patchset #6 (id:160001) has been deleted

[karandeepb, 2017-01-23 04:08:19]

Patchset #6 (id:220001) has been deleted

[karandeepb, 2017-01-23 04:08:36]

Patchset #5 (id:100001) has been deleted

[karandeepb, 2017-01-23 04:13:41]

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

[commit-bot: I haz the power, 2017-01-23 04:13:58]

Dry run: 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, 2017-01-23 05:14:42]

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

[commit-bot: I haz the power, 2017-01-23 05:14:44]

Dry run: This issue passed the CQ dry run.

[karandeepb, 2017-01-23 09:07:32]

Patchset #5 (id:240001) has been deleted

[karandeepb, 2017-01-23 09:07:39]

Patchset #5 (id:260001) has been deleted

[karandeepb, 2017-01-24 02:12:00]

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

[commit-bot: I haz the power, 2017-01-24 02:12:55]

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

[karandeepb, 2017-01-24 03:23:32]

PTAL Trent.

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/external_protocol_dialog.cc (right):

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/external_protocol_dialog.cc:30: void
RunExternalProtocolDialogViews(const GURL& url,
On 2017/01/17 21:17:13, tapted wrote:
> On 2017/01/16 11:13:10, karandeepb wrote:
> > I am assuming it's fine to name the chromeos version with the suffix
"Views".
> 
> seems reasonable.. but I think we can avoid this change with
> BUILDFLAG(MAC_VIEWS_BROWSER) - see below

Done.

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:1551: "views/external_protocol_dialog.cc",
On 2017/01/17 21:17:13, tapted wrote:
> Can external_protocol_dialog_browsertest be switched on in
chrome/test/BUILD.gn?

Done.

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:1555: if (!is_mac || mac_views_browser) {
On 2017/01/17 21:17:13, tapted wrote:
> nesting this also with toolkit_views might be a bit much (and, e.g., there are
> two levels checking platform - is_chromeos, is_mac). Perhaps
> 
> if (!is_chromeos && (!is_mac || mac_views_browser)) ?

Done.

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/external_protocol_dialog_cocoa.mm (right):

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/external_protocol_dialog_cocoa.mm:24: void
ExternalProtocolHandler::RunExternalProtocolDialog(
On 2017/01/17 21:17:13, tapted wrote:
> I think we can move this definition to a new file
> external_protocol_dialog_views_mac.mm - that way we don't have to bounce off
> browser_dialogs

Done.

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/external_protocol_dialog.cc (right):

https://codereview.chromium.org/2632653002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/external_protocol_dialog.cc:40: if
(delegate->program_name().empty()) {
On 2017/01/17 21:17:13, tapted wrote:
> does this mean the same thing on Mac? (i.e. is this check needed -- how does
Mac
> do this same check currently?

Mac checks this here
https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/external_protoco...

[tapted, 2017-01-24 04:02:43]

Is it easy to invoke? (Can you add instructions on http://crbug.com/652508 ).
Otherwise, we should make a DialogBrowserTest for this, but the harness probably
doesn't support this dialog type yet

https://codereview.chromium.org/2632653002/diff/340001/chrome/browser/ui/BUIL...
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2632653002/diff/340001/chrome/browser/ui/BUIL...
chrome/browser/ui/BUILD.gn:1963: if (is_win || is_mac || is_desktop_linux) {
I think this is the right place for the files. (the whole thing could do with a
cleanup, but we should try to re-use existing blocks if possible)

https://codereview.chromium.org/2632653002/diff/340001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/external_protocol_dialog_views_mac.mm (right):

https://codereview.chromium.org/2632653002/diff/340001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/external_protocol_dialog_views_mac.mm:5: #include
"chrome/browser/external_protocol/external_protocol_handler.h"
I would have expected a presubmit to warn against this, I think only 

chrome/browser/ui/cocoa/external_protocol_dialog_[^_.]*.h

should be permitted as the separated-out header

https://codereview.chromium.org/2632653002/diff/340001/chrome/browser/ui/view...
File chrome/browser/ui/views/external_protocol_dialog.cc (right):

https://codereview.chromium.org/2632653002/diff/340001/chrome/browser/ui/view...
chrome/browser/ui/views/external_protocol_dialog.cc:53: #endif
nit: // !OS_MACOSX || MAC_VIEWS_BROWSER

https://codereview.chromium.org/2632653002/diff/340001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2632653002/diff/340001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:2360: sources -= [
it would be nice to be consistent... I suppose this is the most correct section
to duplicate the browsertest file

[karandeepb, 2017-01-24 06:06:31]

Patchset #9 (id:360001) has been deleted

[karandeepb, 2017-01-24 06:08:21]

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

[commit-bot: I haz the power, 2017-01-24 06:08:46]

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

[karandeepb, 2017-01-24 06:12:24]

>>Is it easy to invoke? (Can you add instructions on http://crbug.com/652508 ).
>>Otherwise, we should make a DialogBrowserTest for this, but the harness
probably
>>doesn't support this dialog type yet

Added instructions on the bug report.

Is the native Cocoa dialog an NSAlert? 
Differences with the Views dialog-

It doesn't respond to Esc
Doesn't center with the host window
Is window modal (Views is tab modal)
On Canary, it does not even allow opening a new tab (on Stable it does), which
may even be a regression of some sort.

https://codereview.chromium.org/2632653002/diff/340001/chrome/browser/ui/BUIL...
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/2632653002/diff/340001/chrome/browser/ui/BUIL...
chrome/browser/ui/BUILD.gn:1963: if (is_win || is_mac || is_desktop_linux) {
On 2017/01/24 04:02:43, tapted wrote:
> I think this is the right place for the files. (the whole thing could do with
a
> cleanup, but we should try to re-use existing blocks if possible)

Done. Toolkit views is defined only on all desktop platforms currently right?

https://codereview.chromium.org/2632653002/diff/340001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/external_protocol_dialog_views_mac.mm (right):

https://codereview.chromium.org/2632653002/diff/340001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/external_protocol_dialog_views_mac.mm:5: #include
"chrome/browser/external_protocol/external_protocol_handler.h"
On 2017/01/24 04:02:43, tapted wrote:
> I would have expected a presubmit to warn against this, I think only 
> 
> chrome/browser/ui/cocoa/external_protocol_dialog_[^_.]*.h
> 
> should be permitted as the separated-out header

I was following the example in external_protocol_android.cc -
https://cs.chromium.org/chromium/src/chrome/browser/ui/android/external_proto....


This seems to be allowed by the google style guide as well. Quoting:
"In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or test
the stuff in dir2/foo2.h, order your includes as follows:

dir2/foo2.h.
C system files.
C++ system files.
Google .h files.

dir/foo.cc and dir2/foo2.h are usually in the same directory (e.g.
base/basictypes_test.cc and base/basictypes.h), but may sometimes be in
different directories too (e.g. testing/base/internal/gmock-cardinalities.cc and
testing/base/public/gmock-cardinalities.h).
"

from
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#....

https://codereview.chromium.org/2632653002/diff/340001/chrome/browser/ui/view...
File chrome/browser/ui/views/external_protocol_dialog.cc (right):

https://codereview.chromium.org/2632653002/diff/340001/chrome/browser/ui/view...
chrome/browser/ui/views/external_protocol_dialog.cc:34: #if !defined(OS_MACOSX)
|| BUILDFLAG(MAC_VIEWS_BROWSER)
Should we think of having 

#define COCOA_BROWSER #defined(OS_MACOSX) && !BUILDFLAG(MAC_VIEWS_BROWSER)
somewhere

as this pattern becomes more common.

https://codereview.chromium.org/2632653002/diff/340001/chrome/browser/ui/view...
chrome/browser/ui/views/external_protocol_dialog.cc:53: #endif
On 2017/01/24 04:02:43, tapted wrote:
> nit: // !OS_MACOSX || MAC_VIEWS_BROWSER

Done.

https://codereview.chromium.org/2632653002/diff/340001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2632653002/diff/340001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:2360: sources -= [
On 2017/01/24 04:02:43, tapted wrote:
> it would be nice to be consistent... I suppose this is the most correct
section
> to duplicate the browsertest file

Done.

[tapted, 2017-01-24 06:24:20]

lgtm

[karandeepb, 2017-01-24 06:29:34]

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

[karandeepb, 2017-01-24 06:29:34]

PTAL msw@ for chrome/browser/ui/views/external_protocol_dialog.cc.

[commit-bot: I haz the power, 2017-01-24 07:28:15]

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

[commit-bot: I haz the power, 2017-01-24 07:28:16]

Dry run: This issue passed the CQ dry run.

[msw, 2017-01-24 08:39:37]

lgtm with a minor comment

https://codereview.chromium.org/2632653002/diff/380001/chrome/browser/ui/view...
File chrome/browser/ui/views/external_protocol_dialog.cc (right):

https://codereview.chromium.org/2632653002/diff/380001/chrome/browser/ui/view...
chrome/browser/ui/views/external_protocol_dialog.cc:35: // This should be kept
in sync with RunExternalProtocolDialogViews in
It'd be nice to avoid this duplication, is it tough to avoid that?

[karandeepb, 2017-01-24 08:43:57]

https://codereview.chromium.org/2632653002/diff/380001/chrome/browser/ui/view...
File chrome/browser/ui/views/external_protocol_dialog.cc (right):

https://codereview.chromium.org/2632653002/diff/380001/chrome/browser/ui/view...
chrome/browser/ui/views/external_protocol_dialog.cc:35: // This should be kept
in sync with RunExternalProtocolDialogViews in
On 2017/01/24 08:39:37, msw (vacation jan 24) wrote:
> It'd be nice to avoid this duplication, is it tough to avoid that?

It isn't actually. The rationale for this is that we want to restrict most of
the changes to chrome/browser/ui/cocoa so as to minimize the changes we need to
undo once we switch fully to MacViews.

[karandeepb, 2017-01-24 09:06:19]

The CQ bit was checked by karandeepb@chromium.org

[commit-bot: I haz the power, 2017-01-24 09:06:31]

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, 2017-01-24 09:12:32]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1485248779929790,
"parent_rev": "b7b0799fa40dd39cc93e826e6bacfea41231d989", "commit_rev":
"c8473e8aa7a2b5b140bd5e6c77f8abab98adb539"}

[commit-bot: I haz the power, 2017-01-24 09:13:06]

Description was changed from

==========
MacViews: Enable views based External Protocol dialog behind secondary-ui-md
flag.

This CL puts the views based External Protocol dialog behind the secondary-ui-md
flag. As a result, the views based version is shown with the flag "secondary-ui-
md" enabled and the Cocoa version is shown without it.

BUG=662128, 652508
==========

to

==========
MacViews: Enable views based External Protocol dialog behind secondary-ui-md
flag.

This CL puts the views based External Protocol dialog behind the secondary-ui-md
flag. As a result, the views based version is shown with the flag "secondary-ui-
md" enabled and the Cocoa version is shown without it.

BUG=662128, 652508

Review-Url: https://codereview.chromium.org/2632653002
Cr-Commit-Position: refs/heads/master@{#445688}
Committed:
https://chromium.googlesource.com/chromium/src/+/c8473e8aa7a2b5b140bd5e6c77f8...
==========

[commit-bot: I haz the power, 2017-01-24 09:13:07]

Committed patchset #9 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/c8473e8aa7a2b5b140bd5e6c77f8...