New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 908955 link

Starred by 5 users

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task



Sign in to add a comment

Get rid of ResourceContext

Project Member Reported by jam@chromium.org, Nov 27

Issue description

This is hardly used anymore with the conversion of the code to work with network service. Now seems like a good time to get rid of it (it only has one method left, although it does seem to act as a key sometimes).

Once the network service is the only code path that's used, then I suspect most of the code which runs on the IO thread can move to the UI thread. This would remove unnecessary thread hops (i.e. on navigations to talk to service worker and appcache code that lives on IO thread) and also simplify the code (e.g. to stop mirroring data for features on multiple threads, such as Instant or Extensions). We could then get rid of ResourceContext and ProfileIOData.
 
Cc: michaeln@chromium.org rdevlin....@chromium.org kinuko@chromium.org mmenke@chromium.org
 Issue 529896  has been merged into this issue.
Cc: creis@chromium.org nasko@chromium.org
 Issue 537848  has been merged into this issue.
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6fee69011d64245a957ef9490a8ccac922aae6f3

commit 6fee69011d64245a957ef9490a8ccac922aae6f3
Author: John Abd-El-Malek <jam@chromium.org>
Date: Wed Nov 28 18:30:57 2018

Stop using ResourceContext::GetRequestContext in preparation of getting rid of ResourceContext.

After this change there are no more callers. The next cl will remove the implementations of that
method.

Bug: 908955
Change-Id: Ie424e9d6be5d6bb8702962198c61e718968ac2a7
Reviewed-on: https://chromium-review.googlesource.com/c/1352510
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611786}
[modify] https://crrev.com/6fee69011d64245a957ef9490a8ccac922aae6f3/chrome/browser/extensions/extension_protocols_unittest.cc
[modify] https://crrev.com/6fee69011d64245a957ef9490a8ccac922aae6f3/content/browser/appcache/chrome_appcache_service_unittest.cc
[modify] https://crrev.com/6fee69011d64245a957ef9490a8ccac922aae6f3/content/browser/loader/mojo_async_resource_handler_unittest.cc
[modify] https://crrev.com/6fee69011d64245a957ef9490a8ccac922aae6f3/content/browser/loader/navigation_url_loader_unittest.cc
[modify] https://crrev.com/6fee69011d64245a957ef9490a8ccac922aae6f3/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/6fee69011d64245a957ef9490a8ccac922aae6f3/content/browser/loader/resource_dispatcher_host_unittest.cc
[modify] https://crrev.com/6fee69011d64245a957ef9490a8ccac922aae6f3/content/browser/loader/resource_loader_unittest.cc
[modify] https://crrev.com/6fee69011d64245a957ef9490a8ccac922aae6f3/content/browser/loader/resource_requester_info.cc
[modify] https://crrev.com/6fee69011d64245a957ef9490a8ccac922aae6f3/content/browser/loader/resource_requester_info.h
[modify] https://crrev.com/6fee69011d64245a957ef9490a8ccac922aae6f3/content/browser/loader/url_loader_factory_impl_unittest.cc
[modify] https://crrev.com/6fee69011d64245a957ef9490a8ccac922aae6f3/content/browser/service_worker/service_worker_fetch_dispatcher.cc
[modify] https://crrev.com/6fee69011d64245a957ef9490a8ccac922aae6f3/extensions/browser/content_verify_job_unittest.cc

Description: Show this description
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8104d670b52c6381a6157e60800268e7fc0d5a35

commit 8104d670b52c6381a6157e60800268e7fc0d5a35
Author: John Abd-El-Malek <jam@chromium.org>
Date: Wed Nov 28 20:27:03 2018

Remove ResourceContext::GetRequestContext since it's no longer used.

Also remove a bunch of usages of MockResourceContext that are no longer needed.

Bug: 908955
Change-Id: I3485595c83ef8e22f245c22c97775845c5e0e7fc
Reviewed-on: https://chromium-review.googlesource.com/c/1352035
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611852}
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/android_webview/browser/aw_browser_context.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/android_webview/browser/aw_resource_context.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/android_webview/browser/aw_resource_context.h
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/chrome/browser/data_use_measurement/chrome_data_use_ascriber_unittest.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/chrome/browser/extensions/extension_protocols_unittest.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/chrome/browser/profiles/profile_io_data.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/chrome/browser/profiles/profile_io_data.h
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/chrome/browser/search/iframe_source_unittest.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/chromecast/browser/cast_browser_context.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/content/browser/appcache/chrome_appcache_service_unittest.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/content/browser/loader/resource_loader_unittest.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/content/public/browser/resource_context.h
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/content/public/test/mock_resource_context.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/content/public/test/mock_resource_context.h
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/content/public/test/test_browser_context.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/content/shell/browser/shell_browser_context.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/content/shell/browser/shell_browser_context.h
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/extensions/browser/content_verify_job_unittest.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/extensions/shell/browser/shell_browser_context.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/headless/lib/browser/headless_request_context_manager.cc
[modify] https://crrev.com/8104d670b52c6381a6157e60800268e7fc0d5a35/webrunner/browser/webrunner_browser_context.cc

Great work, John!  I've long wanted to see that gone.
Cc: acolwell@chromium.org alex...@chromium.org lukasza@chromium.org
Components: Internals>Sandbox>SiteIsolation
Comment 4: Wow, that's really interesting about the IO thread!  On the Site Isolation team, we've been looking at how to make security checks on the IO thread when they might depend on the profile (and we'd been considering using ResourceContext for things like looking up extensions).  It would be vastly preferable to have all the checks on the UI thread if all the IO thread code moved over.

Sounds like we should keep in touch about how to migrate towards that.  Maybe there's a way some of the checks can start moving before the IO thread itself is gone...
@creis: yeah, it would be great to look at each callsite one-by-one to see which ones can move now. I suspect a bunch can move already.

Sign in to add a comment