Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1929223002:
Add skeletons for the factories to create the request coordinator and (Closed)
|
Created:
4 years, 7 months ago by Pete Williamson Modified:
4 years ago CC:
chromium-reviews, davidben+watch_chromium.org, cbentzel+watch_chromium.org, romax+watch_chromium.org, tburkard+watch_chromium.org, fgorski+watch_chromium.org, petewil+watch_chromium.org, dewittj+watch_chromium.org, gavinp+prer_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd skeletons for the factories to create the request coordinator and
prerender offliner
BUG=
patch from issue 1914333002 at patchset 60001 (http://crrev.com/1914333002#ps60001)
Committed: https://crrev.com/6e3878cde4c06ff306d39f282316bc2123107076
Cr-Commit-Position: refs/heads/master@{#391826}
Patch Set 1 #Patch Set 2 : Manual merge of changes in the base changelist. #
Total comments: 21
Patch Set 3 : Moved some factories to chrome side to have access to the variables we need #Patch Set 4 : After merge #
Total comments: 5
Patch Set 5 : cleanup #
Total comments: 53
Patch Set 6 : CR fixes per DougArnett and FGorski #
Total comments: 12
Patch Set 7 : CR fixes per FGorski #
Total comments: 4
Patch Set 8 : Rebase and fix some nits #Patch Set 9 : Remove files added in error #Patch Set 10 : Add dependency in BUILD.gn file #Patch Set 11 : Fix dependencies for the right build target. #Patch Set 12 : Fix unit tests, gyp build. #Patch Set 13 : Attempt gypi file fix. #
Total comments: 1
Messages
Total messages: 47 (19 generated)
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... File components/offline_pages/background/offliner_factory.h (right): https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/offliner_factory.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/offliner_factory.h:9: #include "components/offline_pages/background/offliner_factory.h" I think you are trying to include this file. https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/offliner_factory.h:13: struct DefaultSingletonTraits; Why is this included? https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/offliner_factory.h:21: // A factory to create one unique OfflinePageModel. update to refer to the right class. Explain the ownership model for the created Offliners, after which you may update the signature of GetInstance to unique_ptr https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/offliner_factory.h:24: virtual Offliner* GetInstance(OfflinerPolicy* policy); GetInstance sounds more like a singleton. Did you consider different name? e.g. GetOffliner, MakeOffliner? Also, why is policy passed by pointer? https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... File components/offline_pages/background/prerenderer_offliner.cc (right): https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/prerenderer_offliner.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Again this file is not needed. https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... File components/offline_pages/background/prerenderer_offliner.h (right): https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/prerenderer_offliner.h:17: class PrerendererOffliner : public Offliner { I don't think you need this file. And it is not in the right place. See https://codereview.chromium.org/1915983008/ https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... File components/offline_pages/background/prerenderer_offliner_factory.cc (right): https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/prerenderer_offliner_factory.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. again this should be in c/b/a/op/ https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... File components/offline_pages/background/prerenderer_offliner_factory.h (right): https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/prerenderer_offliner_factory.h:24: class PrerendererOfflinerFactory : public OfflinerFactory { This should be in the c/b/a/op/ with the prerendering_offliner implementation. https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/prerenderer_offliner_factory.h:26: PrerendererOffliner* GetInstance(OfflinerPolicy* policy) override; Are you going for a model, where offliners are going to be singletons? This is a bit risky, knowing that some of them could be working in parallel. I think you are trying to mimic the keyed service factory and we might not need that. https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:21: delete policy_; use unique_ptr which solves this. https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:28: OfflinerPolicy* policy, OfflinerFactory* factory); a) what about multiple factories for different offliners b) coordinator should take ownership of this, but we need to discuss the lifecycle of factories. https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator_factory.cc (right): https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_factory.cc:37: OfflinerFactory* prerendererOffliner = new PrerendererOfflinerFactory(); because I am not sure about having this one in components (perhaps we can pull it off) this file might also have to be in c/b/a/op/ with the header.
Thanks for the early feedback! Patch is still not quite ready for review. https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... File components/offline_pages/background/offliner_factory.h (right): https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/offliner_factory.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/29 04:16:46, fgorski wrote: > 2016 Done. https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/offliner_factory.h:9: #include "components/offline_pages/background/offliner_factory.h" On 2016/04/29 04:16:46, fgorski wrote: > I think you are trying to include this file. Done. https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/offliner_factory.h:13: struct DefaultSingletonTraits; On 2016/04/29 04:16:46, fgorski wrote: > Why is this included? Leftover from cut-and-paste. Removed. https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/offliner_factory.h:21: // A factory to create one unique OfflinePageModel. On 2016/04/29 04:16:46, fgorski wrote: > update to refer to the right class. > > Explain the ownership model for the created Offliners, after which you may > update the signature of GetInstance to unique_ptr Updated. I chose not to return a unique_ptr since we might cache the Offliner in the factory to avoid creating it multiple times. https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/offliner_factory.h:24: virtual Offliner* GetInstance(OfflinerPolicy* policy); On 2016/04/29 04:16:46, fgorski wrote: > GetInstance sounds more like a singleton. Did you consider different name? > > e.g. GetOffliner, MakeOffliner? > > > Also, why is policy passed by pointer? Changed to GetOffliner. I didn't want to make a copy, and was hazy on when references are allowed. Would you prefer a reference? Would you prefer some flavor of smart pointer? https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... File components/offline_pages/background/prerenderer_offliner.h (right): https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/prerenderer_offliner.h:17: class PrerendererOffliner : public Offliner { On 2016/04/29 04:16:46, fgorski wrote: > I don't think you need this file. And it is not in the right place. > > See https://codereview.chromium.org/1915983008/ Right. I started this before Doug posted that changelist, and I haven't integrated with it yet. I do plan to move the factories. (As I said when I sent this out yet, this is not yet ready for review, that was one of the reasons). https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:21: delete policy_; On 2016/04/29 04:16:46, fgorski wrote: > use unique_ptr which solves this. I didn't want to use unique_ptr yet, because I think I will need to give a reference to the policy object to the offliner factory. When I nail down the offliner factory a bit more, I'll know what I can do here. https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1929223002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:28: OfflinerPolicy* policy, OfflinerFactory* factory); On 2016/04/29 04:16:46, fgorski wrote: > a) what about multiple factories for different offliners > b) coordinator should take ownership of this, but we need to discuss the > lifecycle of factories. For now we only have one factory. I have a TODO elsewhere to expand to an array of factories once we add a second. Until then, what we have now is simpler. Agreed, we should discuss the lifecycle of factories. I want to nail down the prerenderer_factory workings before I tackle lifecycle, though.
dougarnett@chromium.org changed reviewers: + dougarnett@chromium.org
few advance glance comments https://codereview.chromium.org/1929223002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner_factory.cc (right): https://codereview.chromium.org/1929223002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.cc:32: // TODO(petewil): Create a PrerenderMaanger, use for 2nd arg below will be replacing PrerenderManager ctor arg with BrowserContext so nullptr good for landing this CL https://codereview.chromium.org/1929223002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/1929223002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory.cc:40: // TODO(petewil) When we add another prerenderer type for the server, expand might just say: Add support for multiple offliner types when server rendering supported. without prescribing array (in case we decide to have a factory factory that take a type). https://codereview.chromium.org/1929223002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory.h (right): https://codereview.chromium.org/1929223002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016
Ready for review. Note that the request coordinator factory is not yet hooked up to the menu item, that will come with another patch (and some tests). https://codereview.chromium.org/1929223002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner_factory.cc (right): https://codereview.chromium.org/1929223002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.cc:32: // TODO(petewil): Create a PrerenderMaanger, use for 2nd arg below On 2016/05/03 18:32:40, dougarnett wrote: > will be replacing PrerenderManager ctor arg with BrowserContext so nullptr good > for landing this CL Acknowledged. https://codereview.chromium.org/1929223002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory.h (right): https://codereview.chromium.org/1929223002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/05/03 18:32:40, dougarnett wrote: > 2016 Done.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929223002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929223002/60001
https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner_factory.cc (right): https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.cc:40: return offliner_; too much indention https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner_factory.h (right): https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:24: // A factory to create one unique OfflinePageModel. update comment https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory.cc:10: #include "components/keyed_service/content/browser_context_dependency_manager.h" dupe https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... File components/offline_pages/background/offliner_factory.h (right): https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/offliner_factory.h:20: virtual Offliner* GetOffliner(const OfflinerPolicy* policy); ... = 0; ? https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:10: class OfflinerPolicy { some initial class comment?
https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.h (right): https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.h:10: #include "components/offline_pages/background/offliner_policy.h" forward declare. you are only dealing with a pointer. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner_factory.h (right): https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:9: #include "chrome/browser/android/offline_pages/prerendering_offliner.h" this is forward declared below https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:10: #include "components/offline_pages/background/offliner.h" is this needed? https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:12: #include "content/public/browser/browser_context.h" forward declare https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:16: struct DefaultSingletonTraits; remove, I don't think you will need a singleton for this factory. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:27: PrerenderingOffliner* GetOffliner(const OfflinerPolicy* policy) override; did you consider create/make? also, ctor and dtor should go first. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:34: // friend struct base::DefaultSingletonTraits<PrerenderingOfflinerFactory>; remove this part as well. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:36: DISALLOW_COPY_AND_ASSIGN(PrerenderingOfflinerFactory); this goes last https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:38: PrerenderingOffliner* offliner_; document ownership https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:39: content::BrowserContext* context_; document ownership https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory.cc:19: "RequestCoordinator", Can we include the offline in the name? request is an overloaded term in chromium wile coordinator sounds much like manager... https://codereview.chromium.org/1929223002/diff/80001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1929223002/diff/80001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1561: 'browser/android/offline_pages/offliner.h', according to UML the offliner interface should be defined in the component. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... File components/offline_pages/background/offliner_factory.h (right): https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/offliner_factory.h:22: OfflinerFactory() {} ctor and dtor should be above methods. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.cc:9: #include "components/offline_pages/background/scheduler.h" I don't see this being used. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:23: struct SavePageRequest { remove this. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:44: friend class RequestCoordinatorFactory; I don't think you need this. Constructor can be public even when you are using the factory. After all you may want to test this thing. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:50: OfflinerPolicy* policy_; this should be a unique_ptr. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:51: OfflinerFactory* factory_; who controls the life of a factory? https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:52: int factoryArraySize_; ?
https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.h (right): https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.h:10: #include "components/offline_pages/background/offliner_policy.h" On 2016/05/03 22:48:08, fgorski wrote: > forward declare. you are only dealing with a pointer. Done. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner_factory.cc (right): https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.cc:40: return offliner_; On 2016/05/03 22:30:43, dougarnett wrote: > too much indention Done. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner_factory.h (right): https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:9: #include "chrome/browser/android/offline_pages/prerendering_offliner.h" On 2016/05/03 22:48:08, fgorski wrote: > this is forward declared below Done. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:10: #include "components/offline_pages/background/offliner.h" On 2016/05/03 22:48:16, fgorski wrote: > is this needed? Done. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:12: #include "content/public/browser/browser_context.h" On 2016/05/03 22:48:16, fgorski wrote: > forward declare Done. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:16: struct DefaultSingletonTraits; On 2016/05/03 22:48:08, fgorski wrote: > remove, I don't think you will need a singleton for this factory. Done. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:24: // A factory to create one unique OfflinePageModel. On 2016/05/03 22:30:44, dougarnett wrote: > update comment Done. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:27: PrerenderingOffliner* GetOffliner(const OfflinerPolicy* policy) override; On 2016/05/03 22:48:08, fgorski wrote: > did you consider create/make? > > also, ctor and dtor should go first. ctor and dtor moved. I did consider make, but we sometimes make one, and sometimes return a cached one, so get seemed more correct to me. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:34: // friend struct base::DefaultSingletonTraits<PrerenderingOfflinerFactory>; On 2016/05/03 22:48:16, fgorski wrote: > remove this part as well. Done. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:36: DISALLOW_COPY_AND_ASSIGN(PrerenderingOfflinerFactory); On 2016/05/03 22:48:08, fgorski wrote: > this goes last Done. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:38: PrerenderingOffliner* offliner_; On 2016/05/03 22:48:16, fgorski wrote: > document ownership Done. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:39: content::BrowserContext* context_; On 2016/05/03 22:48:08, fgorski wrote: > document ownership Done. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory.cc:10: #include "components/keyed_service/content/browser_context_dependency_manager.h" On 2016/05/03 22:30:44, dougarnett wrote: > dupe Done. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/request_coordinator_factory.cc:19: "RequestCoordinator", On 2016/05/03 22:48:16, fgorski wrote: > Can we include the offline in the name? > request is an overloaded term in chromium > wile coordinator sounds much like manager... Done. https://codereview.chromium.org/1929223002/diff/80001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1929223002/diff/80001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1561: 'browser/android/offline_pages/offliner.h', On 2016/05/03 22:48:16, fgorski wrote: > according to UML the offliner interface should be defined in the component. Done. (Actually, it already is defined there, this one is an error, removed it.) https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... File components/offline_pages/background/offliner_factory.h (right): https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/offliner_factory.h:20: virtual Offliner* GetOffliner(const OfflinerPolicy* policy); On 2016/05/03 22:30:44, dougarnett wrote: > ... = 0; ? Done. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/offliner_factory.h:22: OfflinerFactory() {} On 2016/05/03 22:48:16, fgorski wrote: > ctor and dtor should be above methods. Done. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:10: class OfflinerPolicy { On 2016/05/03 22:30:44, dougarnett wrote: > some initial class comment? Done. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.cc:9: #include "components/offline_pages/background/scheduler.h" On 2016/05/03 22:48:16, fgorski wrote: > I don't see this being used. Done. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:23: struct SavePageRequest { On 2016/05/03 22:48:17, fgorski wrote: > remove this. Done. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:44: friend class RequestCoordinatorFactory; On 2016/05/03 22:48:16, fgorski wrote: > I don't think you need this. > Constructor can be public even when you are using the factory. > > After all you may want to test this thing. Removed. I'm happy without it, but I thought I recalled that normal practice was to hide the Ctor of anything with a factory. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:50: OfflinerPolicy* policy_; On 2016/05/03 22:48:16, fgorski wrote: > this should be a unique_ptr. Can I use a unique ptr if I am going to pass a pointer to the same object to the request queue and offliner? I'm not sure which is the best pointer, maybe shared_ptr? https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:51: OfflinerFactory* factory_; On 2016/05/03 22:48:17, fgorski wrote: > who controls the life of a factory? The request coordinator should control the lifetime of any offliner factory it makes. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:52: int factoryArraySize_; On 2016/05/03 22:48:17, fgorski wrote: > ? Oops, leftover. Removed.
It's getting there. Let's sort out the lifetime management based on the dependencies that we know off. https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner_factory.h (right): https://codereview.chromium.org/1929223002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:27: PrerenderingOffliner* GetOffliner(const OfflinerPolicy* policy) override; On 2016/05/04 00:39:21, Pete Williamson wrote: > On 2016/05/03 22:48:08, fgorski wrote: > > did you consider create/make? > > > > also, ctor and dtor should go first. > > ctor and dtor moved. > > I did consider make, but we sometimes make one, and sometimes return a cached > one, so get seemed more correct to me. Acknowledged. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:50: OfflinerPolicy* policy_; On 2016/05/04 00:39:21, Pete Williamson wrote: > On 2016/05/03 22:48:16, fgorski wrote: > > this should be a unique_ptr. > > Can I use a unique ptr if I am going to pass a pointer to the same object to the > request queue and offliner? I'm not sure which is the best pointer, maybe > shared_ptr? Here is how I see this: 1. RC owns the policy (unique_ptr is apropriate) 2. RC owns the request_queue (another unique_ptr) 3. RC passes the raw ptr to the request_queue and inside of it you make a comment about 1 and 2 Don't think of unique_ptr as: there is only one pointer to the object, but rather: there is only one handle that manages life of that object. Here is a relevant discussion of the problem I found: https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/ https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:51: OfflinerFactory* factory_; On 2016/05/04 00:39:22, Pete Williamson wrote: > On 2016/05/03 22:48:17, fgorski wrote: > > who controls the life of a factory? > > The request coordinator should control the lifetime of any offliner factory it > makes. unique_ptr then? per response above. https://codereview.chromium.org/1929223002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner_factory.h (right): https://codereview.chromium.org/1929223002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:13: } } // namespace content https://codereview.chromium.org/1929223002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:21: // A factory to create one unique PrerenderingOffliner Can you also explain why the prerendering offliner needs to be unique? Is it that we don't want to have more than one running at a time? Will it be safe to reuse existing one? https://codereview.chromium.org/1929223002/diff/100001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1929223002/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:7: // TODO(fgorski): include file with SavePageRequest definition. I think you can resolve the TODO now. https://codereview.chromium.org/1929223002/diff/100001/components/offline_pag... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1929223002/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator.h:10: #include "components/offline_pages/background/save_page_request.h" I think you can forward declare this one. https://codereview.chromium.org/1929223002/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator.h:19: class RequestCoordinator : public KeyedService { Did you consider adding DISALLOW... at the end? https://codereview.chromium.org/1929223002/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator.h:26: OfflinerPolicy* policy, OfflinerFactory* factory); To clearly communicate ownership you should probably pass these using unique_ptr using std::move. See: https://code.google.com/p/chromium/codesearch#chromium/src/components/offline... for example of that.
https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:50: OfflinerPolicy* policy_; On 2016/05/04 05:20:57, fgorski wrote: > On 2016/05/04 00:39:21, Pete Williamson wrote: > > On 2016/05/03 22:48:16, fgorski wrote: > > > this should be a unique_ptr. > > > > Can I use a unique ptr if I am going to pass a pointer to the same object to > the > > request queue and offliner? I'm not sure which is the best pointer, maybe > > shared_ptr? > > Here is how I see this: > 1. RC owns the policy (unique_ptr is apropriate) > 2. RC owns the request_queue (another unique_ptr) > 3. RC passes the raw ptr to the request_queue and inside of it you make a > comment about 1 and 2 > > Don't think of unique_ptr as: there is only one pointer to the object, but > rather: there is only one handle that manages life of that object. > > Here is a relevant discussion of the problem I found: > https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/ Done. https://codereview.chromium.org/1929223002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:51: OfflinerFactory* factory_; On 2016/05/04 05:20:57, fgorski wrote: > On 2016/05/04 00:39:22, Pete Williamson wrote: > > On 2016/05/03 22:48:17, fgorski wrote: > > > who controls the life of a factory? > > > > The request coordinator should control the lifetime of any offliner factory it > > makes. > > unique_ptr then? per response above. Done. https://codereview.chromium.org/1929223002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner_factory.h (right): https://codereview.chromium.org/1929223002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:13: } On 2016/05/04 05:20:57, fgorski wrote: > } // namespace content Done. https://codereview.chromium.org/1929223002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner_factory.h:21: // A factory to create one unique PrerenderingOffliner On 2016/05/04 05:20:57, fgorski wrote: > Can you also explain why the prerendering offliner needs to be unique? Is it > that we don't want to have more than one running at a time? > > Will it be safe to reuse existing one? Comments updated. https://codereview.chromium.org/1929223002/diff/100001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1929223002/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:7: // TODO(fgorski): include file with SavePageRequest definition. On 2016/05/04 05:20:58, fgorski wrote: > I think you can resolve the TODO now. Done, also moved added the save_page_request.h include. (I made this same change in my other changelist, so I'll have to merge one of these.) https://codereview.chromium.org/1929223002/diff/100001/components/offline_pag... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1929223002/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator.h:10: #include "components/offline_pages/background/save_page_request.h" On 2016/05/04 05:20:58, fgorski wrote: > I think you can forward declare this one. Done. https://codereview.chromium.org/1929223002/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator.h:19: class RequestCoordinator : public KeyedService { On 2016/05/04 05:20:58, fgorski wrote: > Did you consider adding DISALLOW... at the end? Done. https://codereview.chromium.org/1929223002/diff/100001/components/offline_pag... components/offline_pages/background/request_coordinator.h:26: OfflinerPolicy* policy, OfflinerFactory* factory); On 2016/05/04 05:20:58, fgorski wrote: > To clearly communicate ownership you should probably pass these using unique_ptr > using std::move. > > See: > https://code.google.com/p/chromium/codesearch#chromium/src/components/offline... > for example of that. Done.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929223002/80002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929223002/80002
lgtm with nits https://codereview.chromium.org/1929223002/diff/80002/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1929223002/diff/80002/components/offline_page... components/offline_pages/background/request_coordinator.h:28: std::unique_ptr<OfflinerPolicy> policy, did you run git cl format? https://codereview.chromium.org/1929223002/diff/80002/components/offline_page... components/offline_pages/background/request_coordinator.h:53: DISALLOW_COPY_AND_ASSIGN(RequestCoordinator); nit: include "base/macros.h" explicitly
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Also removed unused weak_ptr .h file as noticed in another changelist. https://codereview.chromium.org/1929223002/diff/80002/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1929223002/diff/80002/components/offline_page... components/offline_pages/background/request_coordinator.h:28: std::unique_ptr<OfflinerPolicy> policy, On 2016/05/04 21:38:25, fgorski wrote: > did you run git cl format? Done. (though I thought this style was also allowed.) https://codereview.chromium.org/1929223002/diff/80002/components/offline_page... components/offline_pages/background/request_coordinator.h:53: DISALLOW_COPY_AND_ASSIGN(RequestCoordinator); On 2016/05/04 21:38:25, fgorski wrote: > nit: include "base/macros.h" explicitly Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929223002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929223002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929223002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929223002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929223002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929223002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929223002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929223002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929223002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929223002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/1929223002/#ps220001 (title: "Attempt gypi file fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929223002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929223002/220001
Message was sent while issue was closed.
Committed patchset #13 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add skeletons for the factories to create the request coordinator and prerender offliner BUG= patch from issue 1914333002 at patchset 60001 (http://crrev.com/1914333002#ps60001) ========== to ========== Add skeletons for the factories to create the request coordinator and prerender offliner BUG= patch from issue 1914333002 at patchset 60001 (http://crrev.com/1914333002#ps60001) Committed: https://crrev.com/6e3878cde4c06ff306d39f282316bc2123107076 Cr-Commit-Position: refs/heads/master@{#391826} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/6e3878cde4c06ff306d39f282316bc2123107076 Cr-Commit-Position: refs/heads/master@{#391826}
Message was sent while issue was closed.
dimich@chromium.org changed reviewers: + dimich@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1929223002/diff/220001/components/offline_pag... File components/offline_pages/background/offliner_factory.h (right): https://codereview.chromium.org/1929223002/diff/220001/components/offline_pag... components/offline_pages/background/offliner_factory.h:16: // The returned factory is owned by the offliner, which is allowed to cache the This comment (and design of this factory) seems off. This is a factory that creates offliners, not the other way around? Also, if this factory returns a single cached offliner, then perhaps it is possible to return naked pointer like here. If it ever can produce more then a single cached instance of offliner, it has to return unique_ptr. Given this design, it can only ever return a singleton cached pointer with assumption that lifetime of consumer is guaranteed longer that this factory. Since this factory is not a part of KeyedServices mechanism, there is no explicit contract here for that lifetime assumption. I think a better solution would be to either create an offliner directly and pass it into RequestCoordinator (without having yet another factory that only can produce a singleton), or go with the multiple offliners from the start and return unique_ptr. Those offliners may share state or they may be independent, separately. |
