🔍 Introduce default search engine dialog

🔍 Introduce default search engine dialog

Add a dialog that can be triggered via LocaleManager that allows a user
to choose their default search engine from a randomized list.

DefaultSearchEnginePromoDialog:
* Add a basic dialog that forces a user to select a default search engine.  This
  dialog only shows up for the "existing user" case and (currently) randomizes
  the search engines available as defined in the TemplateUrlService.

* Add the ability to show the DefaultSearchEnginePromoDialog from the
  LocaleManager.  It currently never fires because the logic hasn't landed, but
  it is triggered from ChromeTabbedActivity and SearchActivity.

Things to do:
- Still trying to figure out how to test this.

Screenshots:
https://drive.google.com/corp/drive/u/0/folders/0B7c8ZkXVwskDRmlnZ1g0LXVpMGM

BUG=714223, 712836, 712833
Review-Url: https://codereview.chromium.org/2838833002
Cr-Commit-Position: refs/heads/master@{#467766}
Committed: https://chromium.googlesource.com/chromium/src/+/42776f8a5b7e07ce63442004737150495eed0db0

[gone, 2017-04-25 06:15:12]

Description was changed from

==========
🔍 Introduce default search engine dialog

Add a dialog that can be triggered via LocaleManager that allows a user
to choose their default search engine from a randomized list.

General:
* Add the ability to PromoDialog to be able to keep a header and footer fixed
  while the contents are scrollable.  Scrollable content is indicated by a never
  disappearing scrollbar indicator.

* Adds the ability to add a InfoBarControlLayout to the PromoDialogLayout.  This
  is used to standardize the buttons that are displayed across dialogs.

* Add the ability to add tags to the InfoBarControlLayout radio buttons.  These
  are needed because IDs aren't set for the radio buttons, and because it is
  safer to keep track of the search engine keywords with the RadioButtons that
  represent them.

DefaultSearchEnginePromoDialog:
* Add a basic dialog that forces a user to select a default search engine.  This
  dialog only shows up for the "existing user" case and (currently) randomizes
  the search engines available as defined in the TemplateUrlService.

* Add the ability to show the DefaultSearchEnginePromoDialog from the
  LocaleManager.  It currently never fires.

Things to do:
- InfoBarControlLayout needs to move to the widget/ directory and be generalized
  so that they're not so infobar specific.

- Tests are forthcoming.

Screenshots:
https://drive.google.com/corp/drive/u/0/folders/0B7c8ZkXVwskDRmlnZ1g0LXVpMGM

BUG=
==========

to

==========
🔍 Introduce default search engine dialog

Add a dialog that can be triggered via LocaleManager that allows a user
to choose their default search engine from a randomized list.

General:
* Add the ability to PromoDialog to be able to keep a header and footer fixed
  while the contents are scrollable.  Scrollable content is indicated by a never
  disappearing scrollbar indicator.

* Adds the ability to add a InfoBarControlLayout to the PromoDialogLayout.  This
  is used to standardize the buttons that are displayed across dialogs.

* Add the ability to add tags to the InfoBarControlLayout radio buttons.  These
  are needed because IDs aren't set for the radio buttons, and because it is
  safer to keep track of the search engine keywords with the RadioButtons that
  represent them.

DefaultSearchEnginePromoDialog:
* Add a basic dialog that forces a user to select a default search engine.  This
  dialog only shows up for the "existing user" case and (currently) randomizes
  the search engines available as defined in the TemplateUrlService.

* Add the ability to show the DefaultSearchEnginePromoDialog from the
  LocaleManager.  It currently never fires.

Things to do:
- InfoBarControlLayout needs to move to the widget/ directory and be generalized
  so that they're not so infobar specific.

- Tests are forthcoming.

Screenshots:
https://drive.google.com/corp/drive/u/0/folders/0B7c8ZkXVwskDRmlnZ1g0LXVpMGM

BUG=714223,712836,712833
==========

[gone, 2017-04-25 06:16:47]

dfalcantara@chromium.org changed reviewers:
+ tedchoc@chromium.org, yusufo@chromium.org

[gone, 2017-04-25 06:16:47]

Cleaned this up a bit.  WDYT?  I'm still working on tests but it'd be good to
get some general feedback before plowing ahead.

[gone, 2017-04-25 21:04:58]

I'm having trouble hooking this up to the SearchActivity properly without
dealing with race conditions.  I'm going to prepare another CL that yanks out
anything involving that, so please hold off for now.

[gone, 2017-04-25 21:38:52]

Description was changed from

==========
🔍 Introduce default search engine dialog

Add a dialog that can be triggered via LocaleManager that allows a user
to choose their default search engine from a randomized list.

General:
* Add the ability to PromoDialog to be able to keep a header and footer fixed
  while the contents are scrollable.  Scrollable content is indicated by a never
  disappearing scrollbar indicator.

* Adds the ability to add a InfoBarControlLayout to the PromoDialogLayout.  This
  is used to standardize the buttons that are displayed across dialogs.

* Add the ability to add tags to the InfoBarControlLayout radio buttons.  These
  are needed because IDs aren't set for the radio buttons, and because it is
  safer to keep track of the search engine keywords with the RadioButtons that
  represent them.

DefaultSearchEnginePromoDialog:
* Add a basic dialog that forces a user to select a default search engine.  This
  dialog only shows up for the "existing user" case and (currently) randomizes
  the search engines available as defined in the TemplateUrlService.

* Add the ability to show the DefaultSearchEnginePromoDialog from the
  LocaleManager.  It currently never fires.

Things to do:
- InfoBarControlLayout needs to move to the widget/ directory and be generalized
  so that they're not so infobar specific.

- Tests are forthcoming.

Screenshots:
https://drive.google.com/corp/drive/u/0/folders/0B7c8ZkXVwskDRmlnZ1g0LXVpMGM

BUG=714223,712836,712833
==========

to

==========
🔍 Introduce default search engine dialog

Add a dialog that can be triggered via LocaleManager that allows a user
to choose their default search engine from a randomized list.

General:
* Add the ability to PromoDialog to be able to keep a header and footer fixed
  while the contents are scrollable.  Scrollable content is indicated by a never
  disappearing scrollbar indicator.

* Adds the ability to add a InfoBarControlLayout to the PromoDialogLayout.  This
  is used to standardize the buttons that are displayed across dialogs.

* Add the ability to add tags to the InfoBarControlLayout radio buttons.  These
  are needed because IDs aren't set for the radio buttons, and because it is
  safer to keep track of the search engine keywords with the RadioButtons that
  represent them.

DefaultSearchEnginePromoDialog:
* Add a basic dialog that forces a user to select a default search engine.  This
  dialog only shows up for the "existing user" case and (currently) randomizes
  the search engines available as defined in the TemplateUrlService.

* Add the ability to show the DefaultSearchEnginePromoDialog from the
  LocaleManager.  It currently never fires because the logic hasn't landed, but
  it is triggered from ChromeTabbedActivity and SearchActivity.

Things to do:
- InfoBarControlLayout needs to move to the widget/ directory and be generalized
  so that they're not so infobar specific.

- Tests will come in the next CL.

Screenshots:
https://drive.google.com/corp/drive/u/0/folders/0B7c8ZkXVwskDRmlnZ1g0LXVpMGM

BUG=714223,712836,712833
==========

[gone, 2017-04-25 21:40:19]

I think I got a handle on it.  PTAL.  Testing this manually is driving me nuts
so I'll be working on SearchActivity tests next.  Those would include tests for
clicking on things in the Dialog, as well.

[gone, 2017-04-25 21:46:47]

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

[commit-bot: I haz the power, 2017-04-25 21:47:32]

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-04-25 23:22:04]

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

