Bug 156090 – Web Automation: add support for getting, deleting, and adding cookies

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

.

[Radar WebKit Bug Importer]

<rdar://problem/25477678>

[Blaze Burg]

Created attachment 275355 [details]
WIP

[Blaze Burg]

Created attachment 275721 [details]
Proposed Fix

[Timothy Hatcher]

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.

[Joseph Pecoraro]

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.

[Blaze Burg]

Committed r199091: <http://trac.webkit.org/changeset/199091>

[Joseph Pecoraro]

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?