permissions: refactor Permissions API in blink to vastly reduce duplication

permissions: refactor Permissions API in blink to vastly reduce duplication

With the duplication removal for multiple permission requests on its way,
this class was duplicating a lot of code. Simplifying things here makes
the code more maintainable as well as introducing duplication removal easier.

BUG=(none)

Committed: https://crrev.com/117d408223c5f53f90f2ba20458e6022552cf81b
Cr-Commit-Position: refs/heads/master@{#351068}

[Lalit Maganti, 2015-09-24 20:04:08]

lalitm@google.com changed reviewers:
+ mlamouri@chromium.org

[Lalit Maganti, 2015-09-24 20:04:09]

Mounir: PTAL. I also added some missing type checks in parsePermission along
with the refactoring.

[mlamouri (slow - plz ping), 2015-09-28 10:48:43]

l-g-t-m but I would like to see the next iteration.

https://codereview.chromium.org/1365993003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/permissions/Permissions.cpp (right):

https://codereview.chromium.org/1365993003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:49:
Nullable<WebPermissionType> parsePermission(ScriptState* scriptState, const
Dictionary rawPermission, ExceptionState& exceptionState)
This need some documentation :)

https://codereview.chromium.org/1365993003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:103:
ExceptionState exceptionState(ExceptionState::UnknownContext, nullptr, nullptr,
scriptState->context()->Global(), scriptState->isolate());
Why not passing: |ExceptionState::GetterContext, "query", "Permissions"| ?

https://codereview.chromium.org/1365993003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:125:
ExceptionState exceptionState(ExceptionState::UnknownContext, nullptr, nullptr,
scriptState->context()->Global(), scriptState->isolate());
ditto with "request"

https://codereview.chromium.org/1365993003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:126:
Nullable<WebPermissionType> type = parsePermission(scriptState, rawPermission,
exceptionState);
So here you use |Nullable<>| but you don't actually use the fact that |type|
could be null. You only return null if there is an exception. It's a bit odd.
Could you add least add a comment explaining that |Nullable<>| will be null iif
an exception was thrown and an ASSERT() will be fired if the caller do not check
for the permission and uses a null type? (You can add that in the
|parsePermission()| method comment.

https://codereview.chromium.org/1365993003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:143:
ExceptionState exceptionState(ExceptionState::UnknownContext, nullptr, nullptr,
scriptState->context()->Global(), scriptState->isolate());
ditto with "request"

https://codereview.chromium.org/1365993003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:168:
ExceptionState exceptionState(ExceptionState::UnknownContext, nullptr, nullptr,
scriptState->context()->Global(), scriptState->isolate());
ditto with "revoke"

[Lalit Maganti, 2015-09-28 12:23:09]

https://codereview.chromium.org/1365993003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/permissions/Permissions.cpp (right):

https://codereview.chromium.org/1365993003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:49:
Nullable<WebPermissionType> parsePermission(ScriptState* scriptState, const
Dictionary rawPermission, ExceptionState& exceptionState)
On 2015/09/28 10:48:43, Mounir Lamouri wrote:
> This need some documentation :)

Done.

https://codereview.chromium.org/1365993003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:103:
ExceptionState exceptionState(ExceptionState::UnknownContext, nullptr, nullptr,
scriptState->context()->Global(), scriptState->isolate());
On 2015/09/28 10:48:43, Mounir Lamouri wrote:
> Why not passing: |ExceptionState::GetterContext, "query", "Permissions"| ?

Makes a lot of sense. I scoured code search to figure out what I should do here
but no one seemed to do this "properly".

https://codereview.chromium.org/1365993003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:125:
ExceptionState exceptionState(ExceptionState::UnknownContext, nullptr, nullptr,
scriptState->context()->Global(), scriptState->isolate());
On 2015/09/28 10:48:43, Mounir Lamouri wrote:
> ditto with "request"

Done.

https://codereview.chromium.org/1365993003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:126:
Nullable<WebPermissionType> type = parsePermission(scriptState, rawPermission,
exceptionState);
On 2015/09/28 10:48:43, Mounir Lamouri wrote:
> So here you use |Nullable<>| but you don't actually use the fact that |type|
> could be null. You only return null if there is an exception. It's a bit odd.
> Could you add least add a comment explaining that |Nullable<>| will be null
iif
> an exception was thrown and an ASSERT() will be fired if the caller do not
check
> for the permission and uses a null type? (You can add that in the
> |parsePermission()| method comment.

Done.

https://codereview.chromium.org/1365993003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:143:
ExceptionState exceptionState(ExceptionState::UnknownContext, nullptr, nullptr,
scriptState->context()->Global(), scriptState->isolate());
On 2015/09/28 10:48:43, Mounir Lamouri wrote:
> ditto with "request"

Done.

https://codereview.chromium.org/1365993003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:168:
ExceptionState exceptionState(ExceptionState::UnknownContext, nullptr, nullptr,
scriptState->context()->Global(), scriptState->isolate());
On 2015/09/28 10:48:43, Mounir Lamouri wrote:
> ditto with "revoke"

Done.

[mlamouri (slow - plz ping), 2015-09-28 12:42:03]

lgtm with comment applied.

Thanks!

https://codereview.chromium.org/1365993003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/Permissions.cpp (right):

https://codereview.chromium.org/1365993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:51: // fired
otherwise.
Please, keep line wrapping at 80 chars for comments.

[Lalit Maganti, 2015-09-28 12:45:49]

https://codereview.chromium.org/1365993003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/permissions/Permissions.cpp (right):

https://codereview.chromium.org/1365993003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/permissions/Permissions.cpp:51: // fired
otherwise.
On 2015/09/28 12:42:03, Mounir Lamouri wrote:
> Please, keep line wrapping at 80 chars for comments.

Blink/Chromium code style is still confusing :(

Done.

[Lalit Maganti, 2015-09-28 12:46:05]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-09-28 12:46:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1365993003/#ps40001
(title: "Address review comment")

[commit-bot: I haz the power, 2015-09-28 12:46:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1365993003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1365993003/40001

[commit-bot: I haz the power, 2015-09-28 14:19:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-28 14:20:27]

Patchset 3 (id:??) landed as
https://crrev.com/117d408223c5f53f90f2ba20458e6022552cf81b
Cr-Commit-Position: refs/heads/master@{#351068}