Bug 73753 – Move shouldLayoutFixedElementsRelativeToFrame to Page.cpp

• Component: New Bugs
• Status: RESOLVED DUPLICATE

[Fady Samuel]

Move shouldLayoutFixedElementsRelativeToFrame to Page.cpp

[Fady Samuel]

Created attachment 117772 [details]
Patch

[Fady Samuel]

I don't know if this is better in Page or as a Setting? I've put it in Page until someone can give me a good reason to make this a Setting

[Collabora GTK+ EWS bot]

Comment on attachment 117772 [details]
Patch

Attachment 117772 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10734347

[Darin Adler]

Comment on attachment 117772 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117772&action=review

> Source/WebCore/page/FrameView.cpp:1397
> +    bool shouldLayoutFixedElementsRelativeToFrame = frame()->page() ? frame()->page()->shouldLayoutFixedElementsRelativeToFrame(): false;

&& might be more readable here than ? :

> Source/WebCore/page/Page.cpp:687
> +    if (m_mainFrame && m_mainFrame->document()->renderer())
> +        m_mainFrame->document()->renderer()->setNeedsLayout(true);

Don’t we have to do this for subframes too?

[WebKit Review Bot]

Comment on attachment 117772 [details]
Patch

Attachment 117772 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10732339

New failing tests:
svg/custom/linking-uri-01-b.svg

[Fady Samuel]

Comment on attachment 117772 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117772&action=review

>> Source/WebCore/page/FrameView.cpp:1397
>> +    bool shouldLayoutFixedElementsRelativeToFrame = frame()->page() ? frame()->page()->shouldLayoutFixedElementsRelativeToFrame(): false;
> 
> && might be more readable here than ? :

Sure, I'll do this in a patch refresh.

>> Source/WebCore/page/Page.cpp:687
>> +        m_mainFrame->document()->renderer()->setNeedsLayout(true);
> 
> Don’t we have to do this for subframes too?

This setting has no impact on subframes or at least should have no impact on subframes (There might be a bug here that I need to test for) because frameScaleFactor == 1 for subframes. I have to confirm this isn't broken in a layout test.

[Eric Seidel (no email)]

Comment on attachment 117772 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117772&action=review

Seems reasonable to me, but loks like the EWS bots strongly disagree.  Please upload a new patch which passes EWS. :)

> Source/WebCore/ChangeLog:10
> +        of frame because it's a no-op on subframes. This is a page-level property. To
> +        reduce confusion and to save a few bytes, it has been moved to Page.

Bytes don't really matter on FrameView. :)

[Fady Samuel]

*** This bug has been marked as a duplicate of bug 76459 ***