Bug 56722 – Add +[WebApplicationCache getOriginsWithCache]

• Component: WebKit API
• Status: RESOLVED FIXED

[Anton D'Auria]

WebCore already provides a way to get a list of origins that have an Application Cache. Hook up this API to the Mac port.

[Anton D'Auria]

Created attachment 86280 [details]
Patch

[WebKit Review Bot]

Attachment 86280 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8212172

[Early Warning System Bot]

Attachment 86280 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8214133

[Joseph Pecoraro]

Comment on attachment 86280 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86280&action=review

I'm actually not near a computer where I can look deeper at this, but it looks good to me.

> Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:160
> +    NSInteger count = [origins count];

Nit: NSUInteger here and the iterator, assuming the JS call later is happy with it too.

> LayoutTests/http/tests/appcache/origins-with-appcache.html:40
> +    log("Origins with Application Cache after deletion: " + layoutTestController.getOriginsWithApplicationCache());

Nit: It might be nice to put the clearAllApplicationCaches call next to this log statement.

[Alexey Proskuryakov]

Comment on attachment 86280 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86280&action=review

> Source/WebKit/mac/WebCoreSupport/WebApplicationCache.mm:72
> +    typedef HashSet<RefPtr<SecurityOrigin>, SecurityOriginHash> OriginSet;

Normally we'd have such a typedef somewhere in a header.

> Source/WebKit/mac/WebCoreSupport/WebApplicationCache.mm:81
> +        WebSecurityOrigin *webOrigin = [[WebSecurityOrigin alloc] _initWithWebCoreSecurityOrigin:(*it).get()];

Please use RetainPtr.

[Darin Adler]

Comment on attachment 86280 [details]
Patch

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?

[Anton D'Auria]

Created attachment 86397 [details]
Patch

[Early Warning System Bot]

Attachment 86397 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8212957

[Anton D'Auria]

Created attachment 86400 [details]
Patch

[Anton D'Auria]

In this patch, I addressed all comments except the following:

(In reply to comment #6)
> (From update of attachment 86280 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86280&action=review
> 
> > 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.

If you think it's important, I'll do it, but I don't see why it makes things cleaner. Is it a JS style issue?

> > 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”.

I decided to follow the other comments that capitalize application cache. Let me know if you think I should edit them all to be consistent.

[Alexey Proskuryakov]

> I decided to follow the other comments that capitalize application cache. Let me know if you think I should edit them all to be consistent.

HTML5 doesn't capitalize "application cache": <http://www.whatwg.org/specs/web-apps/current-work/multipage/offline.html>.

[Anton D'Auria]

Created attachment 86429 [details]
Patch

Lower-casing "Application Cache" in comments

[Alexey Proskuryakov]

Comment on attachment 86429 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86429&action=review

> Source/WebCore/loader/appcache/ApplicationCacheStorage.h:40
> +typedef HashSet<RefPtr<SecurityOrigin>, SecurityOriginHash> SecurityOriginSet;

I now see why you were unsure about adding this to a header. It sure feels strange to have a SecurityOriginSet definition in ApplicationCacheStorage.h!

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

Maybe originsWithCaches? I'm not sure.

> Source/WebKit/mac/WebCoreSupport/WebApplicationCache.mm:76
> +    NSMutableArray *webOrigins = [[NSMutableArray alloc] initWithCapacity:coreOrigins.size()];

Please always use RetainPtr.

> Source/WebKit/mac/WebCoreSupport/WebApplicationCache.mm:80
> +        RetainPtr<WebSecurityOrigin> webOrigin = [[WebSecurityOrigin alloc] _initWithWebCoreSecurityOrigin:(*it).get()];

This needs to be RetainPtr<WebSecurityOrigin> webOrigin(AdoptNS, ...).

[Anton D'Auria]

Created attachment 86515 [details]
Patch

Using RetainPtr, removing SecurityOriginSet typedef, not renaming +[WebApplicationCache originsWithCache] to originsWithCaches
because users of this API don't need to know that an origin can have multiple cache groups. The origin uses application cache, and the singular form seems sufficient.

[Alexey Proskuryakov]

Comment on attachment 86515 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86515&action=review

> Source/WebCore/ChangeLog:8
> +        Added typedef for HashSet of SecurityOrigins. Added test that

No.

> Source/WebKit/mac/WebCoreSupport/WebApplicationCache.h:42
> ++ (RetainPtr<NSArray>)originsWithCache;

Sorry for confusing you, but I didn't suggest using a RetainPtr here. I only suggested using it inside the function.

[Alexey Proskuryakov]

I no longer think that you should have changed webOrigins to RetainPtr. Moving autorelease to the same line where you allocate it would be nice, to avoid leaks in case an early return is added in the future.

[Anton D'Auria]

Created attachment 86534 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 86534 [details]
Patch

I didn't look at test changes, which Darin previously reviewed.

[WebKit Commit Bot]

Comment on attachment 86534 [details]
Patch

Clearing flags on attachment: 86534

Committed r81733: <http://trac.webkit.org/changeset/81733>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.