Add end to end test for Media galleries scan API.

Add end to end test for Media galleries scan API.

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

[Lei Zhang, 2014-02-20 23:25:19]

lgtm

https://codereview.chromium.org/174013009/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
(right):

https://codereview.chromium.org/174013009/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:97:
return NULL;
Return |controller| or add a comment to explain why we are returning NULL?

https://codereview.chromium.org/174013009/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:543:
base::CreateDirectory(scan_root.path().AppendASCII("a/b"));
Make sure these succeed.

https://codereview.chromium.org/174013009/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:544:
file_util::WriteFile(scan_root.path().AppendASCII("a/b/c.jpg"),
Check the return results.

https://codereview.chromium.org/174013009/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/media_galleries/cancel_scan/manifest.json
(right):

https://codereview.chromium.org/174013009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/media_galleries/cancel_scan/manifest.json:2:
"name": "chrome.mediaGalleries.scan",
rename

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

https://codereview.chromium.org/174013009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/media_galleries/scan/test.js:20: } else {
Is the state transition: start -> finish -> done?

If so, shouldn't this be: else if (scanProcess == 'finish')

so we don't run it a second time for the 'done' state?

[vandebo (ex-Chrome), 2014-02-20 23:41:38]

https://codereview.chromium.org/174013009/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
(right):

https://codereview.chromium.org/174013009/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:97:
return NULL;
On 2014/02/20 23:25:20, Lei Zhang (OOO starting 02-21) wrote:
> Return |controller| or add a comment to explain why we are returning NULL?

Done.

https://codereview.chromium.org/174013009/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:543:
base::CreateDirectory(scan_root.path().AppendASCII("a/b"));
On 2014/02/20 23:25:20, Lei Zhang (OOO starting 02-21) wrote:
> Make sure these succeed.

Done.

https://codereview.chromium.org/174013009/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:544:
file_util::WriteFile(scan_root.path().AppendASCII("a/b/c.jpg"),
On 2014/02/20 23:25:20, Lei Zhang (OOO starting 02-21) wrote:
> Check the return results.

Done.

https://codereview.chromium.org/174013009/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/media_galleries/cancel_scan/manifest.json
(right):

https://codereview.chromium.org/174013009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/media_galleries/cancel_scan/manifest.json:2:
"name": "chrome.mediaGalleries.scan",
On 2014/02/20 23:25:20, Lei Zhang (OOO starting 02-21) wrote:
> rename

Done.

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

https://codereview.chromium.org/174013009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/media_galleries/scan/test.js:20: } else {
On 2014/02/20 23:25:20, Lei Zhang (OOO starting 02-21) wrote:
> Is the state transition: start -> finish -> done?
> 
> If so, shouldn't this be: else if (scanProcess == 'finish')
> 
> so we don't run it a second time for the 'done' state?

We don't get a third callback, I just want to change the variable so that if we
get two finishes for some reason, that will be an error.

[vandebo (ex-Chrome), 2014-02-20 23:42:42]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-02-20 23:44:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/174013009/90001

[Lei Zhang, 2014-02-20 23:51:33]

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

https://codereview.chromium.org/174013009/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:527: //
The scan result dialog is not implemented on GTK.
Isn't it clearer to write:

#if defined(TOOLKIT_GTK)
// disable
#else
// run
#endif

Probably add a TODO to remove this when GTK goes away.

[vandebo (ex-Chrome), 2014-02-20 23:59:44]

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

https://codereview.chromium.org/174013009/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:527: //
The scan result dialog is not implemented on GTK.
On 2014/02/20 23:51:34, Lei Zhang (OOO starting 02-21) wrote:
> Isn't it clearer to write:
> 
> #if defined(TOOLKIT_GTK)
> // disable
> #else
> // run
> #endif

I actually think it's clearer to have the positive test.  If there was a new
toolkit introduced, for example, this would be wrong.

> Probably add a TODO to remove this when GTK goes away.

Mmee - there's lot of GTK code to be removed that isn't commented.  Having GTK
in the comment should be enough to find this instance when GTK code goes away.

[Lei Zhang, 2014-02-21 00:03:13]

On 2014/02/20 23:59:44, vandebo wrote:
>
https://codereview.chromium.org/174013009/diff/90001/chrome/browser/extension...
> File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
> (right):
> 
>
https://codereview.chromium.org/174013009/diff/90001/chrome/browser/extension...
> chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:527:
//
> The scan result dialog is not implemented on GTK.
> On 2014/02/20 23:51:34, Lei Zhang (OOO starting 02-21) wrote:
> > Isn't it clearer to write:
> > 
> > #if defined(TOOLKIT_GTK)
> > // disable
> > #else
> > // run
> > #endif
> 
> I actually think it's clearer to have the positive test.  If there was a new
> toolkit introduced, for example, this would be wrong.

Not that we would do that any time soon, but if a new platform suddenly appears,
it would fail silently. I'd rather have it fail, acknowledge the failure, and
then explicitly disable it.

> > Probably add a TODO to remove this when GTK goes away.
> 
> Mmee - there's lot of GTK code to be removed that isn't commented.  Having GTK
> in the comment should be enough to find this instance when GTK code goes away.

TOOLKIT_GTK is a better term to search for, but we have other GTK bits to remove
too, so we probably won't forget this one.

[vandebo (ex-Chrome), 2014-02-21 00:09:05]

The CQ bit was unchecked by vandebo@chromium.org

[vandebo (ex-Chrome), 2014-02-21 00:09:13]

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

https://codereview.chromium.org/174013009/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:527: //
The scan result dialog is not implemented on GTK.
On 2014/02/20 23:51:34, Lei Zhang (OOO starting 02-21) wrote:
> Isn't it clearer to write:
> 
> #if defined(TOOLKIT_GTK)
> // disable
> #else
> // run
> #endif
> 
> Probably add a TODO to remove this when GTK goes away.

Done.

[vandebo (ex-Chrome), 2014-02-21 00:10:49]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-02-21 00:18:57]

Try jobs failed on following builders: linux_chromium_rel

[commit-bot: I haz the power, 2014-02-21 00:19:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/174013009/230002

[commit-bot: I haz the power, 2014-02-21 00:36:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-21 00:36:55]

Try jobs failed on following builders: linux_chromium_rel

[vandebo (ex-Chrome), 2014-02-21 22:02:23]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-02-21 22:03:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/174013009/500001

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/174013009/500001

[commit-bot: I haz the power, 2014-02-22 07:43:16]

Change committed as 252756