Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528433004:
[RemotePlayback] Don't expose Remote Playback API for WebView. (Closed)
|
Created:
4 years ago by whywhat Modified:
4 years ago CC:
android-webview-reviews_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, haraken, jam, kinuko+watch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[RemotePlayback] Don't expose Remote Playback API for WebView.
I don't recall if we decided to support disableRemotePlayback on
WebView, but I don't see why we would want it.
BUG=578833
TEST=WebViewLayoutTest
Committed: https://crrev.com/df718b9d35dbd3d98dffc0947e0c26d38d065c12
Cr-Commit-Position: refs/heads/master@{#435504}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Addressed Tim's comments #Patch Set 3 : Verified the CL passes WebView tests #Patch Set 4 : Rebased on the right patchset #Depends on Patchset: Messages
Total messages: 43 (22 generated)
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
avayvod@chromium.org changed reviewers: + timvolodine@chromium.org
PTaL
lgtm with a few comments https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt (left): https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt:2071: setter disableRemotePlayback I guess the RemotePlayback interface is not in this file because it is somewhat stale.. https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/test/data/webexposed/not-webview-exposed.txt (right): https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/webexposed/not-webview-exposed.txt:40: setter disableRemotePlayback also, maybe add a bug reference here, as it doesn't appear related to crbug.com/589500 sinkId and setSinkId https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/webexposed/not-webview-exposed.txt:41: getter disableRemotePlayback are these the only methods regarding remote playback api that should not be exposed? https://codereview.chromium.org/2528433004/diff/1/content/public/common/conte... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2528433004/diff/1/content/public/common/conte... content/public/common/content_switches.cc:264: const char kDisableRemotePlaybackAPI[] = "disable-remote-playback-api"; nit: align "=" with above constants?
Addressed Tim's comments
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
I had a hard time running the tests locally with the up-to-date idl's and even C++ code - ninja says there's no work to do and the test fails with disableRemotePlayback exposed on WebView despite the clear [RuntimeEnabled=RemotePlayback]. I'm very frustrated and hope it will pass on the bots. https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt (left): https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt:2071: setter disableRemotePlayback On 2016/11/24 at 17:06:02, timvolodine wrote: > I guess the RemotePlayback interface is not in this file because it is somewhat stale.. Yes, but with it being disabled, it shouldn't be here, right? https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/test/data/webexposed/not-webview-exposed.txt (right): https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/webexposed/not-webview-exposed.txt:40: setter disableRemotePlayback On 2016/11/24 at 17:06:02, timvolodine wrote: > also, maybe add a bug reference here, as it doesn't appear related to crbug.com/589500 sinkId and setSinkId Will do. https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/webexposed/not-webview-exposed.txt:41: getter disableRemotePlayback On 2016/11/24 at 17:06:03, timvolodine wrote: > are these the only methods regarding remote playback api that should not be exposed? On HTMLMediaElement, yes. Well, there's also the 'remote' property I might have missed? https://codereview.chromium.org/2528433004/diff/1/content/public/common/conte... File content/public/common/content_switches.cc (right): https://codereview.chromium.org/2528433004/diff/1/content/public/common/conte... content/public/common/content_switches.cc:264: const char kDisableRemotePlaybackAPI[] = "disable-remote-playback-api"; On 2016/11/24 at 17:06:03, timvolodine wrote: > nit: align "=" with above constants? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/28 23:50:47, whywhat_OOO_till_Mon_Nov_28 wrote: > I had a hard time running the tests locally with the up-to-date idl's and even > C++ code - ninja says there's no work to do and the test fails with > disableRemotePlayback exposed on WebView despite the clear > [RuntimeEnabled=RemotePlayback]. > > I'm very frustrated and hope it will pass on the bots. I think you're not building/installing the right things? The shell doesn't contain any blink code (or any native code at all) and so it does indeed not need to be recompiled. The shell is testing the actual WebView installed on the current device - you need to build the webview that you want to test against and install it in addition to the test shell. > https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... > File > android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt > (left): > > https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... > android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt:2071: > setter disableRemotePlayback > On 2016/11/24 at 17:06:02, timvolodine wrote: > > I guess the RemotePlayback interface is not in this file because it is > somewhat stale.. > > Yes, but with it being disabled, it shouldn't be here, right? > > https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... > File > android_webview/tools/system_webview_shell/test/data/webexposed/not-webview-exposed.txt > (right): > > https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... > android_webview/tools/system_webview_shell/test/data/webexposed/not-webview-exposed.txt:40: > setter disableRemotePlayback > On 2016/11/24 at 17:06:02, timvolodine wrote: > > also, maybe add a bug reference here, as it doesn't appear related to > crbug.com/589500 sinkId and setSinkId > > Will do. > > https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... > android_webview/tools/system_webview_shell/test/data/webexposed/not-webview-exposed.txt:41: > getter disableRemotePlayback > On 2016/11/24 at 17:06:03, timvolodine wrote: > > are these the only methods regarding remote playback api that should not be > exposed? > > On HTMLMediaElement, yes. Well, there's also the 'remote' property I might have > missed? > > https://codereview.chromium.org/2528433004/diff/1/content/public/common/conte... > File content/public/common/content_switches.cc (right): > > https://codereview.chromium.org/2528433004/diff/1/content/public/common/conte... > content/public/common/content_switches.cc:264: const char > kDisableRemotePlaybackAPI[] = "disable-remote-playback-api"; > On 2016/11/24 at 17:06:03, timvolodine wrote: > > nit: align "=" with above constants? > > Done.
On 2016/11/29 13:26:05, Torne wrote: > On 2016/11/28 23:50:47, whywhat_OOO_till_Mon_Nov_28 wrote: > > I had a hard time running the tests locally with the up-to-date idl's and even > > C++ code - ninja says there's no work to do and the test fails with > > disableRemotePlayback exposed on WebView despite the clear > > [RuntimeEnabled=RemotePlayback]. > > > > I'm very frustrated and hope it will pass on the bots. > > I think you're not building/installing the right things? The shell doesn't > contain any blink code (or any native code at all) and so it does indeed not > need to be recompiled. The shell is testing the actual WebView installed on the > current device - you need to build the webview that you want to test against and > install it in addition to the test shell. > yes, that's what the bots do as well: explicitly build and install the webview to test. I guess this can be confusing, not sure if we should make system_webview_shell_layout_test_apk depend on webview though..
torne@chromium.org changed reviewers: + torne@chromium.org
I think that'd just be even more confusing because you'd wonder why nothing changes even though it rebuilt.
https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... File android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt (left): https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt:2071: setter disableRemotePlayback On 2016/11/28 23:50:46, whywhat_OOO_till_Mon_Nov_28 wrote: > On 2016/11/24 at 17:06:02, timvolodine wrote: > > I guess the RemotePlayback interface is not in this file because it is > somewhat stale.. > > Yes, but with it being disabled, it shouldn't be here, right? oh I see it's actually 'experimental' so yes makes sense
On 2016/11/29 at 15:30:15, timvolodine wrote: > https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... > File android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt (left): > > https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... > android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt:2071: setter disableRemotePlayback > On 2016/11/28 23:50:46, whywhat_OOO_till_Mon_Nov_28 wrote: > > On 2016/11/24 at 17:06:02, timvolodine wrote: > > > I guess the RemotePlayback interface is not in this file because it is > > somewhat stale.. > > > > Yes, but with it being disabled, it shouldn't be here, right? > > oh I see it's actually 'experimental' so yes makes sense No, disableRemotePlayback has shipped as a stable feature last year IIRC and we haven't disabled it for WebView at the time (does VideoFling work on WebView?). I'm putting it behind the RemotePlayback flag in this CL to remove it from WebView -- therefore it should be removed from the expectations I think. However, we may want to leave it if casting videos with the default controls works on WebView and we want to allow developers to opt-out.
On 2016/11/29 15:33:44, whywhat_OOO_till_Mon_Nov_28 wrote: > On 2016/11/29 at 15:30:15, timvolodine wrote: > > > https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... > > File > android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt > (left): > > > > > https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... > > > android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt:2071: > setter disableRemotePlayback > > On 2016/11/28 23:50:46, whywhat_OOO_till_Mon_Nov_28 wrote: > > > On 2016/11/24 at 17:06:02, timvolodine wrote: > > > > I guess the RemotePlayback interface is not in this file because it is > > > somewhat stale.. > > > > > > Yes, but with it being disabled, it shouldn't be here, right? > > > > oh I see it's actually 'experimental' so yes makes sense > > No, disableRemotePlayback has shipped as a stable feature last year IIRC and we > haven't disabled it for WebView at the time (does VideoFling work on WebView?). > I'm putting it behind the RemotePlayback flag in this CL to remove it from > WebView -- therefore it should be removed from the expectations I think. > > However, we may want to leave it if casting videos with the default controls > works on WebView and we want to allow developers to opt-out. I actually meant the RemotePlayback interface which is already guarded by RemotePlayback blink flag which is experimental. We can probably keep it in the non-webview-exposed.txt to have the correct behavior in case the flag goes stable.
On 2016/11/29 at 15:43:48, timvolodine wrote: > On 2016/11/29 15:33:44, whywhat_OOO_till_Mon_Nov_28 wrote: > > On 2016/11/29 at 15:30:15, timvolodine wrote: > > > > > https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... > > > File > > android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt > > (left): > > > > > > > > https://codereview.chromium.org/2528433004/diff/1/android_webview/tools/syste... > > > > > android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt:2071: > > setter disableRemotePlayback > > > On 2016/11/28 23:50:46, whywhat_OOO_till_Mon_Nov_28 wrote: > > > > On 2016/11/24 at 17:06:02, timvolodine wrote: > > > > > I guess the RemotePlayback interface is not in this file because it is > > > > somewhat stale.. > > > > > > > > Yes, but with it being disabled, it shouldn't be here, right? > > > > > > oh I see it's actually 'experimental' so yes makes sense > > > > No, disableRemotePlayback has shipped as a stable feature last year IIRC and we > > haven't disabled it for WebView at the time (does VideoFling work on WebView?). > > I'm putting it behind the RemotePlayback flag in this CL to remove it from > > WebView -- therefore it should be removed from the expectations I think. > > > > However, we may want to leave it if casting videos with the default controls > > works on WebView and we want to allow developers to opt-out. > > I actually meant the RemotePlayback interface which is already guarded by RemotePlayback blink flag which is experimental. > We can probably keep it in the non-webview-exposed.txt to have the correct behavior in case the flag goes stable. That's what I think I do in this change (it depends on the change that makes the flag go stable :)
Verified the CL passes WebView tests
avayvod@chromium.org changed reviewers: + pfeldman@chromium.org
Reviews I still need: - Torne - android_webview/lib - Pavel - third_party/WebKit and content/ Please, take a look.
Rebased on the right patchset
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Oh, sorry, didn't see that you actually needed a review from me. android_webview LGTM.
thakis@chromium.org changed reviewers: + thakis@chromium.org
webkit/ lgtm
avi@chromium.org changed reviewers: + avi@chromium.org
lgtm ok
avayvod@chromium.org changed reviewers: - pfeldman@chromium.org
+avi for content +thakis for WebKit
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/2528433004/#ps60001 (title: "Rebased on the right patchset")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480540823824370,
"parent_rev": "800152da884e41fd7ba74e959667b72a62658fba", "commit_rev":
"d08c4d1ca6a4a64cfc9e8d6aee6ba28ab84d3e4b"}
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [RemotePlayback] Don't expose Remote Playback API for WebView. I don't recall if we decided to support disableRemotePlayback on WebView, but I don't see why we would want it. BUG=578833 TEST=WebViewLayoutTest ========== to ========== [RemotePlayback] Don't expose Remote Playback API for WebView. I don't recall if we decided to support disableRemotePlayback on WebView, but I don't see why we would want it. BUG=578833 TEST=WebViewLayoutTest Committed: https://crrev.com/df718b9d35dbd3d98dffc0947e0c26d38d065c12 Cr-Commit-Position: refs/heads/master@{#435504} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/df718b9d35dbd3d98dffc0947e0c26d38d065c12 Cr-Commit-Position: refs/heads/master@{#435504} |
