Bug 212984

Summary: Subframes should not autosize independently
Product: WebKit Reporter: Timothy Hatcher <timothy>
Component: WebKit APIAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Timothy Hatcher 2020-06-09 12:44:29 PDT
The intrinsicContentsSizeChanged client call should only fire for the main frame.
Comment 1 Radar WebKit Bug Importer 2020-06-09 12:44:39 PDT
<rdar://problem/64175493>
Comment 2 Radar WebKit Bug Importer 2020-06-09 12:45:06 PDT
<rdar://problem/64175514>
Comment 3 Timothy Hatcher 2020-06-09 12:57:36 PDT
<rdar://problem/64175493>
Comment 4 Timothy Hatcher 2020-06-09 13:40:18 PDT
Created attachment 401468 [details]
Patch
Comment 5 Tim Horton 2020-06-09 13:49:55 PDT
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 6 Timothy Hatcher 2020-06-09 14:10:37 PDT
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.
Comment 7 zalan 2020-06-09 14:42:39 PDT
what's the expected subframe behavior with autosizing? How does the mainframe react to a resized subframe?
Comment 8 Timothy Hatcher 2020-06-09 14:46:12 PDT
(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.
Comment 9 Antoine Quint 2020-06-09 15:18:21 PDT
Why not bail out of FrameView::autoSizeIfEnabled() if this FrameView's frame isn't the main frame?
Comment 10 Timothy Hatcher 2020-06-09 15:58:37 PDT
(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.
Comment 11 Antoine Quint 2020-06-10 02:04:02 PDT
Created attachment 401521 [details]
Patch
Comment 12 Antoine Quint 2020-06-10 02:06:41 PDT
Created attachment 401522 [details]
Patch
Comment 13 Simon Fraser (smfr) 2020-06-10 07:56:49 PDT
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?
Comment 14 zalan 2020-06-10 07:58:47 PDT
(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.
Comment 15 Antoine Quint 2020-06-10 09:23:01 PDT
Created attachment 401547 [details]
Patch
Comment 16 Simon Fraser (smfr) 2020-06-10 09:53:39 PDT
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.
Comment 17 Antoine Quint 2020-06-10 10:15:23 PDT
Created attachment 401551 [details]
Patch
Comment 18 EWS 2020-06-10 13:37:08 PDT
Committed r262856: <https://trac.webkit.org/changeset/262856>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401551 [details].