Implement PreauthorizePermission

Implement PreauthorizePermission

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269934

[michaelbai, 2014-05-06 23:09:50]

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
File android_webview/native/permission/permission_request_handler.cc (right):

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
android_webview/native/permission/permission_request_handler.cc:32: }
Note, the all resources will passed to permission callback even if requested
resources are partially preauthorized,

[benm (inactive), 2014-05-07 17:01:20]

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
File android_webview/native/permission/permission_request_handler.cc (right):

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
android_webview/native/permission/permission_request_handler.cc:62:
preauthorized_permission_[key] |= resources;
I think you need to initialise this mapped value in the case that they key
doesn't exist yet?

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
File android_webview/native/permission/permission_request_handler_unittest.cc
(right):

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
android_webview/native/permission/permission_request_handler_unittest.cc:288:
scoped_ptr<AwPermissionRequestDelegate> delegate1;
Any need for the '1'?

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
android_webview/native/permission/permission_request_handler_unittest.cc:312: //
Ask the resources which weren't preauthorized.
would it make sense to split this into two test cases?

[mkosiba (inactive), 2014-05-07 17:25:50]

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
File android_webview/native/permission/permission_request_handler.cc (right):

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
android_webview/native/permission/permission_request_handler.cc:59: if
(key.empty())
ok, so if we get here the user managed to create a Uri that GURL doesn't like. I
think we should LOG(ERROR). Experience shows that silently dropping things on
the floor leads to very unhappy developers.

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
android_webview/native/permission/permission_request_handler.cc:100:
preauthorized_permission_.find(origin.GetOrigin().spec());
I'm paranoid so I'd suggest doing the same thing you do in line 58:

std::string key = ...
if (key.empty())
  return false;

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
android_webview/native/permission/permission_request_handler.cc:101: return
(resources & i->second) == resources;
what if we don't find the origin in the map? you'll reference uninitialized
memory

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
File android_webview/native/permission/permission_request_handler.h (right):

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
android_webview/native/permission/permission_request_handler.h:55: bool
Preauthorized(const GURL& origin, int64 resources);
nit: IsPreauthorized?

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
File android_webview/native/permission/permission_request_handler_unittest.cc
(right):

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
android_webview/native/permission/permission_request_handler_unittest.cc:312: //
Ask the resources which weren't preauthorized.
On 2014/05/07 17:01:21, benm wrote:
> would it make sense to split this into two test cases?

or 4 even?

[michaelbai, 2014-05-07 19:22:25]

PTAL

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
File android_webview/native/permission/permission_request_handler.cc (right):

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
android_webview/native/permission/permission_request_handler.cc:59: if
(key.empty())
On 2014/05/07 17:25:50, mkosiba wrote:
> ok, so if we get here the user managed to create a Uri that GURL doesn't like.
I
> think we should LOG(ERROR). Experience shows that silently dropping things on
> the floor leads to very unhappy developers.

Done.

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
android_webview/native/permission/permission_request_handler.cc:62:
preauthorized_permission_[key] |= resources;
In this case, the default value is 0, we can use it directly.

On 2014/05/07 17:01:21, benm wrote:
> I think you need to initialise this mapped value in the case that they key
> doesn't exist yet?

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
android_webview/native/permission/permission_request_handler.cc:100:
preauthorized_permission_.find(origin.GetOrigin().spec());
I would like to make you happy, but I need to check whether the value is found
anyway, I'd prefer not to check key here :)


On 2014/05/07 17:25:50, mkosiba wrote:
> I'm paranoid so I'd suggest doing the same thing you do in line 58:
> 
> std::string key = ...
> if (key.empty())
>   return false;

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
android_webview/native/permission/permission_request_handler.cc:101: return
(resources & i->second) == resources;
Oops, I were checking it there, it was removed with log by accident.


On 2014/05/07 17:25:50, mkosiba wrote:
> what if we don't find the origin in the map? you'll reference uninitialized
> memory

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
File android_webview/native/permission/permission_request_handler.h (right):

https://codereview.chromium.org/274443002/diff/20001/android_webview/native/p...
android_webview/native/permission/permission_request_handler.h:55: bool
Preauthorized(const GURL& origin, int64 resources);
I prefer the short name.

On 2014/05/07 17:25:50, mkosiba wrote:
> nit: IsPreauthorized?

[benm (inactive), 2014-05-08 14:50:21]

https://codereview.chromium.org/274443002/diff/40001/android_webview/native/p...
File android_webview/native/permission/permission_request_handler.h (right):

https://codereview.chromium.org/274443002/diff/40001/android_webview/native/p...
android_webview/native/permission/permission_request_handler.h:55: bool
Preauthorized(const GURL& origin, int64 resources);
nit: isPreauthorized?

oh, just seen that Marcin had the same comment in the previous round. I think
this is nicer, doesn't chromium normally name methods like this?

https://codereview.chromium.org/274443002/diff/40001/android_webview/native/p...
File android_webview/native/permission/permission_request_handler_unittest.cc
(right):

https://codereview.chromium.org/274443002/diff/40001/android_webview/native/p...
android_webview/native/permission/permission_request_handler_unittest.cc:306:
origin, AwPermissionRequest::AudioCapture,
make this resource param a constant (and use in the assert below)?

Same for each of the tests.

[michaelbai, 2014-05-08 16:44:30]

PTAL

https://codereview.chromium.org/274443002/diff/40001/android_webview/native/p...
File android_webview/native/permission/permission_request_handler.h (right):

https://codereview.chromium.org/274443002/diff/40001/android_webview/native/p...
android_webview/native/permission/permission_request_handler.h:55: bool
Preauthorized(const GURL& origin, int64 resources);
There is no constant usage. I prefer to use Is+adj/noun and past participle, 
like IsVisiable(), Allowed() and Preauthorized here. 


On 2014/05/08 14:50:21, benm wrote:
> nit: isPreauthorized?
> 
> oh, just seen that Marcin had the same comment in the previous round. I think
> this is nicer, doesn't chromium normally name methods like this?

https://codereview.chromium.org/274443002/diff/40001/android_webview/native/p...
File android_webview/native/permission/permission_request_handler_unittest.cc
(right):

https://codereview.chromium.org/274443002/diff/40001/android_webview/native/p...
android_webview/native/permission/permission_request_handler_unittest.cc:306:
origin, AwPermissionRequest::AudioCapture,
On 2014/05/08 14:50:21, benm wrote:
> make this resource param a constant (and use in the assert below)?
> 
> Same for each of the tests.

Done.

[benm (inactive), 2014-05-08 17:26:10]

lgtm

[mkosiba (inactive), 2014-05-12 17:50:45]

lgtm

[michaelbai, 2014-05-12 17:54:12]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2014-05-12 17:56:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelbai@chromium.org/274443002/60001

[commit-bot: I haz the power, 2014-05-12 21:28:05]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-13 00:17:10]

Change committed as 269934