Files.app Test: check the cast icon in drive volume

Files.app Test: check the cast icon in drive volume 

This patch introduces the mock cast API which provide a dummy cast list and the cast icon is shown on the window. So we can check if the icon is shown correctly.

BUG=405415
TEST=manually tested

Committed: https://crrev.com/6939ba35b40b86b97e244ec2e6c445a3872b5334
Cr-Commit-Position: refs/heads/master@{#292944}

[yoshiki, 2014-08-26 00:17:22]

yoshiki@chromium.org changed reviewers:
+ hirono@chromium.org

[yoshiki, 2014-08-26 00:17:51]

@hirono, PTAL. Thanks.

[hirono, 2014-08-26 00:56:58]

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js
(right):

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js:10:
* @const
nit: @enum {string}

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js:12:
chrome.cast.ReceiverAvailability = {
Please freeze it.

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js:34:
* @param {function} onInitSuccess
Could you add arguments and return types to the annotation of function?

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/open_video_files.js
(right):

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/open_video_files.js:53:
return true;
Return true is needed?

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/test_util.js
(right):

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/test_util.js:45:
function loadMockCastExtesntion(appWindow) {
Please add jsdoc.

https://codereview.chromium.org/505043002/diff/1/ui/file_manager/video_player...
File ui/file_manager/video_player/js/cast/caster.js (right):

https://codereview.chromium.org/505043002/diff/1/ui/file_manager/video_player...
ui/file_manager/video_player/js/cast/caster.js:84: function
loadMockCastAPI(callback) {
Is the function used?

[yoshiki, 2014-08-26 05:31:24]

@hirono, PTAL. Thanks.

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js
(right):

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js:10:
* @const
On 2014/08/26 00:56:57, hirono wrote:
> nit: @enum {string}

Done.

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js:12:
chrome.cast.ReceiverAvailability = {
On 2014/08/26 00:56:57, hirono wrote:
> Please freeze it.

Done.

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js:34:
* @param {function} onInitSuccess
On 2014/08/26 00:56:57, hirono wrote:
> Could you add arguments and return types to the annotation of function?

Done.

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/open_video_files.js
(right):

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/open_video_files.js:53:
return true;
On 2014/08/26 00:56:58, hirono wrote:
> Return true is needed?

We don't need it. Removed.

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/test_util.js
(right):

https://codereview.chromium.org/505043002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/test_util.js:45:
function loadMockCastExtesntion(appWindow) {
On 2014/08/26 00:56:58, hirono wrote:
> Please add jsdoc.

Done.

https://codereview.chromium.org/505043002/diff/1/ui/file_manager/video_player...
File ui/file_manager/video_player/js/cast/caster.js (right):

https://codereview.chromium.org/505043002/diff/1/ui/file_manager/video_player...
ui/file_manager/video_player/js/cast/caster.js:84: function
loadMockCastAPI(callback) {
On 2014/08/26 00:56:58, hirono wrote:
> Is the function used?

It was used. Removed.

[hirono, 2014-08-26 05:59:01]

https://codereview.chromium.org/505043002/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js
(right):

https://codereview.chromium.org/505043002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js:11:
* @enum {string}
Just for consistency: The order is usually @enum -> @const.

https://codereview.chromium.org/505043002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js:26:
* @constructor
nit: @param

https://codereview.chromium.org/505043002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js:67:
if (window['__onGCastApiAvailable']) {
{} is not needed.

https://codereview.chromium.org/505043002/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/test_util.js
(right):

https://codereview.chromium.org/505043002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/test_util.js:46:
* Load the mock cast extension to the content page.
nit: Loads

https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
File ui/file_manager/video_player/js/background.js (right):

https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
ui/file_manager/video_player/js/background.js:90: if (chrome.test) {
nit: {} is not needed.

https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
ui/file_manager/video_player/js/background.js:94:
chrome.runtime.sendMessage({ready: true}, function() {});
nit: Can we remove the callback?

https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
File ui/file_manager/video_player/js/cast/caster.js (right):

https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
ui/file_manager/video_player/js/cast/caster.js:74: script.src =
'_modules/mafeflapfdfljijmlienjedomfjfmhpd/cast_sender.js';
How about overriding the path of chrome cat API for testing instead of checking
window.loadMockCastExtensionForTest?

https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
ui/file_manager/video_player/js/cast/caster.js:82: * @param {function} callback
Callback (executed asynchronously).
nit: Please add type annotation to the function.

[yoshiki, 2014-08-26 06:11:02]

@hirono, PTAL

https://codereview.chromium.org/505043002/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js
(right):

https://codereview.chromium.org/505043002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js:11:
* @enum {string}
On 2014/08/26 05:59:01, hirono wrote:
> Just for consistency: The order is usually @enum -> @const.

Done.

https://codereview.chromium.org/505043002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js:26:
* @constructor
On 2014/08/26 05:59:01, hirono wrote:
> nit: @param

Done.

https://codereview.chromium.org/505043002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/cast_extension_mock/load.js:67:
if (window['__onGCastApiAvailable']) {
On 2014/08/26 05:59:01, hirono wrote:
> {} is not needed.

Done.

https://codereview.chromium.org/505043002/diff/60001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/test_util.js
(right):

https://codereview.chromium.org/505043002/diff/60001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_manager_browsertest/video_player/test_util.js:46:
* Load the mock cast extension to the content page.
On 2014/08/26 05:59:01, hirono wrote:
> nit: Loads

Done.

https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
File ui/file_manager/video_player/js/background.js (right):

https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
ui/file_manager/video_player/js/background.js:90: if (chrome.test) {
On 2014/08/26 05:59:01, hirono wrote:
> nit: {} is not needed.

Done.

https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
ui/file_manager/video_player/js/background.js:94:
chrome.runtime.sendMessage({ready: true}, function() {});
On 2014/08/26 05:59:01, hirono wrote:
> nit: Can we remove the callback?

Done.

https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
File ui/file_manager/video_player/js/cast/caster.js (right):

https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
ui/file_manager/video_player/js/cast/caster.js:74: script.src =
'_modules/mafeflapfdfljijmlienjedomfjfmhpd/cast_sender.js';
On 2014/08/26 05:59:01, hirono wrote:
> How about overriding the path of chrome cat API for testing instead of
checking
> window.loadMockCastExtensionForTest?

That's not good because here this doesn't the load cast api extension directly
but the cast sdk loader which call the cast api extension indirectly.

https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
ui/file_manager/video_player/js/cast/caster.js:82: * @param {function} callback
Callback (executed asynchronously).
On 2014/08/26 05:59:01, hirono wrote:
> nit: Please add type annotation to the function.

Done.

[hirono, 2014-08-26 06:33:25]

https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
File ui/file_manager/video_player/js/cast/caster.js (right):

https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
ui/file_manager/video_player/js/cast/caster.js:74: script.src =
'_modules/mafeflapfdfljijmlienjedomfjfmhpd/cast_sender.js';
On 2014/08/26 06:11:02, yoshiki wrote:
> On 2014/08/26 05:59:01, hirono wrote:
> > How about overriding the path of chrome cat API for testing instead of
> checking
> > window.loadMockCastExtensionForTest?
> 
> That's not good because here this doesn't the load cast api extension directly
> but the cast sdk loader which call the cast api extension indirectly.

Cannot we load the mock API instead of the SDK loader?
It looks both scripts have the same role: preparing the cast API and calling
window['__onGCastApiAvailable'].

[yoshiki, 2014-08-26 06:38:33]

On 2014/08/26 06:33:25, hirono wrote:
>
https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
> File ui/file_manager/video_player/js/cast/caster.js (right):
> 
>
https://codereview.chromium.org/505043002/diff/60001/ui/file_manager/video_pl...
> ui/file_manager/video_player/js/cast/caster.js:74: script.src =
> '_modules/mafeflapfdfljijmlienjedomfjfmhpd/cast_sender.js';
> On 2014/08/26 06:11:02, yoshiki wrote:
> > On 2014/08/26 05:59:01, hirono wrote:
> > > How about overriding the path of chrome cat API for testing instead of
> > checking
> > > window.loadMockCastExtensionForTest?
> > 
> > That's not good because here this doesn't the load cast api extension
directly
> > but the cast sdk loader which call the cast api extension indirectly.
> 
> Cannot we load the mock API instead of the SDK loader?
> It looks both scripts have the same role: preparing the cast API and calling
> window['__onGCastApiAvailable'].

The SDK loader is external extension and we can't load it in test.

Technically we can load the mock sdk here. But I think we shouldn't do it. In
the real environment, we load the cast sdk loader and the sdk loader loads the
sdk. Hence, here we loads the cast sdk, not the sdk loader. So I think it's not
good idea to load the mock sdk here.

[hirono, 2014-08-26 07:03:11]

> Technically we can load the mock sdk here. But I think we shouldn't do it. In
> the real environment, we load the cast sdk loader and the sdk loader loads the
> sdk. Hence, here we loads the cast sdk, not the sdk loader. So I think it's
not
> good idea to load the mock sdk here.

If we switches the app behavior depending on it's on test or not, we does not
test the switched behavior.
I'd like to use the same path for both production and test code as possible.
Do the differences between direct / indirect loading affect the behavior of the
video player app?
If no, we can abstract the internal behavior of
'_modules/mafeflapfdfljijmlienjedomfjfmhpd/cast_sender.js'.
Otherwise, we may want to consider to mimic the indirect loading of
'_modules/mafeflapfdfljijmlienjedomfjfmhpd/cast_sender.js' in the mock, to
ensure the video player works correctly even when the cast API is loaded
indirectly.

[yoshiki, 2014-08-26 07:23:45]

On 2014/08/26 07:03:11, hirono wrote:
> > Technically we can load the mock sdk here. But I think we shouldn't do it.
In
> > the real environment, we load the cast sdk loader and the sdk loader loads
the
> > sdk. Hence, here we loads the cast sdk, not the sdk loader. So I think it's
> not
> > good idea to load the mock sdk here.
> 
> If we switches the app behavior depending on it's on test or not, we does not
> test the switched behavior.

In general, you are correct. But in this case, I don't think there is any profit
than losing readability.

If we used loadCastAPI in test as you said, we had to skip the code installing
the extension and add the if-condition. We can't completely remove the
conditions here.

> I'd like to use the same path for both production and test code as possible.
> Do the differences between direct / indirect loading affect the behavior of
the
> video player app?

The behavior should be no difference. But the cast sdk loader and the cast sdk
are not same thing and I don't want to load it in same code for readability
because it is hacky.

> If no, we can abstract the internal behavior of
> '_modules/mafeflapfdfljijmlienjedomfjfmhpd/cast_sender.js'.
> Otherwise, we may want to consider to mimic the indirect loading of
> '_modules/mafeflapfdfljijmlienjedomfjfmhpd/cast_sender.js' in the mock, to
> ensure the video player works correctly even when the cast API is loaded
> indirectly.

I think that is ideal but overkill.

[hirono, 2014-08-26 08:39:05]

> In general, you are correct. But in this case, I don't think there is any
profit
> than losing readability.

I think of the two profits: preventing typos from being mixed in the code path,
and saving the number of test utilities.

> If we used loadCastAPI in test as you said, we had to skip the code installing
> the extension and add the if-condition. We can't completely remove the
> conditions here.

The installation code is called from the onerror handler. So we don't need to
care about it for success case.
For error case, we can override the script source with invalid path, and can
test the code by overriding the chrome.fileBrowserPrivate.installWebstoreItem
API.
(Making the installation code testable is another profit of overriding the
script path)

> The behavior should be no difference. But the cast sdk loader and the cast sdk
> are not same thing and I don't want to load it in same code for readability
> because it is hacky.

Let me think more.
The real code loads the sdk loader, and the test code prepare the mock API
directly.
The two implementation is do different things, but replacing internal
implementation itself is what we usually do for testing.

The main concern is a reader who saw the mock implementation may misunderstand
the implementation of real code.
How about adding a comment about the internal behavior of cast_sender.js? It
prevent readers from misunderstanding.

[yoshiki, 2014-08-26 09:35:31]

On 2014/08/26 08:39:05, hirono wrote:
> > In general, you are correct. But in this case, I don't think there is any
> profit
> > than losing readability.
> 
> I think of the two profits: preventing typos from being mixed in the code
path,
> and saving the number of test utilities.
> 
> > If we used loadCastAPI in test as you said, we had to skip the code
installing
> > the extension and add the if-condition. We can't completely remove the
> > conditions here.
> 
> The installation code is called from the onerror handler. So we don't need to
> care about it for success case.
> For error case, we can override the script source with invalid path, and can
> test the code by overriding the chrome.fileBrowserPrivate.installWebstoreItem
> API.
> (Making the installation code testable is another profit of overriding the
> script path)

If we go in your way, as you mentioned, we can detect the typos only in l.74-77.
Your idea has smaller pros than cons.

And of cause, I'll add a test of installation extension separately.

> > The behavior should be no difference. But the cast sdk loader and the cast
sdk
> > are not same thing and I don't want to load it in same code for readability
> > because it is hacky.
> 
> Let me think more.
> The real code loads the sdk loader, and the test code prepare the mock API
> directly.
> The two implementation is do different things, but replacing internal
> implementation itself is what we usually do for testing.

We usually do "replacing" only in case of using the mock instead of the
corresponding real class (or something). But in this case, the mock is not
really mock of the real sdk loader. So replacing the real _loader_ with the mock
sdk is bad idea. The sdk loader will be changed and have much functionality than
just loading the sdk.

And we don't load the sdk loader in test, so I think it's natural idea to skip
the function (loadCastApi) to load the sdk loader.

> The main concern is a reader who saw the mock implementation may misunderstand
> the implementation of real code.

I don't think so, since the filename contains the word 'mock'. And test-only
variables and methods must contain the word 'ForTest'.

> How about adding a comment about the internal behavior of cast_sender.js? It
> prevent readers from misunderstanding.

I added more comment to caster.js and the mock class file.

[hirono, 2014-08-26 11:20:27]

After offline discussion, lgtm.

[yoshiki, 2014-08-29 03:58:51]

On 2014/08/26 11:20:27, hirono wrote:
> After offline discussion, lgtm.

Thanks, going to commit.

[yoshiki, 2014-08-29 03:58:55]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-08-29 03:59:59]

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

[commit-bot: I haz the power, 2014-08-29 05:17:44]

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

[commit-bot: I haz the power, 2014-08-29 06:04:36]

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

[commit-bot: I haz the power, 2014-08-29 06:04:38]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yoshiki, 2014-09-02 14:36:24]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-09-02 14:37:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoshiki@chromium.org/505043002/120001

[commit-bot: I haz the power, 2014-09-02 16:00:04]

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

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

Committed patchset #7 (id:120001) as 79ff72b7bd5dde121487198706020ff9d0173beb

[commit-bot: I haz the power, 2014-09-10 03:19:02]

Patchset 7 (id:??) landed as
https://crrev.com/6939ba35b40b86b97e244ec2e6c445a3872b5334
Cr-Commit-Position: refs/heads/master@{#292944}