[Presentation API] OffscreenPresentationManager should only interact with top level receiver frame

[Presentation API] OffscreenPresentationManager should only interact with top level receiver frame

When receiver page has sub frames, all frames will try to register receiver callbacks to OPM with the same presentation_id, triggering DCHECK failure. Also any frame reset will end offscreen presentation.

According to https://github.com/w3c/presentation-api/issues/367, only top level frame matters. This patch adds a is_main_frame_ flag to PresentationServiceImpl to indicate top level frame.

BUG=734798
Review-Url: https://codereview.chromium.org/2949053002
Cr-Commit-Position: refs/heads/master@{#483436}
Committed: https://chromium.googlesource.com/chromium/src/+/dca98f635b5337b08df7a3d9480800adfeaba4c5

[zhaobin, 2017-06-21 18:25:45]

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

[imcheng, 2017-06-21 21:45:17]

lgtm

Looks like a ok fix. We can potentially use the same logic for
SetDefaultPresentationUrls -- that check is currently being done in the PSDImpl.

I am not 100% sure if the value of GetMainFrame() will remain unchanged over
time. Could you please check with ncarter@ to be sure?

[zhaobin, 2017-06-23 18:57:32]

zhaobin@chromium.org changed reviewers:
+ nick@chromium.org

[zhaobin, 2017-06-23 18:57:33]

Talked to ncarter@ and he says GetMainFrame() usage seems fine here.

[ncarter (slow), 2017-06-23 19:49:01]

https://codereview.chromium.org/2949053002/diff/20001/content/browser/present...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/2949053002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:65: is_main_frame_ =
rfh_impl->frame_tree_node()->IsMainFrame();
I forgot -- you can do this without the cast by just calling

render_frame_host->GetParent() != nullptr

However, the IsMainFrame() approach is totally fine too. They're equivalent.

[ncarter (slow), 2017-06-23 19:49:34]

https://codereview.chromium.org/2949053002/diff/20001/content/browser/present...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/2949053002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:65: is_main_frame_ =
rfh_impl->frame_tree_node()->IsMainFrame();
On 2017/06/23 19:49:01, ncarter (slow) wrote:
> I forgot -- you can do this without the cast by just calling
> 
> render_frame_host->GetParent() != nullptr
> 
> However, the IsMainFrame() approach is totally fine too. They're equivalent.

(no parent means yes main frame)

[zhaobin, 2017-06-23 21:34:12]

https://codereview.chromium.org/2949053002/diff/20001/content/browser/present...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/2949053002/diff/20001/content/browser/present...
content/browser/presentation/presentation_service_impl.cc:65: is_main_frame_ =
rfh_impl->frame_tree_node()->IsMainFrame();
On 2017/06/23 19:49:01, ncarter (slow) wrote:
> I forgot -- you can do this without the cast by just calling
> 
> render_frame_host->GetParent() != nullptr
> 
> However, the IsMainFrame() approach is totally fine too. They're equivalent.

Done.

[zhaobin, 2017-06-23 21:34:27]

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

[commit-bot: I haz the power, 2017-06-23 21:35:00]

Dry run: 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-06-23 22:46:22]

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

[commit-bot: I haz the power, 2017-06-23 22:46:23]

Dry run: This issue passed the CQ dry run.

[imcheng, 2017-06-23 23:11:02]

lgtm

Thanks Nick!

[mark a. foltz, 2017-06-26 23:00:08]

LGTM

Can this behavior be unit tested?

[zhaobin, 2017-06-27 21:14:23]

On 2017/06/26 23:00:08, mark a. foltz wrote:
> LGTM
> 
> Can this behavior be unit tested?

Done.

[zhaobin, 2017-06-27 22:59:38]

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

[commit-bot: I haz the power, 2017-06-27 23:00:00]

Dry run: 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-06-28 00:53:49]

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

[commit-bot: I haz the power, 2017-06-28 00:53:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[zhaobin, 2017-06-28 17:17:12]

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

[commit-bot: I haz the power, 2017-06-28 17:17:34]

Dry run: 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-06-28 18:01:22]

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

[commit-bot: I haz the power, 2017-06-28 18:01:23]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-06-29 17:37:06]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-06-29 17:37:06]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2949053002/#ps60001
(title: "add unit test")

[commit-bot: I haz the power, 2017-06-29 17:37:21]

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-06-29 18:42:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498757826286660,
"parent_rev": "8870512d4dbb64cec03da1d1353284d9db218187", "commit_rev":
"dca98f635b5337b08df7a3d9480800adfeaba4c5"}

[commit-bot: I haz the power, 2017-06-29 18:42:49]

Description was changed from

==========
[Presentation API] OffscreenPresentationManager should only interact with top
level receiver frame

When receiver page has sub frames, all frames will try to register receiver
callbacks to OPM with the same presentation_id, triggering DCHECK failure. Also
any frame reset will end offscreen presentation.

According to https://github.com/w3c/presentation-api/issues/367, only top level
frame matters. This patch adds a is_main_frame_ flag to PresentationServiceImpl
to indicate top level frame.

BUG=734798
==========

to

==========
[Presentation API] OffscreenPresentationManager should only interact with top
level receiver frame

When receiver page has sub frames, all frames will try to register receiver
callbacks to OPM with the same presentation_id, triggering DCHECK failure. Also
any frame reset will end offscreen presentation.

According to https://github.com/w3c/presentation-api/issues/367, only top level
frame matters. This patch adds a is_main_frame_ flag to PresentationServiceImpl
to indicate top level frame.

BUG=734798

Review-Url: https://codereview.chromium.org/2949053002
Cr-Commit-Position: refs/heads/master@{#483436}
Committed:
https://chromium.googlesource.com/chromium/src/+/dca98f635b5337b08df7a3d94808...
==========

[commit-bot: I haz the power, 2017-06-29 18:42:51]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/dca98f635b5337b08df7a3d94808...