Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2108643004:
Remove use of WebContentsImpl from ResourceDispatcherHostImpl (Closed)
|
Created:
4 years, 5 months ago by scottmg Modified:
4 years, 5 months ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, Randy Smith (Not in Mondays), darin-cc_chromium.org, loading-reviews_chromium.org, mmenke Base URL:
https://chromium.googlesource.com/chromium/src.git@render-view-try-3 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove use of WebContentsImpl from ResourceDispatcherHostImpl
To reduce dependencies from content/browser/loader to the rest of
content. Follows https://codereview.chromium.org/2098923002/.
R=mmenke@chromium.org, jam@chromium.org
BUG=598073
Committed: https://crrev.com/e22a5e5114c66d39894c54e4ce7e8d4400a53d81
Cr-Commit-Position: refs/heads/master@{#403036}
Patch Set 1 #
Total comments: 18
Patch Set 2 : fixes #
Total comments: 2
Patch Set 3 : fix comment #Depends on Patchset: Messages
Total messages: 25 (10 generated)
Description was changed from ========== Remove use of WebContentsImpl from ResourceDispatcherHostImpl R=mmenke@chromium.org BUG=598073 ========== to ========== Remove use of WebContentsImpl from ResourceDispatcherHostImpl Follows https://codereview.chromium.org/2098923002/. R=mmenke@chromium.org BUG=598073 ==========
Description was changed from ========== Remove use of WebContentsImpl from ResourceDispatcherHostImpl Follows https://codereview.chromium.org/2098923002/. R=mmenke@chromium.org BUG=598073 ========== to ========== Remove use of WebContentsImpl from ResourceDispatcherHostImpl Follows https://codereview.chromium.org/2098923002/. R=mmenke@chromium.org, jam BUG=598073 ==========
scottmg@chromium.org changed reviewers: + jam@chromium.org
Description was changed from ========== Remove use of WebContentsImpl from ResourceDispatcherHostImpl Follows https://codereview.chromium.org/2098923002/. R=mmenke@chromium.org, jam BUG=598073 ========== to ========== Remove use of WebContentsImpl from ResourceDispatcherHostImpl Follows https://codereview.chromium.org/2098923002/. R=mmenke@chromium.org, jam@chromium.org BUG=598073 ==========
Description was changed from ========== Remove use of WebContentsImpl from ResourceDispatcherHostImpl Follows https://codereview.chromium.org/2098923002/. R=mmenke@chromium.org, jam@chromium.org BUG=598073 ========== to ========== Remove use of WebContentsImpl from ResourceDispatcherHostImpl To reduce dependencies from content/browser/loader to the rest of content. Follows https://codereview.chromium.org/2098923002/. R=mmenke@chromium.org, jam@chromium.org BUG=598073 ==========
mmenke: c/b/loader/* jam: web_contents_impl.cc
quick comments. https://codereview.chromium.org/2108643004/diff/1/content/browser/loader/load... File content/browser/loader/loader_delegate.h (right): https://codereview.chromium.org/2108643004/diff/1/content/browser/loader/load... content/browser/loader/loader_delegate.h:20: // Delegate from loader to the rest of content. Lives on the IO thread. "Lives on the IO thread", yet the implementation is making certain assumptions about its lifetime on the UI thread. I'm not sure that really falls under the definition of "lives" https://codereview.chromium.org/2108643004/diff/1/content/browser/loader/load... content/browser/loader/loader_delegate.h:25: // since eventually this will be a Mojo interface. See https://crbug.com/622050 In a mojo world, will we still be using render_process_id / render_frame_hosts, or should Mojo eventually take care of that (Not sure how we'd do that - a delegate per frame host?). Just wondering what magic IDs we'll need to keep around, and thinking about requests that won't have either ID, as they're not from a renderer...And requests that will have a child_id and route_id, but no render_frame_host, if they're from another type of process. https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... File content/browser/loader_delegate_impl.cc (right): https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... content/browser/loader_delegate_impl.cc:22: uint64_t upload_size) { These should all DCHECK what thread we're on, for documentation purposes, and as a protection against regressions. https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... content/browser/loader_delegate_impl.cc:48: base::Unretained(this), render_process_id, render_frame_host, What guarantees that the loader hasn't been deleted by the time this task is executed?
https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... File content/browser/loader_delegate_impl.cc (right): https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... content/browser/loader_delegate_impl.cc:48: base::Unretained(this), render_process_id, render_frame_host, On 2016/06/29 15:17:13, mmenke wrote: > What guarantees that the loader hasn't been deleted by the time this task is > executed? Actually...Everything we do on the UI thread can just be static, can't it? Or better, moved into an anonymous namespace, and out of the header. So we don't even need to use |this|, or make assumptions about its lifetimes, as long as it outlives the RDH... Which I don't believe it actually does. What are the lifetime assumptions RDH makes about the LoaderDelegate?
lgtm https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... File content/browser/loader_delegate_impl.cc (right): https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... content/browser/loader_delegate_impl.cc:22: uint64_t upload_size) { On 2016/06/29 15:17:13, mmenke wrote: > These should all DCHECK what thread we're on, for documentation purposes, and as > a protection against regressions. IMO that's needless, i.e. we don't dcheck every single method in content/browser/loader that runs on the IO thread. Public methods that are called by embedders etc are useful to dcheck to make sure they're not calling it incorrectly. but for a delegate like this that's called by RDH methods, we know which threads will call it. https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... content/browser/loader_delegate_impl.cc:48: base::Unretained(this), render_process_id, render_frame_host, On 2016/06/29 15:53:48, mmenke wrote: > On 2016/06/29 15:17:13, mmenke wrote: > > What guarantees that the loader hasn't been deleted by the time this task is > > executed? > > Actually...Everything we do on the UI thread can just be static, can't it? Or > better, moved into an anonymous namespace, and out of the header. So we don't > even need to use |this|, or make assumptions about its lifetimes, as long as it > outlives the RDH... Which I don't believe it actually does. What are the > lifetime assumptions RDH makes about the LoaderDelegate? +1 to moving these methods to anonymous namespace https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... File content/browser/loader_delegate_impl.h (right): https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... content/browser/loader_delegate_impl.h:23: nit: here and below, no blank lines between overridden methods per convention
https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... File content/browser/loader_delegate_impl.cc (right): https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... content/browser/loader_delegate_impl.cc:22: uint64_t upload_size) { On 2016/06/29 17:19:45, jam wrote: > On 2016/06/29 15:17:13, mmenke wrote: > > These should all DCHECK what thread we're on, for documentation purposes, and > as > > a protection against regressions. > > IMO that's needless, i.e. we don't dcheck every single method in > content/browser/loader that runs on the IO thread. Public methods that are > called by embedders etc are useful to dcheck to make sure they're not calling it > incorrectly. but for a delegate like this that's called by RDH methods, we know > which threads will call it. I agree that classes that run on a single thread don't need DCHECKs. This class, however, runs on multiple threads, and is even created on, destroyed on, and owned by an object on a thread that's different from the one it's consumed on, so I think that it's a good idea to have them. They cost us very little, and they protect it from misuse.
Patchset #2 (id:20001) has been deleted
Thanks! https://codereview.chromium.org/2108643004/diff/1/content/browser/loader/load... File content/browser/loader/loader_delegate.h (right): https://codereview.chromium.org/2108643004/diff/1/content/browser/loader/load... content/browser/loader/loader_delegate.h:20: // Delegate from loader to the rest of content. Lives on the IO thread. On 2016/06/29 15:17:12, mmenke wrote: > "Lives on the IO thread", yet the implementation is making certain assumptions > about its lifetime on the UI thread. I'm not sure that really falls under the > definition of "lives" "[s]hould be interacted with" maybe? I added a comment to SetLoaderDelegate() in RDHI too to attempt to clarify. https://codereview.chromium.org/2108643004/diff/1/content/browser/loader/load... content/browser/loader/loader_delegate.h:25: // since eventually this will be a Mojo interface. See https://crbug.com/622050 On 2016/06/29 15:17:13, mmenke wrote: > In a mojo world, will we still be using render_process_id / render_frame_hosts, > or should Mojo eventually take care of that (Not sure how we'd do that - a > delegate per frame host?). > > Just wondering what magic IDs we'll need to keep around, and thinking about > requests that won't have either ID, as they're not from a renderer...And > requests that will have a child_id and route_id, but no render_frame_host, if > they're from another type of process. I... have no idea :) I do find all the styles of int IDs a bit confusing now, so it'd be nice if in the future things could be more directly connected to each other in the future to avoid having a manager that dispatches amongst them. https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... File content/browser/loader_delegate_impl.cc (right): https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... content/browser/loader_delegate_impl.cc:22: uint64_t upload_size) { On 2016/06/29 15:17:13, mmenke wrote: > These should all DCHECK what thread we're on, for documentation purposes, and as > a protection against regressions. Done. (I agree with jam that it's pretty clear right now, but it's low cost, and will likely be helpful in the future as this delegate grows more methods) https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... content/browser/loader_delegate_impl.cc:48: base::Unretained(this), render_process_id, render_frame_host, On 2016/06/29 15:53:48, mmenke wrote: > On 2016/06/29 15:17:13, mmenke wrote: > > What guarantees that the loader hasn't been deleted by the time this task is > > executed? > > Actually...Everything we do on the UI thread can just be static, can't it? Or > better, moved into an anonymous namespace, and out of the header. So we don't > even need to use |this|, or make assumptions about its lifetimes, as long as it > outlives the RDH... Which I don't believe it actually does. What are the > lifetime assumptions RDH makes about the LoaderDelegate? LoaderDelegate does need to outlive the RDH, I added a comment to RDH::SetLoaderDelegate() to that effect. But obviously no point here as you say, so moved to statics. https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... File content/browser/loader_delegate_impl.h (right): https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... content/browser/loader_delegate_impl.h:23: On 2016/06/29 17:19:46, jam wrote: > nit: here and below, no blank lines between overridden methods per convention Done.
https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... File content/browser/loader_delegate_impl.cc (right): https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... content/browser/loader_delegate_impl.cc:22: uint64_t upload_size) { On 2016/06/29 17:52:52, mmenke wrote: > On 2016/06/29 17:19:45, jam wrote: > > On 2016/06/29 15:17:13, mmenke wrote: > > > These should all DCHECK what thread we're on, for documentation purposes, > and > > as > > > a protection against regressions. > > > > IMO that's needless, i.e. we don't dcheck every single method in > > content/browser/loader that runs on the IO thread. Public methods that are > > called by embedders etc are useful to dcheck to make sure they're not calling > it > > incorrectly. but for a delegate like this that's called by RDH methods, we > know > > which threads will call it. > > I agree that classes that run on a single thread don't need DCHECKs. > > This class, however, runs on multiple threads, and is even created on, destroyed > on, and owned by an object on a thread that's different from the one it's > consumed on, so I think that it's a good idea to have them. They cost us very > little, and they protect it from misuse. Regarding running on multiple threads, that's not the case once we remove the static methods to anonymous namespace. Regarding construction/destruction, that's also similar to RDH and many other classes in content that live on the IO thread but are just created on the UI thread (i.e. for example all message filters). I do not think it improves the code quality by putting DCHECKs on all their methods.
https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... File content/browser/loader_delegate_impl.cc (right): https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... content/browser/loader_delegate_impl.cc:22: uint64_t upload_size) { On 2016/06/29 18:13:23, jam wrote: > On 2016/06/29 17:52:52, mmenke wrote: > > On 2016/06/29 17:19:45, jam wrote: > > > On 2016/06/29 15:17:13, mmenke wrote: > > > > These should all DCHECK what thread we're on, for documentation purposes, > > and > > > as > > > > a protection against regressions. > > > > > > IMO that's needless, i.e. we don't dcheck every single method in > > > content/browser/loader that runs on the IO thread. Public methods that are > > > called by embedders etc are useful to dcheck to make sure they're not > calling > > it > > > incorrectly. but for a delegate like this that's called by RDH methods, we > > know > > > which threads will call it. > > > > I agree that classes that run on a single thread don't need DCHECKs. > > > > This class, however, runs on multiple threads, and is even created on, > destroyed > > on, and owned by an object on a thread that's different from the one it's > > consumed on, so I think that it's a good idea to have them. They cost us very > > little, and they protect it from misuse. > > Regarding running on multiple threads, that's not the case once we remove the > static methods to anonymous namespace. > > Regarding construction/destruction, that's also similar to RDH and many other > classes in content that live on the IO thread but are just created on the UI > thread (i.e. for example all message filters). I do not think it improves the > code quality by putting DCHECKs on all their methods. RDH was being used in a non-threadsafe manner on the UI thread until quite recently. DCHECKs would have prevented that.
https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... File content/browser/loader_delegate_impl.cc (right): https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... content/browser/loader_delegate_impl.cc:22: uint64_t upload_size) { On 2016/06/29 18:20:51, mmenke wrote: > On 2016/06/29 18:13:23, jam wrote: > > On 2016/06/29 17:52:52, mmenke wrote: > > > On 2016/06/29 17:19:45, jam wrote: > > > > On 2016/06/29 15:17:13, mmenke wrote: > > > > > These should all DCHECK what thread we're on, for documentation > purposes, > > > and > > > > as > > > > > a protection against regressions. > > > > > > > > IMO that's needless, i.e. we don't dcheck every single method in > > > > content/browser/loader that runs on the IO thread. Public methods that are > > > > called by embedders etc are useful to dcheck to make sure they're not > > calling > > > it > > > > incorrectly. but for a delegate like this that's called by RDH methods, we > > > know > > > > which threads will call it. > > > > > > I agree that classes that run on a single thread don't need DCHECKs. > > > > > > This class, however, runs on multiple threads, and is even created on, > > destroyed > > > on, and owned by an object on a thread that's different from the one it's > > > consumed on, so I think that it's a good idea to have them. They cost us > very > > > little, and they protect it from misuse. > > > > Regarding running on multiple threads, that's not the case once we remove the > > static methods to anonymous namespace. > > > > Regarding construction/destruction, that's also similar to RDH and many other > > classes in content that live on the IO thread but are just created on the UI > > thread (i.e. for example all message filters). I do not think it improves the > > code quality by putting DCHECKs on all their methods. > > RDH was being used in a non-threadsafe manner on the UI thread until quite > recently. DCHECKs would have prevented that. We may be saying the same thing.. i.e. in the bug you mention, who was calling? if it's public RDH methods that are called by other code in content or chrome, then adding dchecks sgtm as I mentioned earlier. but let's say there are internal methods in RDH (or resource loader or RDHDelegate) that are only called by RDH, then that's specifically what I'm referring to.
https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... File content/browser/loader_delegate_impl.cc (right): https://codereview.chromium.org/2108643004/diff/1/content/browser/loader_dele... content/browser/loader_delegate_impl.cc:22: uint64_t upload_size) { On 2016/06/29 20:36:48, jam wrote: > On 2016/06/29 18:20:51, mmenke wrote: > > On 2016/06/29 18:13:23, jam wrote: > > > On 2016/06/29 17:52:52, mmenke wrote: > > > > On 2016/06/29 17:19:45, jam wrote: > > > > > On 2016/06/29 15:17:13, mmenke wrote: > > > > > > These should all DCHECK what thread we're on, for documentation > > purposes, > > > > and > > > > > as > > > > > > a protection against regressions. > > > > > > > > > > IMO that's needless, i.e. we don't dcheck every single method in > > > > > content/browser/loader that runs on the IO thread. Public methods that > are > > > > > called by embedders etc are useful to dcheck to make sure they're not > > > calling > > > > it > > > > > incorrectly. but for a delegate like this that's called by RDH methods, > we > > > > know > > > > > which threads will call it. > > > > > > > > I agree that classes that run on a single thread don't need DCHECKs. > > > > > > > > This class, however, runs on multiple threads, and is even created on, > > > destroyed > > > > on, and owned by an object on a thread that's different from the one it's > > > > consumed on, so I think that it's a good idea to have them. They cost us > > very > > > > little, and they protect it from misuse. > > > > > > Regarding running on multiple threads, that's not the case once we remove > the > > > static methods to anonymous namespace. > > > > > > Regarding construction/destruction, that's also similar to RDH and many > other > > > classes in content that live on the IO thread but are just created on the UI > > > thread (i.e. for example all message filters). I do not think it improves > the > > > code quality by putting DCHECKs on all their methods. > > > > RDH was being used in a non-threadsafe manner on the UI thread until quite > > recently. DCHECKs would have prevented that. > > We may be saying the same thing.. i.e. in the bug you mention, who was calling? > if it's public RDH methods that are called by other code in content or chrome, > then adding dchecks sgtm as I mentioned earlier. but let's say there are > internal methods in RDH (or resource loader or RDHDelegate) that are only called > by RDH, then that's specifically what I'm referring to. They were being called by an external consumer.
Just one comment, otherwise, LGTM https://codereview.chromium.org/2108643004/diff/40001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2108643004/diff/40001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:311: // and on the IO thread. Not sure what you mean by "asynchronously". If you mean the LoaderDelegate shouldn't block tasks, that seems a propertly of the delegate's API, not the RDH method to set it.
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2108643004/#ps60001 (title: "fix comment")
Thanks https://codereview.chromium.org/2108643004/diff/40001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/2108643004/diff/40001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:311: // and on the IO thread. On 2016/06/29 22:05:20, mmenke wrote: > Not sure what you mean by "asynchronously". If you mean the LoaderDelegate > shouldn't block tasks, that seems a propertly of the delegate's API, not the RDH > method to set it. I was trying to get across that it should eventually represent talking to another process, but you're right that that's a property of the delegate api (e.g. shouldn't have methods that return the values of computations), so removed here.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove use of WebContentsImpl from ResourceDispatcherHostImpl To reduce dependencies from content/browser/loader to the rest of content. Follows https://codereview.chromium.org/2098923002/. R=mmenke@chromium.org, jam@chromium.org BUG=598073 ========== to ========== Remove use of WebContentsImpl from ResourceDispatcherHostImpl To reduce dependencies from content/browser/loader to the rest of content. Follows https://codereview.chromium.org/2098923002/. R=mmenke@chromium.org, jam@chromium.org BUG=598073 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Remove use of WebContentsImpl from ResourceDispatcherHostImpl To reduce dependencies from content/browser/loader to the rest of content. Follows https://codereview.chromium.org/2098923002/. R=mmenke@chromium.org, jam@chromium.org BUG=598073 ========== to ========== Remove use of WebContentsImpl from ResourceDispatcherHostImpl To reduce dependencies from content/browser/loader to the rest of content. Follows https://codereview.chromium.org/2098923002/. R=mmenke@chromium.org, jam@chromium.org BUG=598073 Committed: https://crrev.com/e22a5e5114c66d39894c54e4ce7e8d4400a53d81 Cr-Commit-Position: refs/heads/master@{#403036} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e22a5e5114c66d39894c54e4ce7e8d4400a53d81 Cr-Commit-Position: refs/heads/master@{#403036} |
