[Presentation API] Add browser tests for 1-UA mode

[Presentation API] Add browser tests for 1-UA mode

- added MediaRouterOneUAIntegrationBrowserTest
- added MediaRouterOneUANoReceiverIntegrationBrowserTest

MRP side change: cl/153011373

BUG=704989
Review-Url: https://codereview.chromium.org/2820433003
Cr-Commit-Position: refs/heads/master@{#466693}
Committed: https://chromium.googlesource.com/chromium/src/+/1fccc0ce945b93910226f408b66db128e46acc9c

[zhaobin, 2017-04-13 03:25:55]

zhaobin@chromium.org changed reviewers:
+ imcheng@chromium.org, mfoltz@chromium.org

[imcheng, 2017-04-17 19:06:44]

https://codereview.chromium.org/2820433003/diff/1/chrome/test/media_router/me...
File chrome/test/media_router/media_router_integration_browsertest.cc (right):

https://codereview.chromium.org/2820433003/diff/1/chrome/test/media_router/me...
chrome/test/media_router/media_router_integration_browsertest.cc:510:
Wait(base::TimeDelta::FromSeconds(3));
Looking at the JS code again, I am not sure what we are waiting for here.
kCloseSessionScript already ensures the connection is in closed state. Maybe
just remove the Wait()?

https://codereview.chromium.org/2820433003/diff/1/chrome/test/media_router/me...
File chrome/test/media_router/media_router_one_ua_integration_browsertest.cc
(right):

https://codereview.chromium.org/2820433003/diff/1/chrome/test/media_router/me...
chrome/test/media_router/media_router_one_ua_integration_browsertest.cc:27:
FILE_PATH_LITERAL("media_router/browser_test_resources/"));
Please add a comment on why we need this extra setup. Is it because of
presentation_receiver.html?

https://codereview.chromium.org/2820433003/diff/1/chrome/test/media_router/re...
File chrome/test/media_router/resources/common.js (right):

https://codereview.chromium.org/2820433003/diff/1/chrome/test/media_router/re...
chrome/test/media_router/resources/common.js:223: if (message == 'close') {
This is an indirect way to tell presentation_receiver.html to close its
connection, and this message isn't understood by the test mrp, IIRC. I would at
least extract this into its own function (initiateCloseFromReceiverPage).
Another way would be to locate the WebContents hosting the receiver page in the
browser test and execute a JS script to close connection.

[zhaobin, 2017-04-17 21:24:07]

https://codereview.chromium.org/2820433003/diff/1/chrome/test/media_router/me...
File chrome/test/media_router/media_router_integration_browsertest.cc (right):

https://codereview.chromium.org/2820433003/diff/1/chrome/test/media_router/me...
chrome/test/media_router/media_router_integration_browsertest.cc:510:
Wait(base::TimeDelta::FromSeconds(3));
On 2017/04/17 19:06:44, imcheng wrote:
> Looking at the JS code again, I am not sure what we are waiting for here.
> kCloseSessionScript already ensures the connection is in closed state. Maybe
> just remove the Wait()?

Done.

https://codereview.chromium.org/2820433003/diff/1/chrome/test/media_router/me...
File chrome/test/media_router/media_router_one_ua_integration_browsertest.cc
(right):

https://codereview.chromium.org/2820433003/diff/1/chrome/test/media_router/me...
chrome/test/media_router/media_router_one_ua_integration_browsertest.cc:27:
FILE_PATH_LITERAL("media_router/browser_test_resources/"));
On 2017/04/17 19:06:44, imcheng wrote:
> Please add a comment on why we need this extra setup. Is it because of
> presentation_receiver.html? 

Yes.

https://codereview.chromium.org/2820433003/diff/1/chrome/test/media_router/re...
File chrome/test/media_router/resources/common.js (right):

