New issue
Advanced search Search tips
Starred by 9 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocked on:
issue 803751
issue 605917



Sign in to add a comment
link

Issue 592188: Notification resource fetches should be routed through the Service Worker

Reported by peter@chromium.org, Mar 5 2016 Project Member

Issue description

It looks like notification resource fetches currently bypass the Service Worker, whereas they shouldn't– they're just another fetch that can be attributed to the site.

Matt, we're fetching resources using a URLLoader created by blink::Platform. Does anything obvious come to mind that we should be doing differently in order to get the requests be considered by a Service Worker?
 

Comment 1 by mvanouwe...@chromium.org, Mar 7 2016

Components: Blink>ServiceWorker

Comment 2 by falken@chromium.org, Mar 7 2016

Yes probably a "clean slate" URLLoader created by blink::Platform wouldn't be recognized as something controlled by a service worker. For frames, this works by: ServiceWorkerNetworkProvider is attached to the frame in renderer-land, which sends ServiceWorkerHostMsg_ProviderCreated msg to the browser, browser creates a ServiceWorkerProviderHost that it associates with that frame, then ServiceWorkerRequestHandler observes loads from that frame and directs them to the service worker. That's a bit hand-wavy (and I never remember all the details) but you somehow want to get in that chain of events.

Can you point me to the code?

Comment 4 by falken@chromium.org, Mar 7 2016

Hmm yes I think this isn't currently supported. SW intercepts depend on a ServiceWorkerProviderHost being created, which is currently only done for windows and workers, not bare URLLoaders. I'm not sure now what would be a good solution here.

Comment 5 by mvanouwe...@chromium.org, Mar 7 2016

Is there some well known utility that does this correctly already e.g. something like ImageDownloaderImpl? Or is all usage of that also likely to miss service worker integration?

Comment 6 by falken@chromium.org, Mar 8 2016

I believe all that is missing SW integration.

Comment 7 by mvanouwe...@chromium.org, Mar 29 2016

Cc: -mvanouwe...@chromium.org
Owner: mvanouwe...@chromium.org
Status: Started (was: Available)

Comment 8 by bugdroid1@chromium.org, Apr 19 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f

commit 9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f
Author: mvanouwerkerk <mvanouwerkerk@chromium.org>
Date: Tue Apr 19 09:52:06 2016

Move notification resource loading from content/child to blink.

* The Notification and ServiceWorkerRegistrationNotifications classes now kick off the resource loading in blink.
* Add in blink: WebNotificationResources, NotificationImageLoader, NotificationResourcesLoader, NotificationResourcesLoaderTest
* Delete from content: NotificationImageLoader, PendingNotification, PendingNotificationsTracker, PendingNotificationsTrackerTest
* I'll leave restoring the UMA histogram reporting to a followup CL.
* Adds skia/ext to modules/DEPS for skia::ImageOperations

BUG=592188
TBR=kinuko

Review URL: https://codereview.chromium.org/1847863002

