[webkit-reviews] review denied: [Bug 56722] Add +[WebApplicationCache getOriginsWithCache] : [Attachment 86280] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Anton D'Auria <adauria at apple.com>'s
request for review:
Bug 56722: Add +[WebApplicationCache getOriginsWithCache]
https://bugs.webkit.org/show_bug.cgi?id=56722

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=86280&action=review

review- because this doesn’t yet compile on a number of platforms

> Source/WebKit/mac/WebCoreSupport/WebApplicationCache.h:41
> ++ (NSArray *)getOriginsWithCache;

Normally in WebKit, as in AppKit, we do not include a “get” prefix on a method
with a return value that acts as a property. So this would be named
originsWithCache. Same for the various C++ and JavaScript functions.

> Tools/DumpRenderTree/LayoutTestController.cpp:2163
> +	   { "getOriginsWithApplicationCache",
getOriginsWithApplicationCacheCallback, kJSPropertyAttributeReadOnly |
kJSPropertyAttributeDontDelete },

I think this should be a JavaScript property rather than a function, unless
that’s a challenge to implement.

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:763
> +void LayoutTestController::getOriginsWithApplicationCache(const
CppArgumentList& arguments, CppVariant*)
> +{
> +    // FIXME: implement to support getting origins that have Application
Caches.
> +    result->setNull();
> +}

Should remove the name arguments and add the name result so this compiles.

> Tools/DumpRenderTree/chromium/LayoutTestController.h:290
> +    void getOriginsWithApplicationCache(const CppArgumentList& arguments,
CppVariant*);

Should remove the name arguments.

> Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:165
> +	   JSRetainPtr<JSStringRef> str(Adopt,
JSStringCreateWithCFString((CFStringRef)origin));

We normally do not use abbreviations such as “str” in WebKit. I would call this
something like originJS.

It looks like you copied this logic from originsWithLocalStorage. I suggest
making a helper function that turns an NSArray of origins into a JS array of
identifier strings, rather than copying and pasting the code.

> Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp:567
> +void LayoutTestController::getOriginsWithApplicationCache()

This will need a return value and argument like the others, or it won’t
compile.

> Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp:951
> +    // FIXME: implement to get origins that have Application Caches.

Should capitalize sentence comments like this Should not capitalize the words
“application cache”.

> LayoutTests/platform/qt/Skipped:3241
> +http/tests/appcache/origins-with-appcache.html

What about the skipped list entry for WebKit2?