Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737343002:
[Offline Pages] Allow BackgroundLoader to track network bytes using prerenderer hook-in. (Closed)
|
Created:
3 years, 9 months ago by chili Modified:
3 years, 9 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, Randy Smith (Not in Mondays), petewil+watch_chromium.org, chili+watch_chromium.org, loading-reviews_chromium.org, mmenke, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Allow BackgroundLoader to track network bytes using prerenderer hook-in.
While this hook-in is less than ideal, allow us to unblock while architecting a better solution that will account for bytes, does not block net moving OOP, and at a rate frequent enough to be suitable.
BUG=697695
Review-Url: https://codereview.chromium.org/2737343002
Cr-Commit-Position: refs/heads/master@{#457163}
Committed: https://chromium.googlesource.com/chromium/src/+/f2fc623943d585acb44512ebafb626acd7b3f217
Patch Set 1 #
Total comments: 2
Patch Set 2 : move offliner tracking to separate data #Patch Set 3 : mark background contents code as android-only #
Total comments: 10
Patch Set 4 : code review #
Total comments: 2
Patch Set 5 : move offline pages update out of prerender method #
Messages
Total messages: 39 (22 generated)
chili@chromium.org changed reviewers: + dewittj@chromium.org, dimich@chromium.org
manually tested to work on my nexus 5. This should give us a little bit to consider other (hopefully better) options.
https://codereview.chromium.org/2737343002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.h (right): https://codereview.chromium.org/2737343002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.h:64: static BackgroundLoaderOffliner* offliner_; Perhaps this can be avoided by setting the BackgroundLoaderOffliner as a webContents::SetUserData()? Then it can be pulled back from WebContents. See: https://cs.chromium.org/chromium/src/content/public/browser/web_contents_user...
https://codereview.chromium.org/2737343002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2737343002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:49: content::WebContents* contents) { It might be better to get it this way: auto context = contents->GetBrowserContext(); auto request_coordinator = RCFactory::GetForBrowserContext(context); auto offliner = request_coordinator->GetOffliner() Then you don't have dangling static pointers to worry about.
Updated according to dmitry's suggestion. PTAL
The CQ bit was checked by chili@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chili@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chili@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'll let Dmitry comment on the new approach but this still feels fragile and somewhat indirect. My feeling is tied to the uncertain lifetime of this object and its relationship to any given web contents. Also that this object is being treated like a singleton but referenced with raw pointers rather than using a singleton getter or factory interface.
On 2017/03/10 22:41:21, dewittj wrote: > I'll let Dmitry comment on the new approach but this still feels fragile and > somewhat indirect. My feeling is tied to the uncertain lifetime of this object > and its relationship to any given web contents. Also that this object is being > treated like a singleton but referenced with raw pointers rather than using a > singleton getter or factory interface. I don't think this object is being treated like a singleton in the new approach: The Offliner's lifetime should be at least as long as the web content's lifetime. This means that each webcontents can be tied to *at most* one Offliner. In the new approach, we're tying the webcontents to the corresponding offliner when it's created via SetUserData. This is irregardless of how many offliners there are, as different offliners should not share the same webcontents. When we receive a webcontents, we try to get the corresponding offliner by reading the UserData and retrieving the offliner. I agree that it's fragile in that if the offliner somehow crashes and dies before the webcontents, we might end up with an invalid pointer. However, if the Offliner dies, its destructor will destroy the BackgroundLoaderContents which will kill the WebContents. If the order of destruction causes problems, it will cause problems in other parts of the code too, so this doesn't make the code *worse*
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm modulo nits https://codereview.chromium.org/2737343002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2737343002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:34: BackgroundLoaderOffliner* offliner_; Please write a comment explaining the reason that lifetime is safe here. https://codereview.chromium.org/2737343002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:380: loader_.get()->web_contents()->SetUserData(kOfflinerDataKey, nit: combine this (loader_.get()->web_contents()) and above into a single call
lgtm with 2 nits: https://codereview.chromium.org/2737343002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2737343002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:24: const char kOfflinerDataKey[] = "offliner-key"; An even better variant of this is to do that: const int kOfflinerDataKey; ... contents->SetUserData(&kOfflinerDataKey); because the actual content of the string is not used anyways (the SetUserData uses a void* passed in). https://codereview.chromium.org/2737343002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:34: BackgroundLoaderOffliner* offliner_; Would be nice to add a comment to describe why the raw ptr is assumed always pointing to alive object here.
https://codereview.chromium.org/2737343002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2737343002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:281: SavePageRequest request(*pending_request_.get()); Last I remember, .get() is not required to dereference a unique ptr. Also, there is no reason to copy the SavePageRequest in this callback, correct?
Description was changed from ========== [Offline Pages] Allow BackgroundLoader to track network bytes using prerenderer hook-in. While this hook-in is less than ideal, allow us to unblock while architecting a better solution that will account for bytes, does not block net moving OOP, and at a rate frequent enough to be suitable. BUG=697695 ========== to ========== [Offline Pages] Allow BackgroundLoader to track network bytes using prerenderer hook-in. While this hook-in is less than ideal, allow us to unblock while architecting a better solution that will account for bytes, does not block net moving OOP, and at a rate frequent enough to be suitable. BUG=697695 ==========
chili@chromium.org changed reviewers: + jochen@chromium.org
+jochen for chrome_resource_dispatcher_host_delegate changes https://codereview.chromium.org/2737343002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2737343002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:24: const char kOfflinerDataKey[] = "offliner-key"; On 2017/03/11 01:10:28, Dmitry Titov wrote: > An even better variant of this is to do that: > > const int kOfflinerDataKey; > ... > contents->SetUserData(&kOfflinerDataKey); > > because the actual content of the string is not used anyways (the SetUserData > uses a void* passed in). Done. I extended WebContentsUserData, which has int key generation that's used across the board. This is so we don't accidentally use the same key https://codereview.chromium.org/2737343002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:34: BackgroundLoaderOffliner* offliner_; On 2017/03/11 01:10:28, Dmitry Titov wrote: > Would be nice to add a comment to describe why the raw ptr is assumed always > pointing to alive object here. Done. https://codereview.chromium.org/2737343002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:34: BackgroundLoaderOffliner* offliner_; On 2017/03/11 01:05:13, dewittj wrote: > Please write a comment explaining the reason that lifetime is safe here. Done. https://codereview.chromium.org/2737343002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:281: SavePageRequest request(*pending_request_.get()); On 2017/03/13 17:38:20, dewittj wrote: > Last I remember, .get() is not required to dereference a unique ptr. > > Also, there is no reason to copy the SavePageRequest in this callback, correct? Done. https://codereview.chromium.org/2737343002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:380: loader_.get()->web_contents()->SetUserData(kOfflinerDataKey, On 2017/03/11 01:05:13, dewittj wrote: > nit: combine this (loader_.get()->web_contents()) and above into a single call Done.
chili@chromium.org changed reviewers: + sky@chromium.org - jochen@chromium.org
-jochen, +sky for chrome_resource_dispatcher_host_delegate changes
sky@chromium.org changed reviewers: + rdsmith@chromium.org - sky@chromium.org
sky->rdsmith (prefer local owners when possible)
rdsmith@chromium.org changed reviewers: + mmenke@chromium.org
With apologies to Matt, I'm going to defer to him on the chrome/browser/loader change--it's very short, and I think his instincts on it will be better than mine. Looking at this change, I would stamp it. It seems not the cleanest and in an ideal world would be implemented through a platform-generic interface, but my understanding is that this is consistent with how a lot of other interfaces in this space are done, so I don't have an urge to try and find a better way to do it. But Matt has a better understanding of the overall architecture of c/b/l than I do and will be more certain of that conclusion (or its opposite) than I.
LGTM. Yea, I'm with Randy - it's ugly, but how things are currently being done here. https://codereview.chromium.org/2737343002/diff/60001/chrome/browser/loader/c... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2737343002/diff/60001/chrome/browser/loader/c... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:187: #endif // OS_ANDROID Can we put this in NotifyUIThreadOfRequestComplete instead? I don't think this file needs to know of the association between offline_pages and prerender.
Thanks! https://codereview.chromium.org/2737343002/diff/60001/chrome/browser/loader/c... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2737343002/diff/60001/chrome/browser/loader/c... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:187: #endif // OS_ANDROID On 2017/03/15 16:27:26, mmenke wrote: > Can we put this in NotifyUIThreadOfRequestComplete instead? I don't think this > file needs to know of the association between offline_pages and prerender. I moved this section to NotifyUIThreadOfRequestComplete. However, that method is still in this file...?
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org, dewittj@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2737343002/#ps80001 (title: "move offline pages update out of prerender method")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489600990786830,
"parent_rev": "4cc620ab46c8194efedfd1d78e7159d5968d692b", "commit_rev":
"f2fc623943d585acb44512ebafb626acd7b3f217"}
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Allow BackgroundLoader to track network bytes using prerenderer hook-in. While this hook-in is less than ideal, allow us to unblock while architecting a better solution that will account for bytes, does not block net moving OOP, and at a rate frequent enough to be suitable. BUG=697695 ========== to ========== [Offline Pages] Allow BackgroundLoader to track network bytes using prerenderer hook-in. While this hook-in is less than ideal, allow us to unblock while architecting a better solution that will account for bytes, does not block net moving OOP, and at a rate frequent enough to be suitable. BUG=697695 Review-Url: https://codereview.chromium.org/2737343002 Cr-Commit-Position: refs/heads/master@{#457163} Committed: https://chromium.googlesource.com/chromium/src/+/f2fc623943d585acb44512ebafb6... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f2fc623943d585acb44512ebafb6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
