Prevent possible crash unregistering VR BroadcastReceiver

Prevent possible crash unregistering VR BroadcastReceiver

I only saw this crash once, and can't reproduce, but better safe than sorry.

BUG=712823
Review-Url: https://codereview.chromium.org/2830463002
Cr-Commit-Position: refs/heads/master@{#465702}
Committed: https://chromium.googlesource.com/chromium/src/+/3eb6c9fae5bf061ac97480aa6670858c7c8cbc8f

[mthiesse, 2017-04-18 21:45:17]

mthiesse@chromium.org changed reviewers:
+ bshe@chromium.org

[mthiesse, 2017-04-18 21:45:17]

PTAL

[bshe, 2017-04-18 22:14:31]

lgtm

https://codereview.chromium.org/2830463002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2830463002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:162:
} catch (Exception e) {
Does it make sense to print the message of this exception?

[mthiesse, 2017-04-18 22:33:05]

https://codereview.chromium.org/2830463002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2830463002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:162:
} catch (Exception e) {
On 2017/04/18 22:14:30, bshe wrote:
> Does it make sense to print the message of this exception?

It's just an illegalArgumentException that tells us we weren't registered. I
don't think it's ever going to be useful/meaningful.

[amp, 2017-04-18 22:58:25]

Nit, feel free to ignore if you don't agree.

https://codereview.chromium.org/2830463002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2830463002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:162:
} catch (Exception e) {
On 2017/04/18 22:33:05, mthiesse wrote:
> On 2017/04/18 22:14:30, bshe wrote:
> > Does it make sense to print the message of this exception?
> 
> It's just an illegalArgumentException that tells us we weren't registered. I
> don't think it's ever going to be useful/meaningful.

A comment specifying that this is catching the case where the receiver wasn't
registered would make it clear.

Also the Java dev in me (it's been 4+ years, but still) cringes at seeing a
catch of the base 'Exception' instead of the more specific one.  If we know it's
always going to be an IllegalArgumentException let's catch that one instead.

[mthiesse, 2017-04-19 18:39:49]

https://codereview.chromium.org/2830463002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2830463002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:162:
} catch (Exception e) {
On 2017/04/18 22:58:25, amp wrote:
> On 2017/04/18 22:33:05, mthiesse wrote:
> > On 2017/04/18 22:14:30, bshe wrote:
> > > Does it make sense to print the message of this exception?
> > 
> > It's just an illegalArgumentException that tells us we weren't registered. I
> > don't think it's ever going to be useful/meaningful.
> 
> A comment specifying that this is catching the case where the receiver wasn't
> registered would make it clear.
> 
> Also the Java dev in me (it's been 4+ years, but still) cringes at seeing a
> catch of the base 'Exception' instead of the more specific one.  If we know
it's
> always going to be an IllegalArgumentException let's catch that one instead.

Done.

[mthiesse, 2017-04-19 18:40:40]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2017-04-19 18:40:40]

The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2830463002/#ps20001
(title: "Address comments")

[commit-bot: I haz the power, 2017-04-19 18:41:21]

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 19:24:12]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492627240116680,
"parent_rev": "9a2eb472fde51de48c095c9c208101c534ac211e", "commit_rev":
"3eb6c9fae5bf061ac97480aa6670858c7c8cbc8f"}

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

Description was changed from

==========
Prevent possible crash unregistering VR BroadcastReceiver

I only saw this crash once, and can't reproduce, but better safe than sorry.

BUG=712823
==========

to

==========
Prevent possible crash unregistering VR BroadcastReceiver

I only saw this crash once, and can't reproduce, but better safe than sorry.

BUG=712823

Review-Url: https://codereview.chromium.org/2830463002
Cr-Commit-Position: refs/heads/master@{#465702}
Committed:
https://chromium.googlesource.com/chromium/src/+/3eb6c9fae5bf061ac97480aa6670...
==========

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

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/3eb6c9fae5bf061ac97480aa6670...