New issue
Advanced search Search tips

Issue 652366 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 477150



Sign in to add a comment

window.internals.settings.setMediaPlaybackRequiresUserGesture is not replicated across OOPIFs

Project Member Reported by lukasza@chromium.org, Oct 3 2016

Issue description

Repro:
$ third_party/WebKit/Tools/Scripts/run-webkit-tests \
      --additional-drt-flag=--site-per-process \
      -t gn -v http/tests/webaudio/autoplay-crossorigin.html
 

Comment 1 by rtoy@chromium.org, Oct 3 2016

mlamouri@ Can you take a look since you added this test?
It seems that the following lines from http/tests/webaudio/autoplay-crossorigin.html:

  if ('internals' in window)
    window.internals.settings.setMediaPlaybackRequiresUserGesture(true);

only have effect in a single renderer process.  A simple fix would be to replicate the lines above into third_party/WebKit/LayoutTests/http/tests/webaudio/resources/autoplay-crossorigin-iframe.html (FWIW, I've verified that the test starts to pass [in --site-per-process mode] on my machine after this fix).

Anyway - for now I think I'll just quickly prepare a CL to disable the test for --site-per-process mode to make the bot green again (and we can decide about a proper fix later).
Cc: -lukasza@chromium.org
Owner: lukasza@chromium.org
Status: Started (was: Untriaged)
Summary: window.internals.settings.setMediaPlaybackRequiresUserGesture is not replicated across OOPIFs (was: http/tests/webaudio/autoplay-crossorigin.html fails in --site-per-process mode)
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c1bc520322913b5b384414980909b6937beab98f

commit c1bc520322913b5b384414980909b6937beab98f
Author: lukasza <lukasza@chromium.org>
Date: Mon Oct 03 18:58:03 2016

Skip http/tests/webaudio/autoplay-crossorigin.html in --site-per-process mode.

BUG= 652366 
NOTRY=true

Review-Url: https://codereview.chromium.org/2392643002
Cr-Commit-Position: refs/heads/master@{#422477}

[modify] https://crrev.com/c1bc520322913b5b384414980909b6937beab98f/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f33a8c06201766c37344768aaf99846f78857ff3

commit f33a8c06201766c37344768aaf99846f78857ff3
Author: lukasza <lukasza@chromium.org>
Date: Tue Oct 04 10:50:13 2016

Move setMediaPlaybackRequiresUserGesture to the cross-origin frame.

Before this CL the http/tests/webaudio/autoplay-crossorigin.html layout
test would fail if the main frame and the cross-origin frame are in
different renderer processes.  After this CL, the internal settings are
set from the same frame that hosts AudioContext object - this ensures
that setting the settings and getting the settings happens from the same
renderer process (and consequently ensures that the test passes both
with and without --site-per-process flag).

BUG= 652366 

Review-Url: https://codereview.chromium.org/2389963002
Cr-Commit-Position: refs/heads/master@{#422746}

[modify] https://crrev.com/f33a8c06201766c37344768aaf99846f78857ff3/third_party/WebKit/LayoutTests/FlagExpectations/site-per-process
[modify] https://crrev.com/f33a8c06201766c37344768aaf99846f78857ff3/third_party/WebKit/LayoutTests/http/tests/webaudio/autoplay-crossorigin.html
[modify] https://crrev.com/f33a8c06201766c37344768aaf99846f78857ff3/third_party/WebKit/LayoutTests/http/tests/webaudio/resources/autoplay-crossorigin-iframe.html

Should this be marked as fixed?
Cc: dcheng@chromium.org
Status: Fixed (was: Started)
+dcheng to keep me honest wrt the tradeoff between 1) fixing individual tests to make them OOPIF-friendly, vs 2) fixing the test infrastructure so that all tests are automatically OOPIF-friendly.

I think it is indeed okay to mark the whole bug as fixed:

As I pointed out in https://crrev.com/2389963002/#msg4 I don't think we
have a good reason today to start replicating all of blink::WebSettings:

- It should be relatively straightforward to fix individual tests (by
  moving or duplicating calls to window.internals.settings.setFoo(...)

- Hopefully not too many tests will be impacted by lack of replication
  (right now the test found in this bug is the only known case)

- Enabling replication is complicated: there is no notification that
  components/test_runner could listen to, to figure out when WebSettings
  changes (to start replication);  additionally there is no support
  today for serializing and restoring updates to WebSettings (needed
  to replicated the updates).

Sign in to add a comment