[Webkit-unassigned] [Bug 89230] Add support for application cache prefer-online mode

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=89230


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #157872|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #7 from Alexey Proskuryakov <ap at webkit.org>  2012-08-14 12:50:46 PST ---
(From update of attachment 157872)
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(ResourceLoader* 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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.