MacViews: Enable views based Ask Google For Suggestions dialog behind secondary-ui-md flag.

MacViews: Enable views based Ask Google For Suggestions dialog behind secondary-ui-md flag.

This CL puts the views based Ask Google For Suggestions 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.

This CL also enables confirm_bubble_views_unittest.cc on Mac. Since there
doesn't seem to be any existing browser test for the dialog, a browser test
utilizing the DialogBrowserTest framework is also added.

BUG=662128, 654139
TEST=Run out/Default/browser_tests --gtest_filter=BrowserDialogTest.Invoke
--dialog=AskGoogleForSuggestionsDialogTest.InvokeDialog_default --interactive

Review-Url: https://codereview.chromium.org/2650583012
Cr-Commit-Position: refs/heads/master@{#447446}
Committed: https://chromium.googlesource.com/chromium/src/+/80b77aca94a93d7c54cb2b013f02469a6c4cf523

[karandeepb, 2017-01-25 05:24:49]

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

[commit-bot: I haz the power, 2017-01-25 05:25:09]

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-25 06:03:41]

Description was changed from

==========
Convert Ask Google for suggestions dialog.
==========

to

==========
MacViews: Enable views based Ask Google For Suggestions dialog behind
secondary-ui-md flag.

This CL puts the views based Ask Google For Suggestions 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.

This CL also enables confirm_bubble_views_unittest.cc on Mac. Since there
doesn't seem to be any existing browser test for the dialog, a browser test
utilizing the DialogBrowserTest framework is also added.

BUG=662128, 654139
==========

[karandeepb, 2017-01-25 06:15:54]

Description was changed from

==========
MacViews: Enable views based Ask Google For Suggestions dialog behind
secondary-ui-md flag.

This CL puts the views based Ask Google For Suggestions 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.

This CL also enables confirm_bubble_views_unittest.cc on Mac. Since there
doesn't seem to be any existing browser test for the dialog, a browser test
utilizing the DialogBrowserTest framework is also added.

BUG=662128, 654139
==========

to

==========
MacViews: Enable views based Ask Google For Suggestions dialog behind
secondary-ui-md flag.

This CL puts the views based Ask Google For Suggestions 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.

This CL also enables confirm_bubble_views_unittest.cc on Mac. Since there
doesn't seem to be any existing browser test for the dialog, a browser test
utilizing the DialogBrowserTest framework is also added.

BUG=662128, 654139
TEST=Run out/Default/browser_tests --gtest_filter=BrowserDialogTest.Invoke
--dialog=AskGoogleForSuggestionsDialogTest.InvokeDialog_default --interactive
==========

[commit-bot: I haz the power, 2017-01-25 06:23:06]

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

[commit-bot: I haz the power, 2017-01-25 06:23:07]

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-25 11:05:52]

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

[commit-bot: I haz the power, 2017-01-25 11:06:10]

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-25 11:08:27]

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

[karandeepb, 2017-01-25 11:08:28]

PTAL Trent.

https://codereview.chromium.org/2650583012/diff/20001/chrome/browser/ui/ask_g...
File chrome/browser/ui/ask_google_for_suggestions_dialog_browsertest.cc (right):

https://codereview.chromium.org/2650583012/diff/20001/chrome/browser/ui/ask_g...
chrome/browser/ui/ask_google_for_suggestions_dialog_browsertest.cc:32: //
Initially disabled except on Mac due to http://crbug.com/683808.
This seems to be failing on Linux and windows on the trybots. Think this is due
to crbug.com/683808? It passes for me locally on Linux though, so I am not sure
what's wrong.

[commit-bot: I haz the power, 2017-01-25 12:09:03]

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

[commit-bot: I haz the power, 2017-01-25 12:09:04]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-01-27 02:09:50]

nice! lgtm

https://codereview.chromium.org/2650583012/diff/20001/chrome/browser/ui/ask_g...
File chrome/browser/ui/ask_google_for_suggestions_dialog_browsertest.cc (right):

https://codereview.chromium.org/2650583012/diff/20001/chrome/browser/ui/ask_g...
chrome/browser/ui/ask_google_for_suggestions_dialog_browsertest.cc:32: //
Initially disabled except on Mac due to http://crbug.com/683808.
On 2017/01/25 11:08:28, karandeepb wrote:
> This seems to be failing on Linux and windows on the trybots. Think this is
due
> to crbug.com/683808? It passes for me locally on Linux though, so I am not
sure
> what's wrong.

We can try depending on the fix(es) for crbug.com/683808 in a new CL upload --
i.e. https://codereview.chromium.org/2645253002/ (I'll loop in the part that
fixes the dialog testing framework as well, since that seems a relevant thing to
include after sky's latest comments)

