Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2674793005:
Use allSinks to find a sink in kFindSinkScript. (Closed)
|
Created:
3 years, 10 months ago by cliffordcheng1 Modified:
3 years, 10 months ago CC:
chromium-reviews, media-router+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse allSinks to find a sink in kFindSinkScript.
BUG=678315
Review-Url: https://codereview.chromium.org/2674793005
Cr-Commit-Position: refs/heads/master@{#449414}
Committed: https://chromium.googlesource.com/chromium/src/+/67d9e4ced4719f37cc758910bb32fdca538b3704
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add try-finally to kFindSinkScript to avoid exception. #
Total comments: 2
Patch Set 3 : Add null check to kFindSinkScript to avoid exception. #Messages
Total messages: 18 (6 generated)
cliffordcheng@chromium.org changed reviewers: + imcheng@chromium.org, leilei@google.com
leilei@chromium.org changed reviewers: + leilei@chromium.org
lgtm
https://codereview.chromium.org/2674793005/diff/1/chrome/test/media_router/me... File chrome/test/media_router/media_router_integration_browsertest.cc (right): https://codereview.chromium.org/2674793005/diff/1/chrome/test/media_router/me... chrome/test/media_router/media_router_integration_browsertest.cc:86: "var sinks = document.getElementById('media-router-container').allSinks;" I don't think we should be using allSinks, as it is not user-visible and not all sinks in allSinks show up in the sink list. Also it seems the test will just fail later, rather than here, when we try to choose thesink in the test. It looks like the test failure is caused by sink-list not created yet. Can we modify this script to check for non-null before proceeding?
https://codereview.chromium.org/2674793005/diff/1/chrome/test/media_router/me... File chrome/test/media_router/media_router_integration_browsertest.cc (right): https://codereview.chromium.org/2674793005/diff/1/chrome/test/media_router/me... chrome/test/media_router/media_router_integration_browsertest.cc:86: "var sinks = document.getElementById('media-router-container').allSinks;" On 2017/02/06 21:36:03, imcheng wrote: > I don't think we should be using allSinks, as it is not user-visible and not all > sinks in allSinks show up in the sink list. Also it seems the test will just > fail later, rather than here, when we try to choose thesink in the test. > > It looks like the test failure is caused by sink-list not created yet. Can we > modify this script to check for non-null before proceeding? The other option is to check if the sink-list is null or not before proceeding. If it is null, it will just send false. WaitUntilSinkDiscoveredOnUI() will wait 30s before timeout. Does that sound better?
Yes, that sounds good.
On 2017/02/06 23:40:59, imcheng wrote: > Yes, that sounds good. I have updated the JS. PTAL
https://codereview.chromium.org/2674793005/diff/20001/chrome/test/media_route... File chrome/test/media_router/media_router_integration_browsertest.cc (right): https://codereview.chromium.org/2674793005/diff/20001/chrome/test/media_route... chrome/test/media_router/media_router_integration_browsertest.cc:87: " var sinks = document.getElementById('media-router-container')." Why not do a null check? e.g., const sinkList = document .getElementById('media-router-container') .shadowRoot.getElementById('sink-list'); if (!sinkList) domAutomationController.send(false); var sinks = sinkList.getElementsByTagName('span'); ...
On 2017/02/09 18:18:32, imcheng wrote: > https://codereview.chromium.org/2674793005/diff/20001/chrome/test/media_route... > File chrome/test/media_router/media_router_integration_browsertest.cc (right): > > https://codereview.chromium.org/2674793005/diff/20001/chrome/test/media_route... > chrome/test/media_router/media_router_integration_browsertest.cc:87: " var > sinks = document.getElementById('media-router-container')." > Why not do a null check? e.g., > > const sinkList = document > .getElementById('media-router-container') > .shadowRoot.getElementById('sink-list'); > if (!sinkList) > domAutomationController.send(false); > > var sinks = sinkList.getElementsByTagName('span'); > ... Just updated, PTAL.
https://codereview.chromium.org/2674793005/diff/20001/chrome/test/media_route... File chrome/test/media_router/media_router_integration_browsertest.cc (right): https://codereview.chromium.org/2674793005/diff/20001/chrome/test/media_route... chrome/test/media_router/media_router_integration_browsertest.cc:87: " var sinks = document.getElementById('media-router-container')." On 2017/02/09 18:18:31, imcheng wrote: > Why not do a null check? e.g., > > const sinkList = document > .getElementById('media-router-container') > .shadowRoot.getElementById('sink-list'); > if (!sinkList) > domAutomationController.send(false); > > var sinks = sinkList.getElementsByTagName('span'); > ... Done.
lgtm
The CQ bit was checked by cliffordcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from leilei@chromium.org Link to the patchset: https://codereview.chromium.org/2674793005/#ps40001 (title: "Add null check to kFindSinkScript to avoid exception.")
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": 40001, "attempt_start_ts": 1486673135815580,
"parent_rev": "67b4b0c5317c2a8e30e141540697200094b9bff3", "commit_rev":
"67d9e4ced4719f37cc758910bb32fdca538b3704"}
Message was sent while issue was closed.
Description was changed from ========== Use allSinks to find a sink in kFindSinkScript. BUG=678315 ========== to ========== Use allSinks to find a sink in kFindSinkScript. BUG=678315 Review-Url: https://codereview.chromium.org/2674793005 Cr-Commit-Position: refs/heads/master@{#449414} Committed: https://chromium.googlesource.com/chromium/src/+/67d9e4ced4719f37cc758910bb32... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/67d9e4ced4719f37cc758910bb32... |
