Media Galleries API Scanning: Add Scan Permission and basic test.

Media Galleries API Scanning: Add Scan Permission and basic test.

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

[tommycli, 2014-01-29 18:53:58]

vandebo: Here's a basic permission implementation and basic test. (Test works
when 'scan' permission is 'on', fails when it's off).

[tommycli, 2014-01-29 20:39:00]

thestig: Lei, just realized I will need your stamp on this also... thanks!

[Lei Zhang, 2014-01-29 21:16:10]

Add a test to chrome/test/data/extensions/api_test/media_galleries/no_access/ to
make sure not having the scan permission means no scanning?

https://codereview.chromium.org/133463003/diff/50001/chrome/common/extensions...
File chrome/common/extensions/permissions/media_galleries_permission.cc (right):

https://codereview.chromium.org/133463003/diff/50001/chrome/common/extensions...
chrome/common/extensions/permissions/media_galleries_permission.cc:62: if
(it->permission() == kScanPermission) {
Combine with the kAllAutoDetectedPermission if block?

[Lei Zhang, 2014-01-29 21:18:10]

https://codereview.chromium.org/133463003/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/133463003/diff/50001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:623:
MediaGalleriesPermission::CheckParam scan_param(
Why do this here and not earlier in RunImpl() ?

[tommycli, 2014-01-29 22:30:17]

thestig: I can't make a no-access test, because the current behavior of
startMediaScan() without the permission is simply to never trigger the
onScanProgress callback.

I can test for a failed callback, but not for one that just never gets called.
This is a known flaw...

https://codereview.chromium.org/133463003/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/133463003/diff/50001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:623:
MediaGalleriesPermission::CheckParam scan_param(
On 2014/01/29 21:18:11, Lei Zhang wrote:
> Why do this here and not earlier in RunImpl() ?

Done.

https://codereview.chromium.org/133463003/diff/50001/chrome/common/extensions...
File chrome/common/extensions/permissions/media_galleries_permission.cc (right):

https://codereview.chromium.org/133463003/diff/50001/chrome/common/extensions...
chrome/common/extensions/permissions/media_galleries_permission.cc:62: if
(it->permission() == kScanPermission) {
On 2014/01/29 21:16:11, Lei Zhang wrote:
> Combine with the kAllAutoDetectedPermission if block?

Done.

[tommycli, 2014-01-29 22:59:46]

thestig: added an error to the scan progress per discussion

[Lei Zhang, 2014-01-29 23:07:52]

lgtm

https://codereview.chromium.org/133463003/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/133463003/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:634:
MediaGalleriesEventRouter::Get(GetProfile())->
nit: OnScanError(\nGetExtension()->id());

Pointer derefs at end of lines are pointy and might poke someone's eyes out.

https://codereview.chromium.org/133463003/diff/90001/chrome/common/extensions...
File chrome/common/extensions/api/media_galleries.idl (right):

https://codereview.chromium.org/133463003/diff/90001/chrome/common/extensions...
chrome/common/extensions/api/media_galleries.idl:34: // The scan had an error
and could not proceed.
nit: s/had/encountered/ ?

https://codereview.chromium.org/133463003/diff/90001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/media_galleries/scan/test.js (right):

https://codereview.chromium.org/133463003/diff/90001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/scan/test.js:9: var
startListener = function(details) {
nit: maybe startEventListener? Same elsewhere.

[tommycli, 2014-01-29 23:19:56]

https://codereview.chromium.org/133463003/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/133463003/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:634:
MediaGalleriesEventRouter::Get(GetProfile())->
On 2014/01/29 23:07:53, Lei Zhang wrote:
> nit: OnScanError(\nGetExtension()->id());
> 
> Pointer derefs at end of lines are pointy and might poke someone's eyes out.

Done.

https://codereview.chromium.org/133463003/diff/90001/chrome/common/extensions...
File chrome/common/extensions/api/media_galleries.idl (right):

https://codereview.chromium.org/133463003/diff/90001/chrome/common/extensions...
chrome/common/extensions/api/media_galleries.idl:34: // The scan had an error
and could not proceed.
On 2014/01/29 23:07:53, Lei Zhang wrote:
> nit: s/had/encountered/ ?

Done.

https://codereview.chromium.org/133463003/diff/90001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/media_galleries/scan/test.js (right):

https://codereview.chromium.org/133463003/diff/90001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/scan/test.js:9: var
startListener = function(details) {
On 2014/01/29 23:07:53, Lei Zhang wrote:
> nit: maybe startEventListener? Same elsewhere.

Done.

[commit-bot: I haz the power, 2014-01-29 23:22:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/133463003/100001

[commit-bot: I haz the power, 2014-01-30 03:04:03]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 03:10:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/133463003/100001

[commit-bot: I haz the power, 2014-01-30 08:23:55]

Change committed as 247894