Rewrite Fetch API referrer layout tests with service worker

Rewrite Fetch API referrer layout tests with service worker

BUG=630996
Committed: https://crrev.com/b0c153825786146160beb3fa7b5742ea12e3c04c
Cr-Commit-Position: refs/heads/master@{#408607}

[yhirano, 2016-07-28 07:13:08]

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

[commit-bot: I haz the power, 2016-07-28 07:13:25]

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, 2016-07-28 07:14:54]

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

[commit-bot: I haz the power, 2016-07-28 07:14:55]

Dry run: Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[yhirano, 2016-07-28 07:20:10]

yhirano@chromium.org changed reviewers:
+ nhiroki@chromium.org, yukishiino@chromium.org

[Yuki, 2016-07-28 07:22:51]

lgtm

[yhirano, 2016-07-28 07:31:30]

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

[commit-bot: I haz the power, 2016-07-28 07:31:40]

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, 2016-07-28 08:26:07]

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

[commit-bot: I haz the power, 2016-07-28 08:26:08]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-07-28 09:04:49]

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

[commit-bot: I haz the power, 2016-07-28 09:05:06]

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, 2016-07-28 09:06:37]

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

[commit-bot: I haz the power, 2016-07-28 09:06:37]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_...)

[yhirano, 2016-07-28 09:14:30]

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

[commit-bot: I haz the power, 2016-07-28 09:14:43]

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

[yhirano, 2016-07-28 09:18:00]

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

[commit-bot: I haz the power, 2016-07-28 09:18:12]

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, 2016-07-28 10:18:33]

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

[commit-bot: I haz the power, 2016-07-28 10:18:34]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2016-07-29 03:54:46]

https://codereview.chromium.org/2192743002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/fetch/referrer/serviceworker-echo-referrer-from-default-document.html
(right):

https://codereview.chromium.org/2192743002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/referrer/serviceworker-echo-referrer-from-default-document.html:34:
}).then(() => {
Can you put "})" and ".then" to separate lines:

return sw_unregister_and_register(...)
  .then(r => {
      // ...
    })
  .then(() => {
      // ...
    });

See: http://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style
"When wrapping chained property accesses, break before the period."

https://codereview.chromium.org/2192743002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/referrer/serviceworker-echo-referrer-from-default-document.html:36:
}).then(frame => {
Can you add "add_completion_callback(() => frame.remove());" here?

https://codereview.chromium.org/2192743002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/referrer/serviceworker-echo-referrer-from-default-document.html:53:
done();
"done()" might not be necessary because outer promise_test() internally calls
it.

[yhirano, 2016-07-29 05:38:31]

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

[commit-bot: I haz the power, 2016-07-29 05:38:48]

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

[yhirano, 2016-07-29 05:40:47]

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

[commit-bot: I haz the power, 2016-07-29 05:41:00]

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

[yhirano, 2016-07-29 05:47:33]

Description was changed from

==========
Unflake fetch/referrer/serviceworker-echo-referrer-from-default-document.html

The layout test is flaky because it passes when the service worker is already
registered. Unregister the service worker to make it fail reliably.

BUG=630996
==========

to

==========
Rewrite Fetch API referrer layout tests with service worker

BUG=630996
==========

[yhirano, 2016-07-29 05:50:19]

https://codereview.chromium.org/2192743002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/fetch/referrer/serviceworker-echo-referrer-from-default-document.html
(right):

https://codereview.chromium.org/2192743002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/referrer/serviceworker-echo-referrer-from-default-document.html:36:
}).then(frame => {
On 2016/07/29 03:54:46, nhiroki wrote:
> Can you add "add_completion_callback(() => frame.remove());" here?

Adding it breaks the test: Maybe it's executed when this promise_test ends. Is
it possible to add a callback that will run when all tests are done?

https://codereview.chromium.org/2192743002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/referrer/serviceworker-echo-referrer-from-default-document.html:53:
done();
On 2016/07/29 03:54:46, nhiroki wrote:
> "done()" might not be necessary because outer promise_test() internally calls
> it.

Done.

[nhiroki, 2016-07-29 06:21:22]

https://codereview.chromium.org/2192743002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/fetch/referrer/serviceworker-echo-referrer-from-default-document.html
(right):

https://codereview.chromium.org/2192743002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/referrer/serviceworker-echo-referrer-from-default-document.html:36:
}).then(frame => {
On 2016/07/29 05:50:19, yhirano wrote:
> On 2016/07/29 03:54:46, nhiroki wrote:
> > Can you add "add_completion_callback(() => frame.remove());" here?
> 
> Adding it breaks the test: Maybe it's executed when this promise_test ends. Is
> it possible to add a callback that will run when all tests are done?

I suspect you used add_result_callback() instead of add_completion_callback().
According to the comment on a link below, a completion callback should run when
all tests are done.

https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources...

[yhirano, 2016-07-29 06:25:27]

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

[commit-bot: I haz the power, 2016-07-29 06:25:43]

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

[yhirano, 2016-07-29 06:26:43]

https://codereview.chromium.org/2192743002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/fetch/referrer/serviceworker-echo-referrer-from-default-document.html
(right):

https://codereview.chromium.org/2192743002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/referrer/serviceworker-echo-referrer-from-default-document.html:36:
}).then(frame => {
On 2016/07/29 06:21:21, nhiroki wrote:
> On 2016/07/29 05:50:19, yhirano wrote:
> > On 2016/07/29 03:54:46, nhiroki wrote:
> > > Can you add "add_completion_callback(() => frame.remove());" here?
> > 
> > Adding it breaks the test: Maybe it's executed when this promise_test ends.
Is
> > it possible to add a callback that will run when all tests are done?
> 
> I suspect you used add_result_callback() instead of add_completion_callback().
> According to the comment on a link below, a completion callback should run
when
> all tests are done.
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources...

Ah, sorry, you're right. Fixed.

[nhiroki, 2016-07-29 06:55:13]

https://codereview.chromium.org/2192743002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/fetch/referrer/serviceworker-echo-referrer-from-default-document.html
(right):

https://codereview.chromium.org/2192743002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/referrer/serviceworker-echo-referrer-from-default-document.html:32:
add_completion_callback(() => r.unregister());
Just for clarification: In this case, both add_result_callback and
add_completion_callback are OK because unregistration does not affect frames
being controlled.

[nhiroki, 2016-07-29 06:55:20]

lgtm

[commit-bot: I haz the power, 2016-07-29 09:15:22]

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

[commit-bot: I haz the power, 2016-07-29 09:15:23]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-07-29 09:15:44]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-07-29 09:15:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2192743002/#ps140001
(title: "fix")

[commit-bot: I haz the power, 2016-07-29 09:15:55]

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-07-29 09:19:38]

Description was changed from

==========
Rewrite Fetch API referrer layout tests with service worker

BUG=630996
==========

to

==========
Rewrite Fetch API referrer layout tests with service worker

BUG=630996
==========

[commit-bot: I haz the power, 2016-07-29 09:19:39]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-29 09:21:15]

Description was changed from

==========
Rewrite Fetch API referrer layout tests with service worker

BUG=630996
==========

to

==========
Rewrite Fetch API referrer layout tests with service worker

BUG=630996

Committed: https://crrev.com/b0c153825786146160beb3fa7b5742ea12e3c04c
Cr-Commit-Position: refs/heads/master@{#408607}
==========

[commit-bot: I haz the power, 2016-07-29 09:21:17]

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