[karandeepb, 2017-01-27 02:12:47]

The CQ bit was checked by karandeepb@chromium.org

[commit-bot: I haz the power, 2017-01-27 02:14:12]

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-27 02:24:15]

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

[commit-bot: I haz the power, 2017-01-27 02:24:16]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[karandeepb, 2017-01-27 02:26:37]

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

[karandeepb, 2017-01-27 02:26:37]

PTAL msw@ for 
-chrome/browser/ui/views/confirm_bubble_views.cc
and 
-chrome/browser/ui/ask_google_for_suggestions_dialog_browsertest.cc

review.

[msw, 2017-01-27 23:26:55]

nice; lgtm. I hope we can enable the test on other platforms soon.

[tapted, 2017-01-31 23:52:39]

https://codereview.chromium.org/2650583012/diff/20001/chrome/browser/ui/ask_g...
File chrome/browser/ui/ask_google_for_suggestions_dialog_browsertest.cc (right):

https://codereview.chromium.org/2650583012/diff/20001/chrome/browser/ui/ask_g...
chrome/browser/ui/ask_google_for_suggestions_dialog_browsertest.cc:32: //
Initially disabled except on Mac due to http://crbug.com/683808.
On 2017/01/27 02:09:50, tapted wrote:
> On 2017/01/25 11:08:28, karandeepb wrote:
> > This seems to be failing on Linux and windows on the trybots. Think this is
> due
> > to crbug.com/683808? It passes for me locally on Linux though, so I am not
> sure
> > what's wrong.
> 
> We can try depending on the fix(es) for crbug.com/683808 in a new CL upload --
> i.e. https://codereview.chromium.org/2645253002/ (I'll loop in the part that
> fixes the dialog testing framework as well, since that seems a relevant thing
to
> include after sky's latest comments)

The fix for this has landed in r447216, so it should be safe to remove the
MAYBE_ stuff now

[karandeepb, 2017-02-01 00:01:19]

https://codereview.chromium.org/2650583012/diff/20001/chrome/browser/ui/ask_g...
File chrome/browser/ui/ask_google_for_suggestions_dialog_browsertest.cc (right):

https://codereview.chromium.org/2650583012/diff/20001/chrome/browser/ui/ask_g...
chrome/browser/ui/ask_google_for_suggestions_dialog_browsertest.cc:32: //
Initially disabled except on Mac due to http://crbug.com/683808.
On 2017/01/31 23:52:39, tapted wrote:
> On 2017/01/27 02:09:50, tapted wrote:
> > On 2017/01/25 11:08:28, karandeepb wrote:
> > > This seems to be failing on Linux and windows on the trybots. Think this
is
> > due
> > > to crbug.com/683808? It passes for me locally on Linux though, so I am not
> > sure
> > > what's wrong.
> > 
> > We can try depending on the fix(es) for crbug.com/683808 in a new CL upload
--
> > i.e. https://codereview.chromium.org/2645253002/ (I'll loop in the part that
> > fixes the dialog testing framework as well, since that seems a relevant
thing
> to
> > include after sky's latest comments)
> 
> The fix for this has landed in r447216, so it should be safe to remove the
> MAYBE_ stuff now

Will submit a patch.

[karandeepb, 2017-02-01 00:26:44]

Did not realize that I had not submitted this.
Trent, on my Linux workstation, the test still fails. Same for
UpdateRecommendedDialogTest.

[tapted, 2017-02-01 00:32:32]

On 2017/02/01 00:26:44, karandeepb wrote:
> Did not realize that I had not submitted this.
> Trent, on my Linux workstation, the test still fails. Same for
> UpdateRecommendedDialogTest.

Are you past r447216? Can you file a bug, or update http://crbug.com/683808,
with the output? I'm trying the `UpdateRecommendedDialogTest` on trybots at
https://codereview.chromium.org/2663163004

[karandeepb, 2017-02-01 01:54:27]

On 2017/02/01 00:32:32, tapted wrote:
> On 2017/02/01 00:26:44, karandeepb wrote:
> > Did not realize that I had not submitted this.
> > Trent, on my Linux workstation, the test still fails. Same for
> > UpdateRecommendedDialogTest.
> 
> Are you past r447216? Can you file a bug, or update http://crbug.com/683808,
> with the output? I'm trying the `UpdateRecommendedDialogTest` on trybots at
> https://codereview.chromium.org/2663163004