[commit-bot: I haz the power, 2017-04-25 23:22:05]

Dry run: This issue passed the CQ dry run.

[gone, 2017-04-26 05:45:50]

Description was changed from

==========
🔍 Introduce default search engine dialog

Add a dialog that can be triggered via LocaleManager that allows a user
to choose their default search engine from a randomized list.

General:
* Add the ability to PromoDialog to be able to keep a header and footer fixed
  while the contents are scrollable.  Scrollable content is indicated by a never
  disappearing scrollbar indicator.

* Adds the ability to add a InfoBarControlLayout to the PromoDialogLayout.  This
  is used to standardize the buttons that are displayed across dialogs.

* Add the ability to add tags to the InfoBarControlLayout radio buttons.  These
  are needed because IDs aren't set for the radio buttons, and because it is
  safer to keep track of the search engine keywords with the RadioButtons that
  represent them.

DefaultSearchEnginePromoDialog:
* Add a basic dialog that forces a user to select a default search engine.  This
  dialog only shows up for the "existing user" case and (currently) randomizes
  the search engines available as defined in the TemplateUrlService.

* Add the ability to show the DefaultSearchEnginePromoDialog from the
  LocaleManager.  It currently never fires because the logic hasn't landed, but
  it is triggered from ChromeTabbedActivity and SearchActivity.

