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

Issue 2873843002: Support autopresenting WebVr content. (Closed)

Created:
6 months, 2 weeks ago by ymalik
Modified:
6 months 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

Messages

Total messages: 45 (22 generated)
ymalik
Mike, PTAL :)
6 months, 2 weeks ago (2017-05-10 00:09:28 UTC) #3
ymalik
6 months, 2 weeks ago (2017-05-10 00:09:39 UTC) #4
ymalik
+tedchoc, please sign-off on the first-party verification part as we talked offline.
6 months, 2 weeks 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 ...
6 months, 2 weeks 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: ...
6 months, 2 weeks 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 ...
6 months, 2 weeks 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: ...
6 months, 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 ...
6 months, 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 ...
6 months, 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 ...
6 months, 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 ...
6 months, 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
6 months, 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: ...
6 months, 2 weeks ago (2017-05-11 22:44:23 UTC) #18
ymalik
6 months, 2 weeks 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, ...
6 months, 1 week 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 months, 1 week ago (2017-05-19 15:33:31 UTC) #25
ymalik
+mariakhomenko to confirm the changes in IntentHandler re whitelisting
6 months, 1 week ago (2017-05-19 15:38:08 UTC) #28
mthiesse
flag changes lgtm
6 months, 1 week 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 months, 1 week ago (2017-05-19 16:50:03 UTC) #32
estark
Thanks for the changes! lgtm
6 months, 1 week 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 months, 1 week 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 months, 1 week ago (2017-05-19 20:42:47 UTC) #44
dominickn
6 months 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.

Powered by Google App Engine
This is Rietveld efc10ee0f