ServiceWorker: Should throw TypeError instead of Unknown/SecurityError.

ServiceWorker: Should throw TypeError instead of Unknown/SecurityError.

If navigate() or openWindow() faces errors during navigation, we should throw
TypeError instead of UnknownError or SecurityError. They aren't defined anywhere
in the spec[1] as well as, especially UnknownError is only used in IndexedDB[2].
To match the spec, this change adds ErrorTypeNavigation and then process it
via NavigateClientCallback instead of CallbackPromiseAdapter.

[1] http://slightlyoff.github.io/ServiceWorker/spec/service_worker
[2] http://www.w3.org/TR/IndexedDB/#h-exceptions

BUG=500911, 540503
Committed: https://crrev.com/fea1cbc16107faa81a589b070ff857d64a0f56e3
Cr-Commit-Position: refs/heads/master@{#363435}

[zino, 2015-11-26 10:37:32]

Description was changed from

==========
ServiceWorker: Should throw TypeError instead of Unknown/SecurityError.

For the following reasons, we should throw TypeError instead of UnknownError or
SecurityError if WebServiceWorkerError is ErrorTypeUnknown or ErrorTypeSecurity.
 - The UnknownError is only used for IndexedDB[1].
 - The spec[2] doesn't say to throw the UnknownError anywhere.
 - According to spec[2], There is no SecurityError in navigate().

[1] http://www.w3.org/TR/IndexedDB/#h-exceptions
[2] http://slightlyoff.github.io/ServiceWorker/spec/service_worker

BUG=500911,540503
==========

to

==========
ServiceWorker: Should throw TypeError instead of Unknown/SecurityError.

For the following reasons, we should throw TypeError instead of UnknownError or
SecurityError if WebServiceWorkerError is ErrorTypeUnknown or ErrorTypeSecurity.
 - The UnknownError is only used for IndexedDB[1].
 - The spec[2] doesn't say to throw the UnknownError anywhere.
 - According to spec[2], There is no SecurityError in navigate().

[1] http://www.w3.org/TR/IndexedDB/#h-exceptions
[2] http://slightlyoff.github.io/ServiceWorker/spec/service_worker

BUG=500911,540503
==========

[zino, 2015-11-26 10:37:32]

jinho.bang@samsung.com changed reviewers:
+ nhiroki@chromium.org

[zino, 2015-11-26 10:37:47]

Please take a look.

Thank you.

[nhiroki, 2015-11-27 03:25:13]

https://codereview.chromium.org/1481523006/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp
(right):

https://codereview.chromium.org/1481523006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:81:
if (error.errorType == WebServiceWorkerError::ErrorTypeUnknown)  {
I'd prefer not to give specific meaning to the unknown error type. How about
adding an unique error type for navigate() like ErrorNavigationError, and then
making ServiceWorkerError::take() filter it out as following code?

DOMException* ServiceWorkerError::take(ScriptPromiseResolver*, const
WebServiceWorkerError& webError) {
     switch (webError.errorType) {
     case WebServiceWorkerError::ErrorTypeNavigation:
         // ErrorTypeNavigation should have bailed out before calling this.
         ASSERT_NOT_REACHED();
         return DOMException::create(UnknownError);
     }
}

Bonus: openWindow() could reuse ErrorTypeNavigation.

[zino, 2015-11-27 03:41:29]

Thank you for review.

I've just uploaded a new patch set.

PTAL.

https://codereview.chromium.org/1481523006/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp
(right):

https://codereview.chromium.org/1481523006/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:81:
if (error.errorType == WebServiceWorkerError::ErrorTypeUnknown)  {
On 2015/11/27 03:25:13, nhiroki wrote:
> I'd prefer not to give specific meaning to the unknown error type. How about
> adding an unique error type for navigate() like ErrorNavigationError, and then
> making ServiceWorkerError::take() filter it out as following code?
> 
> DOMException* ServiceWorkerError::take(ScriptPromiseResolver*, const
> WebServiceWorkerError& webError) {
>      switch (webError.errorType) {
>      case WebServiceWorkerError::ErrorTypeNavigation:
>          // ErrorTypeNavigation should have bailed out before calling this.
>          ASSERT_NOT_REACHED();
>          return DOMException::create(UnknownError);
>      }
> }
> 
> Bonus: openWindow() could reuse ErrorTypeNavigation.

Done.

[nhiroki, 2015-11-27 07:52:01]

Looks good.

Can you update the CL description based on the latest patch set?

https://codereview.chromium.org/1481523006/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp
(right):

https://codereview.chromium.org/1481523006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:64:
class NavigateClientCallback : public WebServiceWorkerClientCallbacks {
nit: can you move this class definition to line 22 and wrap it with anonymous
namespace?

https://codereview.chromium.org/1481523006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:89:
private:
nit: can you add a blank line before "private:"?

[zino, 2015-11-30 05:40:03]

Description was changed from

==========
ServiceWorker: Should throw TypeError instead of Unknown/SecurityError.

For the following reasons, we should throw TypeError instead of UnknownError or
SecurityError if WebServiceWorkerError is ErrorTypeUnknown or ErrorTypeSecurity.
 - The UnknownError is only used for IndexedDB[1].
 - The spec[2] doesn't say to throw the UnknownError anywhere.
 - According to spec[2], There is no SecurityError in navigate().

[1] http://www.w3.org/TR/IndexedDB/#h-exceptions
[2] http://slightlyoff.github.io/ServiceWorker/spec/service_worker

BUG=500911,540503
==========

to

==========
ServiceWorker: Add ErrorTypeNavigation for navigate() and openWindow().

If navigate() or openWindow() faces errors during navigation, we should throw
TypeError instead of UnknownError or SecurityError. Especially, UnknownError or
SecurityError isn't defined anywhere in the spec[1] as well as it's only used
for IndexedDB[2]. To match the spec, this change adds ErrorTypeNavigation and
then process it via NavigateClientCallback instead of CallbackPromiseAdapter.

[1] http://www.w3.org/TR/IndexedDB/#h-exceptions
[2] http://slightlyoff.github.io/ServiceWorker/spec/service_worker

BUG=500911,540503
==========

[zino, 2015-11-30 05:49:25]

Description was changed from

==========
ServiceWorker: Add ErrorTypeNavigation for navigate() and openWindow().

If navigate() or openWindow() faces errors during navigation, we should throw
TypeError instead of UnknownError or SecurityError. Especially, UnknownError or
SecurityError isn't defined anywhere in the spec[1] as well as it's only used
for IndexedDB[2]. To match the spec, this change adds ErrorTypeNavigation and
then process it via NavigateClientCallback instead of CallbackPromiseAdapter.

[1] http://www.w3.org/TR/IndexedDB/#h-exceptions
[2] http://slightlyoff.github.io/ServiceWorker/spec/service_worker

BUG=500911,540503
==========

to

==========
ServiceWorker: Add ErrorTypeNavigation for navigate() and openWindow().

If navigate() or openWindow() faces errors during navigation, we should throw
TypeError instead of UnknownError or SecurityError. They aren't defined anywhere
in the spec[1] as well as, especially UnknownError is only used in IndexedDB[2].
To match the spec, this change adds ErrorTypeNavigation and then process it
via NavigateClientCallback instead of CallbackPromiseAdapter.

[1] http://www.w3.org/TR/IndexedDB/#h-exceptions
[2] http://slightlyoff.github.io/ServiceWorker/spec/service_worker

BUG=500911,540503
==========

[zino, 2015-11-30 05:49:57]

Description was changed from

==========
ServiceWorker: Add ErrorTypeNavigation for navigate() and openWindow().

If navigate() or openWindow() faces errors during navigation, we should throw
TypeError instead of UnknownError or SecurityError. They aren't defined anywhere
in the spec[1] as well as, especially UnknownError is only used in IndexedDB[2].
To match the spec, this change adds ErrorTypeNavigation and then process it
via NavigateClientCallback instead of CallbackPromiseAdapter.

[1] http://www.w3.org/TR/IndexedDB/#h-exceptions
[2] http://slightlyoff.github.io/ServiceWorker/spec/service_worker

BUG=500911,540503
==========

to

==========
ServiceWorker: Add ErrorTypeNavigation for navigate() and openWindow().

If navigate() or openWindow() faces errors during navigation, we should throw
TypeError instead of UnknownError or SecurityError. They aren't defined anywhere
in the spec[1] as well as, especially UnknownError is only used in IndexedDB[2].
To match the spec, this change adds ErrorTypeNavigation and then process it
via NavigateClientCallback instead of CallbackPromiseAdapter.

[1] http://slightlyoff.github.io/ServiceWorker/spec/service_worker
[2] http://www.w3.org/TR/IndexedDB/#h-exceptions

BUG=500911,540503
==========

[zino, 2015-11-30 05:50:58]

Description was changed from

==========
ServiceWorker: Add ErrorTypeNavigation for navigate() and openWindow().

If navigate() or openWindow() faces errors during navigation, we should throw
TypeError instead of UnknownError or SecurityError. They aren't defined anywhere
in the spec[1] as well as, especially UnknownError is only used in IndexedDB[2].
To match the spec, this change adds ErrorTypeNavigation and then process it
via NavigateClientCallback instead of CallbackPromiseAdapter.

[1] http://slightlyoff.github.io/ServiceWorker/spec/service_worker
[2] http://www.w3.org/TR/IndexedDB/#h-exceptions

BUG=500911,540503
==========

to

==========
ServiceWorker: Should throw TypeError instead of Unknown/SecurityError.

If navigate() or openWindow() faces errors during navigation, we should throw
TypeError instead of UnknownError or SecurityError. They aren't defined anywhere
in the spec[1] as well as, especially UnknownError is only used in IndexedDB[2].
To match the spec, this change adds ErrorTypeNavigation and then process it
via NavigateClientCallback instead of CallbackPromiseAdapter.

[1] http://slightlyoff.github.io/ServiceWorker/spec/service_worker
[2] http://www.w3.org/TR/IndexedDB/#h-exceptions

BUG=500911,540503
==========

[zino, 2015-11-30 05:52:45]

jinho.bang@samsung.com changed reviewers:
+ tkent@chromium.org

[zino, 2015-11-30 05:57:31]

Thank you for review. PTAL again.

https://codereview.chromium.org/1481523006/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp
(right):

https://codereview.chromium.org/1481523006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:64:
class NavigateClientCallback : public WebServiceWorkerClientCallbacks {
On 2015/11/27 07:52:01, nhiroki (ooo back on Dec. 4) wrote:
> nit: can you move this class definition to line 22 and wrap it with anonymous
> namespace?

Done.

https://codereview.chromium.org/1481523006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerWindowClient.cpp:89:
private:
On 2015/11/27 07:52:01, nhiroki (ooo back on Dec. 4) wrote:
> nit: can you add a blank line before "private:"?

Done.

[zino, 2015-11-30 05:58:27]

+ tkent@ for Blink/public/*

PTAL.

[tkent, 2015-11-30 06:13:03]

On 2015/11/30 at 05:58:27, jinho.bang wrote:
> + tkent@ for Blink/public/*
> 
> PTAL.


third_party/WebKit/public/ lgtm.

[nhiroki, 2015-12-04 07:17:21]

LGTM, sorry for my delayed response.

[zino, 2015-12-04 19:40:15]

On 2015/12/04 07:17:21, nhiroki wrote:
> LGTM, sorry for my delayed response.

No problem. I already saw OOO in your id.

Thank you.

[zino, 2015-12-04 19:40:24]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2015-12-04 19:41:54]

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

[commit-bot: I haz the power, 2015-12-04 20:34:20]

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

[commit-bot: I haz the power, 2015-12-04 20:34:21]

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

[zino, 2015-12-05 02:03:17]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2015-12-05 02:08:33]

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

[commit-bot: I haz the power, 2015-12-05 03:29:00]

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

[commit-bot: I haz the power, 2015-12-05 03:29:01]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zino, 2015-12-05 07:01:53]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2015-12-05 07:03:02]

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

[commit-bot: I haz the power, 2015-12-05 08:23:54]

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

[commit-bot: I haz the power, 2015-12-05 08:23:55]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[zino, 2015-12-07 06:35:53]

Patchset #4 (id:60001) has been deleted

[zino, 2015-12-07 06:43:06]

On 2015/12/05 08:23:55, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

nhiroki@,

PTAL again?

I've just fixed trybot error.

[nhiroki, 2015-12-07 06:54:54]

lgtm

[zino, 2015-12-07 08:11:11]

The CQ bit was checked by jinho.bang@samsung.com

[zino, 2015-12-07 08:11:11]

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

[commit-bot: I haz the power, 2015-12-07 08:12:06]

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

[commit-bot: I haz the power, 2015-12-07 09:57:57]

Description was changed from

==========
ServiceWorker: Should throw TypeError instead of Unknown/SecurityError.

If navigate() or openWindow() faces errors during navigation, we should throw
TypeError instead of UnknownError or SecurityError. They aren't defined anywhere
in the spec[1] as well as, especially UnknownError is only used in IndexedDB[2].
To match the spec, this change adds ErrorTypeNavigation and then process it
via NavigateClientCallback instead of CallbackPromiseAdapter.

[1] http://slightlyoff.github.io/ServiceWorker/spec/service_worker
[2] http://www.w3.org/TR/IndexedDB/#h-exceptions

BUG=500911,540503
==========

to

==========
ServiceWorker: Should throw TypeError instead of Unknown/SecurityError.

If navigate() or openWindow() faces errors during navigation, we should throw
TypeError instead of UnknownError or SecurityError. They aren't defined anywhere
in the spec[1] as well as, especially UnknownError is only used in IndexedDB[2].
To match the spec, this change adds ErrorTypeNavigation and then process it
via NavigateClientCallback instead of CallbackPromiseAdapter.

[1] http://slightlyoff.github.io/ServiceWorker/spec/service_worker
[2] http://www.w3.org/TR/IndexedDB/#h-exceptions

BUG=500911,540503
==========

[commit-bot: I haz the power, 2015-12-07 09:57:58]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-12-07 09:59:04]

Description was changed from

==========
ServiceWorker: Should throw TypeError instead of Unknown/SecurityError.

If navigate() or openWindow() faces errors during navigation, we should throw
TypeError instead of UnknownError or SecurityError. They aren't defined anywhere
in the spec[1] as well as, especially UnknownError is only used in IndexedDB[2].
To match the spec, this change adds ErrorTypeNavigation and then process it
via NavigateClientCallback instead of CallbackPromiseAdapter.

[1] http://slightlyoff.github.io/ServiceWorker/spec/service_worker
[2] http://www.w3.org/TR/IndexedDB/#h-exceptions

BUG=500911,540503
==========

to

==========
ServiceWorker: Should throw TypeError instead of Unknown/SecurityError.

If navigate() or openWindow() faces errors during navigation, we should throw
TypeError instead of UnknownError or SecurityError. They aren't defined anywhere
in the spec[1] as well as, especially UnknownError is only used in IndexedDB[2].
To match the spec, this change adds ErrorTypeNavigation and then process it
via NavigateClientCallback instead of CallbackPromiseAdapter.

[1] http://slightlyoff.github.io/ServiceWorker/spec/service_worker
[2] http://www.w3.org/TR/IndexedDB/#h-exceptions

BUG=500911,540503

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

[commit-bot: I haz the power, 2015-12-07 09:59:05]

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