Things to do:
- InfoBarControlLayout needs to move to the widget/ directory and be generalized
  so that they're not so infobar specific.

- Tests will come in the next CL.

Screenshots:
https://drive.google.com/corp/drive/u/0/folders/0B7c8ZkXVwskDRmlnZ1g0LXVpMGM

BUG=714223,712836,712833
==========

to

==========
🔍 Introduce default search engine dialog

Add a dialog that can be triggered via LocaleManager that allows a user
to choose their default search engine from a randomized list.

General:
* Add the ability to PromoDialog to be able to keep a header and footer fixed
  while the contents are scrollable.  Scrollable content is indicated by a never
  disappearing scrollbar indicator.

* Adds the ability to add a InfoBarControlLayout to the PromoDialogLayout.  This
  is used to standardize the buttons that are displayed across dialogs.

* Add the ability to add tags to the InfoBarControlLayout radio buttons.  These
  are needed because IDs aren't set for the radio buttons, and because it is
  safer to keep track of the search engine keywords with the RadioButtons that
  represent them.

DefaultSearchEnginePromoDialog:
* Add a basic dialog that forces a user to select a default search engine.  This
  dialog only shows up for the "existing user" case and (currently) randomizes
  the search engines available as defined in the TemplateUrlService.

* Add the ability to show the DefaultSearchEnginePromoDialog from the
  LocaleManager.  It currently never fires because the logic hasn't landed, but
  it is triggered from ChromeTabbedActivity and SearchActivity.

Things to do:
- InfoBarControlLayout needs to move to the widget/ directory and be generalized
  so that they're not so infobar specific.

- Tests will come in the next CL:
  https://chromiumcodereview.appspot.com/2842943002/

Screenshots:
https://drive.google.com/corp/drive/u/0/folders/0B7c8ZkXVwskDRmlnZ1g0LXVpMGM

BUG=714223,712836,712833
==========

[gone, 2017-04-26 05:47:24]

Description was changed from

==========
🔍 Introduce default search engine dialog

Add a dialog that can be triggered via LocaleManager that allows a user
to choose their default search engine from a randomized list.

General:
* Add the ability to PromoDialog to be able to keep a header and footer fixed
  while the contents are scrollable.  Scrollable content is indicated by a never
  disappearing scrollbar indicator.

* Adds the ability to add a InfoBarControlLayout to the PromoDialogLayout.  This
  is used to standardize the buttons that are displayed across dialogs.

* Add the ability to add tags to the InfoBarControlLayout radio buttons.  These
  are needed because IDs aren't set for the radio buttons, and because it is
  safer to keep track of the search engine keywords with the RadioButtons that
  represent them.

DefaultSearchEnginePromoDialog:
* Add a basic dialog that forces a user to select a default search engine.  This
  dialog only shows up for the "existing user" case and (currently) randomizes
  the search engines available as defined in the TemplateUrlService.

* Add the ability to show the DefaultSearchEnginePromoDialog from the
  LocaleManager.  It currently never fires because the logic hasn't landed, but
  it is triggered from ChromeTabbedActivity and SearchActivity.

Things to do:
- InfoBarControlLayout needs to move to the widget/ directory and be generalized
  so that they're not so infobar specific.

- Tests will come in the next CL:
  https://chromiumcodereview.appspot.com/2842943002/

Screenshots:
https://drive.google.com/corp/drive/u/0/folders/0B7c8ZkXVwskDRmlnZ1g0LXVpMGM

BUG=714223,712836,712833
==========

to

==========
🔍 Introduce default search engine dialog

