Bug 74513 - [chromium] Plumb through flag for enabling partial swap
Summary: [chromium] Plumb through flag for enabling partial swap
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Backer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-14 10:15 PST by Jonathan Backer
Modified: 2011-12-15 11:30 PST (History)
5 users (show)

See Also:


Attachments
Patch (9.15 KB, patch)
2011-12-14 10:16 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch (9.13 KB, patch)
2011-12-14 10:56 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch (9.72 KB, patch)
2011-12-14 11:10 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Backer 2011-12-14 10:15:02 PST
[chromium] NOT FOR REVIEW Plumb through flag for enabling partial swap
Comment 1 Jonathan Backer 2011-12-14 10:16:36 PST
Created attachment 119241 [details]
Patch
Comment 2 Antoine Labour 2011-12-14 10:48:26 PST
Comment on attachment 119241 [details]
Patch

LGTM
Comment 3 WebKit Review Bot 2011-12-14 10:51:13 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 Jonathan Backer 2011-12-14 10:56:33 PST
Created attachment 119251 [details]
Patch
Comment 5 James Robinson 2011-12-14 11:00:06 PST
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.
Comment 6 Jonathan Backer 2011-12-14 11:10:44 PST
Created attachment 119255 [details]
Patch
Comment 7 James Robinson 2011-12-14 11:41:47 PST
Comment on attachment 119255 [details]
Patch

R=me
Comment 8 WebKit Review Bot 2011-12-14 13:25:17 PST
Comment on attachment 119255 [details]
Patch

Clearing flags on attachment: 119255

Committed r102817: <http://trac.webkit.org/changeset/102817>
Comment 9 WebKit Review Bot 2011-12-14 13:25:21 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Simon Fraser (smfr) 2011-12-14 13:46:38 PST
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.
Comment 11 James Robinson 2011-12-14 14:43:45 PST
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.
Comment 12 James Robinson 2011-12-14 14:54:19 PST
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.
Comment 13 Darin Fisher (:fishd, Google) 2011-12-15 11:30:29 PST
(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.