[Presentation API] Use iframe in presentation-navigation test

[Presentation API] Use iframe in presentation-navigation test

In presentation navigation layout tests, reload iframe instead of reloading current page.

BUG=680298
Review-Url: https://codereview.chromium.org/2641663002
Cr-Commit-Position: refs/heads/master@{#444503}
Committed: https://chromium.googlesource.com/chromium/src/+/920c2baec25e9e298765b23fd0d738c5c78c7ffc

[zhaobin, 2017-01-18 00:00:15]

Description was changed from

==========
[Presentation API] use iframe in presentation-navigation test

In presentation navigation layout tests, reload iframe instead of reloading
current page.

BUG=680298
==========

to

==========
[Presentation API] Use iframe in presentation-navigation test

In presentation navigation layout tests, reload iframe instead of reloading
current page.

BUG=680298
==========

[zhaobin, 2017-01-18 00:13:34]

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

[mlamouri (slow - plz ping), 2017-01-18 10:27:40]

lgtm

[foolip, 2017-01-18 11:13:53]

lgtm % nits and a GC question that apply to both tests.

https://codereview.chromium.org/2641663002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/presentation/presentation-navigation.html
(right):

https://codereview.chromium.org/2641663002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/presentation/presentation-navigation.html:12:
t.step(function() {
The argument to async_test is called as a step, so it's not necessary to wrap in
t.step() here. (It is needed with a `t=async_test('description');t.step(...)`
structure.)

https://codereview.chromium.org/2641663002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/presentation/presentation-navigation.html:13:
iframe.onload = () => {
This needs to be iframe.onload = t.step_func(() => { ... }).

https://codereview.chromium.org/2641663002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/presentation/presentation-navigation.html:14: var
defaultRequest = iframe.contentWindow.navigator.presentation.defaultRequest;
Extracting this out won't work, then you're operating only on a copy of the
value. You could use `var presentation =
iframe.contentWindow.navigator.presentation` however.

https://codereview.chromium.org/2641663002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/presentation/presentation-navigation.html:16:
assert_equals(defaultRequest, null);
If this test passed like this, how did defaultRequest become null? Is it a weak
pointer of some sort such that GC is observable?

[zhaobin, 2017-01-18 19:22:42]

foolip@, thanks a lot for the comments about how to write layout tests
correctly! For the gc question, we do not GC defaultRequest object, but verify a
different defaultRequest object equals to null.

https://codereview.chromium.org/2641663002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/presentation/presentation-navigation.html
(right):

https://codereview.chromium.org/2641663002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/presentation/presentation-navigation.html:12:
t.step(function() {
On 2017/01/18 11:13:52, foolip_slow_very_sorry wrote:
> The argument to async_test is called as a step, so it's not necessary to wrap
in
> t.step() here. (It is needed with a `t=async_test('description');t.step(...)`
> structure.)

Done.

https://codereview.chromium.org/2641663002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/presentation/presentation-navigation.html:13:
iframe.onload = () => {
On 2017/01/18 11:13:52, foolip_slow_very_sorry wrote:
> This needs to be iframe.onload = t.step_func(() => { ... }).

Done.

https://codereview.chromium.org/2641663002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/presentation/presentation-navigation.html:14: var
defaultRequest = iframe.contentWindow.navigator.presentation.defaultRequest;
On 2017/01/18 11:13:52, foolip_slow_very_sorry wrote:
> Extracting this out won't work, then you're operating only on a copy of the
> value. You could use `var presentation =
> iframe.contentWindow.navigator.presentation` however.

Done.

https://codereview.chromium.org/2641663002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/presentation/presentation-navigation.html:16:
assert_equals(defaultRequest, null);
On 2017/01/18 11:13:52, foolip_slow_very_sorry wrote:
> If this test passed like this, how did defaultRequest become null? Is it a
weak
> pointer of some sort such that GC is observable?

We load iframe twice:

1: iframe loads 'resources/blank.html' and creates defaultRequest object
2. iframe reloads 'resources/blank.html#after-reload' and does not create
defaultRequest object. Verify defaultRequest == null here.

defaultRequest in 2. is a different object from defaultRequest in 1. so it could
be null.

[zhaobin, 2017-01-18 19:27:04]

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

[commit-bot: I haz the power, 2017-01-18 19:27:39]

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

[foolip, 2017-01-18 19:42:45]

lgtm

https://codereview.chromium.org/2641663002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/presentation/presentation-navigation.html
(right):

https://codereview.chromium.org/2641663002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/presentation/presentation-navigation.html:16:
assert_equals(defaultRequest, null);
On 2017/01/18 19:22:42, zhaobin wrote:
> On 2017/01/18 11:13:52, foolip_slow_very_sorry wrote:
> > If this test passed like this, how did defaultRequest become null? Is it a
> weak
> > pointer of some sort such that GC is observable?
> 
> We load iframe twice:
> 
> 1: iframe loads 'resources/blank.html' and creates defaultRequest object
> 2. iframe reloads 'resources/blank.html#after-reload' and does not create
> defaultRequest object. Verify defaultRequest == null here.
> 
> defaultRequest in 2. is a different object from defaultRequest in 1. so it
could
> be null.

Oh, now I see my misunderstanding. defaultRequest would in fact be assigned
twice, the assignment is inside the load event handler and not outside. Setting
the defaultRequest var in the else branch made this seem odd, but with the new
structure (var presentation) there is no problem. Thanks!

[mark a. foltz, 2017-01-18 20:27:10]

lgtm

[commit-bot: I haz the power, 2017-01-18 21:50:03]

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

[commit-bot: I haz the power, 2017-01-18 21:50:04]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-01-18 21:50:40]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-01-18 21:50:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2641663002/#ps20001
(title: "resolve code review comments from foolip")

[commit-bot: I haz the power, 2017-01-18 21:51:25]

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-18 21:57:44]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484776240259920,
"parent_rev": "9a9e707b680a636755883805166f8e6f2b62b997", "commit_rev":
"920c2baec25e9e298765b23fd0d738c5c78c7ffc"}

[commit-bot: I haz the power, 2017-01-18 21:58:13]

Description was changed from

==========
[Presentation API] Use iframe in presentation-navigation test

In presentation navigation layout tests, reload iframe instead of reloading
current page.

BUG=680298
==========

to

==========
[Presentation API] Use iframe in presentation-navigation test

In presentation navigation layout tests, reload iframe instead of reloading
current page.

BUG=680298

Review-Url: https://codereview.chromium.org/2641663002
Cr-Commit-Position: refs/heads/master@{#444503}
Committed:
https://chromium.googlesource.com/chromium/src/+/920c2baec25e9e298765b23fd0d7...
==========

[commit-bot: I haz the power, 2017-01-18 21:58:14]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/920c2baec25e9e298765b23fd0d7...