Add a dialog that can be triggered via LocaleManager that allows a user
to choose their default search engine from a randomized list.

General:
* Add the ability to PromoDialog to be able to keep a header and footer fixed
  while the contents are scrollable.  Scrollable content is indicated by a never
  disappearing scrollbar indicator.

* Adds the ability to add a InfoBarControlLayout to the PromoDialogLayout.  This
  is used to standardize the buttons that are displayed across dialogs.

* Add the ability to add tags to the InfoBarControlLayout radio buttons.  These
  are needed because IDs aren't set for the radio buttons, and because it is
  safer to keep track of the search engine keywords with the RadioButtons that
  represent them.

DefaultSearchEnginePromoDialog:
* Add a basic dialog that forces a user to select a default search engine.  This
  dialog only shows up for the "existing user" case and (currently) randomizes
  the search engines available as defined in the TemplateUrlService.

* Add the ability to show the DefaultSearchEnginePromoDialog from the
  LocaleManager.  It currently never fires because the logic hasn't landed, but
  it is triggered from ChromeTabbedActivity and SearchActivity.

Things to do:
- InfoBarControlLayout needs to move to the widget/ directory and be generalized
  so that they're not so infobar specific.

- Basic tests will come in the next CL, with more to follow afterward:
  https://chromiumcodereview.appspot.com/2842943002/

Screenshots:
https://drive.google.com/corp/drive/u/0/folders/0B7c8ZkXVwskDRmlnZ1g0LXVpMGM

BUG=714223,712836,712833
==========

[gone, 2017-04-26 05:52:36]

https://codereview.chromium.org/2838833002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/PromoDialogLayout.java
(right):

https://codereview.chromium.org/2838833002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/widget/PromoDialogLayout.java:24:
* specific behaviors (see go/snowflake-dialogs for details):
Clarified why this class is a bit messy by expanding the comment, but I'm
hesitant to link to an internal design doc that can't be viewed externally. 
Thoughts?

[gone, 2017-04-26 18:58:58]

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

[commit-bot: I haz the power, 2017-04-26 19:00:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gone, 2017-04-26 19:00:30]

Something about the refactoring I did between patch sets 11 through 13 seems to
have fixed the threading issue I was seeing, so this should be reviewable again
:/  Sorry for the churn.

[gone, 2017-04-26 19:27:27]

Description was changed from

==========
🔍 Introduce default search engine dialog

Add a dialog that can be triggered via LocaleManager that allows a user
to choose their default search engine from a randomized list.

General:
* Add the ability to PromoDialog to be able to keep a header and footer fixed
  while the contents are scrollable.  Scrollable content is indicated by a never
  disappearing scrollbar indicator.

* Adds the ability to add a InfoBarControlLayout to the PromoDialogLayout.  This
  is used to standardize the buttons that are displayed across dialogs.

* Add the ability to add tags to the InfoBarControlLayout radio buttons.  These
  are needed because IDs aren't set for the radio buttons, and because it is
  safer to keep track of the search engine keywords with the RadioButtons that
  represent them.

DefaultSearchEnginePromoDialog:
* Add a basic dialog that forces a user to select a default search engine.  This
  dialog only shows up for the "existing user" case and (currently) randomizes
  the search engines available as defined in the TemplateUrlService.

* Add the ability to show the DefaultSearchEnginePromoDialog from the
  LocaleManager.  It currently never fires because the logic hasn't landed, but
  it is triggered from ChromeTabbedActivity and SearchActivity.

Things to do:
- InfoBarControlLayout needs to move to the widget/ directory and be generalized
  so that they're not so infobar specific.

- Basic tests will come in the next CL, with more to follow afterward:
  https://chromiumcodereview.appspot.com/2842943002/

Screenshots:
https://drive.google.com/corp/drive/u/0/folders/0B7c8ZkXVwskDRmlnZ1g0LXVpMGM

BUG=714223,712836,712833
==========

to

==========
🔍 Introduce default search engine dialog

Add a dialog that can be triggered via LocaleManager that allows a user
to choose their default search engine from a randomized list.

DefaultSearchEnginePromoDialog:
* Add a basic dialog that forces a user to select a default search engine.  This
  dialog only shows up for the "existing user" case and (currently) randomizes
  the search engines available as defined in the TemplateUrlService.

