Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2435243002:
[Presentation API] add 'connecting' state to WebPresentationConnectionState (Closed)
DescriptionAdd 'connecting' state to WebPresentationConnectionState
BUG=655840, 575351
Committed: https://crrev.com/b30d8e28048325fff57fca54be22b4f0370e93cd
Cr-Commit-Position: refs/heads/master@{#427791}
Patch Set 1 #
Total comments: 19
Patch Set 2 : resolve code review comments #
Total comments: 2
Patch Set 3 : resolve code review comments from mlamouri #
Total comments: 2
Patch Set 4 : resolve code review comments from mlamouri #
Dependent Patchsets: Messages
Total messages: 24 (11 generated)
zhaobin@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org, mlamouri@chromium.org
https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp (right): https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:414: wrapPersistent(this), state)); Why are you making this asynchronous? https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:461: return; NOTREACHED() ? https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:470: return; NOTREACHED() ?
https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp (right): https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:155: m_state(WebPresentationConnectionState::Connected), Add a TODO to change this to connecting? https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:381: if (m_state != WebPresentationConnectionState::Connected) do we allow close()/terminate() on a PresentationConnection that is in connecting state? It would be worthwhile to double check the current logic to see if they also apply to connecting state. https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/presentation/PresentationConnection.h (right): https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.h:80: // Queue a task to notifies the connection about its state change. Queues a task to notify... https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.h:116: void didChangeStateInternal(WebPresentationConnectionState); nit: blank line before this to separate it from the blob loader callbacks https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h (right): https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h:14: Connecting = 0, Can you also change GetWebPresentationConnectionStateFromMojo in presentation_dispatcher.cc to handle Connecting and add 575351 to BUG=?
https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp (right): https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:381: if (m_state != WebPresentationConnectionState::Connected) On 2016/10/21 at 23:58:15, imcheng wrote: > do we allow close()/terminate() on a PresentationConnection that is in connecting state? It would be worthwhile to double check the current logic to see if they also apply to connecting state. close() is allowed. terminate() is a no-op in a connecting state. On the one hand, this makes sense because you don't know if there will be any presentation to terminate. On the other hand this is a little unfortunate in that it doesn't allow a site to cancel a pending presentation request easily. https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:414: wrapPersistent(this), state)); On 2016/10/21 at 09:58:12, mlamouri wrote: > Why are you making this asynchronous? I believe this is to ensure the connect event is dispatched after the start() Promise is resolved. Bin, can you add comments explaining why this is async?
Description was changed from ========== [Presentation API] (part1) fire PresentationConnection.onconnect event - Add 'connecting' state to WebPresentationConnectionState - Queue a tast for PresentationConnection state change events (TODO) change PresentationConnection initial state to 'connecting' (will break cast MRP) BUG=655840 ========== to ========== [Presentation API] (part1) fire PresentationConnection.onconnect event - Add 'connecting' state to WebPresentationConnectionState - Queue a tast for PresentationConnection state change events (TODO) change PresentationConnection initial state to 'connecting' (will break cast MRP) BUG=655840,575351 ==========
https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp (right): https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:155: m_state(WebPresentationConnectionState::Connected), On 2016/10/21 23:58:15, imcheng wrote: > Add a TODO to change this to connecting? Done. https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:381: if (m_state != WebPresentationConnectionState::Connected) On 2016/10/21 23:58:15, imcheng wrote: > do we allow close()/terminate() on a PresentationConnection that is in > connecting state? It would be worthwhile to double check the current logic to > see if they also apply to connecting state. Done. https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:414: wrapPersistent(this), state)); On 2016/10/21 09:58:12, mlamouri wrote: > Why are you making this asynchronous? reverted. https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:414: wrapPersistent(this), state)); On 2016/10/24 18:28:16, mark a. foltz wrote: > On 2016/10/21 at 09:58:12, mlamouri wrote: > > Why are you making this asynchronous? > > I believe this is to ensure the connect event is dispatched after the start() > Promise is resolved. Bin, can you add comments explaining why this is async? Reverted. We used to make this async to dispatch connect event after start() promise is resolved, because we invoked PresentationConnection::didChangeState() in the same task as the task resolving start() promise (PresentationConnectionCallbacks::onSuccess). Now we notifiy state change with settimeout in MR (CL/136787485). It is a different task from the task resolving start() promise. So aysnc is no longer needed. https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:461: return; On 2016/10/21 09:58:12, mlamouri wrote: > NOTREACHED() ? Done. https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:470: return; On 2016/10/21 09:58:12, mlamouri wrote: > NOTREACHED() ? Done. https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/presentation/PresentationConnection.h (right): https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.h:80: // Queue a task to notifies the connection about its state change. On 2016/10/21 23:58:15, imcheng wrote: > Queues a task to notify... reverted. https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/presentation/PresentationConnection.h:116: void didChangeStateInternal(WebPresentationConnectionState); On 2016/10/21 23:58:15, imcheng wrote: > nit: blank line before this to separate it from the blob loader callbacks reverted. https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h (right): https://codereview.chromium.org/2435243002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h:14: Connecting = 0, On 2016/10/21 23:58:15, imcheng wrote: > Can you also change GetWebPresentationConnectionStateFromMojo in > presentation_dispatcher.cc to handle Connecting and add 575351 to BUG=? Done.
Not sure what this change is about now. The description doesn't seem to match reality: where is the task fired? (btw, typo "tast") Is this still WIP and not ready for review? https://codereview.chromium.org/2435243002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp (right): https://codereview.chromium.org/2435243002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:384: return; style: wrap with { }
Description was changed from ========== [Presentation API] (part1) fire PresentationConnection.onconnect event - Add 'connecting' state to WebPresentationConnectionState - Queue a tast for PresentationConnection state change events (TODO) change PresentationConnection initial state to 'connecting' (will break cast MRP) BUG=655840,575351 ========== to ========== Add 'connecting' state to WebPresentationConnectionState BUG=655840,575351 ==========
Updated issue description. It is ready for review. We are going to change initial state to 'connecting' in another CL. https://codereview.chromium.org/2435243002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp (right): https://codereview.chromium.org/2435243002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:384: return; On 2016/10/25 10:17:05, mlamouri wrote: > style: wrap with { } Done.
lgtm https://codereview.chromium.org/2435243002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp (right): https://codereview.chromium.org/2435243002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:154: // TODO(zhaobin): change initial state to Connecting bug number?
lgtm
The CQ bit was checked by zhaobin@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.
https://codereview.chromium.org/2435243002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp (right): https://codereview.chromium.org/2435243002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:154: // TODO(zhaobin): change initial state to Connecting On 2016/10/25 17:43:37, mlamouri wrote: > bug number? Done.
The CQ bit was checked by zhaobin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2435243002/#ps60001 (title: "resolve code review comments from mlamouri")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add 'connecting' state to WebPresentationConnectionState BUG=655840,575351 ========== to ========== Add 'connecting' state to WebPresentationConnectionState BUG=655840,575351 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add 'connecting' state to WebPresentationConnectionState BUG=655840,575351 ========== to ========== Add 'connecting' state to WebPresentationConnectionState BUG=655840,575351 Committed: https://crrev.com/b30d8e28048325fff57fca54be22b4f0370e93cd Cr-Commit-Position: refs/heads/master@{#427791} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b30d8e28048325fff57fca54be22b4f0370e93cd Cr-Commit-Position: refs/heads/master@{#427791} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
