🔍 General widget fixes

🔍 General widget fixes

* Clean up initialization slightly.  Now that there's no animation to show,
  we can reduce the deviation from the main pathway.

* Add a "temporary" title to the search widget so it doesn't just say
  "Chromium" when adding the widget.

* Swallow exceptions when the BroadcastReceiver is triggered via an Intent
  and tries to do anything.  This prevents us from getting into situations
  where Android believes the "process is bad" and refuses to let the widget
  work, but it does mask when real crashes happen.  After three crashes we
  just let it crash as intended so that the dump uploads.

BUG=708844, 712061
Review-Url: https://codereview.chromium.org/2822633002
Cr-Commit-Position: refs/heads/master@{#466127}
Committed: https://chromium.googlesource.com/chromium/src/+/f32cf78bd306f2750fbc5deb14e33169c894be70

[gone, 2017-04-14 00:09:24]

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

[gone, 2017-04-14 00:09:25]

We _seem_ to be in a good spot for now.  Going to start working on tests after
this.

[gone, 2017-04-14 07:22:09]

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

[commit-bot: I haz the power, 2017-04-14 07:22:25]

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-14 08:00:13]

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

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

Dry run: This issue passed the CQ dry run.

[gone, 2017-04-14 23:34:48]

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

[commit-bot: I haz the power, 2017-04-14 23:35:34]

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-15 00:15:05]

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

[commit-bot: I haz the power, 2017-04-15 00:15:06]

Dry run: This issue passed the CQ dry run.

[gone, 2017-04-17 00:26:33]

Description was changed from

==========
🔍 Further widget cleanup

* Clean up initialization slightly.  Now that there's no animation to show,
  we can reduce the deviation from the main pathway.

* Add a "temporary" title to the search widget so it doesn't just say
  "Chromium" when adding the widget.

BUG=708844
==========

to

==========
🔍 General widget fixes

* Clean up initialization slightly.  Now that there's no animation to show,
  we can reduce the deviation from the main pathway.

* Add a "temporary" title to the search widget so it doesn't just say
  "Chromium" when adding the widget.

* Swallow exceptions when the BroadcastReceiver is triggered via an Intent
  and tries to do anything.  This prevents us from getting into situations
  where Android believes the "process is bad" and refuses to let the widget
  work, but it does mask when real crashes happen.

BUG=708844
==========

[gone, 2017-04-17 00:45:16]

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

[commit-bot: I haz the power, 2017-04-17 00:45:19]

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-17 00:46:32]

Description was changed from

==========
🔍 General widget fixes

* Clean up initialization slightly.  Now that there's no animation to show,
  we can reduce the deviation from the main pathway.

* Add a "temporary" title to the search widget so it doesn't just say
  "Chromium" when adding the widget.

* Swallow exceptions when the BroadcastReceiver is triggered via an Intent
  and tries to do anything.  This prevents us from getting into situations
  where Android believes the "process is bad" and refuses to let the widget
  work, but it does mask when real crashes happen.

BUG=708844
==========

to

==========
🔍 General widget fixes

* Clean up initialization slightly.  Now that there's no animation to show,
  we can reduce the deviation from the main pathway.

* Add a "temporary" title to the search widget so it doesn't just say
  "Chromium" when adding the widget.

* Swallow exceptions when the BroadcastReceiver is triggered via an Intent
  and tries to do anything.  This prevents us from getting into situations
  where Android believes the "process is bad" and refuses to let the widget
  work, but it does mask when real crashes happen.

BUG=708844,712061
==========

[commit-bot: I haz the power, 2017-04-17 01:21:06]

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

[commit-bot: I haz the power, 2017-04-17 01:21:07]

Dry run: This issue passed the CQ dry run.

[gone, 2017-04-18 16:21:32]

WDYT about swallowing Exceptions only on Stable?  That gives us the chance to
fix things for the other channels before they become super problematic and
uncatchable.

[Ted C, 2017-04-18 17:33:58]

https://codereview.chromium.org/2822633002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java
(right):

https://codereview.chromium.org/2822633002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:162:
// This is necessary to prevent Android refusing to launch the widget again.
From the other CL...

Here and below, we should try to find a way to hook into:
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...

Ideally, we could use that and still ensure the widget doesn't get hosed.

[gone, 2017-04-18 17:35:41]

https://codereview.chromium.org/2822633002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java
(right):