Cr-Commit-Position: refs/heads/master@{#388174}

[modify] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/content/child/blink_platform_impl.cc
[modify] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/content/child/notifications/notification_dispatcher.cc
[delete] https://crrev.com/e9e0b967e517853fa5ed179491a21427c9984069/content/child/notifications/notification_image_loader.cc
[delete] https://crrev.com/e9e0b967e517853fa5ed179491a21427c9984069/content/child/notifications/notification_image_loader.h
[modify] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/content/child/notifications/notification_manager.cc
[modify] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/content/child/notifications/notification_manager.h
[delete] https://crrev.com/e9e0b967e517853fa5ed179491a21427c9984069/content/child/notifications/pending_notification.cc
[delete] https://crrev.com/e9e0b967e517853fa5ed179491a21427c9984069/content/child/notifications/pending_notification.h
[delete] https://crrev.com/e9e0b967e517853fa5ed179491a21427c9984069/content/child/notifications/pending_notifications_tracker.cc
[delete] https://crrev.com/e9e0b967e517853fa5ed179491a21427c9984069/content/child/notifications/pending_notifications_tracker.h
[delete] https://crrev.com/e9e0b967e517853fa5ed179491a21427c9984069/content/child/notifications/pending_notifications_tracker_unittest.cc
[modify] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/content/content_child.gypi
[modify] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/content/content_tests.gypi
[modify] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/modules/DEPS
[modify] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/modules/modules.gypi
[modify] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/modules/notifications/Notification.cpp
[modify] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/modules/notifications/Notification.h
[add] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
[add] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h
[add] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp
[add] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.h
[add] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/modules/notifications/NotificationResourcesLoaderTest.cpp
[modify] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
[modify] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.h
[rename] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/web/tests/data/notifications/100x100.png
[rename] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/web/tests/data/notifications/110x110.png
[rename] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/web/tests/data/notifications/120x120.png
[rename] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/web/tests/data/notifications/48x48.png
[rename] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/Source/web/tests/data/notifications/500x500.png
[modify] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/public/blink_headers.gypi
[modify] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/public/platform/modules/notifications/WebNotificationManager.h
[add] https://crrev.com/9148e8aa500ceb93d3f1cb3abbd3b09afd65d12f/third_party/WebKit/public/platform/modules/notifications/WebNotificationResources.h

Comment 9 by mvanouwe...@chromium.org, Apr 22 2016

Blockedon: 605917

Comment 10 by bugdroid1@chromium.org, Apr 22 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/28a768838e7b3c1c9735035b7085610359f749f0

commit 28a768838e7b3c1c9735035b7085610359f749f0
Author: mvanouwerkerk <mvanouwerkerk@chromium.org>
Date: Fri Apr 22 13:46:59 2016

Add layout tests for notification icon with redirect loop.

BUG=592188

Review URL: https://codereview.chromium.org/1910723003

Cr-Commit-Position: refs/heads/master@{#389088}

[add] https://crrev.com/28a768838e7b3c1c9735035b7085610359f749f0/third_party/WebKit/LayoutTests/http/tests/notifications/serviceworkerregistration-document-image-redirect-loop.html
[add] https://crrev.com/28a768838e7b3c1c9735035b7085610359f749f0/third_party/WebKit/LayoutTests/http/tests/notifications/serviceworkerregistration-service-worker-image-redirect-loop.html
[rename] https://crrev.com/28a768838e7b3c1c9735035b7085610359f749f0/third_party/WebKit/LayoutTests/http/tests/resources/infinite-loop.php
[modify] https://crrev.com/28a768838e7b3c1c9735035b7085610359f749f0/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/XMLHttpRequestException-expected.txt
[modify] https://crrev.com/28a768838e7b3c1c9735035b7085610359f749f0/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/XMLHttpRequestException.html
[modify] https://crrev.com/28a768838e7b3c1c9735035b7085610359f749f0/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/connection-error-sync-expected.txt
[modify] https://crrev.com/28a768838e7b3c1c9735035b7085610359f749f0/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/connection-error-sync.html
[modify] https://crrev.com/28a768838e7b3c1c9735035b7085610359f749f0/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-error.html
[modify] https://crrev.com/28a768838e7b3c1c9735035b7085610359f749f0/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/onloadend-event-after-sync-requests.html

Comment 12 by bugdroid1@chromium.org, May 11 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1cdaa38724146a00b299b52fc2fc226049a3e363

commit 1cdaa38724146a00b299b52fc2fc226049a3e363
Author: mvanouwerkerk <mvanouwerkerk@chromium.org>
Date: Wed May 11 16:15:26 2016

Restore the histograms for notification resource loading.

These were lost when this functionality was ported from content to blink.

* Originally defined in https://codereview.chromium.org/1715763002
* Notifications.Icon.LoadFinishTime was originally defined using UMA_HISTOGRAM_LONG_TIMES so matches that configuration.
* Notifications.Icon.LoadFailTime was originally defined using UMA_HISTOGRAM_LONG_TIMES so matches that configuration.
* Notifications.Icon.ScaleDownTime was originally defined using SCOPED_UMA_HISTOGRAM_TIMER so matches that configuration.

BUG=592188

Review-Url: https://codereview.chromium.org/1904293003
Cr-Commit-Position: refs/heads/master@{#392943}

[modify] https://crrev.com/1cdaa38724146a00b299b52fc2fc226049a3e363/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
[modify] https://crrev.com/1cdaa38724146a00b299b52fc2fc226049a3e363/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.h
[modify] https://crrev.com/1cdaa38724146a00b299b52fc2fc226049a3e363/third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp

Comment 13 by bugdroid1@chromium.org, May 12 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2bf6a1f6379b1c1715ea5db0cb65d17e7884563b

commit 2bf6a1f6379b1c1715ea5db0cb65d17e7884563b
Author: peter <peter@chromium.org>
Date: Thu May 12 15:22:59 2016

Notification histograms should be thread-safe statics

These histograms will be counted on the context thread of the
notification, which can differ because notifications are available in
both documents and the various kinds of workers.

The histograms were re-introduced by the following CL:
    https://codereview.chromium.org/1904293003/

BUG=592188

Review-Url: https://codereview.chromium.org/1975803002
Cr-Commit-Position: refs/heads/master@{#393252}

[modify] https://crrev.com/2bf6a1f6379b1c1715ea5db0cb65d17e7884563b/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
[modify] https://crrev.com/2bf6a1f6379b1c1715ea5db0cb65d17e7884563b/third_party/WebKit/Source/modules/notifications/NotificationResourcesLoader.cpp

Comment 14 by mvanouwe...@chromium.org, Jun 22 2016

Owner: ----
Status: Available (was: Started)

Comment 15 by falken@chromium.org, Oct 5 2016

Labels: -Pri-2 Pri-3
Status: ExternalDependency (was: Available)
Seems like this is blocked on https://github.com/whatwg/fetch/issues/303

Comment 16 by peter@chromium.org, Oct 14 2016

Owner: jakearchibald@chromium.org
Yup. Let me assign this to Jake as we'd like to resolve this in Q4. Many developers are confused about this.

Comment 18 by peter@chromium.org, Feb 14 2017

Owner: peter@chromium.org

Comment 19 by horo@chromium.org, Feb 14 2017

Cc: horo@chromium.org

Comment 20 by falken@chromium.org, Feb 15 2017

Status: Assigned (was: ExternalDependency)

Comment 21 by bugdroid1@chromium.org, Feb 28 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/34669ef1214c412d168021ba04bce9ac1dce4bd5

commit 34669ef1214c412d168021ba04bce9ac1dce4bd5
Author: peter <peter@chromium.org>
Date: Tue Feb 28 01:11:14 2017

Rename SkipServiceWorker to ServiceWorkerMode

This was changed in the following PR to the Fetch standard:
  https://github.com/whatwg/fetch/pull/435

BUG=592188

Review-Url: https://codereview.chromium.org/2714423002
Cr-Commit-Position: refs/heads/master@{#453432}

[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/browser/service_worker/foreign_fetch_request_handler.cc
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/browser/service_worker/foreign_fetch_request_handler.h
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/browser/service_worker/foreign_fetch_request_handler_unittest.cc
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/child/web_url_loader_impl.cc
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/child/web_url_loader_impl.h
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/child/web_url_request_util.cc
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/child/web_url_request_util.h
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/common/resource_messages.h
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/common/resource_request.h
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/common/service_worker/service_worker_types.h
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/public/renderer/associated_resource_fetcher.h
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/renderer/fetchers/associated_resource_fetcher_impl.cc
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/renderer/fetchers/associated_resource_fetcher_impl.h
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/renderer/pepper/pepper_url_loader_host.cc
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/content/renderer/shared_worker/embedded_shared_worker_stub.cc
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/third_party/WebKit/Source/modules/fetch/FetchManager.cpp
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/third_party/WebKit/Source/platform/exported/WebURLRequest.cpp
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/third_party/WebKit/Source/platform/loader/fetch/CrossOriginAccessControl.cpp
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/third_party/WebKit/Source/platform/network/ResourceRequest.cpp
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/third_party/WebKit/Source/platform/network/ResourceRequest.h
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/third_party/WebKit/Source/platform/network/ResourceRequestTest.cpp
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/third_party/WebKit/Source/platform/testing/weburl_loader_mock.cc
[modify] https://crrev.com/34669ef1214c412d168021ba04bce9ac1dce4bd5/third_party/WebKit/public/platform/WebURLRequest.h

Comment 22 by peter@chromium.org, Jul 4 2017

Cc: jakearchibald@chromium.org peter@chromium.org kinuko@chromium.org
 Issue 605917  has been merged into this issue.

Comment 23 by falken@chromium.org, Apr 4 2018

Blockedon: 803751

Sign in to add a comment