[webkit-reviews] review denied: [Bug 89230] Add support for application cache prefer-online mode : [Attachment 157872] patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied huangxueqing
<huangxueqing at baidu.com>'s request for review:
Bug 89230: Add support for application cache prefer-online mode
https://bugs.webkit.org/show_bug.cgi?id=89230

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=157872&action=review


I don't think that this is implementing anything like what the spec says. The
prefer-online mode changes navigation behavior, not subresource fetching. If
you are actually implementing the spec, please add per-function comments in
ChangeLog, explaining why each change is being made.

This patch didn't apply cleanly. It needs to be updated to apply to current tip
of tree, so that EWS could check it.

> LayoutTests/http/tests/appcache/prefer-online.html:5
> +<div id="result"></duv>

Typo: duv.

> LayoutTests/http/tests/appcache/prefer-online.html:33
> +    if (load("resources/counter.php") == load("resource/counter.php")) {

The second URL is wrong here.

It's not great that a mistake like this invalidates the whole test.

> LayoutTests/http/tests/appcache/resources/prefer-online.manifest:5
> +prefer-online

Please add tests that verify how whitespace is handled. From the spec:

When the current section is the settings section, data lines must consist of
zero or more U+0020 SPACE and U+0009 CHARACTER TABULATION (tab) characters, a
setting, and then zero or more U+0020 SPACE and U+0009 CHARACTER TABULATION
(tab) characters.

Also, please test what happens when unknown settings are encountered.

Please test empty lines in the SETTINGS section.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:446
> +    return resource ? true: false;

There is no need for the ternary operator, a pointer will be cast to a boolean
implicitly.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:466
> +bool
ApplicationCacheHost::scheduleLoadExcplicitResourceFromApplicationCache(Resourc
eLoader* loader, ApplicationCache* cache)

Typo: Excplicit.

> Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:619
> +    executeSQLCommand("CREATE TABLE IF NOT EXISTS CacheModes (cacheMode
INTERGER NOT NULL ON CONFLICT FAIL, cache INTERGER NOT NULL ON CONFLICT
FAIL)");

Typo: INTERGER. How did you test this code?