https://codereview.chromium.org/2820433003/diff/1/chrome/test/media_router/re...
chrome/test/media_router/resources/common.js:223: if (message == 'close') {
On 2017/04/17 19:06:44, imcheng wrote:
> This is an indirect way to tell presentation_receiver.html to close its
> connection, and this message isn't understood by the test mrp, IIRC. I would
at
> least extract this into its own function (initiateCloseFromReceiverPage).
> Another way would be to locate the WebContents hosting the receiver page in
the
> browser test and execute a JS script to close connection.

Done.

[mark a. foltz, 2017-04-18 01:03:44]

LGTM with some minor comments.  Thanks for doing this!

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
File chrome/test/media_router/media_router_integration_browsertest.h (right):

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/media_router_integration_browsertest.h:156: // Runs a
test in which we start a session and send a message.
s/session/presentation/g

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
File chrome/test/media_router/media_router_one_ua_integration_browsertest.cc
(right):

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/media_router_one_ua_integration_browsertest.cc:29: //
Set up embeded test server to serve offscreen presentation with relative
nit: embedded

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
File chrome/test/media_router/resources/common.js (right):

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/common.js:16: if
(pageUrl.indexOf('__is_android__=true') >= 0) {
Can you use URL.searchParams to parse the URL instead?

https://developer.mozilla.org/en-US/docs/Web/API/URL/searchParams

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/common.js:245: startedConnection.onclose =
function(event) {
Slight preference for arrow function syntax: (event) => {

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/common.js:246: var reason = event.reason;
s/var/const/

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
File chrome/test/media_router/resources/presentation_receiver.html (right):

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/presentation_receiver.html:2: <html
xmlns="http://www.w3.org/1999/xhtml">
Spot checking the LayoutTests, I don't think we use XHTML normally in test
pages.  Just <html> ?

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/presentation_receiver.html:4: <title>MR
Integration Basic Test</title>
Maybe:
Media Router Integration Test - Receiver Page

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/presentation_receiver.html:5: <script
type="text/javascript">
Just <script>

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/presentation_receiver.html:10:
connection.close();
Use |this| or |connection| consistently in the event handler.

[zhaobin, 2017-04-18 03:17:11]

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
File chrome/test/media_router/media_router_integration_browsertest.h (right):

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/media_router_integration_browsertest.h:156: // Runs a
test in which we start a session and send a message.
On 2017/04/18 01:03:43, mark a. foltz wrote:
> s/session/presentation/g

Done.

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
File chrome/test/media_router/media_router_one_ua_integration_browsertest.cc
(right):

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/media_router_one_ua_integration_browsertest.cc:29: //
Set up embeded test server to serve offscreen presentation with relative
On 2017/04/18 01:03:43, mark a. foltz wrote:
> nit: embedded

Done.

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
File chrome/test/media_router/resources/common.js (right):

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/common.js:16: if
(pageUrl.indexOf('__is_android__=true') >= 0) {
On 2017/04/18 01:03:44, mark a. foltz wrote:
> Can you use URL.searchParams to parse the URL instead?
> 
> https://developer.mozilla.org/en-US/docs/Web/API/URL/searchParams

Done.

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/common.js:245: startedConnection.onclose =
function(event) {
On 2017/04/18 01:03:44, mark a. foltz wrote:
> Slight preference for arrow function syntax: (event) => {

Done.

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/common.js:246: var reason = event.reason;
On 2017/04/18 01:03:44, mark a. foltz wrote:
> s/var/const/

Done.

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
File chrome/test/media_router/resources/presentation_receiver.html (right):

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/presentation_receiver.html:2: <html
xmlns="http://www.w3.org/1999/xhtml">
On 2017/04/18 01:03:44, mark a. foltz wrote:
> Spot checking the LayoutTests, I don't think we use XHTML normally in test
> pages.  Just <html> ?

Done.

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/presentation_receiver.html:4: <title>MR
Integration Basic Test</title>
On 2017/04/18 01:03:44, mark a. foltz wrote:
> Maybe:
> Media Router Integration Test - Receiver Page

Done.

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/presentation_receiver.html:5: <script
type="text/javascript">
On 2017/04/18 01:03:44, mark a. foltz wrote:
> Just <script>

Done.

https://codereview.chromium.org/2820433003/diff/20001/chrome/test/media_route...
chrome/test/media_router/resources/presentation_receiver.html:10:
connection.close();
On 2017/04/18 01:03:44, mark a. foltz wrote:
> Use |this| or |connection| consistently in the event handler.

Done.

[imcheng, 2017-04-21 21:07:33]

lgtm

[zhaobin, 2017-04-22 00:49:12]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-22 00:49:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-22 02:27:34]

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

[commit-bot: I haz the power, 2017-04-22 02:27:34]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[zhaobin, 2017-04-22 05:40:51]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-22 05:41:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-22 06:47:46]

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

[commit-bot: I haz the power, 2017-04-22 06:47:47]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-04-24 17:24:05]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-04-24 17:24:05]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2820433003/#ps70001
(title: "fix android test failures")

[commit-bot: I haz the power, 2017-04-24 17:24:38]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 18:25:16]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1493054645478710,
"parent_rev": "29a6f94954d7c4e6cc7031f0bfc689cc4ee63bd4", "commit_rev":
"1fccc0ce945b93910226f408b66db128e46acc9c"}

[commit-bot: I haz the power, 2017-04-24 18:25:38]

Description was changed from

==========
[Presentation API] Add browser tests for 1-UA mode

- added MediaRouterOneUAIntegrationBrowserTest
- added MediaRouterOneUANoReceiverIntegrationBrowserTest

MRP side change: cl/153011373

BUG=704989
==========

to

==========
[Presentation API] Add browser tests for 1-UA mode

- added MediaRouterOneUAIntegrationBrowserTest
- added MediaRouterOneUANoReceiverIntegrationBrowserTest

MRP side change: cl/153011373

BUG=704989

Review-Url: https://codereview.chromium.org/2820433003
Cr-Commit-Position: refs/heads/master@{#466693}
Committed:
https://chromium.googlesource.com/chromium/src/+/1fccc0ce945b93910226f408b66d...
==========

[commit-bot: I haz the power, 2017-04-24 18:25:39]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/1fccc0ce945b93910226f408b66d...