* Add the ability to show the DefaultSearchEnginePromoDialog from the
  LocaleManager.  It currently never fires because the logic hasn't landed, but
  it is triggered from ChromeTabbedActivity and SearchActivity.

Things to do:
- Still trying to figure out how to test this.

Screenshots:
https://drive.google.com/corp/drive/u/0/folders/0B7c8ZkXVwskDRmlnZ1g0LXVpMGM

BUG=714223,712836,712833
==========

[gone, 2017-04-26 19:28:35]

I had to yank out the PromoDialogLayout logic.  This CL was sucking in a lot of
Activity changes becoming an easy target for reverting everything at once.  PTAL

[gone, 2017-04-26 19:30:31]

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

[gone, 2017-04-26 19:30:52]

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

[commit-bot: I haz the power, 2017-04-26 19:31:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ted C, 2017-04-26 20:21:53]

Comments from a few versions ago...hopefully not entirely invalid anymore

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java
(right):

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java:380:

maybe add

if (tags != null) assert tags.size() == messages.size();

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java
(right):

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:42:
public static void create(
maybe this should be called show?

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:43:
final Context context, final int dialogType, @Nullable final Runnable
onDismissed) {
Should/can dialogType have an associated IntDef?

Looks like onDismissed is more like onSuccess or onSearchEngineSelected

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:82:
assert LibraryLoader.isInitialized();
Since this happens in the one place you create it above, I think this one
probably isn't necessary IMO

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:149:
if (getOwnerActivity() != null) getOwnerActivity().finish();
should this be finish and remove task?

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:163:
ArrayList<Integer> indices = new ArrayList<>();
Could we use:
https://developer.android.com/reference/java/util/Collections.html#shuffle(ja...,
java.util.Random)

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchActivity.java
(right):

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchActivity.java:46:
CustomTabsConnection.getInstance(getApplication()).warmup(0);
wha?

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchBoxDataProvider.java
(right):

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchBoxDataProvider.java:57:
return SearchWidgetProvider.sVerbatimUrl;
I would rather this be a static getter function.

How do you know the updateCached... method is called in the other file before
getting here?  The ordering dependency is a bit scary IMO.

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java
(right):

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:301:
sVerbatimUrl =
LocationBarLayout.splitPathFromUrlDisplayText(searchEngineUrl).first;
Should this be sDefaultSearchEngineUrl?

Seems like the verbatim is the implementation detail in the url but not relevant
here.

[commit-bot: I haz the power, 2017-04-26 20:51:44]

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

[commit-bot: I haz the power, 2017-04-26 20:51:45]

Dry run: This issue passed the CQ dry run.

[gone, 2017-04-26 21:44:18]

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

[gone, 2017-04-26 21:45:15]

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java
(right):

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarControlLayout.java:380:

On 2017/04/26 20:21:52, Ted C wrote:
> maybe add
> 
> if (tags != null) assert tags.size() == messages.size();

Done on the forked copy.

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java
(right):

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:42:
public static void create(
On 2017/04/26 20:21:52, Ted C wrote:
> maybe this should be called show?

Done.

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:43:
final Context context, final int dialogType, @Nullable final Runnable
onDismissed) {
On 2017/04/26 20:21:52, Ted C wrote:
> Should/can dialogType have an associated IntDef?
> 
> Looks like onDismissed is more like onSuccess or onSearchEngineSelected

Seems like it'd be fine to move it to always be called.  Moved it over.

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:82:
assert LibraryLoader.isInitialized();
On 2017/04/26 20:21:52, Ted C wrote:
> Since this happens in the one place you create it above, I think this one
> probably isn't necessary IMO

Done.

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:149:
if (getOwnerActivity() != null) getOwnerActivity().finish();
On 2017/04/26 20:21:52, Ted C wrote:
> should this be finish and remove task?

I'd argue for a regular finish() so that the activity stays findable in recents
(if it normally is).

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:163:
ArrayList<Integer> indices = new ArrayList<>();
On 2017/04/26 20:21:52, Ted C wrote:
> Could we use:
>
https://developer.android.com/reference/java/util/Collections.html#shuffle(ja...,
> java.util.Random)

