Expose gallery watching functionality to the public mediaGalleries API.

Expose gallery watching functionality to the public mediaGalleries API.

BUG=144491
Committed: https://crrev.com/d0aae1854cb98a5ac6db3657b3213be9c69b7c75
Cr-Commit-Position: refs/heads/master@{#292271}

[tommycli, 2014-08-19 16:36:03]

overall looking good. I don't see any structural deficiencies... mostly nits
below.

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

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:72:
extensions::api::media_galleries::AddGalleryWatch;
make consistent with above declarations... i.e. MediaGalleries::AddGalleryWatch.
And below 2 also.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:130: //
Gets the |gallery_file_path| and |gallery_pref_id| of the gallery specified
Make briefer. Probably don't need both first sentence and second. Should be
"Returns true and sets..." if you choose to use the second sentence.

Probably update the comment in the private api too.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1065:
///////////////////////////////////////////////////////////////////////////////
We don't use this block to divide between classes in this file. But I do think
it's a good idea. maybe add them for the other classes?

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1068:
MediaGalleriesAddGalleryWatchFunction::
I assume you ran git cl format on everything right? If you did, I will say
nothing about formatting whatsoever...

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1105:
HandleResponse(gallery_pref_id, error_str);
If you have an error here, you still check for an event listener in the
HandleResponse section... you should probably just return an error
immediately... 

These two errors are handled inconsistently. In one, you set a result, and in
the other, you don't.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1128:
api::media_galleries::AddGalleryWatchResult result;
Does the callback still get called when there's an error?

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1143:
results_->GetDictionary(0, &dv);
This looks strange. What's the purpose of this?

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1227:
results_ = GetAllGalleryWatch::Results::Create(result);
I wonder if results_ = or SetResult is the correct thing here. They have
different meaning.

It's worth testing this.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.h
(right):

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:301: //
Implements the chrome.mediaGalleries.addGalleryWatch method.
Since the other classes don't have this comment in this file, better remove this
also.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:311: //
AsyncExtensionFunction overrides.
Also remove this comment and above newline, make consistent with the other
classes.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:321: //
Implements the chrome.mediaGalleries.removeGalleryWatch method.
Ditto on this comment.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:331: //
SyncExtensionFunction overrides.
Remove comment and above newline. This comment is also straight-up inaccurate.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:338: //
Implements the chrome.mediaGalleries.getAllGalleryWatch method.
Ditto

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:347: //
SyncExtensionFunction overrides.
Ditto

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:354: //
Implements the chrome.mediaGalleries.removeAllGalleryWatch method.
Ditto

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:363: //
SyncExtensionFunction overrides.
Ditto

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc
(right):

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc:194:
#if defined(OS_MACOSX)
This should not be MAYBE anymore. We should fix the OSX crash. I could
potentially be persuaded that we can do that in a separate patch though.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc:227:
DISABLED_RemoveListenerAndModifyGallery) {
This should not be DISABLED anymore, since we are overhauling the test.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc:260:
ExecuteCmdAndCheckReply(
This concerns me since you should get multiple messages (one for each gallery),
but we proceed after getting one right?

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/manifest.json
(right):

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/manifest.json:2:
"key":
"MIICeAIBADANBgkqhkiG9w0BAQEFAASCAmIwggJeAgEAAoGBAO3OlBjA3w16pm0kjJJDYIua1wEIYLjEi+VhfHw28OW+49ekz3qms9PmFfZvJCAOJEC1lPNj4ZROG3qBE4JEJ6IgeP5tF4fSdie6kL6VOH6tiDHe/ozI5gwcOMXIa9x8nyVyHwozBKMHQTT4ZNFhjbhkxyGYixhkOnk1GOHPiMxXAgMBAAECgYEA6Hrs0yoypRXDf62ccxzFVJWCHXNYQyl6bjuYnEKFzMSKL3Xik9BGuyATVoCeu2u3yz75J9v8WqTlPiTTsqT4bCeC4r1RGDmnS4gftWVKqHpl/HhVKRFvtu5wlPL9jdECnZdB7Viro0k8LqOqOUG2rPbKeK1JolKQEKp+Zc7u6SkCQQD6VWcwrgNQ3+cXFM7WgrOrNYtd4dpRVrfdMOZuNG9QeUkVUM+dypfKGt3iujbyTFOGIpcQXgrjrp1Cvg6die97AkEA8zCWdskxOZk0XsLhxqlNqwDNnOdTOCYsSODJ/0S/tY7aasjQ9l/CaO7Ewk4KFBcKfUEFxqnS43dUESQuxGox1QJBAOLwEUAcQH1MvZ47CteEXYTUYaeWW+hpCP3Imt1FQ7Rcf1iHje+p/bkGaihkT/OQJhfvu6rm1lBZt88NpQpERYkCQB1XOGAeiAzd+h6QvrK8TH2cPDJ5CmZU3LTyQgzFW6fpE1sc3XFMPmVwoVacGcUwvng8Nxkv9hyxkff7eJUEBv0CQQCzTxZw14iXKAW5vxNYE6BGSjoEk7tQ+ljHzGOZmotl3WQ+6BKdMgQIGvH+XJzrYOEdTCCL1/YzIs2533fm67md",
Do we need this 'key' field? The other manifest.jsons in
chrome/test/data/extensions/api_test/media_galleries don't seem to have it. I've
never seen this before.

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/manifest.json:5:
"description": "test for chrome.mediaGalleries.addGalleryWatch,
chrome.mediaGalleries.removeGalleryWatch and onGalleryChanged events ",
do we also test the getAllGalleryWatch and removeAllGalleryWatch functions?

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js
(right):

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js:21: if
(!!chrome.runtime.lastError) {
never seen the double bang trick before. Woo learn something new everyday! =)

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js:41:
for (var i = 0; i < results.length; ++i) {
I'm glad we got someone who knows JS to look at our test.js files. You'll
probably find tons of errors in the javascript files of Chrome, since we are
mostly JS novices.

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js:73:
onAddWatchRequestCallback);
indent

https://codereview.chromium.org/440813002/diff/40001/extensions/browser/exten...
File extensions/browser/extension_function.cc (left):

https://codereview.chromium.org/440813002/diff/40001/extensions/browser/exten...
extensions/browser/extension_function.cc:324: LOG(ERROR) << "Bad extension
message " << name_;
Interesting... why?

[Oren Blasberg, 2014-08-20 22:59:38]

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

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:72:
extensions::api::media_galleries::AddGalleryWatch;
On 2014/08/19 16:36:02, tommycli wrote:
> make consistent with above declarations... i.e.
MediaGalleries::AddGalleryWatch.
> And below 2 also.

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:130: //
Gets the |gallery_file_path| and |gallery_pref_id| of the gallery specified
On 2014/08/19 16:36:02, tommycli wrote:
> Make briefer. Probably don't need both first sentence and second. Should be
> "Returns true and sets..." if you choose to use the second sentence.
> 
> Probably update the comment in the private api too.

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1065:
///////////////////////////////////////////////////////////////////////////////
On 2014/08/19 16:36:02, tommycli wrote:
> We don't use this block to divide between classes in this file. But I do think
> it's a good idea. maybe add them for the other classes?

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1068:
MediaGalleriesAddGalleryWatchFunction::
On 2014/08/19 16:36:02, tommycli wrote:
> I assume you ran git cl format on everything right? If you did, I will say
> nothing about formatting whatsoever...


Didn't at first, did in patch 4.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1105:
HandleResponse(gallery_pref_id, error_str);
On 2014/08/19 16:36:02, tommycli wrote:
> If you have an error here, you still check for an event listener in the
> HandleResponse section... you should probably just return an error
> immediately... 
> 
> These two errors are handled inconsistently. In one, you set a result, and in
> the other, you don't.

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1128:
api::media_galleries::AddGalleryWatchResult result;
On 2014/08/19 16:36:02, tommycli wrote:
> Does the callback still get called when there's an error?

It does, as evidenced by the setupWatchOnUnlistenedValidGalleries test.
Also I tried it. It does, with success false, and the error occurs too.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1143:
results_->GetDictionary(0, &dv);
On 2014/08/19 16:36:02, tommycli wrote:
> This looks strange. What's the purpose of this?


Whoops. It was part of debugging code I forgot to remove.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1227:
results_ = GetAllGalleryWatch::Results::Create(result);
On 2014/08/19 16:36:02, tommycli wrote:
> I wonder if results_ = or SetResult is the correct thing here. They have
> different meaning.
> 
> It's worth testing this.

It looks like SetResult is used when you have just one Value:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

It's a convenience for making it into a ListValue.

In this case, we already have a list so it shouldn't be needed and setting
results_ directly is the most direct.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.h
(right):

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:301: //
Implements the chrome.mediaGalleries.addGalleryWatch method.
On 2014/08/19 16:36:02, tommycli wrote:
> Since the other classes don't have this comment in this file, better remove
this
> also.

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:311: //
AsyncExtensionFunction overrides.
On 2014/08/19 16:36:02, tommycli wrote:
> Also remove this comment and above newline, make consistent with the other
> classes.

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:321: //
Implements the chrome.mediaGalleries.removeGalleryWatch method.
On 2014/08/19 16:36:02, tommycli wrote:
> Ditto on this comment.

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:331: //
SyncExtensionFunction overrides.
On 2014/08/19 16:36:02, tommycli wrote:
> Remove comment and above newline. This comment is also straight-up inaccurate.

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:338: //
Implements the chrome.mediaGalleries.getAllGalleryWatch method.
On 2014/08/19 16:36:02, tommycli wrote:
> Ditto

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:347: //
SyncExtensionFunction overrides.
On 2014/08/19 16:36:02, tommycli wrote:
> Ditto

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:354: //
Implements the chrome.mediaGalleries.removeAllGalleryWatch method.
On 2014/08/19 16:36:02, tommycli wrote:
> Ditto

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:363: //
SyncExtensionFunction overrides.
On 2014/08/19 16:36:02, tommycli wrote:
> Ditto

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc
(right):

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc:194:
#if defined(OS_MACOSX)
On 2014/08/19 16:36:03, tommycli wrote:
> This should not be MAYBE anymore. We should fix the OSX crash. I could
> potentially be persuaded that we can do that in a separate patch though.

Will do that in separate cl.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc:227:
DISABLED_RemoveListenerAndModifyGallery) {
On 2014/08/19 16:36:03, tommycli wrote:
> This should not be DISABLED anymore, since we are overhauling the test.

Will remove it in followup CL.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc:260:
ExecuteCmdAndCheckReply(
On 2014/08/19 16:36:02, tommycli wrote:
> This concerns me since you should get multiple messages (one for each
gallery),
> but we proceed after getting one right?

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/manifest.json
(right):

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/manifest.json:2:
"key":
"MIICeAIBADANBgkqhkiG9w0BAQEFAASCAmIwggJeAgEAAoGBAO3OlBjA3w16pm0kjJJDYIua1wEIYLjEi+VhfHw28OW+49ekz3qms9PmFfZvJCAOJEC1lPNj4ZROG3qBE4JEJ6IgeP5tF4fSdie6kL6VOH6tiDHe/ozI5gwcOMXIa9x8nyVyHwozBKMHQTT4ZNFhjbhkxyGYixhkOnk1GOHPiMxXAgMBAAECgYEA6Hrs0yoypRXDf62ccxzFVJWCHXNYQyl6bjuYnEKFzMSKL3Xik9BGuyATVoCeu2u3yz75J9v8WqTlPiTTsqT4bCeC4r1RGDmnS4gftWVKqHpl/HhVKRFvtu5wlPL9jdECnZdB7Viro0k8LqOqOUG2rPbKeK1JolKQEKp+Zc7u6SkCQQD6VWcwrgNQ3+cXFM7WgrOrNYtd4dpRVrfdMOZuNG9QeUkVUM+dypfKGt3iujbyTFOGIpcQXgrjrp1Cvg6die97AkEA8zCWdskxOZk0XsLhxqlNqwDNnOdTOCYsSODJ/0S/tY7aasjQ9l/CaO7Ewk4KFBcKfUEFxqnS43dUESQuxGox1QJBAOLwEUAcQH1MvZ47CteEXYTUYaeWW+hpCP3Imt1FQ7Rcf1iHje+p/bkGaihkT/OQJhfvu6rm1lBZt88NpQpERYkCQB1XOGAeiAzd+h6QvrK8TH2cPDJ5CmZU3LTyQgzFW6fpE1sc3XFMPmVwoVacGcUwvng8Nxkv9hyxkff7eJUEBv0CQQCzTxZw14iXKAW5vxNYE6BGSjoEk7tQ+ljHzGOZmotl3WQ+6BKdMgQIGvH+XJzrYOEdTCCL1/YzIs2533fm67md",
On 2014/08/19 16:36:03, tommycli wrote:
> Do we need this 'key' field? The other manifest.jsons in
> chrome/test/data/extensions/api_test/media_galleries don't seem to have it.
I've
> never seen this before.

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/manifest.json:2:
"key":
"MIICeAIBADANBgkqhkiG9w0BAQEFAASCAmIwggJeAgEAAoGBAO3OlBjA3w16pm0kjJJDYIua1wEIYLjEi+VhfHw28OW+49ekz3qms9PmFfZvJCAOJEC1lPNj4ZROG3qBE4JEJ6IgeP5tF4fSdie6kL6VOH6tiDHe/ozI5gwcOMXIa9x8nyVyHwozBKMHQTT4ZNFhjbhkxyGYixhkOnk1GOHPiMxXAgMBAAECgYEA6Hrs0yoypRXDf62ccxzFVJWCHXNYQyl6bjuYnEKFzMSKL3Xik9BGuyATVoCeu2u3yz75J9v8WqTlPiTTsqT4bCeC4r1RGDmnS4gftWVKqHpl/HhVKRFvtu5wlPL9jdECnZdB7Viro0k8LqOqOUG2rPbKeK1JolKQEKp+Zc7u6SkCQQD6VWcwrgNQ3+cXFM7WgrOrNYtd4dpRVrfdMOZuNG9QeUkVUM+dypfKGt3iujbyTFOGIpcQXgrjrp1Cvg6die97AkEA8zCWdskxOZk0XsLhxqlNqwDNnOdTOCYsSODJ/0S/tY7aasjQ9l/CaO7Ewk4KFBcKfUEFxqnS43dUESQuxGox1QJBAOLwEUAcQH1MvZ47CteEXYTUYaeWW+hpCP3Imt1FQ7Rcf1iHje+p/bkGaihkT/OQJhfvu6rm1lBZt88NpQpERYkCQB1XOGAeiAzd+h6QvrK8TH2cPDJ5CmZU3LTyQgzFW6fpE1sc3XFMPmVwoVacGcUwvng8Nxkv9hyxkff7eJUEBv0CQQCzTxZw14iXKAW5vxNYE6BGSjoEk7tQ+ljHzGOZmotl3WQ+6BKdMgQIGvH+XJzrYOEdTCCL1/YzIs2533fm67md",
On 2014/08/19 16:36:03, tommycli wrote:
> Do we need this 'key' field? The other manifest.jsons in
> chrome/test/data/extensions/api_test/media_galleries don't seem to have it.
I've
> never seen this before.

Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/manifest.json:5:
"description": "test for chrome.mediaGalleries.addGalleryWatch,
chrome.mediaGalleries.removeGalleryWatch and onGalleryChanged events ",
On 2014/08/19 16:36:03, tommycli wrote:
> do we also test the getAllGalleryWatch and removeAllGalleryWatch functions?

Yes. Done.

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js
(right):

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js:21: if
(!!chrome.runtime.lastError) {
On 2014/08/19 16:36:03, tommycli wrote:
> never seen the double bang trick before. Woo learn something new everyday! =)

It's actually kind of bad since !!0 will still return false. If we're checking
for the presence of something it's generally safer to explicitly convert to null
and undefined .. but in this test we can be reasonably confident about it.

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js:41:
for (var i = 0; i < results.length; ++i) {
On 2014/08/19 16:36:03, tommycli wrote:
> I'm glad we got someone who knows JS to look at our test.js files. You'll
> probably find tons of errors in the javascript files of Chrome, since we are
> mostly JS novices.
Hehe, thanks. Having a js compiler helps.

https://codereview.chromium.org/440813002/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js:73:
onAddWatchRequestCallback);
On 2014/08/19 16:36:03, tommycli wrote:
> indent

Done.

https://codereview.chromium.org/440813002/diff/40001/extensions/browser/exten...
File extensions/browser/extension_function.cc (left):

https://codereview.chromium.org/440813002/diff/40001/extensions/browser/exten...
extensions/browser/extension_function.cc:324: LOG(ERROR) << "Bad extension
message " << name_;
On 2014/08/19 16:36:03, tommycli wrote:
> Interesting... why?

Ugh. My bad, this was removed in error (i was debugging this file earlier)
Thanks for combing over my CL and finding these.

[tommycli, 2014-08-20 23:21:13]

orenb: Great, only one blocking issue below:

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

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1128:
api::media_galleries::AddGalleryWatchResult result;
On 2014/08/20 22:59:36, Oren Blasberg wrote:
> On 2014/08/19 16:36:02, tommycli wrote:
> > Does the callback still get called when there's an error?
> 
> It does, as evidenced by the setupWatchOnUnlistenedValidGalleries test.
> Also I tried it. It does, with success false, and the error occurs too.
> 
> 

Awesome. Thanks for testing that out.

https://codereview.chromium.org/440813002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1227:
results_ = GetAllGalleryWatch::Results::Create(result);
On 2014/08/20 22:59:36, Oren Blasberg wrote:
> On 2014/08/19 16:36:02, tommycli wrote:
> > I wonder if results_ = or SetResult is the correct thing here. They have
> > different meaning.
> > 
> > It's worth testing this.
> 
> It looks like SetResult is used when you have just one Value:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> 
> It's a convenience for making it into a ListValue.
> 
> In this case, we already have a list so it shouldn't be needed and setting
> results_ directly is the most direct.

Oh okay, great. thanks for verifyincg

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

https://codereview.chromium.org/440813002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1126:
result.success = false;
You don't SetResult here, so this result is never given to the callback.

I assume there's no automated test that catches this case?

https://codereview.chromium.org/440813002/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js
(right):

https://codereview.chromium.org/440813002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js:64:
function getUnlistenedAddWatchCallback(expectedNumCalls) {
Since this is a bit non-obvious, maybe add some documentation for the next guy.

[tommycli, 2014-08-20 23:44:10]

orenb: Hey, I did a bit more investigation on your tests. I'm confused... See
below

https://codereview.chromium.org/440813002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc
(right):

https://codereview.chromium.org/440813002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc:256:
ExtensionTestMessageListener add_watch_runtime_error_received(
I believe you don't need to make ExtensionTestMessageListeners here. Look at the
body of ExecuteCmdAndCheckReply.

Shouldn't you just pass kAddGalleryWatchRequestRuntimeError to that call?

Why is this test currently passing ?

https://codereview.chromium.org/440813002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc:287:
ExtensionTestMessageListener invalid_gallery_watch_request_finished(
Same comment as above. Why is there a separate ExtensionTestMessageListener from
the one inth ebody of ExecuteCmdAndCheckReply?

Moreover, how is this test even passing if we currently aren't giving a result
to the callback?

I feel confused.

[Oren Blasberg, 2014-08-21 02:45:21]

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

https://codereview.chromium.org/440813002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1126:
result.success = false;
On 2014/08/20 23:21:13, tommycli wrote:
> You don't SetResult here, so this result is never given to the callback.
> 
> I assume there's no automated test that catches this case?

As discussed in the test file, I've fixed this. Also I updated that test so that
it would catch this case.

https://codereview.chromium.org/440813002/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc
(right):

https://codereview.chromium.org/440813002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc:256:
ExtensionTestMessageListener add_watch_runtime_error_received(
On 2014/08/20 23:44:09, tommycli wrote:
> I believe you don't need to make ExtensionTestMessageListeners here. Look at
the
> body of ExecuteCmdAndCheckReply.
> 
> Shouldn't you just pass kAddGalleryWatchRequestRuntimeError to that call?
> 
> Why is this test currently passing ?

The test is passing because "setupWatchOnUnlistenedValidGalleries" does end
with:

  chrome.test.sendMessage('add_gallery_watch_ok');

so the ExecuteCmdAndCheckReply does indeed get that reply as expected.

But you're right, it isn't really adding any value to the test, so I just
ipmlemented your suggestion. Thanks!

https://codereview.chromium.org/440813002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc:287:
ExtensionTestMessageListener invalid_gallery_watch_request_finished(
On 2014/08/20 23:44:09, tommycli wrote:
> Same comment as above. Why is there a separate ExtensionTestMessageListener
from
> the one inth ebody of ExecuteCmdAndCheckReply?
> 
> Moreover, how is this test even passing if we currently aren't giving a result
> to the callback?
> 
> I feel confused.

Same reason as above - the setupWatchOnInvalidGallery method ends by sending the
'ok' message, prior to any callbacks executing which would send the
'kAddGalleryWatchRequestFailed' message.

You're right, we aren't giving a result to the callback. That's a bug which I am
fixing in the api file. But the test still passes because
onAddWatchRequestCallback treats the absence of 'details' as a failure; it goes
into the first IF.

So, I've removed the "!details" part of that IF, which makes the test fail
(timeout). Then I updated the API to fix the other bug. And finally I've made
the same change here as I did for the above comment, getting rid of the "OK"
message.

Thanks so much for noticing this stuff!

https://codereview.chromium.org/440813002/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js
(right):

https://codereview.chromium.org/440813002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js:64:
function getUnlistenedAddWatchCallback(expectedNumCalls) {
On 2014/08/20 23:21:13, tommycli wrote:
> Since this is a bit non-obvious, maybe add some documentation for the next
guy.

Done.

[tommycli, 2014-08-21 03:20:17]

very nice work on the tests. lgtm.

[Oren Blasberg, 2014-08-22 20:31:37]

Thanks for the LG! Just waiting on lgtm from other OWNERS of these files.

[Alexei Svitkine (slow), 2014-08-22 20:32:53]

lgtm

[Lei Zhang, 2014-08-25 22:45:21]

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1097:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
nit: we usually check what thread we are on before anything else.

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1119:
MediaGalleryPrefId gallery_pref_id = 0;
Use kInvalidMediaGalleryPrefId instead of 0

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1161:
api::media_galleries::AddGalleryWatchResult result;
These two lines are redundant with lines 1152/1153 above.

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.h
(right):

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:316: };
We probably should have DISALLOW_COPY_AND_ASSIGN() at the bottom of these
classes, but never did it because we kept on copy + pasting from code that din't
have it.

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
File
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc
(right):

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
nit: "(c) 2012" -> 2014, ditto in
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc:197:
#define MAYBE_BasicGalleryWatch DISABLED_BasicGalleryWatch
Maybe you should enable your new tests on OS X?
DISABLED_RemoveListenerAndModifyGallery below as well...

https://codereview.chromium.org/440813002/diff/120001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js
(left):

https://codereview.chromium.org/440813002/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js:36:
for (var i = 0; i < results.lendth; ++i) {
!! how did this ever work?

https://codereview.chromium.org/440813002/diff/120001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js
(right):

https://codereview.chromium.org/440813002/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js:21: if
(!details.success) {
flip this block?

if (detauls.success) {
  // success
} else {
  // fail!!!!
}

[Oren Blasberg, 2014-08-26 00:40:49]

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1097:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2014/08/25 22:45:21, Lei Zhang wrote:
> nit: we usually check what thread we are on before anything else.

Done.

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1119:
MediaGalleryPrefId gallery_pref_id = 0;
On 2014/08/25 22:45:21, Lei Zhang wrote:
> Use kInvalidMediaGalleryPrefId instead of 0

Done.

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:1161:
api::media_galleries::AddGalleryWatchResult result;
On 2014/08/25 22:45:21, Lei Zhang wrote:
> These two lines are redundant with lines 1152/1153 above.

Done.

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.h
(right):

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_api.h:316: };
On 2014/08/25 22:45:21, Lei Zhang wrote:
> We probably should have DISALLOW_COPY_AND_ASSIGN() at the bottom of these
> classes, but never did it because we kept on copy + pasting from code that
din't
> have it.

Done.

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
File
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc
(right):

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2014/08/25 22:45:21, Lei Zhang wrote:
> nit: "(c) 2012" -> 2014, ditto in
> chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js

Done.

https://codereview.chromium.org/440813002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries/media_galleries_watch_apitest.cc:197:
#define MAYBE_BasicGalleryWatch DISABLED_BasicGalleryWatch
On 2014/08/25 22:45:21, Lei Zhang wrote:
> Maybe you should enable your new tests on OS X?
> DISABLED_RemoveListenerAndModifyGallery below as well...

I discussed w/ Tommy. I will do this in a follow up CL immediately after this
one. That way if those tests end up flaky we can handle them independently of
rolling back this entire CL.

https://codereview.chromium.org/440813002/diff/120001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js
(left):

https://codereview.chromium.org/440813002/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js:36:
for (var i = 0; i < results.lendth; ++i) {
On 2014/08/25 22:45:21, Lei Zhang wrote:
> !! how did this ever work?

Lol, probably because every number < undefined == false. The cc test probably
wasn't stringent enough before to catch the issue.

https://codereview.chromium.org/440813002/diff/120001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js
(right):

https://codereview.chromium.org/440813002/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/media_galleries/gallerywatch/test.js:21: if
(!details.success) {
On 2014/08/25 22:45:21, Lei Zhang wrote:
> flip this block?
> 
> if (detauls.success) {
>   // success
> } else {
>   // fail!!!!
> }

Done.

[Lei Zhang, 2014-08-26 00:57:53]

lgtm

[Oren Blasberg, 2014-08-27 21:51:45]

The CQ bit was checked by orenb@chromium.org

[commit-bot: I haz the power, 2014-08-27 21:53:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/orenb@chromium.org/440813002/200001

[commit-bot: I haz the power, 2014-08-27 22:54:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-27 23:52:27]

Committed patchset #11 (id:200001) as c88388a0250b1387050599878dee14a70b0d1a53

[commit-bot: I haz the power, 2014-09-10 02:55:45]

Patchset 11 (id:??) landed as
https://crrev.com/d0aae1854cb98a5ac6db3657b3213be9c69b7c75
Cr-Commit-Position: refs/heads/master@{#292271}