Add browser test to the sharing dialog feature in Files.app.

Add browser test to the sharing dialog feature in Files.app.

This patch adds an end-to-end test for the sharing dialog. It uses a mocked sharing dialog implementation. This browser test validates:
- If passing the sharing url from the drive service works correctly.
- If clicking on the Share button invokes the sharing dialog.
- If the communication over messages with the share dialog works correctly: Handshake, displaying contents, synchronizing dimensions.
- If the window is closed when the button in the sharing dialog is pressed.

All of these steps are tested for both - a file and a directory.

TEST=browser_tests:*ShareDialog/FileManagerBrowserTest.Test/*
BUG=268221
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215810

[mtomasz, 2013-08-05 11:53:05]

@yoshiki: PTAL at JS.
@hidehiko: PTAL at C++.

Thanks!

[hidehiko, 2013-08-05 15:07:44]

https://codereview.chromium.org/22185002/diff/3001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
(right):

https://codereview.chromium.org/22185002/diff/3001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:38:
#include "net/test/embedded_test_server/http_request.h"
Do you need this and below include directive?

https://codereview.chromium.org/22185002/diff/3001/chrome/browser/drive/fake_...
File chrome/browser/drive/fake_drive_service.cc (right):

https://codereview.chromium.org/22185002/diff/3001/chrome/browser/drive/fake_...
chrome/browser/drive/fake_drive_service.cc:942: const GURL share_url =
share_url_base_.Resolve("?" + directory_title);
Seems not used?

https://codereview.chromium.org/22185002/diff/3001/chrome/browser/drive/fake_...
chrome/browser/drive/fake_drive_service.cc:1398: const GURL share_url =
share_url_base_.Resolve("?" + title);
nit: Usually, we have key=value style query param.
How about using net::AppendOrReplaceQueryParameter?

[mtomasz, 2013-08-05 15:18:31]

Thanks for the comments.

https://codereview.chromium.org/22185002/diff/3001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc
(right):

https://codereview.chromium.org/22185002/diff/3001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_manager/file_manager_browsertest.cc:38:
#include "net/test/embedded_test_server/http_request.h"
On 2013/08/05 15:07:44, hidehiko wrote:
> Do you need this and below include directive?

Done.

https://codereview.chromium.org/22185002/diff/3001/chrome/browser/drive/fake_...
File chrome/browser/drive/fake_drive_service.cc (right):

https://codereview.chromium.org/22185002/diff/3001/chrome/browser/drive/fake_...
chrome/browser/drive/fake_drive_service.cc:942: const GURL share_url =
share_url_base_.Resolve("?" + directory_title);
On 2013/08/05 15:07:44, hidehiko wrote:
> Seems not used?

Done.

https://codereview.chromium.org/22185002/diff/3001/chrome/browser/drive/fake_...
chrome/browser/drive/fake_drive_service.cc:1398: const GURL share_url =
share_url_base_.Resolve("?" + title);
On 2013/08/05 15:07:44, hidehiko wrote:
> nit: Usually, we have key=value style query param.
> How about using net::AppendOrReplaceQueryParameter?

Sounds good to me.

[hidehiko, 2013-08-05 15:29:42]

LGTM for c++ side. Please wait for yoshik@'s review, too.

[yoshiki, 2013-08-05 16:19:37]

Nice patch. I'll refer this in writing CWS-integration tests.

https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/share_client.js (right):

https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/share_client.js:80: if (e.origin !=
ShareClient.SHARE_TARGET && !window.IN_TEST)
How about creating utility functions to return the target origin and to check
the source origin? I think spreading "window.IN_TEST" makes the code hard to
read, and may let us forget to add IN_TEST when adding code in feature.

[mtomasz, 2013-08-05 16:33:36]

https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/share_client.js (right):

https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/share_client.js:80: if (e.origin !=
ShareClient.SHARE_TARGET && !window.IN_TEST)
On 2013/08/05 16:19:38, yoshiki wrote:
> How about creating utility functions to return the target origin and to check
> the source origin? I think spreading "window.IN_TEST" makes the code hard to
> read, and may let us forget to add IN_TEST when adding code in feature.

The origin can change for each test, since the test server may start on
different ports, so we can't easily put the testing origin in a constant.
Pushing it from C++ to Javascript would be tricky. I was thinking about it, but
for simplicity I decided just to allow any origin when in tests.

Window.IN_TEST is always true when running Files.app within a test. We don't
have to worry about adding it. It is always set to true once the testing
extension connects to Files.app. Without it, we would have to manually let the
share client know to accept different share target than normally. I think it
will just add lots of unnecessary complexity.

We could add a utility function like. util.IsInTest() to make the code cleaner.
WDYT?

[mtomasz, 2013-08-05 16:57:26]

On 2013/08/05 16:33:36, mtomasz wrote:
>
https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
> File chrome/browser/resources/file_manager/js/share_client.js (right):
> 
>
https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
> chrome/browser/resources/file_manager/js/share_client.js:80: if (e.origin !=
> ShareClient.SHARE_TARGET && !window.IN_TEST)
> On 2013/08/05 16:19:38, yoshiki wrote:
> > How about creating utility functions to return the target origin and to
check
> > the source origin? I think spreading "window.IN_TEST" makes the code hard to
> > read, and may let us forget to add IN_TEST when adding code in feature.
> 
> The origin can change for each test, since the test server may start on
> different ports, so we can't easily put the testing origin in a constant.
> Pushing it from C++ to Javascript would be tricky. I was thinking about it,
but
> for simplicity I decided just to allow any origin when in tests.
> 
> Window.IN_TEST is always true when running Files.app within a test. We don't
> have to worry about adding it. It is always set to true once the testing
> extension connects to Files.app. Without it, we would have to manually let the
> share client know to accept different share target than normally. I think it
> will just add lots of unnecessary complexity.
> 
> We could add a utility function like. util.IsInTest() to make the code
cleaner.
> WDYT?

Hm, I think I've misunderstood your comments. Let me try to address it.

[mtomasz, 2013-08-05 17:01:04]

On 2013/08/05 16:57:26, mtomasz wrote:
> On 2013/08/05 16:33:36, mtomasz wrote:
> >
>
https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
> > File chrome/browser/resources/file_manager/js/share_client.js (right):
> > 
> >
>
https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
> > chrome/browser/resources/file_manager/js/share_client.js:80: if (e.origin !=
> > ShareClient.SHARE_TARGET && !window.IN_TEST)
> > On 2013/08/05 16:19:38, yoshiki wrote:
> > > How about creating utility functions to return the target origin and to
> check
> > > the source origin? I think spreading "window.IN_TEST" makes the code hard
to
> > > read, and may let us forget to add IN_TEST when adding code in feature.
> > 
> > The origin can change for each test, since the test server may start on
> > different ports, so we can't easily put the testing origin in a constant.
> > Pushing it from C++ to Javascript would be tricky. I was thinking about it,
> but
> > for simplicity I decided just to allow any origin when in tests.
> > 
> > Window.IN_TEST is always true when running Files.app within a test. We don't
> > have to worry about adding it. It is always set to true once the testing
> > extension connects to Files.app. Without it, we would have to manually let
the
> > share client know to accept different share target than normally. I think it
> > will just add lots of unnecessary complexity.
> > 
> > We could add a utility function like. util.IsInTest() to make the code
> cleaner.
> > WDYT?
> 
> Hm, I think I've misunderstood your comments. Let me try to address it.

But on the other hand. I am not sure if we can extract checking the origin and
returning the origin to a utility function. The origin is specific to the share
client. I don't want to move the constant outside of the share_client.js file.
Also, note that in share_dialog.js the origin for testing is not '*' but
'<all_urls>', so not the same. Hard to share the code imho. WDYT?

[yoshiki, 2013-08-06 01:16:45]

On 2013/08/05 17:01:04, mtomasz wrote:
> On 2013/08/05 16:57:26, mtomasz wrote:
> > On 2013/08/05 16:33:36, mtomasz wrote:
> > >
> >
>
https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
> > > File chrome/browser/resources/file_manager/js/share_client.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
> > > chrome/browser/resources/file_manager/js/share_client.js:80: if (e.origin
!=
> > > ShareClient.SHARE_TARGET && !window.IN_TEST)
> > > On 2013/08/05 16:19:38, yoshiki wrote:
> > > > How about creating utility functions to return the target origin and to
> > check
> > > > the source origin? I think spreading "window.IN_TEST" makes the code
hard
> to
> > > > read, and may let us forget to add IN_TEST when adding code in feature.
> > > 
> > > The origin can change for each test, since the test server may start on
> > > different ports, so we can't easily put the testing origin in a constant.
> > > Pushing it from C++ to Javascript would be tricky. I was thinking about
it,
> > but
> > > for simplicity I decided just to allow any origin when in tests.
> > > 
> > > Window.IN_TEST is always true when running Files.app within a test. We
don't
> > > have to worry about adding it. It is always set to true once the testing
> > > extension connects to Files.app. Without it, we would have to manually let
> the
> > > share client know to accept different share target than normally. I think
it
> > > will just add lots of unnecessary complexity.
> > > 
> > > We could add a utility function like. util.IsInTest() to make the code
> > cleaner.
> > > WDYT?
> > 
> > Hm, I think I've misunderstood your comments. Let me try to address it.
> 
> But on the other hand. I am not sure if we can extract checking the origin and
> returning the origin to a utility function. The origin is specific to the
share
> client. I don't want to move the constant outside of the share_client.js file.
> Also, note that in share_dialog.js the origin for testing is not '*' but
> '<all_urls>', so not the same. Hard to share the code imho. WDYT?

Ok, I got it. Let's keep as it is.

[yoshiki, 2013-08-06 01:17:01]

https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/share_dialog.js (right):

https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/share_dialog.js:223: !window.IN_TEST ?
ShareClient.SHARE_TARGET : '<all_urls>',
ShareClient.SHARE_TARGET is an origin, but "<all_urls>" is not an origin but a
pattern.

How about passing a pattern instead of an origin even in non-test to be
consistent?

eg.
this.webViewAuthorizer_ = new ShareDialog.WebViewAuthorizer(
    !window.IN_TEST ? ShareClient.SHARE_TARGET + '/*' : '<all_urls>',
    ...
);

https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/test_util.js (right):

https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/test_util.js:237: return opt_inverse ?
true : false;
nit: return !!opt_inverse;

https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/test_util.js:245: return opt_inverse ?
false : true;
nit: !opt_inverse;

https://codereview.chromium.org/22185002/diff/37001/chrome/test/data/chromeos...
File chrome/test/data/chromeos/file_manager/share_dialog_mock/main.js (right):

https://codereview.chromium.org/22185002/diff/37001/chrome/test/data/chromeos...
chrome/test/data/chromeos/file_manager/share_dialog_mock/main.js:2: var
share_dialog = {};
Name of namespace should be camelCase.

https://codereview.chromium.org/22185002/diff/37001/chrome/test/data/chromeos...
chrome/test/data/chromeos/file_manager/share_dialog_mock/main.js:57: data.args =
args;
opt_args?

https://codereview.chromium.org/22185002/diff/37001/chrome/test/data/chromeos...
chrome/test/data/chromeos/file_manager/share_dialog_mock/main.js:60:
share_dialog.EMBEDDER_ORIGIN);
nit: indentation

https://codereview.chromium.org/22185002/diff/37001/chrome/test/data/chromeos...
chrome/test/data/chromeos/file_manager/share_dialog_mock/main.js:121:
share_dialog.pendingMessages[i].args);
nit: indentation is off.

[mtomasz, 2013-08-06 02:02:35]

https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/share_dialog.js (right):

https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/share_dialog.js:223: !window.IN_TEST ?
ShareClient.SHARE_TARGET : '<all_urls>',
On 2013/08/06 01:17:01, yoshiki wrote:
> ShareClient.SHARE_TARGET is an origin, but "<all_urls>" is not an origin but a
> pattern.
> 
> How about passing a pattern instead of an origin even in non-test to be
> consistent?
> 
> eg.
> this.webViewAuthorizer_ = new ShareDialog.WebViewAuthorizer(
>     !window.IN_TEST ? ShareClient.SHARE_TARGET + '/*' : '<all_urls>',
>     ...
> );

Good idea! Done.

https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/test_util.js (right):

https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/test_util.js:237: return opt_inverse ?
true : false;
On 2013/08/06 01:17:01, yoshiki wrote:
> nit: return !!opt_inverse;

Done.

https://codereview.chromium.org/22185002/diff/37001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/test_util.js:245: return opt_inverse ?
false : true;
On 2013/08/06 01:17:01, yoshiki wrote:
> nit: !opt_inverse;

Done.

https://codereview.chromium.org/22185002/diff/37001/chrome/test/data/chromeos...
File chrome/test/data/chromeos/file_manager/share_dialog_mock/main.js (right):

https://codereview.chromium.org/22185002/diff/37001/chrome/test/data/chromeos...
chrome/test/data/chromeos/file_manager/share_dialog_mock/main.js:2: var
share_dialog = {};
On 2013/08/06 01:17:01, yoshiki wrote:
> Name of namespace should be camelCase.

Done.

https://codereview.chromium.org/22185002/diff/37001/chrome/test/data/chromeos...
chrome/test/data/chromeos/file_manager/share_dialog_mock/main.js:57: data.args =
args;
On 2013/08/06 01:17:01, yoshiki wrote:
> opt_args?

Oops! Done.

https://codereview.chromium.org/22185002/diff/37001/chrome/test/data/chromeos...
chrome/test/data/chromeos/file_manager/share_dialog_mock/main.js:60:
share_dialog.EMBEDDER_ORIGIN);
On 2013/08/06 01:17:01, yoshiki wrote:
> nit: indentation

Done.

https://codereview.chromium.org/22185002/diff/37001/chrome/test/data/chromeos...
chrome/test/data/chromeos/file_manager/share_dialog_mock/main.js:121:
share_dialog.pendingMessages[i].args);
On 2013/08/06 01:17:01, yoshiki wrote:
> nit: indentation is off.

Done.

[yoshiki, 2013-08-06 03:16:40]

lgtm

[commit-bot: I haz the power, 2013-08-06 03:31:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/22185002/50001

[commit-bot: I haz the power, 2013-08-06 03:44:28]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-08-06 03:59:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/22185002/66001

[commit-bot: I haz the power, 2013-08-06 05:59:24]

Change committed as 215810