ServiceWorker: Refer to the current provider in ServiceWorkerRegistration

ServiceWorker: Refer to the current provider in ServiceWorkerRegistration

Currently |m_provider| is kept at the constructor in ServiceWorkerRegistration.
Once the provider is removed (e.g. the registration is belong to an iframe and
iframe.remove() is called), the registration would refer the invalid
|m_provider|. This patch removes |m_provider| and gets a corresponding provider
via SWContainerClient.
Additionally, reject is changed to rejectWithDOMException to reject immediately.
This change enables us to get DOMException on the parent frame used to own the
reclaimed iframe. A test is also added for tracking the exception.

BUG=603817
TEST=./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default http/tests/serviceworker/chromium/unregister-on-detached-iframe.html

Committed: https://crrev.com/e0281ef6c52c10a3baf7e0a043c944ab14c9a712
Cr-Commit-Position: refs/heads/master@{#389700}

[shimazu, 2016-04-19 09:08:00]

shimazu@chromium.org changed reviewers:
+ nhiroki@chromium.org

[shimazu, 2016-04-19 09:08:01]

PTAL:)

[nhiroki, 2016-04-20 03:42:16]

https://codereview.chromium.org/1893363003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html
(right):

https://codereview.chromium.org/1893363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html:11:
var scope = 'resources/reclam-registration-after-iframe-deletion';
s/reclam/reclaim/

BTW, "reclaim-registration" might not correctly explain the behavior of this
test because this does not call register() but unregister() after the iframe is
removed. How about renaming it to something else, for example,
"unregister-on-detached-frame"?

https://codereview.chromium.org/1893363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html:28:
service_worker_unregister.bind(null, t, scope));
This couldn't work because "service_worker_unregister" consists of multiple
async operations (getRegistration and unregister) and add_completion_callback
does not wait for the completion of them.

Instead, how about like this?

  .then(function() {
      ...
      // Register on the main frame.
      return navigator.serviceWorker.register(...);
    })
  .then(function(r) {
      add_completion_callback(function() { r.unregister(); });

      // Get a registration on the iframe.
      return frame.contentWindow.navigator.serviceWorker.getRegistration();
    })
  .then(function(r) {
      frame.remove();
      assert_promise_rejects(r.unregister(), ...);
    })

https://codereview.chromium.org/1893363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html:32:
.then(function() {
Maybe we don't have to separate these operations into two .then blocks (we can
just remove line 31 and 32).

https://codereview.chromium.org/1893363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html:33:
return assert_promise_rejects(
'return' is not necessary.

https://codereview.chromium.org/1893363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html:39:
}, 'unregistration to  should be finished successfully');
There is an extra space between "to" and "should".

[shimazu, 2016-04-21 03:57:50]

Incorporated with the comments. 
PTAL again.

https://codereview.chromium.org/1893363003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html
(right):

https://codereview.chromium.org/1893363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html:11:
var scope = 'resources/reclam-registration-after-iframe-deletion';
On 2016/04/20 03:42:16, nhiroki wrote:
> s/reclam/reclaim/
> 
> BTW, "reclaim-registration" might not correctly explain the behavior of this
> test because this does not call register() but unregister() after the iframe
is
> removed. How about renaming it to something else, for example,
> "unregister-on-detached-frame"?

Done.

https://codereview.chromium.org/1893363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html:28:
service_worker_unregister.bind(null, t, scope));
On 2016/04/20 03:42:15, nhiroki wrote:
> This couldn't work because "service_worker_unregister" consists of multiple
> async operations (getRegistration and unregister) and add_completion_callback
> does not wait for the completion of them.
> 
> Instead, how about like this?
> 
>   .then(function() {
>       ...
>       // Register on the main frame.
>       return navigator.serviceWorker.register(...);
>     })
>   .then(function(r) {
>       add_completion_callback(function() { r.unregister(); });
> 
>       // Get a registration on the iframe.
>       return frame.contentWindow.navigator.serviceWorker.getRegistration();
>     })
>   .then(function(r) {
>       frame.remove();
>       assert_promise_rejects(r.unregister(), ...);
>     })

Done.

https://codereview.chromium.org/1893363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html:32:
.then(function() {
On 2016/04/20 03:42:16, nhiroki wrote:
> Maybe we don't have to separate these operations into two .then blocks (we can
> just remove line 31 and 32).

Done.

https://codereview.chromium.org/1893363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html:33:
return assert_promise_rejects(
On 2016/04/20 03:42:16, nhiroki wrote:
> 'return' is not necessary.

assert_promise_rejects() returns Promise object containing the result of
'registration.unregister()'. I think it's better to return the Promise to
promise_test. How is it?

https://codereview.chromium.org/1893363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html:39:
}, 'unregistration to  should be finished successfully');
On 2016/04/20 03:42:16, nhiroki wrote:
> There is an extra space between "to" and "should".

