Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(15)

Issue 2873843002: Support autopresenting WebVr content. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 2 days ago by ymalik
Modified:
4 days, 1 hour ago
CC:
chromium-reviews, feature-vr-reviews_chromium.org, agrieve+watch_chromium.org, tiborg
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Support autopresenting WebVr content. We support autopresenting if the intent - launches a ChromeTabbedActivity - has a VR specific extra - originates from a first-party app There's no perfect way to know the sender of an intent. We use PendingIntents since it has a getCreatorPackage function that cannot be spoofed. That is, a trusted sender adds a PendingIntent as an extra which tags the intent with the sender credentials. The obvious risk here is that if something in between can get a hold of the PendingIntent, it can pretend to be the send indefinitely. That means that we must really trust the sender. This CL takes over bshe@'s https://codereview.chromium.org/2829193003/ Follow-up work will be to respond to this intent more elegantly so that we can get rid of the browser UI that will show up before we start autopresenting (see TODO). BUG=713369 TBR=dominickn@chromium.org Review-Url: https://codereview.chromium.org/2873843002 Cr-Commit-Position: refs/heads/master@{#473303} Committed: https://chromium.googlesource.com/chromium/src/+/e78a83bef3d557cd2f375caa189e7fe256b7b1ba

Patch Set 1 #

Total comments: 15

Patch Set 2 : Address review comments #

Total comments: 4

Patch Set 3 : address more review comments #

Total comments: 3

Patch Set 4 : move VrShellDelegate.onNewIntent call to CTA #

Total comments: 2

Patch Set 5 : tedchoc nit #

Patch Set 6 : nit #

Patch Set 7 : guard autopresentation behind a flag #

Patch Set 8 : add inVr assert #

Patch Set 9 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+90 lines, -1 line) Patch
M chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java View 1 2 3 4 5 6 7 8 1 chunk +15 lines, -0 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java View 1 2 3 4 5 6 7 8 9 chunks +51 lines, -1 line 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/android/chrome_feature_list.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/android/chrome_feature_list.cc View 1 2 3 4 5 6 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/flag_descriptions.h View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/flag_descriptions.cc View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download
M tools/metrics/histograms/enums.xml View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 45 (22 generated)
ymalik
Mike, PTAL :)
2 weeks, 2 days ago (2017-05-10 00:09:28 UTC) #3
ymalik
2 weeks, 2 days ago (2017-05-10 00:09:39 UTC) #4
ymalik
+tedchoc, please sign-off on the first-party verification part as we talked offline.
2 weeks, 2 days ago (2017-05-10 00:11:00 UTC) #6
mthiesse
https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode650 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:650: public boolean canAutopresentVr() { I don't know what Ted's ...
2 weeks, 1 day ago (2017-05-10 14:52:14 UTC) #8
Ted C
https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode650 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:650: public boolean canAutopresentVr() { On 2017/05/10 14:52:14, mthiesse wrote: ...
2 weeks, 1 day ago (2017-05-11 00:05:03 UTC) #9
ymalik
mthiesse@, tedchoc@ PTAL :) https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode650 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:650: public boolean canAutopresentVr() { On ...
2 weeks, 1 day ago (2017-05-11 03:32:27 UTC) #10
mthiesse
https://codereview.chromium.org/2873843002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode919 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:919: VrShellDelegate.onNewIntent(this, getIntent()); s/getIntent()/intent https://codereview.chromium.org/2873843002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2873843002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java#newcode870 chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:870: ...
2 weeks ago (2017-05-11 14:25:17 UTC) #11
ymalik
mthiesse, PTAL :) I also added a safety check when VrShellDelegate handles the intent to ...
2 weeks ago (2017-05-11 17:22:56 UTC) #13
mthiesse
lgtm https://codereview.chromium.org/2873843002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode745 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:745: // We send this intent so that we ...
2 weeks ago (2017-05-11 17:29:15 UTC) #14
Ted C
https://codereview.chromium.org/2873843002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode745 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:745: // We send this intent so that we can ...
2 weeks ago (2017-05-11 18:10:38 UTC) #15
ymalik
@tedchoc PTAL :) https://codereview.chromium.org/2873843002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java#newcode745 chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:745: // We send this intent so ...
2 weeks ago (2017-05-11 21:17:53 UTC) #16
Ted C
lgtm https://codereview.chromium.org/2873843002/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2873843002/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java#newcode543 chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:543: if (intent.getBooleanExtra(DAYDREAM_VR_EXTRA, false) nit, I would use IntentUtils.safeGetBooleanExtra
2 weeks ago (2017-05-11 21:51:18 UTC) #17
ymalik
https://codereview.chromium.org/2873843002/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2873843002/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java#newcode543 chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:543: if (intent.getBooleanExtra(DAYDREAM_VR_EXTRA, false) On 2017/05/11 21:51:17, Ted C wrote: ...
2 weeks ago (2017-05-11 22:44:23 UTC) #18
ymalik
1 week, 6 days ago (2017-05-12 17:46:36 UTC) #23
estark
Conceptually, we're okay with this for security. However, can you please add dominickn@ as TBR, ...
1 week, 3 days ago (2017-05-15 18:59:59 UTC) #24
ymalik
On 2017/05/15 18:59:59, estark (temporarily slow) wrote: > Conceptually, we're okay with this for security. ...
6 days, 15 hours ago (2017-05-19 15:33:31 UTC) #25
ymalik
+mariakhomenko to confirm the changes in IntentHandler re whitelisting
6 days, 15 hours ago (2017-05-19 15:38:08 UTC) #28
mthiesse
flag changes lgtm
6 days, 15 hours ago (2017-05-19 15:44:07 UTC) #31
Maria
On 2017/05/19 15:44:07, mthiesse wrote: > flag changes lgtm IntentHandler code lgtm
6 days, 14 hours ago (2017-05-19 16:50:03 UTC) #32
estark
Thanks for the changes! lgtm
6 days, 12 hours ago (2017-05-19 18:30:58 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2873843002/160001
6 days, 10 hours ago (2017-05-19 20:35:25 UTC) #41
commit-bot: I haz the power
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/e78a83bef3d557cd2f375caa189e7fe256b7b1ba
6 days, 10 hours ago (2017-05-19 20:42:47 UTC) #44
dominickn
4 days, 1 hour ago (2017-05-22 05:53:41 UTC) #45
Message was sent while issue was closed.
Thanks for the TBR here. This lgtm from a Security UX perspective, assuming that
crbug.com/724361 is addressed prior to launch.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06