Summary: | Subframes should not autosize independently | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Timothy Hatcher <timothy> | ||||||||||||
Component: | WebKit API | Assignee: | Antoine Quint <graouts> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | graouts, graouts, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh, zalan | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Local Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Timothy Hatcher
2020-06-09 12:44:29 PDT
Created attachment 401468 [details]
Patch
Comment on attachment 401468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401468&action=review > Source/WebCore/ChangeLog:10 > + (WebCore::FrameView::autoSizeIfEnabled): Only call the client for the main frame. This seems like we already got wayyyyy too far. Shouldn't we not even propagate the autosizing bit to subframes? Comment on attachment 401468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401468&action=review >> Source/WebCore/ChangeLog:10 >> + (WebCore::FrameView::autoSizeIfEnabled): Only call the client for the main frame. > > This seems like we already got wayyyyy too far. Shouldn't we not even propagate the autosizing bit to subframes? The autosize code is called for the normal AutoSizeMode::FixedWidth case too, which is what the subframes are using. what's the expected subframe behavior with autosizing? How does the mainframe react to a resized subframe? (In reply to zalan from comment #7) > what's the expected subframe behavior with autosizing? How does the > mainframe react to a resized subframe? We don't use content autosizing for subframes. This client call is only useful for the main frame, the WebKit layer only expects it for the main frame. Why not bail out of FrameView::autoSizeIfEnabled() if this FrameView's frame isn't the main frame? (In reply to Antoine Quint from comment #9) > Why not bail out of FrameView::autoSizeIfEnabled() if this FrameView's frame > isn't the main frame? If that is the best option then, sure. I wasn't comfortable doing that. I just know the client and WebKit isn't expecting to be called for sub-frames. Someone else is free to make that change if that would be better for this code. Created attachment 401521 [details]
Patch
Created attachment 401522 [details]
Patch
Comment on attachment 401522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401522&action=review > Source/WebCore/page/FrameView.cpp:3412 > + if (!frame().isMainFrame()) > + return; Why did we ever get here for the non-main frame? How did m_shouldAutoSize get set on a non-main frame? (In reply to Simon Fraser (smfr) from comment #13) > Comment on attachment 401522 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401522&action=review > > > Source/WebCore/page/FrameView.cpp:3412 > > + if (!frame().isMainFrame()) > > + return; > > Why did we ever get here for the non-main frame? How did m_shouldAutoSize > get set on a non-main frame? Yeah, maybe a better fix would be to not let autosize enabled on subframes and assert for mainframe here. Created attachment 401547 [details]
Patch
Comment on attachment 401547 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401547&action=review > Source/WebCore/page/FrameView.cpp:4599 > + if (!frame().isMainFrame()) > + return; This should be ASSERT(frame().isMainFrame()); and you should find who's calling this on subframes and stop them from doing so. Created attachment 401551 [details]
Patch
Committed r262856: <https://trac.webkit.org/changeset/262856> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401551 [details]. |