Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834603004:
[Media Router] Merge createMediaRouteController and setMediaRouteStatusObserver in bindings (Closed)
Description[Media Router] Merge createMediaRouteController and setMediaRouteStatusObserver
in bindings
We merged the methods createMediaRouteController and setMediaRouteStatusObserver
in the MediaRouteProvider mojo interface in this CL [1]. This patch applies that
change to media_router_bindings.js.
[1] https://codereview.chromium.org/2728543009/
BUG=684636
Review-Url: https://codereview.chromium.org/2834603004
Cr-Commit-Position: refs/heads/master@{#467049}
Committed: https://chromium.googlesource.com/chromium/src/+/bcc92ee77b05de4fec004c3d1b78366ab6f6c350
Patch Set 1 #Patch Set 2 : Set observer asynchronously #
Total comments: 4
Patch Set 3 : Address Derek's comments #
Total comments: 2
Patch Set 4 : Make createMediaRouteController resolve with void #Messages
Total messages: 25 (17 generated)
Description was changed from ========== [Media Router] Merge createMediaRouteController and setMediaRouteStatusObserver in bindings BUG=684636 ========== to ========== [Media Router] Merge createMediaRouteController and setMediaRouteStatusObserver in bindings We merged the methods createMediaRouteController and setMediaRouteStatusObserver in the MediaRouteProvider mojo interface in this CL [1]. This patch applies that change to media_router_bindings.js. [1] https://codereview.chromium.org/2728543009/ BUG=684636 ==========
Description was changed from ========== [Media Router] Merge createMediaRouteController and setMediaRouteStatusObserver in bindings We merged the methods createMediaRouteController and setMediaRouteStatusObserver in the MediaRouteProvider mojo interface in this CL [1]. This patch applies that change to media_router_bindings.js. [1] https://codereview.chromium.org/2728543009/ BUG=684636 ========== to ========== [Media Router] Merge createMediaRouteController and setMediaRouteStatusObserver in bindings We merged the methods createMediaRouteController and setMediaRouteStatusObserver in the MediaRouteProvider mojo interface in this CL [1]. This patch applies that change to media_router_bindings.js. [1] https://codereview.chromium.org/2728543009/ BUG=684636 ==========
takumif@chromium.org changed reviewers: + imcheng@chromium.org
Please take a look, thanks!
https://codereview.chromium.org/2834603004/diff/20001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/media_router_bindings.js (right): https://codereview.chromium.org/2834603004/diff/20001/chrome/renderer/resourc... chrome/renderer/resources/extensions/media_router_bindings.js:933: // TODO(imcheng): Remove this check when M59 is in stable. s/59/60 https://codereview.chromium.org/2834603004/diff/20001/chrome/renderer/resourc... chrome/renderer/resources/extensions/media_router_bindings.js:934: if (!this.handlers_.createMediaRouteController || It probably makes sense to use a combined API in the component as well, so as to minimize the logic that is handled by the bindings code.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2834603004/diff/20001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/media_router_bindings.js (right): https://codereview.chromium.org/2834603004/diff/20001/chrome/renderer/resourc... chrome/renderer/resources/extensions/media_router_bindings.js:933: // TODO(imcheng): Remove this check when M59 is in stable. On 2017/04/21 21:25:15, imcheng wrote: > s/59/60 Done. https://codereview.chromium.org/2834603004/diff/20001/chrome/renderer/resourc... chrome/renderer/resources/extensions/media_router_bindings.js:934: if (!this.handlers_.createMediaRouteController || On 2017/04/21 21:25:15, imcheng wrote: > It probably makes sense to use a combined API in the component as well, so as to > minimize the logic that is handled by the bindings code. Done.
lgtm
https://codereview.chromium.org/2834603004/diff/60001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/media_router_bindings.js (right): https://codereview.chromium.org/2834603004/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/extensions/media_router_bindings.js:936: .then(controller => {success: true}, e => {success: false}); One more thing: let's make this return void for now. I will need to update the API on the extension side anyway.
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2834603004/diff/60001/chrome/renderer/resourc... File chrome/renderer/resources/extensions/media_router_bindings.js (right): https://codereview.chromium.org/2834603004/diff/60001/chrome/renderer/resourc... chrome/renderer/resources/extensions/media_router_bindings.js:936: .then(controller => {success: true}, e => {success: false}); On 2017/04/24 21:04:10, imcheng wrote: > One more thing: let's make this return void for now. I will need to update the > API on the extension side anyway. Done.
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 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
The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2834603004/#ps100001 (title: "Make createMediaRouteController resolve with void")
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": 100001, "attempt_start_ts": 1493143654604720,
"parent_rev": "5883c030885c31d037fb0ee4b5f465ee3e8e1dcb", "commit_rev":
"bcc92ee77b05de4fec004c3d1b78366ab6f6c350"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Merge createMediaRouteController and setMediaRouteStatusObserver in bindings We merged the methods createMediaRouteController and setMediaRouteStatusObserver in the MediaRouteProvider mojo interface in this CL [1]. This patch applies that change to media_router_bindings.js. [1] https://codereview.chromium.org/2728543009/ BUG=684636 ========== to ========== [Media Router] Merge createMediaRouteController and setMediaRouteStatusObserver in bindings We merged the methods createMediaRouteController and setMediaRouteStatusObserver in the MediaRouteProvider mojo interface in this CL [1]. This patch applies that change to media_router_bindings.js. [1] https://codereview.chromium.org/2728543009/ BUG=684636 Review-Url: https://codereview.chromium.org/2834603004 Cr-Commit-Position: refs/heads/master@{#467049} Committed: https://chromium.googlesource.com/chromium/src/+/bcc92ee77b05de4fec004c3d1b78... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/bcc92ee77b05de4fec004c3d1b78... |