Works for me.

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchActivity.java
(right):

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchActivity.java:46:
CustomTabsConnection.getInstance(getApplication()).warmup(0);
On 2017/04/26 20:21:52, Ted C wrote:
> wha?

/me shrugs.  It was already there.  It's been pushed back to where it used to be
in the last patch set.

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchBoxDataProvider.java
(right):

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchBoxDataProvider.java:57:
return SearchWidgetProvider.sVerbatimUrl;
On 2017/04/26 20:21:52, Ted C wrote:
> I would rather this be a static getter function.
> 
> How do you know the updateCached... method is called in the other file before
> getting here?  The ordering dependency is a bit scary IMO.

It doesn't seem to be functionally any different.  I think getCurrentUrl() can
already be called before the TemplateUrlService is finished loading.

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java
(right):

https://codereview.chromium.org/2838833002/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:301:
sVerbatimUrl =
LocationBarLayout.splitPathFromUrlDisplayText(searchEngineUrl).first;
On 2017/04/26 20:21:52, Ted C wrote:
> Should this be sDefaultSearchEngineUrl?
> 
> Seems like the verbatim is the implementation detail in the url but not
relevant
> here.

Done.

[commit-bot: I haz the power, 2017-04-26 21:45:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gone, 2017-04-26 21:48:24]

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

[commit-bot: I haz the power, 2017-04-26 21:49:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gone, 2017-04-26 23:28:45]

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

[commit-bot: I haz the power, 2017-04-26 23:29:32]

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-04-27 02:54:14]

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

[commit-bot: I haz the power, 2017-04-27 02:54:15]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gone, 2017-04-27 03:19:25]

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

[commit-bot: I haz the power, 2017-04-27 03:20: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-04-27 04:59:33]

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

[commit-bot: I haz the power, 2017-04-27 04:59:34]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-04-27 16:40:46]

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java
(right):

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:31:
private final int mDialogType;
I think my comment from the previous CL was lost in a multi-comment comment.

But can this be an IntDef @SearchEnginePromoType?

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:41:
* been loaded.
I think we should have javadoc for the params (in particular the callback to say
what the boolean indicates)

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:115:
mSelectedKeyword = null;
can the contents of this method be replaced with:

mSelectedKeyword = (String) group.findViewById(checkedId).getTag();
updateButtonState();

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
(right):

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java:182:
* @param onDismissed Run when the dialog has been dismissed.
update javadoc

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchActivity.java
(right):

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchActivity.java:147:
finish();
and remove task?

pre-L, will this still be in recents and clickable?  does it show the dialog
again then?

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java
(right):

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:399:
return sDefaultSearchEngineUrl;
can/should this ever be null?  Wondering if we should assert that
updateCachedEngineName was called first (even if this doesn't change the
existing behavior we should add checks to ensure that was correct).

[gone, 2017-04-27 17:48:52]

Added a TODO for yusuf there.

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java
(right):

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:31:
private final int mDialogType;
On 2017/04/27 16:40:45, Ted C wrote:
> I think my comment from the previous CL was lost in a multi-comment comment.
> 
> But can this be an IntDef @SearchEnginePromoType?

Done.

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/DefaultSearchEnginePromoDialog.java:41:
* been loaded.
On 2017/04/27 16:40:45, Ted C wrote:
> I think we should have javadoc for the params (in particular the callback to
say
> what the boolean indicates)

Done.

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
(right):

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java:182:
* @param onDismissed Run when the dialog has been dismissed.
On 2017/04/27 16:40:45, Ted C wrote:
> update javadoc

Done.

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchActivity.java
(right):

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchActivity.java:147:
finish();
On 2017/04/27 16:40:46, Ted C wrote:
> and remove task?
> 
> pre-L, will this still be in recents and clickable?  does it show the dialog
> again then?

It doesn't seem to be in recents; the AndroidManifest hides it explicitly with
an "excludeFromRecents".

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java
(right):

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:399:
return sDefaultSearchEngineUrl;
On 2017/04/27 16:40:46, Ted C wrote:
> can/should this ever be null?  Wondering if we should assert that
> updateCachedEngineName was called first (even if this doesn't change the
> existing behavior we should add checks to ensure that was correct).

