Bug 74513 – [chromium] Plumb through flag for enabling partial swap

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jonathan Backer]

[chromium] NOT FOR REVIEW Plumb through flag for enabling partial swap

[Jonathan Backer]

Created attachment 119241 [details]
Patch

[Antoine Labour]

Comment on attachment 119241 [details]
Patch

LGTM

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Jonathan Backer]

Created attachment 119251 [details]
Patch

[James Robinson]

Comment on attachment 119251 [details]
Patch

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

> Source/WebCore/page/Settings.h:646
> +        bool m_partialSwapEnabled : 1;

you need to initialize this in the Settings constructor or its value will be undefined and propagate out to the rest of the code.

[Jonathan Backer]

Created attachment 119255 [details]
Patch

[James Robinson]

Comment on attachment 119255 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 119255 [details]
Patch

Clearing flags on attachment: 119255

Committed r102817: <http://trac.webkit.org/changeset/102817>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 119255 [details]
Patch

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

> Source/WebCore/page/Settings.h:510
>          void setPerTileDrawingEnabled(bool enabled) { m_perTileDrawingEnabled = enabled; }
>          bool perTileDrawingEnabled() const { return m_perTileDrawingEnabled; }
>  
> +        void setPartialSwapEnabled(bool enabled) { m_partialSwapEnabled = enabled; }
> +        bool partialSwapEnabled() const { return m_partialSwapEnabled; }

Why is this Chromium-specific stuff not #ifdeffed? You're bloating Settings on all platforms.

Also I have no idea what "partial swap" is and whether it applies to my platform.

[James Robinson]

We don't have any #if PLATFORM() guards on things in Settings today, even though many of the bits only make sense for a certain platform or set of platforms. We could start, although 1 bit per Page is a pretty darn small cost.

[James Robinson]

Actually, I think for this flag we could keep everything out of Settings (by using the chromium WebKit API's WebSettings interface), but if we want to allow layout tests to set this flag via window.internals I think the bit needs to go on WebCore::Settings.

[Darin Fisher (:fishd, Google)]

(In reply to comment #12)
> Actually, I think for this flag we could keep everything out of Settings (by using the chromium WebKit API's WebSettings interface), but if we want to allow layout tests to set this flag via window.internals I think the bit needs to go on WebCore::Settings.

Can't layoutTestController.overridePreference be used for such things?  That allows tests to manipulate settings through whatever WebKit API each port provides.