| Summary: | Add WebCookieManager to WebKit mac port to list hosts with cookies, delete cookies for a host, delete all cookies | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Anton D'Auria <adauria> | ||||||||||
| Component: | WebKit API | Assignee: | Anton D'Auria <adauria> | ||||||||||
| Status: | RESOLVED WONTFIX | ||||||||||||
| Severity: | Normal | CC: | bburg, bweinstein, commit-queue, ddkilzer, eric | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | OS X 10.5 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Anton D'Auria
2011-03-24 14:50:37 PDT
Created attachment 86854 [details]
Patch
Attachment 86854 [details] did not build on qt: Build output: http://queues.webkit.org/results/8235654 Comment on attachment 86854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86854&action=review I haven't looked at the testing portion yet, but the code looks good, just some SecurityOrigin stuff that doesn't seem to be needed. > Source/WebKit/mac/Cookies/WebCookieManager.mm:29 > +#import <WebCore/SecurityOrigin.h> Is this #import needed? > Source/WebKit/mac/Cookies/WebCookieManager.mm:31 > +#import <wtf/RetainPtr.h> Or this one? > Source/WebKit/mac/Cookies/WebCookieManagerPrivate.h:26 > +@class WebSecurityOrigin; I'm surprised this is necessary, since you are only using NSStrings and NSArrays here. Comment on attachment 86854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86854&action=review > Tools/ChangeLog:7 > + Some comments (especially alongside the functions you implemented) would be nice. > Tools/DumpRenderTree/LayoutTestController.h:68 > + void deleteCookiesForHostname(JSStringRef hostname); Hostname here ("JSStringRef hostname") doesn't add anything. > Tools/DumpRenderTree/qt/LayoutTestControllerQt.h:197 > + void deleteCookiesForHostname(const QString& hostname); Hostname here doesn't add anything. > LayoutTests/http/tests/cookies/delete-all-cookies.html:7 > +resetCookies(); I feel like this should be done at the start of runTest - to make it clear that it is part of running the test. > LayoutTests/http/tests/cookies/delete-cookies-for-hostname.html:7 > +resetCookies(); Ditto about putting this at the start of runTest. > LayoutTests/http/tests/cookies/delete-cookies-for-hostname.html:31 > + var contains = false; > + var i = hostsWithCookies.length; > + while (i--) { > + if (hostsWithCookies[i] === drtHost) { > + contains = true; > + break; > + } > + } I think you could just use array.indexOf here. > LayoutTests/http/tests/cookies/hostnames-with-cookies.html:6 > +resetCookies(); Once more about runTest. > LayoutTests/http/tests/cookies/hostnames-with-cookies.html:31 > + var contains = false; > + var i = hostsWithCookies.length; > + while (i--) { > + if (hostsWithCookies[i] === drtHost) { > + contains = true; > + break; > + } > + } Ditto about using array.indexOf. Created attachment 86863 [details]
Patch
Comment on attachment 86863 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86863&action=review If you could link to https://bugs.webkit.org/show_bug.cgi?id=57055 in the Skipped lists of the platforms you're not implementing this for, that would be great. > Source/WebKit/WebKit.xcodeproj/project.pbxproj:821 > + 3A1C2D93133A70FB00A420E5 /* Cookies */, If these are alphabetical, Cookies should be above Default Delegates and DOM. > Source/WebKit/mac/WebKit.exp:7 > +.objc_class_name_WebCookieManager Alphabetical again - WebCookieManager < WebCoreScrollView. > Source/WebKit/mac/Cookies/WebCookieManager.mm:27 > +#import "WebSecurityOriginInternal.h" Is this include necessary? Also, I think our style dictates a blank like between the import of WebCookieManagerPrivate and the other imports. > Tools/DumpRenderTree/LayoutTestController.cpp:640 > + ASSERT(!*exception); Do you also want to assert exception is non-null here? ASSERT(exception); ASSERT(!*exception). I don't know what our style is, if it's usually just the ASSERT(!*exception), that's fine. > Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:180 > + JSRetainPtr<JSStringRef> jsString(Adopt, JSStringCreateWithCFString((CFStringRef)string)); Can this be a C++ style cast? > LayoutTests/http/tests/cookies/delete-all-cookies.html:27 > + You might also want to run logContainsCookies here - to show that we are in a clean state before the test starts. > LayoutTests/http/tests/cookies/delete-cookies-for-hostname.html:22 > + var contains = (hostsWithCookies.indexOf(drtHost) != -1); I'm not wild about this name. Maybe hasDRTCookies? I don't have a strong opinion about this though. > LayoutTests/http/tests/cookies/hostnames-with-cookies.html:6 > +resetCookies(); Don't forget to put resetCookies in runTest here (unless there's a reason not to). > LayoutTests/http/tests/cookies/hostnames-with-cookies.html:25 > + var contains = (hostsWithCookies.indexOf(drtHost) != -1); Same comment about contains variable name here. Created attachment 86889 [details]
Patch
Created attachment 86994 [details]
Patch
WebCookieManagerPrivate.h was not exporteted as private
Comment on attachment 86994 [details] Patch Rejecting attachment 86994 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2 Last 500 characters of output: ...... webarchive ..................... webarchive/loading .... http/tests/appcache ........................................................... http/tests/cache ........ http/tests/canvas/philip/tests .......... http/tests/canvas/webgl . http/tests/cookies . http/tests/cookies/delete-all-cookies.html -> failed Exiting early after 1 failures. 22065 tests run. 563.74s total testing time 22064 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 14 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/8254415 (In reply to comment #9) > (From update of attachment 86994 [details]) > Rejecting attachment 86994 [details] from commit-queue. > > Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2 > > Last 500 characters of output: > ...... > webarchive ..................... > webarchive/loading .... > http/tests/appcache ........................................................... > http/tests/cache ........ > http/tests/canvas/philip/tests .......... > http/tests/canvas/webgl . > http/tests/cookies . > http/tests/cookies/delete-all-cookies.html -> failed > > Exiting early after 1 failures. 22065 tests run. > 563.74s total testing time > > 22064 test cases (99%) succeeded > 1 test case (<1%) had incorrect layout > 14 test cases (<1%) had stderr output > > Full output: http://queues.webkit.org/results/8254415 I can't reproduce this locally yet. Is it possible to view the layout test results generated by the commit queue? (In reply to comment #10) > I can't reproduce this locally yet. Is it possible to view the layout test results generated by the commit queue? Sadly, not easily. I need to fix it to upload the failing results. (In reply to comment #11) > (In reply to comment #10) > > I can't reproduce this locally yet. Is it possible to view the layout test results generated by the commit queue? > > Sadly, not easily. I need to fix it to upload the failing results. I'll be grateful if you can post the diff this once, or modify the commit queue to upload failing results :) Sure thing. Will post in a few hours once I'm in the office. (In reply to comment #13) > Sure thing. Will post in a few hours once I'm in the office. Eric, I think https://bugs.webkit.org/show_bug.cgi?id=57127 is failing for the same reason. In both, I'm hardcoding the origin URL of drt's httpd. Finally fixed the cq to upload failing diffs with bug 58348. Is this still needed? Closing, we don't need to add new WK1 APIs. |