Extending "ChromeSitePerProcessPDFTest.EmbeddedPDFInsideCrossOriginFrame" to verify guest destructi…

Extending "ChromeSitePerProcessPDFTest.EmbeddedPDFInsideCrossOriginFrame" to verify guest destruction as well.

This CL will also add the logic to remove the embedder frame and verify
that the guest is destroyed.

BUG=666088
Committed: https://crrev.com/8f1f1e94902d0a8ce71c287fbf8bd3119483e2a3
Cr-Commit-Position: refs/heads/master@{#434074}

[EhsanK, 2016-11-21 23:28:43]

ekaramad@chromium.org changed reviewers:
+ alexmos@chromium.org, creis@chromium.org

[EhsanK, 2016-11-21 23:28:44]

PTAL.

I could make it a separate test but there will be too much code in common.

Thanks!

[alexmos, 2016-11-23 00:23:51]

Thanks, a few comments below, and also just to clarify, is there any actual fix
needed to clean up the guest in this case, or is this only about adding test
coverage?

https://codereview.chromium.org/2525473002/diff/1/chrome/browser/chrome_site_...
File chrome/browser/chrome_site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2525473002/diff/1/chrome/browser/chrome_site_...
chrome/browser/chrome_site_per_process_browsertest.cc:324: class
WebContentsDestructionObserver : content::WebContentsObserver {
There's a WebContentsDestroyedWatcher in content/public/test/test_utils.h.  Is
it possible to use that one instead of a new class?

https://codereview.chromium.org/2525473002/diff/1/chrome/browser/chrome_site_...
chrome/browser/chrome_site_per_process_browsertest.cc:382:
test_guest_view_manager()->WaitForSingleGuestCreated();
Can you save the result of this and use it below, instead of calling
GetLastGuestCreated again?

https://codereview.chromium.org/2525473002/diff/1/chrome/browser/chrome_site_...
chrome/browser/chrome_site_per_process_browsertest.cc:390: observer.Wait();
Would it be useful to also check that GetNumGuestsActive() is 0 after this
point?

[EhsanK, 2016-11-23 00:45:15]

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

[commit-bot: I haz the power, 2016-11-23 00:45:53]

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

[EhsanK, 2016-11-23 00:51:39]

Thanks Alex. PTAL.

FYI, the fix was added in the CL that landed the original test, here:
https://codereview.chromium.org/2417693002/diff/440001/components/guest_view/...

But to rush landing the CL before the cut the test was postponed to later.

Thanks!

https://codereview.chromium.org/2525473002/diff/1/chrome/browser/chrome_site_...
File chrome/browser/chrome_site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2525473002/diff/1/chrome/browser/chrome_site_...
chrome/browser/chrome_site_per_process_browsertest.cc:324: class
WebContentsDestructionObserver : content::WebContentsObserver {
On 2016/11/23 00:23:51, alexmos wrote:
> There's a WebContentsDestroyedWatcher in content/public/test/test_utils.h.  Is
> it possible to use that one instead of a new class?

Yes indeed. Thanks for the suggestion.

https://codereview.chromium.org/2525473002/diff/1/chrome/browser/chrome_site_...
chrome/browser/chrome_site_per_process_browsertest.cc:382:
test_guest_view_manager()->WaitForSingleGuestCreated();
On 2016/11/23 00:23:51, alexmos wrote:
> Can you save the result of this and use it below, instead of calling
> GetLastGuestCreated again?

Acknowledged.

https://codereview.chromium.org/2525473002/diff/1/chrome/browser/chrome_site_...
chrome/browser/chrome_site_per_process_browsertest.cc:390: observer.Wait();
On 2016/11/23 00:23:51, alexmos wrote:
> Would it be useful to also check that GetNumGuestsActive() is 0 after this
> point?

Acknowledged.

[EhsanK, 2016-11-23 00:51:56]

[+site-isolation-reviews]

[alexmos, 2016-11-23 00:59:14]

LGTM

[EhsanK, 2016-11-23 01:21:36]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-11-23 01:21:37]

The CQ bit was checked by ekaramad@chromium.org

[commit-bot: I haz the power, 2016-11-23 01:22:04]

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, 2016-11-23 01:38:38]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479864097056940,
"parent_rev": "999299b8178ea241aa31fdb309565fbfb95031af", "commit_rev":
"b563186ffe1f67bbeb4988d5be04f6d8643b2d19"}

[commit-bot: I haz the power, 2016-11-23 01:39:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-23 01:40:44]

Description was changed from

==========
Extending "ChromeSitePerProcessPDFTest.EmbeddedPDFInsideCrossOriginFrame" to
verify guest destruction as well.

This CL will also add the logic to remove the embedder frame and verify
that the guest is destroyed.

BUG=666088
==========

to

==========
Extending "ChromeSitePerProcessPDFTest.EmbeddedPDFInsideCrossOriginFrame" to
verify guest destruction as well.

This CL will also add the logic to remove the embedder frame and verify
that the guest is destroyed.

BUG=666088

Committed: https://crrev.com/8f1f1e94902d0a8ce71c287fbf8bd3119483e2a3
Cr-Commit-Position: refs/heads/master@{#434074}
==========

[commit-bot: I haz the power, 2016-11-23 01:40:45]

Patchset 2 (id:??) landed as
https://crrev.com/8f1f1e94902d0a8ce71c287fbf8bd3119483e2a3
Cr-Commit-Position: refs/heads/master@{#434074}

[Charlie Reis, 2016-11-23 06:38:02]

Thanks!  LGTM too.