Done.

[nhiroki, 2016-04-21 08:23:30]

https://codereview.chromium.org/1893363003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html
(right):

https://codereview.chromium.org/1893363003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html:33:
return assert_promise_rejects(
On 2016/04/21 03:57:50, makoto wrote:
> On 2016/04/20 03:42:16, nhiroki wrote:
> > 'return' is not necessary.
> 
> assert_promise_rejects() returns Promise object containing the result of
> 'registration.unregister()'. I think it's better to return the Promise to
> promise_test. How is it?

Ah, sorry! I wrongly assumed that assert_promise_rejects returns nothing like
other assert variants. Keeping 'returns' looks good.

https://codereview.chromium.org/1893363003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/unregister-on-detached-iframe.html
(right):

https://codereview.chromium.org/1893363003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/unregister-on-detached-iframe.html:10:
var resources_dir = 'resources';
I think |resources_dir| does not help to reduce the complexity. How about just
prepending 'resources' to each path?

https://codereview.chromium.org/1893363003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/unregister-on-detached-iframe.html:19:
return with_iframe(url, {auto_remove: false});
We could make 'auto_remove' true for the case where an exception is thrown in
the middle of the test (I assume that multiple remove() calls on the frame is
not harmful).

[nhiroki, 2016-04-21 08:32:15]

Can you revise the description?

> Currently |m_provider| is kept at the constructor in
ServiceWorkerRegistration. Once the provider is reclaimed (e.g. the registration
is belong to an iframe and iframe.remove() is called), the registration would
refer the invalid |m_provider|. This patch removes |m_provider| and gets a
corresponding provider via SWContainerClient.

reclaimed -> destroyed/deleted?
refer -> refer to

Can you wrap the description at the 80th column?
(see
http://www.chromium.org/developers/contributing-code#TOC-Change-List-Descript...)

> TEST=./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default
http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html

Test file name looks outdated.

[shimazu, 2016-04-21 09:03:19]

Description was changed from

==========
ServiceWorker: Refer the current provider in ServiceWorkerRegistration

Currently |m_provider| is kept at the constructor in ServiceWorkerRegistration.
Once the provider is reclaimed (e.g. the registration is belong to an iframe and
iframe.remove() is called), the registration would refer the invalid
|m_provider|. This patch removes |m_provider| and gets a corresponding provider
via SWContainerClient.

Additionally, reject is changed to rejectWithDOMException to reject immediately.
This change enables us to get DOMException on the parent frame used to own the
reclaimed iframe. A test is also added for tracking the exception.

BUG=603817
TEST=./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default
http/tests/serviceworker/chromium/reclaim-registration-after-iframe-deletion.html
==========

to

==========
ServiceWorker: Refer to the current provider in ServiceWorkerRegistration

Currently |m_provider| is kept at the constructor in ServiceWorkerRegistration.
Once the provider is removed (e.g. the registration is belong to an iframe and
iframe.remove() is called), the registration would refer the invalid
|m_provider|. This patch removes |m_provider| and gets a corresponding provider
via SWContainerClient.
Additionally, reject is changed to rejectWithDOMException to reject immediately.
This change enables us to get DOMException on the parent frame used to own the
reclaimed iframe. A test is also added for tracking the exception.

BUG=603817
TEST=./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default
http/tests/serviceworker/chromium/unregister-on-detached-iframe.html
==========

[shimazu, 2016-04-21 09:06:36]

Thanks for the reviews!

I also updated the description.

https://codereview.chromium.org/1893363003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/unregister-on-detached-iframe.html
(right):

https://codereview.chromium.org/1893363003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/unregister-on-detached-iframe.html:10:
var resources_dir = 'resources';
On 2016/04/21 08:23:30, nhiroki wrote:
> I think |resources_dir| does not help to reduce the complexity. How about just
> prepending 'resources' to each path?

Done.

https://codereview.chromium.org/1893363003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/unregister-on-detached-iframe.html:19:
return with_iframe(url, {auto_remove: false});
On 2016/04/21 08:23:30, nhiroki wrote:
> We could make 'auto_remove' true for the case where an exception is thrown in
> the middle of the test (I assume that multiple remove() calls on the frame is
> not harmful).

Done.

[Marijn Kruisselbrink, 2016-04-21 16:47:19]

https://codereview.chromium.org/1893363003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/unregister-on-detached-iframe.html
(right):

https://codereview.chromium.org/1893363003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/unregister-on-detached-iframe.html:32:
return assert_promise_rejects(
drive-by nit: new code should probably use promise_rejects (from testharness.js)
rather than assert_promise_rejects from testharness-helpers.js? (and we should
actually get rid of assert_promise_rejects and use promise_rejects everywhere).

[shimazu, 2016-04-22 05:44:02]

https://codereview.chromium.org/1893363003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/unregister-on-detached-iframe.html
(right):

https://codereview.chromium.org/1893363003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/unregister-on-detached-iframe.html:32:
return assert_promise_rejects(
On 2016/04/21 16:47:18, Marijn Kruisselbrink wrote:
> drive-by nit: new code should probably use promise_rejects (from
testharness.js)
> rather than assert_promise_rejects from testharness-helpers.js? (and we should
> actually get rid of assert_promise_rejects and use promise_rejects
everywhere).

Thanks, I didn't know that. Fixed.

[nhiroki, 2016-04-25 04:56:35]

lgtm

https://codereview.chromium.org/1893363003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/unregister-on-detached-iframe.html
(right):

https://codereview.chromium.org/1893363003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/unregister-on-detached-iframe.html:28:
scope_for_iframe);
+2 space indents

[shimazu, 2016-04-26 02:33:29]

https://codereview.chromium.org/1893363003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/unregister-on-detached-iframe.html
(right):

https://codereview.chromium.org/1893363003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/unregister-on-detached-iframe.html:28:
scope_for_iframe);
On 2016/04/25 04:56:35, nhiroki wrote:
> +2 space indents

I think in this case 2-space indent is correct.
[1, Indent two spaces] says wrapping by 4 when the actual parameters is followed
by the property access of return value.

I'll commit as is.

[1] http://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style

[shimazu, 2016-04-26 02:33:35]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2016-04-26 02:33:35]

The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/1893363003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2016-04-26 02:33:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1893363003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1893363003/80001

[commit-bot: I haz the power, 2016-04-26 05:02:28]

Description was changed from

==========
ServiceWorker: Refer to the current provider in ServiceWorkerRegistration

Currently |m_provider| is kept at the constructor in ServiceWorkerRegistration.
Once the provider is removed (e.g. the registration is belong to an iframe and
iframe.remove() is called), the registration would refer the invalid
|m_provider|. This patch removes |m_provider| and gets a corresponding provider
via SWContainerClient.
Additionally, reject is changed to rejectWithDOMException to reject immediately.
This change enables us to get DOMException on the parent frame used to own the
reclaimed iframe. A test is also added for tracking the exception.

BUG=603817
TEST=./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default
http/tests/serviceworker/chromium/unregister-on-detached-iframe.html
==========

to

==========
ServiceWorker: Refer to the current provider in ServiceWorkerRegistration

Currently |m_provider| is kept at the constructor in ServiceWorkerRegistration.
Once the provider is removed (e.g. the registration is belong to an iframe and
iframe.remove() is called), the registration would refer the invalid
|m_provider|. This patch removes |m_provider| and gets a corresponding provider
via SWContainerClient.
Additionally, reject is changed to rejectWithDOMException to reject immediately.
This change enables us to get DOMException on the parent frame used to own the
reclaimed iframe. A test is also added for tracking the exception.

BUG=603817
TEST=./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default
http/tests/serviceworker/chromium/unregister-on-detached-iframe.html
==========

[commit-bot: I haz the power, 2016-04-26 05:02:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-26 05:03:52]

Description was changed from

==========
ServiceWorker: Refer to the current provider in ServiceWorkerRegistration

Currently |m_provider| is kept at the constructor in ServiceWorkerRegistration.
Once the provider is removed (e.g. the registration is belong to an iframe and
iframe.remove() is called), the registration would refer the invalid
|m_provider|. This patch removes |m_provider| and gets a corresponding provider
via SWContainerClient.
Additionally, reject is changed to rejectWithDOMException to reject immediately.
This change enables us to get DOMException on the parent frame used to own the
reclaimed iframe. A test is also added for tracking the exception.

BUG=603817
TEST=./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default
http/tests/serviceworker/chromium/unregister-on-detached-iframe.html
==========

to

==========
ServiceWorker: Refer to the current provider in ServiceWorkerRegistration

Currently |m_provider| is kept at the constructor in ServiceWorkerRegistration.
Once the provider is removed (e.g. the registration is belong to an iframe and
iframe.remove() is called), the registration would refer the invalid
|m_provider|. This patch removes |m_provider| and gets a corresponding provider
via SWContainerClient.
Additionally, reject is changed to rejectWithDOMException to reject immediately.
This change enables us to get DOMException on the parent frame used to own the
reclaimed iframe. A test is also added for tracking the exception.

BUG=603817
TEST=./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default
http/tests/serviceworker/chromium/unregister-on-detached-iframe.html

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

[commit-bot: I haz the power, 2016-04-26 05:03:53]

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