[webkit-reviews] review denied: [Bug 79269] Separate PageCachePolicy from PageCache action : [Attachment 128295] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Gavin Peters <gavinp at chromium.org>'s
request for review:
Bug 79269: Separate PageCachePolicy from PageCache action
https://bugs.webkit.org/show_bug.cgi?id=79269

Attachment 128295: Patch
https://bugs.webkit.org/attachment.cgi?id=128295&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=128295&action=review


> Source/WebCore/ChangeLog:10
> +	   This refactor separates out pagecache policy from the page cache
> +	   itself.  It also adds a factory mechanism, so that ports that want
> +	   to have distinct policies can override the default.

Can we skip the factory method until needed?  Generally, the way we handle this
is with a static create() function in different CPP files for each
port/configuration rather than using the factory pattern directly.

Also, WebKit tends to avoid introducing these sorts of abstract mechanisms
until we have at least one use case.  Is the Chromium port planning to use a
different PageCachePolicy?  My understanding was that Chromium doesn't use the
PageCache at all.

> Source/WebCore/history/PageCachePolicy.h:53
> +    PageCachePolicy(Page*);

explicit

> Source/WebCore/history/PageCachePolicy.h:54
> +    bool CanCachePage();

WebKit uses lower-case for the initial letter of member functions.

> Source/WebCore/history/PageCachePolicy.h:59
> +protected:

private ?  Are you expecting subclasses?