Yes. Updated issue 683808, seems the trybots show the same error for
https://codereview.chromium.org/2663163004. Will land the test only on Mac for
now. Then it can be enabled together with UpdateRecommendedDialogTest.

[tapted, 2017-02-01 01:56:45]

On 2017/02/01 01:54:27, karandeepb wrote:
> On 2017/02/01 00:32:32, tapted wrote:
> > On 2017/02/01 00:26:44, karandeepb wrote:
> > > Did not realize that I had not submitted this.
> > > Trent, on my Linux workstation, the test still fails. Same for
> > > UpdateRecommendedDialogTest.
> > 
> > Are you past r447216? Can you file a bug, or update http://crbug.com/683808,
> > with the output? I'm trying the `UpdateRecommendedDialogTest` on trybots at
> > https://codereview.chromium.org/2663163004
> 
> Yes. Updated issue 683808, seems the trybots show the same error for
> https://codereview.chromium.org/2663163004. Will land the test only on Mac for
> now. Then it can be enabled together with UpdateRecommendedDialogTest.

sgtm - seems maybe there is a bug with
DesktopWindowTreeHostX11::GetAllOpenWindows() :/

[karandeepb, 2017-02-01 02:00:28]

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

[commit-bot: I haz the power, 2017-02-01 02:01:25]

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

[karandeepb, 2017-02-01 02:02:16]

On 2017/02/01 01:56:45, tapted wrote:
> On 2017/02/01 01:54:27, karandeepb wrote:
> > On 2017/02/01 00:32:32, tapted wrote:
> > > On 2017/02/01 00:26:44, karandeepb wrote:
> > > > Did not realize that I had not submitted this.
> > > > Trent, on my Linux workstation, the test still fails. Same for
> > > > UpdateRecommendedDialogTest.
> > > 
> > > Are you past r447216? Can you file a bug, or update
http://crbug.com/683808,
> > > with the output? I'm trying the `UpdateRecommendedDialogTest` on trybots
at
> > > https://codereview.chromium.org/2663163004
> > 
> > Yes. Updated issue 683808, seems the trybots show the same error for
> > https://codereview.chromium.org/2663163004. Will land the test only on Mac
for
> > now. Then it can be enabled together with UpdateRecommendedDialogTest.
> 
> sgtm - seems maybe there is a bug with
> DesktopWindowTreeHostX11::GetAllOpenWindows() :/

Oh ok, weirdly I remember it passing on my Linux workstation in Sydney without
your patch (it failed on bots though).

[commit-bot: I haz the power, 2017-02-01 03:08:12]

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

[commit-bot: I haz the power, 2017-02-01 03:08:13]

Dry run: This issue passed the CQ dry run.

[karandeepb, 2017-02-01 04:12:01]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2017-02-01 04:12:01]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2650583012/#ps40001
(title: "Rebase.")

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

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-02-01 04:16:26]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485922321359330,
"parent_rev": "58116ee5b2710dd751d968024489b305ae18b038", "commit_rev":
"80b77aca94a93d7c54cb2b013f02469a6c4cf523"}

[commit-bot: I haz the power, 2017-02-01 04:17:57]

Description was changed from

==========
MacViews: Enable views based Ask Google For Suggestions dialog behind
secondary-ui-md flag.

This CL puts the views based Ask Google For Suggestions 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.

This CL also enables confirm_bubble_views_unittest.cc on Mac. Since there
doesn't seem to be any existing browser test for the dialog, a browser test
utilizing the DialogBrowserTest framework is also added.

BUG=662128, 654139
TEST=Run out/Default/browser_tests --gtest_filter=BrowserDialogTest.Invoke
--dialog=AskGoogleForSuggestionsDialogTest.InvokeDialog_default --interactive
==========

to

==========
MacViews: Enable views based Ask Google For Suggestions dialog behind
secondary-ui-md flag.

This CL puts the views based Ask Google For Suggestions 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.

This CL also enables confirm_bubble_views_unittest.cc on Mac. Since there
doesn't seem to be any existing browser test for the dialog, a browser test
utilizing the DialogBrowserTest framework is also added.

BUG=662128, 654139
TEST=Run out/Default/browser_tests --gtest_filter=BrowserDialogTest.Invoke
--dialog=AskGoogleForSuggestionsDialogTest.InvokeDialog_default --interactive

Review-Url: https://codereview.chromium.org/2650583012
Cr-Commit-Position: refs/heads/master@{#447446}
Committed:
https://chromium.googlesource.com/chromium/src/+/80b77aca94a93d7c54cb2b013f02...
==========

[commit-bot: I haz the power, 2017-02-01 04:17:58]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/80b77aca94a93d7c54cb2b013f02...