https://codereview.chromium.org/2822633002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:162:
// This is necessary to prevent Android refusing to launch the widget again.
On 2017/04/18 17:33:58, Ted C wrote:
> From the other CL...
> 
> Here and below, we should try to find a way to hook into:
>
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...
> 
> Ideally, we could use that and still ensure the widget doesn't get hosed.

Yeah, that'd be nice... it'd require native to be loaded, though, right?  If we
hit this point, I don't know if we'd be able to guarantee it.  Should it be best
effort?

[Ted C, 2017-04-18 17:38:16]

https://codereview.chromium.org/2822633002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java
(right):

https://codereview.chromium.org/2822633002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:162:
// This is necessary to prevent Android refusing to launch the widget again.
On 2017/04/18 17:35:41, ping past 24hrs (dfalcantara) wrote:
> On 2017/04/18 17:33:58, Ted C wrote:
> > From the other CL...
> > 
> > Here and below, we should try to find a way to hook into:
> >
>
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...
> > 
> > Ideally, we could use that and still ensure the widget doesn't get hosed.
> 
> Yeah, that'd be nice... it'd require native to be loaded, though, right?  If
we
> hit this point, I don't know if we'd be able to guarantee it.  Should it be
best
> effort?

I think best effort is fine.  One thing we could also do is keep a static crash
count (or shared pref).  If we crash X times or within X time limit, we maybe
should consider crashing to have a chance to capture this.  If native hasn't
initialized, then I think we are going to be relying on android feedback
mechanisms.

[gone, 2017-04-18 17:39:18]

https://codereview.chromium.org/2822633002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java
(right):

https://codereview.chromium.org/2822633002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:162:
// This is necessary to prevent Android refusing to launch the widget again.
On 2017/04/18 17:38:16, Ted C wrote:
> On 2017/04/18 17:35:41, ping past 24hrs (dfalcantara) wrote:
> > On 2017/04/18 17:33:58, Ted C wrote:
> > > From the other CL...
> > > 
> > > Here and below, we should try to find a way to hook into:
> > >
> >
>
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...
> > > 
> > > Ideally, we could use that and still ensure the widget doesn't get hosed.
> > 
> > Yeah, that'd be nice... it'd require native to be loaded, though, right?  If
> we
> > hit this point, I don't know if we'd be able to guarantee it.  Should it be
> best
> > effort?
> 
> I think best effort is fine.  One thing we could also do is keep a static
crash
> count (or shared pref).  If we crash X times or within X time limit, we maybe
> should consider crashing to have a chance to capture this.  If native hasn't
> initialized, then I think we are going to be relying on android feedback
> mechanisms.

Yeah, was thinking something similar.  Figured we could add a metric here, as
well.

[gone, 2017-04-18 23:21:01]

Description was changed from

==========
🔍 General widget fixes

* Clean up initialization slightly.  Now that there's no animation to show,
  we can reduce the deviation from the main pathway.

* Add a "temporary" title to the search widget so it doesn't just say
  "Chromium" when adding the widget.

* Swallow exceptions when the BroadcastReceiver is triggered via an Intent
  and tries to do anything.  This prevents us from getting into situations
  where Android believes the "process is bad" and refuses to let the widget
  work, but it does mask when real crashes happen.

BUG=708844,712061
==========

to

==========
🔍 General widget fixes

* Clean up initialization slightly.  Now that there's no animation to show,
  we can reduce the deviation from the main pathway.

* Add a "temporary" title to the search widget so it doesn't just say
  "Chromium" when adding the widget.

* Swallow exceptions when the BroadcastReceiver is triggered via an Intent
  and tries to do anything.  This prevents us from getting into situations
  where Android believes the "process is bad" and refuses to let the widget
  work, but it does mask when real crashes happen.  After three crashes we
  just let it crash as intended so that the dump uploads.

BUG=708844,712061
==========

[gone, 2017-04-18 23:22:41]

WDYT about this?  Third consecutive crash lets the crashes happen.

Yusuf: I also moved the initialization out of ChromeBrowserInitializer and into
AsyncInitializationActivity, instead.  Seemed a safer place to put it.

[gone, 2017-04-18 23:23:01]

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

[commit-bot: I haz the power, 2017-04-18 23:23:53]

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-18 23:47:17]

https://codereview.chromium.org/2822633002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
(left):

https://codereview.chromium.org/2822633002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:411:
SearchWidgetProvider.initialize();
Why don't you want this here?  How often do you want this to be initialized? 
Once per browser process or once per activity?

In general, I prefer init stuff to be here for "needs to be run for each
component" or in ProcessInitializationHandler#handlePostNativeInitialization if
it should be once per process.

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

