service worker: Clarify the navigation preload network error message.

service worker: Clarify the navigation preload network error message.

BUG=649558
Review-Url: https://codereview.chromium.org/2812513004
Cr-Commit-Position: refs/heads/master@{#463984}
Committed: https://chromium.googlesource.com/chromium/src/+/946fc08307c66bbece7eb0f54d08cd864b1fd7d9

[falken, 2017-04-11 08:54:20]

Description was changed from

==========
service worker: Clarify the navigation preload network error message

BUG=649558
==========

to

==========
service worker: Clarify the navigation preload network error message.

BUG=649558
==========

[falken, 2017-04-11 08:55:05]

falken@chromium.org changed reviewers:
+ horo@chromium.org

[falken, 2017-04-11 08:55:05]

ptal

I considered removing the abort error message completely, but the error message
code seems to be involved in object lifetime so I decided not to change it.

[falken, 2017-04-11 08:55:11]

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

[commit-bot: I haz the power, 2017-04-11 08:55:33]

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, 2017-04-11 10:15:01]

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

[commit-bot: I haz the power, 2017-04-11 10:15:02]

Dry run: This issue passed the CQ dry run.

[horo, 2017-04-12 00:32:03]

https://codereview.chromium.org/2812513004/diff/1/content/renderer/service_wo...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2812513004/diff/1/content/renderer/service_wo...
content/renderer/service_worker/service_worker_context_client.cc:438:
status.error_code);
I think we should not expose the error code for security and privacy reason.
Is there any other JS API which expose the network error code?

[falken, 2017-04-12 00:53:52]

On 2017/04/12 00:32:03, horo wrote:
>
https://codereview.chromium.org/2812513004/diff/1/content/renderer/service_wo...
> File content/renderer/service_worker/service_worker_context_client.cc (right):
> 
>
https://codereview.chromium.org/2812513004/diff/1/content/renderer/service_wo...
> content/renderer/service_worker/service_worker_context_client.cc:438:
> status.error_code);
> I think we should not expose the error code for security and privacy reason.
> Is there any other JS API which expose the network error code?

Hm, I didn't think about that. I'm not trying to expose to JS, I just want a
somewhat debuggable error message displayed to the developer so at least when we
get a report we can investigate.

How can I display the error code on the console. Should FetchDispatcher try to
call EmbeddedWorkerInstance::AddMessageToConsole? Or should
ServiceWorkerContextClient call ReportException/ReportConsoleMessage here? Could
ServiceWorkerContextClient call AddMessageToConsole somehow?

[horo, 2017-04-12 03:38:06]

On 2017/04/12 00:53:52, falken wrote:
> On 2017/04/12 00:32:03, horo wrote:
> >
>
https://codereview.chromium.org/2812513004/diff/1/content/renderer/service_wo...
> > File content/renderer/service_worker/service_worker_context_client.cc
(right):
> > 
> >
>
https://codereview.chromium.org/2812513004/diff/1/content/renderer/service_wo...
> > content/renderer/service_worker/service_worker_context_client.cc:438:
> > status.error_code);
> > I think we should not expose the error code for security and privacy reason.
> > Is there any other JS API which expose the network error code?
> 
> Hm, I didn't think about that. I'm not trying to expose to JS, I just want a
> somewhat debuggable error message displayed to the developer so at least when
we
> get a report we can investigate.
> 
> How can I display the error code on the console. Should FetchDispatcher try to
> call EmbeddedWorkerInstance::AddMessageToConsole? Or should
> ServiceWorkerContextClient call ReportException/ReportConsoleMessage here?
Could
> ServiceWorkerContextClient call AddMessageToConsole somehow?

You can call WorkerGlobalScope::AddConsoleMessage() from
ServiceWorkerGlobalScopeProxy::OnNavigationPreloadError().

[falken, 2017-04-12 07:10:43]

revised, ptal

[falken, 2017-04-12 08:03:17]

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

[commit-bot: I haz the power, 2017-04-12 08:03:37]

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

[horo, 2017-04-12 08:16:05]

lgtm with a nit.

https://codereview.chromium.org/2812513004/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2812513004/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:420:
"'preloadResponse' settled. If you intend to use "
nit: strange line break.

[falken, 2017-04-12 08:22:22]

falken@chromium.org changed reviewers:
+ keishi@chromium.org

[falken, 2017-04-12 08:22:23]

keishi: can you review web?

[keishi, 2017-04-12 08:34:38]

web/ LGTM

[falken, 2017-04-12 08:38:17]

The CQ bit was checked by falken@chromium.org

[falken, 2017-04-12 08:38:17]

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

[commit-bot: I haz the power, 2017-04-12 08:38:28]

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-04-12 10:26:53]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491986297336770,
"parent_rev": "0dec02be917eae770b54eb8438ee9ac2d9ee249b", "commit_rev":
"946fc08307c66bbece7eb0f54d08cd864b1fd7d9"}

[commit-bot: I haz the power, 2017-04-12 10:27:46]

Description was changed from

==========
service worker: Clarify the navigation preload network error message.

BUG=649558
==========

to

==========
service worker: Clarify the navigation preload network error message.

BUG=649558

Review-Url: https://codereview.chromium.org/2812513004
Cr-Commit-Position: refs/heads/master@{#463984}
Committed:
https://chromium.googlesource.com/chromium/src/+/946fc08307c66bbece7eb0f54d08...
==========

[commit-bot: I haz the power, 2017-04-12 10:27:48]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/946fc08307c66bbece7eb0f54d08...