Implement mocks for identity and host-list, add a browser test to test the app in various failure m…

Implement mocks for identity and host-list, add a browser test to test the app in various failure modes.

* Minor refactoring of RemoteDesktopBrowerTest to better support deferred-start tests without needing them to duplicate too much code.
* Added some JavaScript helper functions.
* Interfacified the host-list and OAuth REST APIs, and added mocks.
* Added a mock for the chrome.identity API.
* Added a (partial, due to bugs that I'll fix separately) browser test to check the behaviour of the app in various failure modes.

Note that the first two items could conceivably be a separate CL, since they're likely to be useful independently of this CL. LMK if you'd like me to split it.

Committed: https://crrev.com/499b99a8ba2d7f4f2e78e75f70c583e78b6a01d0
Cr-Commit-Position: refs/heads/master@{#310885}

[Jamie, 2015-01-08 18:57:18]

jamiewalch@chromium.org changed reviewers:
+ kelvinp@chromium.org

[Jamie, 2015-01-08 18:57:18]

jamiewalch@chromium.org changed reviewers:
+ kelvinp@chromium.org

[Jamie, 2015-01-08 18:57:19]

ptal

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/crd/js/o...
File remoting/webapp/crd/js/oauth2_api.js (right):

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/crd/js/o...
remoting/webapp/crd/js/oauth2_api.js:58: *     to match remoting.HostListApi.
I don't want to do this just yet because it will make this already-large CL even
larger.

[Jamie, 2015-01-08 18:57:19]

ptal

[kelvinp, 2015-01-08 22:54:41]

https://codereview.chromium.org/840023004/diff/20001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/840023004/diff/20001/chrome/test/remoting/rem...
chrome/test/remoting/remote_desktop_browsertest.cc:455: content::WebContents*
content = app_web_content();
As per your suggestion in the other CL
https://codereview.chromium.org/838543002/
I am thinking of getting rid of app_web_content() have LaunchChromotingApp
returns a content::WebContent* of the app.
This will make our code cleaner for test that involves two instances of the
Chromoting app.

In light of that, I think LoadBrowserTestJavaScript should takes the
content::WebContents* as a parameter.

https://codereview.chromium.org/840023004/diff/20001/chrome/test/remoting/una...
File chrome/test/remoting/unauthenticated_browsertest.cc (right):

https://codereview.chromium.org/840023004/diff/20001/chrome/test/remoting/una...
chrome/test/remoting/unauthenticated_browsertest.cc:8: #include
"chrome/test/remoting/waiter.h"
I think you can remove all the includes except
 #include "chrome/test/remoting/remote_desktop_browsertest.h"

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_host_list_api.js (right):

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_host_list_api.js:19: remoting.MockHostListApi
= function() {
I would imagine some test would like to supply a custom list of hosts.  Do you
think it would be useful to make hosts a parameter in the constructor or expose
a setter function (mock$setHosts) for the hosts?

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_host_list_api.js:107: remoting.hostListApi =
active ? new remoting.MockHostListApi()
Should we cache the old HostListApiImpl instead of replacing it with a new
instance every time?

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_oauth2_api.js (right):

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_oauth2_api.js:43:
onDone(remoting.MockIdentity.AccessToken.VALID, 60 * 60);
With the mock, all callbacks are now invoked synchronously.  For example

remoting.oauth2Api.refreshAccessToken(function() {
console.log("Statement A");
})
console.log("Statement B");

that would output the following before
"Statement B"
"Statement A"

would now output
"Statement A"
"Statement B"

Would that be a problem?

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
File remoting/webapp/browser_test/timeout_waiter.js (right):

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/timeout_waiter.js:33: * @param {number=} opt_number
s/opt_timeout/opt_number  should also probably call out opt_timeout is in
milliseconds

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/timeout_waiter.js:67: return
element.getBoundingClientRect().width != 0;
Just want to call out that this check will not work for scenarios when an
element is hidden by using (visibility:none) as it will still participate in
layout.  

I don't think this will be a problem for now as we always hide element with
display:none.

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
File remoting/webapp/browser_test/unauthenticated_browser_test.js (right):

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/unauthenticated_browser_test.js:58: function() {
Not sure what this block is for.  Some clarification would be great.

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/unauthenticated_browser_test.js:62: function(value)
{
value is unused, consider removing?

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/crd/js/h...
File remoting/webapp/crd/js/host_list_api.js (left):

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/crd/js/h...
remoting/webapp/crd/js/host_list_api.js:16: remoting.HostListApi = function() {
Not part of the CL.  Essentially the hostListApi is responsible for fetching the
data from the directory from the Service.  I think the name HostListApi is over
generic, consider renaming to HostListProvider?

[Jamie, 2015-01-08 23:43:09]

ptal

https://codereview.chromium.org/840023004/diff/20001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/840023004/diff/20001/chrome/test/remoting/rem...
chrome/test/remoting/remote_desktop_browsertest.cc:455: content::WebContents*
content = app_web_content();
On 2015/01/08 22:54:40, kelvinp wrote:
> As per your suggestion in the other CL
> https://codereview.chromium.org/838543002/
> I am thinking of getting rid of app_web_content() have LaunchChromotingApp
> returns a content::WebContent* of the app.
> This will make our code cleaner for test that involves two instances of the
> Chromoting app.
> 
> In light of that, I think LoadBrowserTestJavaScript should takes the
> content::WebContents* as a parameter.

Done.

https://codereview.chromium.org/840023004/diff/20001/chrome/test/remoting/una...
File chrome/test/remoting/unauthenticated_browsertest.cc (right):

https://codereview.chromium.org/840023004/diff/20001/chrome/test/remoting/una...
chrome/test/remoting/unauthenticated_browsertest.cc:8: #include
"chrome/test/remoting/waiter.h"
On 2015/01/08 22:54:40, kelvinp wrote:
> I think you can remove all the includes except
>  #include "chrome/test/remoting/remote_desktop_browsertest.h"

Done.

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_host_list_api.js (right):

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_host_list_api.js:19: remoting.MockHostListApi
= function() {
On 2015/01/08 22:54:40, kelvinp wrote:
> I would imagine some test would like to supply a custom list of hosts.  Do you
> think it would be useful to make hosts a parameter in the constructor or
expose
> a setter function (mock$setHosts) for the hosts?

I would prefer to wait until we have such a test and add a setter at that point.

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_host_list_api.js:107: remoting.hostListApi =
active ? new remoting.MockHostListApi()
On 2015/01/08 22:54:40, kelvinp wrote:
> Should we cache the old HostListApiImpl instead of replacing it with a new
> instance every time?

We could, but I think a similar argument could be made for MockHostListApi and
I'd prefer not to add complexity until we need it. Note that this is different
to MockIdentity, where we have no choice but to save the old API because it's
built-in.

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
File remoting/webapp/browser_test/mock_oauth2_api.js (right):

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/mock_oauth2_api.js:43:
onDone(remoting.MockIdentity.AccessToken.VALID, 60 * 60);
On 2015/01/08 22:54:40, kelvinp wrote:
> With the mock, all callbacks are now invoked synchronously.  For example
> 
> remoting.oauth2Api.refreshAccessToken(function() {
> console.log("Statement A");
> })
> console.log("Statement B");
> 
> that would output the following before
> "Statement B"
> "Statement A"
> 
> would now output
> "Statement A"
> "Statement B"
> 
> Would that be a problem?

We're unlikely ever to use this in mocks, since it's apps v1 only, but you're
right; it should be asynchronous.

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
File remoting/webapp/browser_test/timeout_waiter.js (right):

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/timeout_waiter.js:33: * @param {number=} opt_number
On 2015/01/08 22:54:41, kelvinp wrote:
> s/opt_timeout/opt_number  should also probably call out opt_timeout is in
> milliseconds

Done.

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/timeout_waiter.js:67: return
element.getBoundingClientRect().width != 0;
On 2015/01/08 22:54:41, kelvinp wrote:
> Just want to call out that this check will not work for scenarios when an
> element is hidden by using (visibility:none) as it will still participate in
> layout.  
> 
> I don't think this will be a problem for now as we always hide element with
> display:none.

Ack.

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
File remoting/webapp/browser_test/unauthenticated_browser_test.js (right):

https://codereview.chromium.org/840023004/diff/20001/remoting/webapp/browser_...
remoting/webapp/browser_test/unauthenticated_browser_test.js:58: function() {
On 2015/01/08 22:54:41, kelvinp wrote:
> Not sure what this block is for.  Some clarification would be great.

I originally had some code here to click the "sign in" button and continue with
other tests. Unfortunately, there's a bug in the sign-in code that assumes apps
v1, so I removed it, but forgot to remove this block.

[kelvinp, 2015-01-09 19:23:36]

lgtm

[Jamie, 2015-01-09 22:09:58]

The CQ bit was checked by jamiewalch@chromium.org

[commit-bot: I haz the power, 2015-01-09 22:10:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/840023004/60001

[commit-bot: I haz the power, 2015-01-09 23:19:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-09 23:20:43]

Patchset 4 (id:??) landed as
https://crrev.com/499b99a8ba2d7f4f2e78e75f70c583e78b6a01d0
Cr-Commit-Position: refs/heads/master@{#310885}