Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2916703002:
Force resize event to be sent to OOPIF subframes when main frame resizes. (Closed)
|
Created:
3 years, 6 months ago by lfg Modified:
3 years, 6 months ago CC:
chromium-reviews, mlamouri+watch-screen-orientation_chromium.org, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionForce resize event to be sent to OOPIF subframes when main frame resizes.
Also adds a test for the screen orientation lock API on OOPIF subframes.
BUG=498287
Review-Url: https://codereview.chromium.org/2916703002
Cr-Commit-Position: refs/heads/master@{#477815}
Committed: https://chromium.googlesource.com/chromium/src/+/95f00bd53528fee18ef3ccaa08690c6a1996045d
Patch Set 1 #Patch Set 2 : disable test #
Total comments: 2
Patch Set 3 : rebase #Patch Set 4 : add comment #
Messages
Total messages: 24 (17 generated)
The CQ bit was checked by lfg@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by lfg@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.
lfg@chromium.org changed reviewers: + mlamouri@chromium.org
Mounir, can you take a look? I've tried writing a test that tests the lock API, but the bots don't like this test. I suspect that the bots run with orientation locked, which would prevent the test from passing.
On 2017/06/05 at 15:28:07, lfg wrote: > Mounir, can you take a look? > > I've tried writing a test that tests the lock API, but the bots don't like this test. I suspect that the bots run with orientation locked, which would prevent the test from passing. The bots have system portrait lock. Though, the Screen Orientation API should work on top of it. lgtm. Thanks :)
lfg@chromium.org changed reviewers: + creis@chromium.org
Charlie can you review?
LGTM, though it'd be nicer if we had a way forward for actually running the test. :) https://codereview.chromium.org/2916703002/diff/20001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_browsertest.cc (right): https://codereview.chromium.org/2916703002/diff/20001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_browsertest.cc:361: DISABLED_ScreenOrientationLock) { Please include a comment for why this is disabled. Is there any way that we'll be able to enable it in the future? And is it meaningful on any other platforms?
The CQ bit was checked by lfg@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...
https://codereview.chromium.org/2916703002/diff/20001/content/browser/screen_... File content/browser/screen_orientation/screen_orientation_browsertest.cc (right): https://codereview.chromium.org/2916703002/diff/20001/content/browser/screen_... content/browser/screen_orientation/screen_orientation_browsertest.cc:361: DISABLED_ScreenOrientationLock) { On 2017/06/06 22:41:37, Charlie Reis wrote: > Please include a comment for why this is disabled. Done. > Is there any way that we'll be able to enable it in the future? And is it > meaningful on any other platforms? This test isn't meaningful (as of today) on other platforms, since we only support the Screen Orientation lock APIs on Android (the other platforms do have orientation change, but not lock). As for enabling in the future, I've looked at a couple of options: 1. Use the renderer mock interfaces (MockScreenOrientationClient) that we currently use to test this API. Those are used for testing this API with LayoutTests. - The problem here is that there's no way to replicate the locking information across OOPIF renderers, which makes it difficult. 2. Mock browser process interfaces. - I believe we would need to mock WebContentsViewAndroid and the ScreenOrientationProvider classes, and possibly RenderWidgetHostViewnAndroid. The problem here is that there's no framework to mock those classes so there's a lot of boilerplate code that needs to be written.
The CQ bit was unchecked by lfg@chromium.org
The CQ bit was checked by lfg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2916703002/#ps60001 (title: "add comment")
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": 1496877662278140,
"parent_rev": "48de879ed5cb3ea81d10710caaf33eec1e2c5d08", "commit_rev":
"95f00bd53528fee18ef3ccaa08690c6a1996045d"}
Message was sent while issue was closed.
Description was changed from ========== Force resize event to be sent to OOPIF subframes when main frame resizes. Also adds a test for the screen orientation lock API on OOPIF subframes. BUG=498287 ========== to ========== Force resize event to be sent to OOPIF subframes when main frame resizes. Also adds a test for the screen orientation lock API on OOPIF subframes. BUG=498287 Review-Url: https://codereview.chromium.org/2916703002 Cr-Commit-Position: refs/heads/master@{#477815} Committed: https://chromium.googlesource.com/chromium/src/+/95f00bd53528fee18ef3ccaa0869... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/95f00bd53528fee18ef3ccaa0869... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
