🔍 Don't display the search engine until First Run completes

🔍 Don't display the search engine until First Run completes

* Adds a check to prevent showing branding if First Run hasn't completed.

* Updates the widget so that it is updated when an AsyncInitializationActivity
  subclass calls onDeferredStartup().

* Changes the resource for the microphone to match the omnibox's, as well as
  uses its content description.

* Adds tests to confirm that the search engine is updated once First Run
  completes.

BUG=708844, 710952
Review-Url: https://codereview.chromium.org/2834253002
Cr-Commit-Position: refs/heads/master@{#466839}
Committed: https://chromium.googlesource.com/chromium/src/+/ffdac2746296923b2658e9b7fa4a4e6f4ee79184

[gone, 2017-04-22 00:57:01]

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

[Ted C, 2017-04-22 20:17:06]

https://codereview.chromium.org/2834253002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
(right):

https://codereview.chromium.org/2834253002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:217:
mHandler.post(new Runnable() {
same question as the other init, do you want this to be per activity or just
once per process initialization?

If the latter, I think it is better to go in
ProcessInitializationHandler#handleDeferredStartupTasksInitialization
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

[gone, 2017-04-23 20:57:41]

Description was changed from

==========
🔍 Don't display the search engine until First Run completes

* Adds a check to prevent showing branding if First Run hasn't completed.

* Updates the widget so that it is updated when an AsyncInitializationActivity
  subclass calls onDeferredStartup().

* Changes the resource for the microphone to match the omnibox's, as well as
  uses its content description.

BUG=708844,710952
==========

to

==========
🔍 Don't display the search engine until First Run completes

* Adds a check to prevent showing branding if First Run hasn't completed.

* Updates the widget so that it is updated when an AsyncInitializationActivity
  subclass calls onDeferredStartup().

* Changes the resource for the microphone to match the omnibox's, as well as
  uses its content description.

* Adds tests to confirm that the search engine is updated once First Run
  completes.

BUG=708844,710952
==========

[gone, 2017-04-23 21:00:34]

Added some tests to see if this is working correctly.  Also started a thread
with Yoland about how to ensure tests that don't inherit from
ChromeActivityTestCaseBase clean up properly between runs because I've hit that
twice now.

https://codereview.chromium.org/2834253002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
(right):

https://codereview.chromium.org/2834253002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java:217:
mHandler.post(new Runnable() {
On 2017/04/22 20:17:06, Ted C wrote:
> same question as the other init, do you want this to be per activity or just
> once per process initialization?
> 
> If the latter, I think it is better to go in
> ProcessInitializationHandler#handleDeferredStartupTasksInitialization
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

Moved it to when the FirstRunActivity marks itself compelte.  I'll need another
check once the the promo dialog is in to force a widget update there.

[gone, 2017-04-23 21:00:39]

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

[commit-bot: I haz the power, 2017-04-23 21:00:51]

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-23 21:03:59]

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

[commit-bot: I haz the power, 2017-04-23 21:04:00]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  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-...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[gone, 2017-04-24 02:22:39]

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

[commit-bot: I haz the power, 2017-04-24 02:22:53]

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-24 02:45:09]

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

[commit-bot: I haz the power, 2017-04-24 02:45:10]

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-24 03:06:20]

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

[commit-bot: I haz the power, 2017-04-24 03:06:23]

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-24 04:26:09]

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

[commit-bot: I haz the power, 2017-04-24 04:26:10]

Dry run: This issue passed the CQ dry run.

[gone, 2017-04-24 20:52:29]

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

[commit-bot: I haz the power, 2017-04-24 20:53:05]

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-24 21:23:38]

lgtm

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

https://codereview.chromium.org/2834253002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:259:
String text = TextUtils.isEmpty(engineName) || !shouldShowFullString()
is this check necessary?  Should we have received an empty value here?

Worth asserting in that case?

https://codereview.chromium.org/2834253002/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/firstrun/FirstRunIntegrationTest.java
(right):

https://codereview.chromium.org/2834253002/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/firstrun/FirstRunIntegrationTest.java:209:
@SmallTest
I'd argue MediumTest at least...a fair amount of clicks and interactions

https://codereview.chromium.org/2834253002/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/firstrun/FirstRunIntegrationTest.java:236:
mTestObserver.jumpToPageCallback.waitForCallback(0);
For all these waitForCallback's, we should use the one that takes a string param
to make the failure cases clearer

[gone, 2017-04-24 21:36:18]

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

https://codereview.chromium.org/2834253002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:259:
String text = TextUtils.isEmpty(engineName) || !shouldShowFullString()
On 2017/04/24 21:23:38, Ted C wrote:
> is this check necessary?  Should we have received an empty value here?
> 
> Worth asserting in that case?

I _believe_ it's necessary in the case where Android automatically tells us to
update the widget (currently once per day).  It's a fallback to catch cases like
that.

https://codereview.chromium.org/2834253002/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/firstrun/FirstRunIntegrationTest.java
(right):

https://codereview.chromium.org/2834253002/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/firstrun/FirstRunIntegrationTest.java:209:
@SmallTest
On 2017/04/24 21:23:38, Ted C wrote:
> I'd argue MediumTest at least...a fair amount of clicks and interactions

Done.

https://codereview.chromium.org/2834253002/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/firstrun/FirstRunIntegrationTest.java:236:
mTestObserver.jumpToPageCallback.waitForCallback(0);
On 2017/04/24 21:23:38, Ted C wrote:
> For all these waitForCallback's, we should use the one that takes a string
param
> to make the failure cases clearer

Done.

[gone, 2017-04-24 21:36:39]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2017-04-24 21:36:39]

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/2834253002/#ps160001
(title: "Cmments")

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

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-24 22:37:06]

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

[commit-bot: I haz the power, 2017-04-24 22:37:07]

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

[gone, 2017-04-25 00:07:51]

The CQ bit was checked by dfalcantara@chromium.org

[commit-bot: I haz the power, 2017-04-25 00:08:26]

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 00:36:02]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493078871782210,
"parent_rev": "84eea314227407de46fbfde376d082bd41faa438", "commit_rev":
"ffdac2746296923b2658e9b7fa4a4e6f4ee79184"}

[commit-bot: I haz the power, 2017-04-25 00:36:56]

Description was changed from

==========
🔍 Don't display the search engine until First Run completes

* Adds a check to prevent showing branding if First Run hasn't completed.

* Updates the widget so that it is updated when an AsyncInitializationActivity
  subclass calls onDeferredStartup().

* Changes the resource for the microphone to match the omnibox's, as well as
  uses its content description.

* Adds tests to confirm that the search engine is updated once First Run
  completes.

BUG=708844,710952
==========

to

==========
🔍 Don't display the search engine until First Run completes

* Adds a check to prevent showing branding if First Run hasn't completed.

* Updates the widget so that it is updated when an AsyncInitializationActivity
  subclass calls onDeferredStartup().

* Changes the resource for the microphone to match the omnibox's, as well as
  uses its content description.

* Adds tests to confirm that the search engine is updated once First Run
  completes.

BUG=708844,710952

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

[commit-bot: I haz the power, 2017-04-25 00:36:57]

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

[perezju, 2017-04-25 11:55:07]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2839943002/ by perezju@chromium.org.

The reason for reverting is: Appears to have broken
FirstRunIntegrationTest#testClickThroughFirstRun
crbug.com/715054.