Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2896453002:
Remove redundant call and early check when media remoting is disabled. (Closed)
|
Created:
3 years, 7 months ago by xjz Modified:
3 years, 7 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, mlamouri+watch-blink_chromium.org, Srirama Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove redundant call and early check when media remoting is disabled.
Remove the redundant call to hide the interstitial when media remoting
is disabled. And also removed the early return check when media
remoting is stopped.
BUG=724223
Review-Url: https://codereview.chromium.org/2896453002
Cr-Commit-Position: refs/heads/master@{#475123}
Committed: https://chromium.googlesource.com/chromium/src/+/108ef61710f08366884633492aa05a23b699bc44
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comments. #Messages
Total messages: 27 (17 generated)
The CQ bit was checked by xjz@chromium.org to run a CQ dry run
Description was changed from ========== Remove redundant call and early check when media remoting is disabled. Remove the redundant call to hide the interstitial when media remoting is disabled. And also removed the early return check when media remoting is stopped. BUG=724223 ========== to ========== Remove redundant call and early check when media remoting is disabled. Remove the redundant call to hide the interstitial when media remoting is disabled. And also removed the early return check when media remoting is stopped. BUG=724223 ==========
xjz@chromium.org changed reviewers: + avayvod@chromium.org
PTAL. Thanks!
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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm https://codereview.chromium.org/2896453002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (left): https://codereview.chromium.org/2896453002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:515: DCHECK(media_remoting_status_ == MediaRemotingStatus::kStarted); nit: could you leave a DCHECK that the status can only either be kStarted or kDisabled? https://codereview.chromium.org/2896453002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (right): https://codereview.chromium.org/2896453002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:520: media_remoting_status_ = MediaRemotingStatus::kDisabled; Should this be set before requesting to disable remoting to avoid cases where it's not async?
The CQ bit was checked by xjz@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 xjz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/2896453002/#ps20001 (title: "Addressed comments.")
Thanks for the reviewing. Addressed comments. https://codereview.chromium.org/2896453002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (left): https://codereview.chromium.org/2896453002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:515: DCHECK(media_remoting_status_ == MediaRemotingStatus::kStarted); On 2017/05/19 15:58:19, whywhat wrote: > nit: could you leave a DCHECK that the status can only either be kStarted or > kDisabled? Done. https://codereview.chromium.org/2896453002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (right): https://codereview.chromium.org/2896453002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:520: media_remoting_status_ = MediaRemotingStatus::kDisabled; On 2017/05/19 15:58:19, whywhat wrote: > Should this be set before requesting to disable remoting to avoid cases where > it's not async? Done.
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
xjz@chromium.org changed reviewers: + mlamouri@chromium.org
mlamouri@: need owner's RS. PTAL. Thanks!
mlamouri@: friendly ping. Need owner's RS. Thanks!
lgtm
The CQ bit was checked by xjz@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": 20001, "attempt_start_ts": 1495817591933690,
"parent_rev": "1d6585623812b57dca641bb5ecb0fff82f9c9da4", "commit_rev":
"108ef61710f08366884633492aa05a23b699bc44"}
Message was sent while issue was closed.
Description was changed from ========== Remove redundant call and early check when media remoting is disabled. Remove the redundant call to hide the interstitial when media remoting is disabled. And also removed the early return check when media remoting is stopped. BUG=724223 ========== to ========== Remove redundant call and early check when media remoting is disabled. Remove the redundant call to hide the interstitial when media remoting is disabled. And also removed the early return check when media remoting is stopped. BUG=724223 Review-Url: https://codereview.chromium.org/2896453002 Cr-Commit-Position: refs/heads/master@{#475123} Committed: https://chromium.googlesource.com/chromium/src/+/108ef61710f08366884633492aa0... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/108ef61710f08366884633492aa0... |
