RESOLVED FIXED 56722
Add +[WebApplicationCache getOriginsWithCache]
https://bugs.webkit.org/show_bug.cgi?id=56722
Summary Add +[WebApplicationCache getOriginsWithCache]
Anton D'Auria
Reported 2011-03-20 00:26:22 PDT
WebCore already provides a way to get a list of origins that have an Application Cache. Hook up this API to the Mac port.
Attachments
Patch (18.52 KB, patch)
2011-03-20 00:57 PDT, Anton D'Auria
no flags
Patch (23.96 KB, patch)
2011-03-21 17:59 PDT, Anton D'Auria
no flags
Patch (23.99 KB, patch)
2011-03-21 18:15 PDT, Anton D'Auria
no flags
Patch (27.00 KB, patch)
2011-03-21 23:54 PDT, Anton D'Auria
no flags
Patch (25.71 KB, patch)
2011-03-22 15:06 PDT, Anton D'Auria
no flags
Patch (25.70 KB, patch)
2011-03-22 16:16 PDT, Anton D'Auria
no flags
Anton D'Auria
Comment 1 2011-03-20 00:57:07 PDT
WebKit Review Bot
Comment 2 2011-03-20 01:02:18 PDT
Early Warning System Bot
Comment 3 2011-03-20 01:05:24 PDT
Joseph Pecoraro
Comment 4 2011-03-20 01:36:28 PDT
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 5 2011-03-20 12:42:19 PDT
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 6 2011-03-20 15:19:29 PDT
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
Comment 7 2011-03-21 17:59:12 PDT
Early Warning System Bot
Comment 8 2011-03-21 18:11:10 PDT
Anton D'Auria
Comment 9 2011-03-21 18:15:53 PDT
Anton D'Auria
Comment 10 2011-03-21 22:53:24 PDT
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
Comment 11 2011-03-21 23:02:56 PDT
> 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
Comment 12 2011-03-21 23:54:56 PDT
Created attachment 86429 [details] Patch Lower-casing "Application Cache" in comments
Alexey Proskuryakov
Comment 13 2011-03-22 11:14:49 PDT
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
Comment 14 2011-03-22 15:06:38 PDT
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 15 2011-03-22 15:35:16 PDT
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
Comment 16 2011-03-22 15:40:43 PDT
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
Comment 17 2011-03-22 16:16:10 PDT
Alexey Proskuryakov
Comment 18 2011-03-22 16:22:39 PDT
Comment on attachment 86534 [details] Patch I didn't look at test changes, which Darin previously reviewed.
WebKit Commit Bot
Comment 19 2011-03-22 17:57:52 PDT
Comment on attachment 86534 [details] Patch Clearing flags on attachment: 86534 Committed r81733: <http://trac.webkit.org/changeset/81733>
WebKit Commit Bot
Comment 20 2011-03-22 17:57:57 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.