Summary: | Web Automation: add support for getting, deleting, and adding cookies | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | BJ Burg <bburg> | ||||||
Component: | Web Inspector | Assignee: | BJ Burg <bburg> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bburg, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | 156091 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
BJ Burg
2016-03-31 16:30:11 PDT
Created attachment 275355 [details]
WIP
Created attachment 275721 [details]
Proposed Fix
Comment on attachment 275721 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=275721&action=review > Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:753 > + // FIXME: if the incoming cookie's domain is the string '(inherit)', > + // then it should be inherited from the main frame's domain. That seems like a detail that should be handled separate from the driver spec. Like should empty string imply inherit? > Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:508 > + WebCore::Document* document = frame->coreFrame()->document(); > + // This returns the same list of cookies as when evaluating `document.cookies` in JavaScript. > + Vector<WebCore::Cookie> foundCookies; I don't like comments between lines like this. I would add a newline above the comment. > Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.messages.in:39 > + GetCookiesForFrame(uint64_t frameID, uint64_t callbackID) > + # Deletes any cookies matching cookieName if they are visible to the frame. > + DeleteCookie(uint64_t frameID, String cookieName, uint64_t callbackID) Newline above comment, or drop comment. No other commands have comments. Comment on attachment 275721 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=275721&action=review > Source/WebKit2/UIProcess/Automation/Automation.json:438 > + "description": "Returns all cookies visible to the specified browsing context.", Nit: Move "description"s up under the name for all of these? It flows so much better. > Source/WebKit2/UIProcess/Automation/Automation.json:454 > + { "name": "data", "type": "Cookie", "description": "Data for the cookie that should be added." } Hmm, shouldn't this be `"$ref": "Cookie"`, not `"type": "Cookie"`? I'm surprised the code generator doesn't issue a warning about this. > Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:698 > + cookies->addItem(buildObjectForCookie(cookie)); Nit: I think you can WTFMove(buildObjectForCookie(cookie)) here. But maybe not. > Source/WebKit2/WebProcess/Automation/WebAutomationSessionProxy.cpp:498 > + String frameNotFoundErrorType = Inspector::Protocol::AutomationHelpers::getEnumConstantValue(Inspector::Protocol::Automation::ErrorMessage::FrameNotFound); I wonder if we should make a macro for these instead of making a local at the top of all of these methods and adding ~2 lines per method. We could also have something for the no error case. #define FRAME_NOT_FOUND_ERROR_STRING = Inspector::Protocol::AutomationHelpers::getEnumConstantValue(Inspector::Protocol::Automation::ErrorMessage::FrameNotFound) #define NO_ERROR_STRING = String() Or a PREDEFINED_ERROR_STRING(enum) like the other case. Just a thought, you don't need to do this. Committed r199091: <http://trac.webkit.org/changeset/199091> Comment on attachment 275721 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=275721&action=review > Source/WebKit2/UIProcess/Automation/WebAutomationSession.cpp:760 > +void WebAutomationSession::deleteAllCookies(ErrorString& errorString, const String& browsingContextHandle, Ref<DeleteAllCookiesCallback>&& callback) This never calls the callback. Perhaps it shouldn't be async? |