This was added by Yusuf for showing the engine's URL as an autocomplete
suggestion, IIRC.  I don't think it's safe to assert null here, but if it's null
nothing seems to break.

[gone, 2017-04-27 17:51:55]

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

[commit-bot: I haz the power, 2017-04-27 17:52:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ted C, 2017-04-27 18:10:32]

lgtm

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchActivity.java
(right):

https://codereview.chromium.org/2838833002/diff/320001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchActivity.java:147:
finish();
On 2017/04/27 17:48:52, slow (dfalcantara) wrote:
> On 2017/04/27 16:40:46, Ted C wrote:
> > and remove task?
> > 
> > pre-L, will this still be in recents and clickable?  does it show the dialog
> > again then?
> 
> It doesn't seem to be in recents; the AndroidManifest hides it explicitly with
> an "excludeFromRecents".

Acknowledged.

https://codereview.chromium.org/2838833002/diff/380001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
(right):

https://codereview.chromium.org/2838833002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java:182:
* @param onDismissed Notified when the dialog is dismissed about whether the
user acted on it.
dismissed [and] whether the user acted on it?

[gone, 2017-04-27 18:15:22]

https://codereview.chromium.org/2838833002/diff/380001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
(right):

https://codereview.chromium.org/2838833002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java:182:
* @param onDismissed Notified when the dialog is dismissed about whether the
user acted on it.
On 2017/04/27 18:10:32, Ted C wrote:
> dismissed [and] whether the user acted on it?

little weird, but i'll go with it

[gone, 2017-04-27 18:15:40]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2017-04-27 18:15:41]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2838833002/#ps400001
(title: "COmments")

[commit-bot: I haz the power, 2017-04-27 18:16:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gone, 2017-04-27 18:30:01]

The CQ bit was unchecked by dfalcantara@chromium.org

[gone, 2017-04-27 18:30:02]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2017-04-27 18:30:27]

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-04-27 20:28:20]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1493317802152080,
"parent_rev": "4ab7eea2c4f7ee425a2c38ceab1a84be2fc27daf", "commit_rev":
"42776f8a5b7e07ce63442004737150495eed0db0"}

[commit-bot: I haz the power, 2017-04-27 20:28:30]

Description was changed from

==========
🔍 Introduce default search engine dialog

Add a dialog that can be triggered via LocaleManager that allows a user
to choose their default search engine from a randomized list.

DefaultSearchEnginePromoDialog:
* Add a basic dialog that forces a user to select a default search engine.  This
  dialog only shows up for the "existing user" case and (currently) randomizes
  the search engines available as defined in the TemplateUrlService.

* Add the ability to show the DefaultSearchEnginePromoDialog from the
  LocaleManager.  It currently never fires because the logic hasn't landed, but
  it is triggered from ChromeTabbedActivity and SearchActivity.

Things to do:
- Still trying to figure out how to test this.

Screenshots:
https://drive.google.com/corp/drive/u/0/folders/0B7c8ZkXVwskDRmlnZ1g0LXVpMGM

BUG=714223,712836,712833
==========

to

==========
🔍 Introduce default search engine dialog

Add a dialog that can be triggered via LocaleManager that allows a user
to choose their default search engine from a randomized list.

DefaultSearchEnginePromoDialog:
* Add a basic dialog that forces a user to select a default search engine.  This
  dialog only shows up for the "existing user" case and (currently) randomizes
  the search engines available as defined in the TemplateUrlService.

* Add the ability to show the DefaultSearchEnginePromoDialog from the
  LocaleManager.  It currently never fires because the logic hasn't landed, but
  it is triggered from ChromeTabbedActivity and SearchActivity.

Things to do:
- Still trying to figure out how to test this.

Screenshots:
https://drive.google.com/corp/drive/u/0/folders/0B7c8ZkXVwskDRmlnZ1g0LXVpMGM

BUG=714223,712836,712833

Review-Url: https://codereview.chromium.org/2838833002
Cr-Commit-Position: refs/heads/master@{#467766}
Committed:
https://chromium.googlesource.com/chromium/src/+/42776f8a5b7e07ce634420047371...
==========

[commit-bot: I haz the power, 2017-04-27 20:28:31]

Committed patchset #21 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/42776f8a5b7e07ce634420047371...