New issue
Advanced search Search tips

Issue 592188 link

Starred by 9 users
Status: Assigned
Owner:
peter@chromium.org
Cc:
falken@chromium.org
jakearchibald@chromium.org
kinuko@chromium.org
peter@chromium.org
horo@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocked on:
issue 803751
issue 605917


Sign in to add a comment

Notification resource fetches should be routed through the Service Worker

Project Member Reported by peter@chromium.org, Mar 5 2016 
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)
Project Member

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

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
Project Member

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

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
Project Member

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

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
Project Member

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

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)
Project Member

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

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