[webkit-reviews] review granted: [Bug 177405] Web Automation: add commands to get and set mock user permissions for pages in an automation session : [Attachment 321617] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 177405: Web Automation: add commands to get and set mock user permissions
for pages in an automation session
https://bugs.webkit.org/show_bug.cgi?id=177405

Attachment 321617: Patch

https://bugs.webkit.org/attachment.cgi?id=321617&action=review




--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 321617
  --> https://bugs.webkit.org/attachment.cgi?id=321617
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321617&action=review

r=me. You might need a WK2 Owner to approve the
UserMediaPermissionRequestManagerProxy change, but the rest looks good to me.

> Source/WebKit/UIProcess/Automation/Automation.json:222
> +	       "id": "MockPermission",

I'm not loving the Mock part of this name. Effectively you are setting the
permission for the session, so I don't think Mock is necessary. If the intent
is to portray that this is unique from the real system / user permissions then
replacing Mock with Session would be event clearer. `getMockPermissions` =>
`getSessionPermissions`. Whether or not Mock stays in the commands below, it
doesn't need to be included in these type names.

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1259
>> +	    }
> 
> I would add a break here, even if there aren't more options yet.

+1. I'm surprised this didn't get a compiler warning. Maybe we haven't enabled
-Wimplicit-fallthrough in all files? I think C++ is supposed to have it
thought.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1268
> +

Nit: Extra newline.