Bug 212984 – Subframes should not autosize independently

• Component: WebKit API
• Status: RESOLVED FIXED

[Timothy Hatcher]

The intrinsicContentsSizeChanged client call should only fire for the main frame.

[Radar WebKit Bug Importer]

<rdar://problem/64175493>

[Radar WebKit Bug Importer]

<rdar://problem/64175514>

[Timothy Hatcher]

<rdar://problem/64175493>

[Timothy Hatcher]

Created attachment 401468 [details]
Patch

[Tim Horton]

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?

[Timothy Hatcher]

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.

[alan]

what's the expected subframe behavior with autosizing? How does the mainframe react to a resized subframe?

[Timothy Hatcher]

(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.

[Antoine Quint]

Why not bail out of FrameView::autoSizeIfEnabled() if this FrameView's frame isn't the main frame?

[Timothy Hatcher]

(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.

[Antoine Quint]

Created attachment 401521 [details]
Patch

[Antoine Quint]

Created attachment 401522 [details]
Patch

[Simon Fraser (smfr)]

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?

[alan]

(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.

[Antoine Quint]

Created attachment 401547 [details]
Patch

[Simon Fraser (smfr)]

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.

[Antoine Quint]

Created attachment 401551 [details]
Patch

[EWS]

Committed r262856: <https://trac.webkit.org/changeset/262856>

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