https://codereview.chromium.org/2822633002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:312:
editor.commit();
that likely will need to be wrapped in a strictmode exception

https://codereview.chromium.org/2822633002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:346:
Log.e(TAG, "Absorbing exception caught when attempting to launch widget.", e);
Should we show a toast in this scenario?

Uh oh! Something failed. Please try again. K thx

And you think native would never be initialized so hooking into the
javaexception reporter would be overkill?

[gone, 2017-04-19 00:01:20]

https://codereview.chromium.org/2822633002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
(left):

https://codereview.chromium.org/2822633002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java:411:
SearchWidgetProvider.initialize();
On 2017/04/18 23:47:17, Ted C wrote:
> Why don't you want this here?  How often do you want this to be initialized? 
> Once per browser process or once per activity?
> 
> In general, I prefer init stuff to be here for "needs to be run for each
> component" or in ProcessInitializationHandler#handlePostNativeInitialization
if
> it should be once per process.

Done.

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

https://codereview.chromium.org/2822633002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:312:
editor.commit();
On 2017/04/18 23:47:17, Ted C wrote:
> that likely will need to be wrapped in a strictmode exception

Done.

https://codereview.chromium.org/2822633002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java:346:
Log.e(TAG, "Absorbing exception caught when attempting to launch widget.", e);
On 2017/04/18 23:47:17, Ted C wrote:
> Should we show a toast in this scenario?
> 
> Uh oh! Something failed. Please try again. K thx
> 
> And you think native would never be initialized so hooking into the
> javaexception reporter would be overkill?

Toast is a UX question, but I don't know how actionable it would be for the
user.

Is there a way to know if the browser process is initialized so that we'd be
able to call the reporter?  Also, does it actually take in Exceptions?  I'm not
sure how to get a stack trace from one.

[gone, 2017-04-19 00:02:27]

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

[commit-bot: I haz the power, 2017-04-19 00:03:22]

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-19 00:42:53]

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

[commit-bot: I haz the power, 2017-04-19 00:42:54]

Dry run: This issue passed the CQ dry run.

[Ted C, 2017-04-20 17:57:00]

lgtm

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

https://codereview.chromium.org/2822633002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchBoxDataProvider.java:29:
if (!service.isLoaded()) service.load();
load does nothing if it is already loaded, so I don't think you need this check
(the load listener will always be called even if it is already loaded BTW)

[gone, 2017-04-20 20:13:55]

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

https://codereview.chromium.org/2822633002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchBoxDataProvider.java:29:
if (!service.isLoaded()) service.load();
On 2017/04/20 17:57:00, Ted C wrote:
> load does nothing if it is already loaded, so I don't think you need this
check
> (the load listener will always be called even if it is already loaded BTW)

Done.

[gone, 2017-04-20 20:14:01]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2017-04-20 20:14:02]

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/2822633002/#ps140001
(title: "🔍 Further widget cleanup")

[commit-bot: I haz the power, 2017-04-20 20:15:08]

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-20 21:01:53]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492719241796660,
"parent_rev": "e510ebfc9f8565fc8cec92955b631aae49c17fd3", "commit_rev":
"f32cf78bd306f2750fbc5deb14e33169c894be70"}

[commit-bot: I haz the power, 2017-04-20 21:06:09]

Description was changed from

==========
🔍 General widget fixes

* Clean up initialization slightly.  Now that there's no animation to show,
  we can reduce the deviation from the main pathway.

* Add a "temporary" title to the search widget so it doesn't just say
  "Chromium" when adding the widget.

* Swallow exceptions when the BroadcastReceiver is triggered via an Intent
  and tries to do anything.  This prevents us from getting into situations
  where Android believes the "process is bad" and refuses to let the widget
  work, but it does mask when real crashes happen.  After three crashes we
  just let it crash as intended so that the dump uploads.

BUG=708844,712061
==========

to

==========
🔍 General widget fixes

* Clean up initialization slightly.  Now that there's no animation to show,
  we can reduce the deviation from the main pathway.

* Add a "temporary" title to the search widget so it doesn't just say
  "Chromium" when adding the widget.

* Swallow exceptions when the BroadcastReceiver is triggered via an Intent
  and tries to do anything.  This prevents us from getting into situations
  where Android believes the "process is bad" and refuses to let the widget
  work, but it does mask when real crashes happen.  After three crashes we
  just let it crash as intended so that the dump uploads.

BUG=708844,712061

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

[commit-bot: I haz the power, 2017-04-20 21:06:11]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/f32cf78bd306f2750fbc5deb14e3...