[Presentation API] Resolve PresentationRequest::reconnect() with existing presentation connection i…

[Presentation API] Resolve PresentationRequest::reconnect() with existing presentation connection if such connection exists

Check if any presentation connection with specific presentationUrl and presentationId exists in current browsing context. If so, resolve reconnect() with existing connection instead of creating a new connection.

BUG=677540
Committed: https://crrev.com/91a2313e82fff41419fbb149a2bf6debeed894f7
Cr-Commit-Position: refs/heads/master@{#441462}

[zhaobin, 2016-12-23 01:14:41]

zhaobin@chromium.org changed reviewers:
+ imcheng@chromium.org, mfoltz@chromium.org, mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-12-23 10:35:58]

Can you open a bug with context explaining why we are doing this and also add a
layout test for the fix? :)

[haraken, 2016-12-23 14:26:09]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-12-23 14:26:10]

https://codereview.chromium.org/2602523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/2602523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:175:
presentationController(getExecutionContext());

Not related to this CL, getExecutionContext() returns null when the context is
detached from the frame.

[mark a. foltz, 2016-12-23 18:39:43]

lgtm with one comment; please write a layout test before submitting.

The Mojo stub that mlamouri@ checked in can help with constructing the initial
connection.

https://chromium.googlesource.com/chromium/src.git/+/703fb72fa7b04e34ea67a82d...

https://codereview.chromium.org/2602523002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp
(right):

