Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2479653003:
Fix crash when computing media controls width for out of process iframe (Closed)
|
Created:
4 years, 1 month ago by mlamouri (slow - plz ping) Modified:
4 years, 1 month ago Reviewers:
dcheng CC:
blink-reviews, blink-reviews-layout_chromium.org, bokan, chromium-reviews, eae+blinkwatch, eric.carlson_apple.com, feature-media-reviews_chromium.org, jchaffraix+rendering, leviw+renderwatch, mlamouri+watch-blink_chromium.org, pdr+renderingwatchlist_chromium.org, Srirama, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix crash when computing media controls width for out of process iframe
BUG=662035
R=dcheng@chromium.org
Committed: https://crrev.com/e01c254c50ae20e862051332a04b6a2617de9928
Cr-Commit-Position: refs/heads/master@{#430047}
Patch Set 1 #
Total comments: 3
Patch Set 2 : add todo/bug #Messages
Total messages: 20 (9 generated)
The CQ bit was checked by mlamouri@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/2479653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/2479653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutMedia.cpp:164: if (document().page()->mainFrame()->isRemoteFrame()) What breaks if we don't go through the rest of this path for OOPIF? Do we need to handle that case at some point?
https://codereview.chromium.org/2479653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/2479653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutMedia.cpp:164: if (document().page()->mainFrame()->isRemoteFrame()) On 2016/11/04 at 16:48:36, dcheng wrote: > What breaks if we don't go through the rest of this path for OOPIF? Do we need to handle that case at some point? The feature is meant to make the default controls resize to match the viewport if the user can't scroll horizontally. It's mostly targeted for mobile (but would work on desktop though). On OOPIF, it would behave exactly like today where part of the controls are not visible. It's not ideal but not a serious concerns especially given that OOPIF might not be enabled on Android any time soon, right?
On 2016/11/04 17:00:56, mlamouri wrote: > https://codereview.chromium.org/2479653003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (right): > > https://codereview.chromium.org/2479653003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutMedia.cpp:164: if > (document().page()->mainFrame()->isRemoteFrame()) > On 2016/11/04 at 16:48:36, dcheng wrote: > > What breaks if we don't go through the rest of this path for OOPIF? Do we need > to handle that case at some point? > > The feature is meant to make the default controls resize to match the viewport > if the user can't scroll horizontally. It's mostly targeted for mobile (but > would work on desktop though). On OOPIF, it would behave exactly like today > where part of the controls are not visible. It's not ideal but not a serious > concerns especially given that OOPIF might not be enabled on Android any time > soon, right? Top Document Isolation (putting all third-party iframes in a single, separate process) is a project that will use OOPI on Android. It's not likely to launch in the next few weeks, but I don't think I'd characterize it as "not any time soon" either =)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2479653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/2479653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutMedia.cpp:164: if (document().page()->mainFrame()->isRemoteFrame()) On 2016/11/04 at 17:00:56, mlamouri wrote: > On 2016/11/04 at 16:48:36, dcheng wrote: > > What breaks if we don't go through the rest of this path for OOPIF? Do we need to handle that case at some point? > > The feature is meant to make the default controls resize to match the viewport if the user can't scroll horizontally. It's mostly targeted for mobile (but would work on desktop though). On OOPIF, it would behave exactly like today where part of the controls are not visible. It's not ideal but not a serious concerns especially given that OOPIF might not be enabled on Android any time soon, right? Do you want me to file a bug and add a TODO?
On 2016/11/04 18:17:55, mlamouri wrote: > https://codereview.chromium.org/2479653003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutMedia.cpp (right): > > https://codereview.chromium.org/2479653003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutMedia.cpp:164: if > (document().page()->mainFrame()->isRemoteFrame()) > On 2016/11/04 at 17:00:56, mlamouri wrote: > > On 2016/11/04 at 16:48:36, dcheng wrote: > > > What breaks if we don't go through the rest of this path for OOPIF? Do we > need to handle that case at some point? > > > > The feature is meant to make the default controls resize to match the viewport > if the user can't scroll horizontally. It's mostly targeted for mobile (but > would work on desktop though). On OOPIF, it would behave exactly like today > where part of the controls are not visible. It's not ideal but not a serious > concerns especially given that OOPIF might not be enabled on Android any time > soon, right? > > Do you want me to file a bug and add a TODO? Yeah, that sounds like the right thing to do here. Thanks!
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Done, PTAL.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by mlamouri@chromium.org
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix crash when computing media controls width for out of process iframe BUG=662035 R=dcheng@chromium.org ========== to ========== Fix crash when computing media controls width for out of process iframe BUG=662035 R=dcheng@chromium.org Committed: https://crrev.com/e01c254c50ae20e862051332a04b6a2617de9928 Cr-Commit-Position: refs/heads/master@{#430047} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e01c254c50ae20e862051332a04b6a2617de9928 Cr-Commit-Position: refs/heads/master@{#430047} |
