Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2621723003:
[Media Router] Add browser tests for showing the dialog (Closed)
Description[Media Router] Add browser tests for showing the dialog
This CL adds test cases for opening the media router dialog from the page
context menu and the app menu.
BUG=678472
Review-Url: https://codereview.chromium.org/2621723003
Cr-Commit-Position: refs/heads/master@{#443398}
Committed: https://chromium.googlesource.com/chromium/src/+/4809bbc10e25dff14277270bbff6d6f504178a12
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebase #Patch Set 3 : Address Jennifer's comments #
Total comments: 4
Patch Set 4 : Address Jennifer's comments #Messages
Total messages: 19 (11 generated)
Description was changed from ========== [Media Router] Add browser tests for showing the dialog This CL adds test cases for opening the dialog from the page context menu and the app menu. BUG=678472 ========== to ========== [Media Router] Add browser tests for showing the dialog This CL adds test cases for opening the media router dialog from the page context menu and the app menu. BUG=678472 ==========
Description was changed from ========== [Media Router] Add browser tests for showing the dialog This CL adds test cases for opening the media router dialog from the page context menu and the app menu. BUG=678472 ========== to ========== [Media Router] Add browser tests for showing the dialog This CL adds test cases for opening the media router dialog from the page context menu and the app menu. BUG=678472 ==========
takumif@chromium.org changed reviewers: + apacible@chromium.org
Please take a look, thank you!
Suggestion for some more tests related to this: - Open MR dialog on multiple tabs (multiple dialogs). - Selecting "Cast..." on the menu multiple times on the same tab (only one dialog). https://codereview.chromium.org/2621723003/diff/1/chrome/browser/ui/views/med... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2621723003/diff/1/chrome/browser/ui/views/med... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:157: // Start with one tab showing ablut:blank. nit: about. https://codereview.chromium.org/2621723003/diff/1/chrome/browser/ui/views/med... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:177: // Start with one tab showing ablut:blank. nit: about.
Patchset #2 (id:20001) has been deleted
Thanks for reviewing! I've added a test case for opening dialogs in multiple tabs, and changed the test cases for app menu and context menu to choose "Cast..." multiple times. https://codereview.chromium.org/2621723003/diff/1/chrome/browser/ui/views/med... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2621723003/diff/1/chrome/browser/ui/views/med... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:157: // Start with one tab showing ablut:blank. On 2017/01/11 23:23:01, apacible wrote: > nit: about. Oops. Fixed. https://codereview.chromium.org/2621723003/diff/1/chrome/browser/ui/views/med... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:177: // Start with one tab showing ablut:blank. On 2017/01/11 23:23:01, apacible wrote: > nit: about. Done.
https://codereview.chromium.org/2621723003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2621723003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:178: // dialog to close. Suggestion: "...should only be one dialog opened per tab." https://codereview.chromium.org/2621723003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:221: SetAlwaysShowActionPref(true); Is this needed? Why is this different from the other tests added in this CL?
https://codereview.chromium.org/2621723003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc (right): https://codereview.chromium.org/2621723003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:178: // dialog to close. On 2017/01/12 19:06:50, apacible wrote: > Suggestion: "...should only be one dialog opened per tab." Done. https://codereview.chromium.org/2621723003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/media_router/media_router_ui_browsertest.cc:221: SetAlwaysShowActionPref(true); On 2017/01/12 19:06:50, apacible wrote: > Is this needed? Why is this different from the other tests added in this CL? This is to show the toolbar action which I use in this test (but not in the other tests). I've added a comment to mention that.
lgtm
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by takumif@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484262102306250,
"parent_rev": "7766221ff43e992fdb7d36ae8aaaf69cad584c3b", "commit_rev":
"4809bbc10e25dff14277270bbff6d6f504178a12"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Add browser tests for showing the dialog This CL adds test cases for opening the media router dialog from the page context menu and the app menu. BUG=678472 ========== to ========== [Media Router] Add browser tests for showing the dialog This CL adds test cases for opening the media router dialog from the page context menu and the app menu. BUG=678472 Review-Url: https://codereview.chromium.org/2621723003 Cr-Commit-Position: refs/heads/master@{#443398} Committed: https://chromium.googlesource.com/chromium/src/+/4809bbc10e25dff14277270bbff6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4809bbc10e25dff14277270bbff6... |