https://codereview.chromium.org/2602523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp:69:
void ExistingPresentationConnectionCallbacks::onError(
Looking at the spec, I don't think there's an error case possible in the case
for checking an existing connection; it will always fall through to check for a
connection from another browsing context.  So this can be NOTREACHED().

[zhaobin, 2016-12-29 19:52:37]

Description was changed from

==========
[Presentation API] Resolve PresentationRequest::reconnect() with existing
presentation connection if such connection exists

Check if any presentation connection with specific presentationUrl and
presentationId exists in current browsing context. If so, resolve reconnect()
with existing connection instead of creating a new connection.

BUG=
==========

to

==========
[Presentation API] Resolve PresentationRequest::reconnect() with existing
presentation connection if such connection exists

Check if any presentation connection with specific presentationUrl and
presentationId exists in current browsing context. If so, resolve reconnect()
with existing connection instead of creating a new connection.

BUG=677540
==========

[zhaobin, 2016-12-30 05:05:01]

Added a layout test.

https://codereview.chromium.org/2602523002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp
(right):

https://codereview.chromium.org/2602523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp:69:
void ExistingPresentationConnectionCallbacks::onError(
On 2016/12/23 18:39:43, mfoltz_ooo_until_jan_3 wrote:
> Looking at the spec, I don't think there's an error case possible in the case
> for checking an existing connection; it will always fall through to check for
a
> connection from another browsing context.  So this can be NOTREACHED().

Done.

https://codereview.chromium.org/2602523002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/2602523002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:175:
presentationController(getExecutionContext());
On 2016/12/23 14:26:10, haraken wrote:
> 
> Not related to this CL, getExecutionContext() returns null when the context is
> detached from the frame.

Done.

[mlamouri (slow - plz ping), 2017-01-03 12:02:12]

lgtm with comments applied.

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/presentation/presentation-reconnect.html
(right):

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-reconnect.html:30: var
connection;
 = null;

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp
(right):

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp:61:
return;
style: add { } because of 2-line condition. You can also update the code above.

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.h
(right):

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.h:45:
class ExistingPresentationConnectionCallbacks final
Can you create a new file for this callback maybe?

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationController.cpp
(right):

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationController.cpp:159:
return connection.get();
style: you need { } because the condition is more than one line.

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:153: if
(!getExecutionContext()) {
Why are you adding this check? can the ExecutionContext be null here? If I
understand correctly, the current assumes it's never null because
`presentationClient()` does a `DCHECK()` on the `executionContext`.

[zhaobin, 2017-01-03 20:12:07]

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/presentation/presentation-reconnect.html
(right):

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-reconnect.html:30: var
connection;
On 2017/01/03 12:02:11, mlamouri wrote:
>  = null;

Done.

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp
(right):

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp:61:
return;
On 2017/01/03 12:02:11, mlamouri wrote:
> style: add { } because of 2-line condition. You can also update the code
above.

Done.

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.h
(right):

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.h:45:
class ExistingPresentationConnectionCallbacks final
On 2017/01/03 12:02:11, mlamouri wrote:
> Can you create a new file for this callback maybe?

Done.

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationController.cpp
(right):

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationController.cpp:159:
return connection.get();
On 2017/01/03 12:02:11, mlamouri wrote:
> style: you need { } because the condition is more than one line.

Done.

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
(right):

https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:153: if
(!getExecutionContext()) {
On 2017/01/03 12:02:11, mlamouri wrote:
> Why are you adding this check? can the ExecutionContext be null here? If I
> understand correctly, the current assumes it's never null because
> `presentationClient()` does a `DCHECK()` on the `executionContext`.

toDocument(getExecutionContext())->isSandboxed(...) uses getExecutionContext()
before presentationClient()?

haraken commented in patch set 1:

"Not related to this CL, getExecutionContext() returns null when the context is
detached from the frame."

Thought that he suggests to add a check for getExecutionContext().

[zhaobin, 2017-01-03 21:32:50]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 21:33:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-01-03 21:55:38]

On 2017/01/03 20:12:07, zhaobin wrote:
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/presentation/presentation-reconnect.html
> (right):
> 
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/presentation/presentation-reconnect.html:30:
var
> connection;
> On 2017/01/03 12:02:11, mlamouri wrote:
> >  = null;
> 
> Done.
> 
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp
> (right):
> 
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp:61:
> return;
> On 2017/01/03 12:02:11, mlamouri wrote:
> > style: add { } because of 2-line condition. You can also update the code
> above.
> 
> Done.
> 
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.h
> (right):
> 
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.h:45:
> class ExistingPresentationConnectionCallbacks final
> On 2017/01/03 12:02:11, mlamouri wrote:
> > Can you create a new file for this callback maybe?
> 
> Done.
> 
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/presentation/PresentationController.cpp
> (right):
> 
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/presentation/PresentationController.cpp:159:
> return connection.get();
> On 2017/01/03 12:02:11, mlamouri wrote:
> > style: you need { } because the condition is more than one line.
> 
> Done.
> 
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
> (right):
> 
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:153: if
> (!getExecutionContext()) {
> On 2017/01/03 12:02:11, mlamouri wrote:
> > Why are you adding this check? can the ExecutionContext be null here? If I
> > understand correctly, the current assumes it's never null because
> > `presentationClient()` does a `DCHECK()` on the `executionContext`.
> 
> toDocument(getExecutionContext())->isSandboxed(...) uses getExecutionContext()
> before presentationClient()?
> 
> haraken commented in patch set 1:
> 
> "Not related to this CL, getExecutionContext() returns null when the context
is
> detached from the frame."
> 
> Thought that he suggests to add a check for getExecutionContext().

Ah, I didn't notice that it is checked in presentationClient. Ignore my
comment:)

[commit-bot: I haz the power, 2017-01-03 23:47:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-03 23:47:33]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-01-04 19:03:17]

On 2017/01/03 21:55:38, haraken wrote:
> On 2017/01/03 20:12:07, zhaobin wrote:
> >
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Layo...
> > File third_party/WebKit/LayoutTests/presentation/presentation-reconnect.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Layo...
> > third_party/WebKit/LayoutTests/presentation/presentation-reconnect.html:30:
> var
> > connection;
> > On 2017/01/03 12:02:11, mlamouri wrote:
> > >  = null;
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
> > File
> >
>
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
> >
>
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp:61:
> > return;
> > On 2017/01/03 12:02:11, mlamouri wrote:
> > > style: add { } because of 2-line condition. You can also update the code
> > above.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
> > File
> >
>
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
> >
>
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.h:45:
> > class ExistingPresentationConnectionCallbacks final
> > On 2017/01/03 12:02:11, mlamouri wrote:
> > > Can you create a new file for this callback maybe?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
> > File
third_party/WebKit/Source/modules/presentation/PresentationController.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
> >
third_party/WebKit/Source/modules/presentation/PresentationController.cpp:159:
> > return connection.get();
> > On 2017/01/03 12:02:11, mlamouri wrote:
> > > style: you need { } because the condition is more than one line.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2602523002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/modules/presentation/PresentationRequest.cpp:153:
if
> > (!getExecutionContext()) {
> > On 2017/01/03 12:02:11, mlamouri wrote:
> > > Why are you adding this check? can the ExecutionContext be null here? If I
> > > understand correctly, the current assumes it's never null because
> > > `presentationClient()` does a `DCHECK()` on the `executionContext`.
> > 
> > toDocument(getExecutionContext())->isSandboxed(...) uses
getExecutionContext()
> > before presentationClient()?
> > 
> > haraken commented in patch set 1:
> > 
> > "Not related to this CL, getExecutionContext() returns null when the context
> is
> > detached from the frame."
> > 
> > Thought that he suggests to add a check for getExecutionContext().
> 
> Ah, I didn't notice that it is checked in presentationClient. Ignore my
> comment:)

Removed check for getExecutionContext() in PresentationRequest::reconnect().

[zhaobin, 2017-01-04 19:03:57]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-01-04 19:03:57]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2602523002/#ps80001
(title: "remove getExecutionContext() null check in
PresentationRequest::reconnect()")

[commit-bot: I haz the power, 2017-01-04 19:04:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 21:06:14]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483556637394610,
"parent_rev": "0051c9d4f4bf584e7e84aea1225ad8ec19ded1a1", "commit_rev":
"27c235453a41738d03b8f9344f980314190f488d"}

[commit-bot: I haz the power, 2017-01-04 21:06:41]

Description was changed from

==========
[Presentation API] Resolve PresentationRequest::reconnect() with existing
presentation connection if such connection exists

Check if any presentation connection with specific presentationUrl and
presentationId exists in current browsing context. If so, resolve reconnect()
with existing connection instead of creating a new connection.

BUG=677540
==========

to

==========
[Presentation API] Resolve PresentationRequest::reconnect() with existing
presentation connection if such connection exists

Check if any presentation connection with specific presentationUrl and
presentationId exists in current browsing context. If so, resolve reconnect()
with existing connection instead of creating a new connection.

BUG=677540

Review-Url: https://codereview.chromium.org/2602523002
==========

[commit-bot: I haz the power, 2017-01-04 21:06:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2017-01-04 21:09:03]

Description was changed from

==========
[Presentation API] Resolve PresentationRequest::reconnect() with existing
presentation connection if such connection exists

Check if any presentation connection with specific presentationUrl and
presentationId exists in current browsing context. If so, resolve reconnect()
with existing connection instead of creating a new connection.

BUG=677540

Review-Url: https://codereview.chromium.org/2602523002
==========

to

==========
[Presentation API] Resolve PresentationRequest::reconnect() with existing
presentation connection if such connection exists

Check if any presentation connection with specific presentationUrl and
presentationId exists in current browsing context. If so, resolve reconnect()
with existing connection instead of creating a new connection.

BUG=677540

Committed: https://crrev.com/91a2313e82fff41419fbb149a2bf6debeed894f7
Cr-Commit-Position: refs/heads/master@{#441462}
==========

[commit-bot: I haz the power, 2017-01-04 21:09:03]

Patchset 5 (id:??) landed as
https://crrev.com/91a2313e82fff41419fbb149a2bf6debeed894f7
Cr-Commit-Position: refs/heads/master@{#441462}