Bug 156090 - Web Automation: add support for getting, deleting, and adding cookies
Summary: Web Automation: add support for getting, deleting, and adding cookies
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on: 156091
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-31 16:30 PDT by Brian Burg
Modified: 2016-07-22 21:09 PDT (History)
7 users (show)

See Also:


Attachments
WIP (5.22 KB, patch)
2016-03-31 16:37 PDT, Brian Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (19.59 KB, patch)
2016-04-05 17:00 PDT, Brian Burg
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2016-03-31 16:30:11 PDT
.
Comment 1 Radar WebKit Bug Importer 2016-03-31 16:36:46 PDT
<rdar://problem/25477678>
Comment 2 Brian Burg 2016-03-31 16:37:49 PDT
Created attachment 275355 [details]
WIP
Comment 3 Brian Burg 2016-04-05 17:00:31 PDT
Created attachment 275721 [details]
Proposed Fix
Comment 4 Timothy Hatcher 2016-04-05 17:12:38 PDT
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 5 Joseph Pecoraro 2016-04-05 17:29:40 PDT
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.
Comment 6 Brian Burg 2016-04-05 20:23:22 PDT
Committed r199091: <http://trac.webkit.org/changeset/199091>
Comment 7 Joseph Pecoraro 2016-07-22 21:09:01 PDT
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?