Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1922533003:
Defines initial inner circle of interfaces for background offlining. (Closed)
|
Created:
4 years, 8 months ago by dougarnett Modified:
4 years, 8 months ago CC:
chromium-reviews, fgorski+watch_chromium.org, romax+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_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. |
DescriptionDefines initial inner circle of interfaces for background offlining.
BUG=
Committed: https://crrev.com/7ccea9c5ace7e2e11dfcce807707b1962341a784
Cr-Commit-Position: refs/heads/master@{#389846}
Patch Set 1 #
Total comments: 31
Patch Set 2 : Feedback updates (does not yet rename Coordinator though). #
Total comments: 2
Patch Set 3 : Moved headers to new background subdir. #Patch Set 4 : RequestCoordinator now extends KeyedService and also now has empty ctor #
Total comments: 6
Patch Set 5 : Improved Coordinator Stop/Start method comments and some other feedback addressed #
Total comments: 2
Patch Set 6 : Removed KeyedService reference for this CL #Patch Set 7 : Changed forward reference of SavePageRequest to empty stuct for this CL per dewittj feedback. #
Messages
Total messages: 38 (10 generated)
Description was changed from ========== Defines initial inner circle of interfaces for background offlining. BUG= ========== to ========== Defines initial inner circle of interfaces for background offlining. BUG= ==========
dougarnett@chromium.org changed reviewers: + fgorski@chromium.org, petewil@chromium.org
Let use this CL to resolve some naming and seed the offlining impl.
https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... File components/offline_pages/background_offliner.h (right): https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_offliner.h:30: Seems like we need a constructor, which brings up the question about how this class is configured, whether we need to pass in policy at config time, or have a hardcoded policy class that uses a data store somewhere for flexibility. https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_offliner.h:35: // request is not accepted. Will we always be limited to one request? To me that seems like a limitation of the current pre-render implementation. How about deleting the "Only one request may be outstanding and" part, and leaving the warning about returning false if not accepted? https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... File components/offline_pages/background_request_coordinator.h (right): https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:15: class BackgroundRequestCoordinator { "BackgroundRequestCoordinator" is a long name. What do you think about just "RequestCoordinator"? I do see the utility in having all the files start with background, but there aren't so many other files in offline pages that the distinction becomes that important, I think. (and if there were, maybe we should put these into a new directory anyway). https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:17: // Completion status of processing an offline page request. We probably want completion status in only one header file, either here or the background_offliner, but not both. https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:27: typedef base::Callback<void()> ProcessingDoneCallback; The processing done callback should have a parameter to identify which request finished. If there is only one request at a time, then we could save it as a global, but it seems cleaner for the callback to be explicit, and it would allow processing more than one request at a time if the pre-renderer can ever support it. https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:29: virtual ~BackgroundRequestCoordinator() {} We should add a constructor, too. That brings up the question of how we configure the background offliner, does this class create it using a factory? If so, does the factory get passed into the constructor, or do we hardcode it? Sorry if this has already been discussed, I don't remember what we decided.
https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... File components/offline_pages/background_offliner.h (right): https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_offliner.h:8: #include <stdint.h> nit: do you need this? https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_offliner.h:24: TIMEOUT = 2, Would timeout be a different thing than one of the two below? Also should fialed_xxx offer details of what failed, like timeout and it would be up to the coordinator to interpret the result as retry vs. not retry? https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_offliner.h:35: // request is not accepted. On 2016/04/25 20:12:16, Pete Williamson wrote: > Will we always be limited to one request? To me that seems like a limitation of > the current pre-render implementation. How about deleting the "Only one request > may be outstanding and" part, and leaving the warning about returning false if > not accepted? +1 on only one request https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_offliner.h:41: virtual void CancelCurrent() = 0; Just Cancel. https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... File components/offline_pages/background_request_coordinator.h (right): https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:8: #include <stdint.h> nit: do you need this? https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:15: class BackgroundRequestCoordinator { On 2016/04/25 20:12:16, Pete Williamson wrote: > "BackgroundRequestCoordinator" is a long name. What do you think about just > "RequestCoordinator"? I do see the utility in having all the files start with > background, but there aren't so many other files in offline pages that the > distinction becomes that important, I think. (and if there were, maybe we should > put these into a new directory anyway). Both ideas seem fine. c/o/background ?? https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:27: typedef base::Callback<void()> ProcessingDoneCallback; On 2016/04/25 20:12:16, Pete Williamson wrote: > The processing done callback should have a parameter to identify which request > finished. If there is only one request at a time, then we could save it as a > global, but it seems cleaner for the callback to be explicit, and it would allow > processing more than one request at a time if the pre-renderer can ever support > it. And should pass completion status... https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:29: virtual ~BackgroundRequestCoordinator() {} On 2016/04/25 20:12:16, Pete Williamson wrote: > We should add a constructor, too. That brings up the question of how we > configure the background offliner, does this class create it using a factory? > If so, does the factory get passed into the constructor, or do we hardcode it? > Sorry if this has already been discussed, I don't remember what we decided. I think we mentioned this will be a KeyedService. https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... File components/offline_pages/background_scheduler.h (right): https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_scheduler.h:8: #include <stdint.h> what is this needed for?
https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... File components/offline_pages/background_offliner.h (right): https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_offliner.h:8: #include <stdint.h> On 2016/04/25 21:07:37, fgorski wrote: > nit: do you need this? Done. https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_offliner.h:24: TIMEOUT = 2, On 2016/04/25 21:07:37, fgorski wrote: > Would timeout be a different thing than one of the two below? > > Also should fialed_xxx offer details of what failed, like timeout and it would > be up to the coordinator to interpret the result as retry vs. not retry? There is a question about what the Coordinator will care about. At this point, I am thinking about retry policy but not sure about more. Thinking the Offliner can provider Loader-specific UMA and interpret whether retriable or not. Would like to avoid passing superset of client and server failure types to Coordinator unless useful to it. I could drop the TIMEOUT one for now - just the two chunked types of FAILUREs? Was thinking that TIMEOUT might be an interesting distinction for retry policy but really not sure about it yet - so can add later if/when makes sense. https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_offliner.h:30: On 2016/04/25 20:12:16, Pete Williamson wrote: > Seems like we need a constructor, which brings up the question about how this > class is configured, whether we need to pass in policy at config time, or have a > hardcoded policy class that uses a data store somewhere for flexibility. Yep, good stuff. Trying to get minimum/basline connections/terminology in *.h's that we can expand from. Can increase the scope of the CL if you think it is important. https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_offliner.h:35: // request is not accepted. On 2016/04/25 20:12:16, Pete Williamson wrote: > Will we always be limited to one request? To me that seems like a limitation of > the current pre-render implementation. How about deleting the "Only one request > may be outstanding and" part, and leaving the warning about returning false if > not accepted? Ok, generalized the wording. I do think we stick to one initially though. If we were to end up supporting multiple requests, we likely will want to change the interface (to take list or at least return a request id that the callback references). https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_offliner.h:41: virtual void CancelCurrent() = 0; On 2016/04/25 21:07:37, fgorski wrote: > Just Cancel. Done. https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... File components/offline_pages/background_request_coordinator.h (right): https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:8: #include <stdint.h> On 2016/04/25 21:07:37, fgorski wrote: > nit: do you need this? Done. https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:17: // Completion status of processing an offline page request. On 2016/04/25 20:12:16, Pete Williamson wrote: > We probably want completion status in only one header file, either here or the > background_offliner, but not both. woops - thanks https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:27: typedef base::Callback<void()> ProcessingDoneCallback; On 2016/04/25 21:07:38, fgorski wrote: > On 2016/04/25 20:12:16, Pete Williamson wrote: > > The processing done callback should have a parameter to identify which request > > finished. If there is only one request at a time, then we could save it as a > > global, but it seems cleaner for the callback to be explicit, and it would > allow > > processing more than one request at a time if the pre-renderer can ever > support > > it. > > And should pass completion status... Am thinking this is single callback for however much processing we do (perhaps several requests with different completion statuses). This simple callback is meant to cause Task return to release WakeLock. I don't think the Task cares about the specific status (ie, what will it do with that information). https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:29: virtual ~BackgroundRequestCoordinator() {} On 2016/04/25 21:07:38, fgorski wrote: > On 2016/04/25 20:12:16, Pete Williamson wrote: > > We should add a constructor, too. That brings up the question of how we > > configure the background offliner, does this class create it using a factory? > > If so, does the factory get passed into the constructor, or do we hardcode it? > > > Sorry if this has already been discussed, I don't remember what we decided. > > I think we mentioned this will be a KeyedService. And also discussed prospect of injecting factories for Offliners to it. https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... File components/offline_pages/background_scheduler.h (right): https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_scheduler.h:8: #include <stdint.h> On 2016/04/25 21:07:38, fgorski wrote: > what is this needed for? Done.
https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... File components/offline_pages/background_request_coordinator.h (right): https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:15: class BackgroundRequestCoordinator { On 2016/04/25 21:07:38, fgorski wrote: > On 2016/04/25 20:12:16, Pete Williamson wrote: > > "BackgroundRequestCoordinator" is a long name. What do you think about just > > "RequestCoordinator"? I do see the utility in having all the files start with > > background, but there aren't so many other files in offline pages that the > > distinction becomes that important, I think. (and if there were, maybe we > should > > put these into a new directory anyway). > > Both ideas seem fine. > > c/o/background ?? Yes, like the subdirectory idea https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:27: typedef base::Callback<void()> ProcessingDoneCallback; On 2016/04/25 21:49:08, dougarnett wrote: > On 2016/04/25 21:07:38, fgorski wrote: > > On 2016/04/25 20:12:16, Pete Williamson wrote: > > > The processing done callback should have a parameter to identify which > request > > > finished. If there is only one request at a time, then we could save it as > a > > > global, but it seems cleaner for the callback to be explicit, and it would > > allow > > > processing more than one request at a time if the pre-renderer can ever > > support > > > it. > > > > And should pass completion status... > > Am thinking this is single callback for however much processing we do (perhaps > several requests with different completion statuses). This simple callback is > meant to cause Task return to release WakeLock. I don't think the Task cares > about the specific status (ie, what will it do with that information). Although returning a saved page count or such could be useful for testing.
https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... File components/offline_pages/background_request_coordinator.h (right): https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:27: typedef base::Callback<void()> ProcessingDoneCallback; On 2016/04/25 21:49:08, dougarnett wrote: > On 2016/04/25 21:07:38, fgorski wrote: > > On 2016/04/25 20:12:16, Pete Williamson wrote: > > > The processing done callback should have a parameter to identify which > request > > > finished. If there is only one request at a time, then we could save it as > a > > > global, but it seems cleaner for the callback to be explicit, and it would > > allow > > > processing more than one request at a time if the pre-renderer can ever > > support > > > it. > > > > And should pass completion status... > > Am thinking this is single callback for however much processing we do (perhaps > several requests with different completion statuses). This simple callback is > meant to cause Task return to release WakeLock. I don't think the Task cares > about the specific status (ie, what will it do with that information). I guess I'm thinking about two different callbacks. The first one informs the scheduler that we are done with *all* processing. (Callback from requestcoordinator to scheduler). The first callback needs no args. The second is that the request coordinator needs a callback from the background offliner that work is done for that page. The second callback should have an arg identifying which page was completed, so the request queue can be properly updated. We could get by without an arg (at least for now) if we keep a global variable, but having an arg makes the code cleaner. I was thinking that this callback is the *second* callback described above, and that the first callback should be in the scheduler .h file, not here.
On 2016/04/25 21:59:57, Pete Williamson wrote: > https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... > File components/offline_pages/background_request_coordinator.h (right): > > https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... > components/offline_pages/background_request_coordinator.h:27: typedef > base::Callback<void()> ProcessingDoneCallback; > On 2016/04/25 21:49:08, dougarnett wrote: > > On 2016/04/25 21:07:38, fgorski wrote: > > > On 2016/04/25 20:12:16, Pete Williamson wrote: > > > > The processing done callback should have a parameter to identify which > > request > > > > finished. If there is only one request at a time, then we could save it > as > > a > > > > global, but it seems cleaner for the callback to be explicit, and it would > > > allow > > > > processing more than one request at a time if the pre-renderer can ever > > > support > > > > it. > > > > > > And should pass completion status... > > > > Am thinking this is single callback for however much processing we do (perhaps > > several requests with different completion statuses). This simple callback is > > meant to cause Task return to release WakeLock. I don't think the Task cares > > about the specific status (ie, what will it do with that information). > > I guess I'm thinking about two different callbacks. > > The first one informs the scheduler that we are done with *all* processing. > (Callback from requestcoordinator to scheduler). The first callback needs no > args. > > The second is that the request coordinator needs a callback from the background > offliner that work is done for that page. The second callback should have an > arg identifying which page was completed, so the request queue can be properly > updated. We could get by without an arg (at least for now) if we keep a global > variable, but having an arg makes the code cleaner. > > I was thinking that this callback is the *second* callback described above, and > that the first callback should be in the scheduler .h file, not here. Ah, yes, where to define the Callback types. I was defining the Callback types in the classes the receive them as part of their method signatures. Does that look good? As for the Offliner's CompletionCallback, should I just pass the request back in the callback? Or do you think its url or define some request id?
https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... File components/offline_pages/background_request_coordinator.h (right): https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:27: typedef base::Callback<void()> ProcessingDoneCallback; On 2016/04/25 21:59:57, Pete Williamson wrote: > On 2016/04/25 21:49:08, dougarnett wrote: > > On 2016/04/25 21:07:38, fgorski wrote: > > > On 2016/04/25 20:12:16, Pete Williamson wrote: > > > > The processing done callback should have a parameter to identify which > > request > > > > finished. If there is only one request at a time, then we could save it > as > > a > > > > global, but it seems cleaner for the callback to be explicit, and it would > > > allow > > > > processing more than one request at a time if the pre-renderer can ever > > > support > > > > it. > > > > > > And should pass completion status... > > > > Am thinking this is single callback for however much processing we do (perhaps > > several requests with different completion statuses). This simple callback is > > meant to cause Task return to release WakeLock. I don't think the Task cares > > about the specific status (ie, what will it do with that information). > > I guess I'm thinking about two different callbacks. > > The first one informs the scheduler that we are done with *all* processing. > (Callback from requestcoordinator to scheduler). The first callback needs no > args. > > The second is that the request coordinator needs a callback from the background > offliner that work is done for that page. The second callback should have an > arg identifying which page was completed, so the request queue can be properly > updated. We could get by without an arg (at least for now) if we keep a global > variable, but having an arg makes the code cleaner. > > I was thinking that this callback is the *second* callback described above, and > that the first callback should be in the scheduler .h file, not here. I agree that the callback should be defined in the class that receives it. (That implies to me that "the first callback" above goes into the scheduler, and the "second callback" goes into the request coordinator.) Passing back the entire request is fine, but if it makes life easier for the offliner, I could get by with just enough information to identify the request uniquely (the URL and the client ID, or perhaps a page ID composed from both the URL and client ID).
On 2016/04/25 22:18:31, Pete Williamson wrote: > https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... > File components/offline_pages/background_request_coordinator.h (right): > > https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... > components/offline_pages/background_request_coordinator.h:27: typedef > base::Callback<void()> ProcessingDoneCallback; > On 2016/04/25 21:59:57, Pete Williamson wrote: > > On 2016/04/25 21:49:08, dougarnett wrote: > > > On 2016/04/25 21:07:38, fgorski wrote: > > > > On 2016/04/25 20:12:16, Pete Williamson wrote: > > > > > The processing done callback should have a parameter to identify which > > > request > > > > > finished. If there is only one request at a time, then we could save it > > as > > > a > > > > > global, but it seems cleaner for the callback to be explicit, and it > would > > > > allow > > > > > processing more than one request at a time if the pre-renderer can ever > > > > support > > > > > it. > > > > > > > > And should pass completion status... > > > > > > Am thinking this is single callback for however much processing we do > (perhaps > > > several requests with different completion statuses). This simple callback > is > > > meant to cause Task return to release WakeLock. I don't think the Task cares > > > about the specific status (ie, what will it do with that information). > > > > I guess I'm thinking about two different callbacks. > > > > The first one informs the scheduler that we are done with *all* processing. > > (Callback from requestcoordinator to scheduler). The first callback needs no > > args. > > > > The second is that the request coordinator needs a callback from the > background > > offliner that work is done for that page. The second callback should have an > > arg identifying which page was completed, so the request queue can be properly > > updated. We could get by without an arg (at least for now) if we keep a > global > > variable, but having an arg makes the code cleaner. > > > > I was thinking that this callback is the *second* callback described above, > and > > that the first callback should be in the scheduler .h file, not here. > > I agree that the callback should be defined in the class that receives it. > (That implies to me that "the first callback" above goes into the scheduler, and > the "second callback" goes into the request coordinator.) > > Passing back the entire request is fine, but if it makes life easier for the > offliner, I could get by with just enough information to identify the request > uniquely (the URL and the client ID, or perhaps a page ID composed from both the > URL and client ID). Hmm, sounds like we didn't agree actually - I meant the class that receives the callback argument. Not the class that defines the instance of the callback that will be called back. My view was that in general the class taking the callback argument may have more than one caller (although not expected in this case). For example, offline_page_model.h defines callback type that its methods take as arguments.
https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... File components/offline_pages/background_request_coordinator.h (right): https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:27: typedef base::Callback<void()> ProcessingDoneCallback; On 2016/04/25 22:18:31, Pete Williamson wrote: > On 2016/04/25 21:59:57, Pete Williamson wrote: > > On 2016/04/25 21:49:08, dougarnett wrote: > > > On 2016/04/25 21:07:38, fgorski wrote: > > > > On 2016/04/25 20:12:16, Pete Williamson wrote: > > > > > The processing done callback should have a parameter to identify which > > > request > > > > > finished. If there is only one request at a time, then we could save it > > as > > > a > > > > > global, but it seems cleaner for the callback to be explicit, and it > would > > > > allow > > > > > processing more than one request at a time if the pre-renderer can ever > > > > support > > > > > it. > > > > > > > > And should pass completion status... > > > > > > Am thinking this is single callback for however much processing we do > (perhaps > > > several requests with different completion statuses). This simple callback > is > > > meant to cause Task return to release WakeLock. I don't think the Task cares > > > about the specific status (ie, what will it do with that information). > > > > I guess I'm thinking about two different callbacks. > > > > The first one informs the scheduler that we are done with *all* processing. > > (Callback from requestcoordinator to scheduler). The first callback needs no > > args. > > > > The second is that the request coordinator needs a callback from the > background > > offliner that work is done for that page. The second callback should have an > > arg identifying which page was completed, so the request queue can be properly > > updated. We could get by without an arg (at least for now) if we keep a > global > > variable, but having an arg makes the code cleaner. > > > > I was thinking that this callback is the *second* callback described above, > and > > that the first callback should be in the scheduler .h file, not here. > > I agree that the callback should be defined in the class that receives it. > (That implies to me that "the first callback" above goes into the scheduler, and > the "second callback" goes into the request coordinator.) > > Passing back the entire request is fine, but if it makes life easier for the > offliner, I could get by with just enough information to identify the request > uniquely (the URL and the client ID, or perhaps a page ID composed from both the > URL and client ID). Oops, looks like we were not in agreement after all. Checking the rest of chrome, it seems more common that the caller defines the callback, not the receiver, so this is OK as is (though the COmpletionCallback in the Offliner still needs an ID.)
https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... File components/offline_pages/background_request_coordinator.h (right): https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... components/offline_pages/background_request_coordinator.h:15: class BackgroundRequestCoordinator { On 2016/04/25 21:54:13, dougarnett wrote: > On 2016/04/25 21:07:38, fgorski wrote: > > On 2016/04/25 20:12:16, Pete Williamson wrote: > > > "BackgroundRequestCoordinator" is a long name. What do you think about just > > > "RequestCoordinator"? I do see the utility in having all the files start > with > > > background, but there aren't so many other files in offline pages that the > > > distinction becomes that important, I think. (and if there were, maybe we > > should > > > put these into a new directory anyway). > > > > Both ideas seem fine. > > > > c/o/background ?? > > Yes, like the subdirectory idea Ok, I have move these files to a background subdirectory and dropped the Background prefix from their names. See if you like better (in particular dropping prefix from other two names). Will we want to define a new library name for this subpackage (eg, with own BUILD.gn) or just include in "offline_pages" library?
On 2016/04/25 23:23:39, dougarnett wrote: > https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... > File components/offline_pages/background_request_coordinator.h (right): > > https://codereview.chromium.org/1922533003/diff/1/components/offline_pages/ba... > components/offline_pages/background_request_coordinator.h:15: class > BackgroundRequestCoordinator { > On 2016/04/25 21:54:13, dougarnett wrote: > > On 2016/04/25 21:07:38, fgorski wrote: > > > On 2016/04/25 20:12:16, Pete Williamson wrote: > > > > "BackgroundRequestCoordinator" is a long name. What do you think about > just > > > > "RequestCoordinator"? I do see the utility in having all the files start > > with > > > > background, but there aren't so many other files in offline pages that the > > > > distinction becomes that important, I think. (and if there were, maybe we > > > should > > > > put these into a new directory anyway). > > > > > > Both ideas seem fine. > > > > > > c/o/background ?? > > > > Yes, like the subdirectory idea > > Ok, I have move these files to a background subdirectory and dropped the > Background prefix > from their names. See if you like better (in particular dropping prefix from > other two names). > > Will we want to define a new library name for this subpackage (eg, with own > BUILD.gn) or just > include in "offline_pages" library? Changes look good. I'd still like to see constructors for these classes. I have a preference for a new BUILD.gn, but I don't have enough experience with GN yet to know if there are any reasons to prefer using the existing one at a higher level, so I could be convinced either way.
dimich@chromium.org changed reviewers: + dimich@chromium.org
Drive-by: https://codereview.chromium.org/1922533003/diff/20001/components/offline_page... File components/offline_pages/background_request_coordinator.h (right): https://codereview.chromium.org/1922533003/diff/20001/components/offline_page... components/offline_pages/background_request_coordinator.h:23: // Starts processing of one or more queued save page later requests. Consider adding comments on: - can StartProcessing be called multiple times (while processing is udnerway) or are subsequent calls ignored, or callback is invoked with some ALREADY_IN_PROGRESS indicator - Is StopProcessing always guaranteed to be called pair-wise with StartProcessing? - Must ProcessingDone be called in response to StopProcessing or is in fact it's other way around, and ProcessingDone callback actually causes the StopProcessing? - If StopProcessing is optional (conditions changed on the device) then should callback be called only if StopProcessing is not invoked? - Can this class process multiple requests or is ti one URL per 'processing'?
https://codereview.chromium.org/1922533003/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1922533003/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:13: class RequestCoordinator : public KeyedService { KeyedService -> BrowserContextKeyedService Once we have a KeyedService, we will also need a factory (maybe better to not inherit from KeyedService yet, and add that with a future change including the factory) https://codereview.chromium.org/1922533003/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:24: virtual void Shutdown() = 0; Why make these pure virtual? We don't really need an abstract base class for an interface, I think a regular .h file would work as well (we could always separate out an implementation class later if we want one, but until then, we could have the implementation in the RequestCoordinator class).
https://codereview.chromium.org/1922533003/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1922533003/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:13: class RequestCoordinator : public KeyedService { On 2016/04/26 00:52:19, Pete Williamson wrote: > KeyedService -> BrowserContextKeyedService > > Once we have a KeyedService, we will also need a factory (maybe better to not > inherit from KeyedService yet, and add that with a future change including the > factory) Oops, my bad, it looks like it is KeyedService after all. (The Factory is BrowserContextKeyedServiceFactory).
https://codereview.chromium.org/1922533003/diff/20001/components/offline_page... File components/offline_pages/background_request_coordinator.h (right): https://codereview.chromium.org/1922533003/diff/20001/components/offline_page... components/offline_pages/background_request_coordinator.h:23: // Starts processing of one or more queued save page later requests. On 2016/04/26 00:20:31, Dmitry Titov wrote: > Consider adding comments on: > - can StartProcessing be called multiple times (while processing is udnerway) or > are subsequent calls ignored, or callback is invoked with some > ALREADY_IN_PROGRESS indicator > - Is StopProcessing always guaranteed to be called pair-wise with > StartProcessing? > - Must ProcessingDone be called in response to StopProcessing or is in fact it's > other way around, and ProcessingDone callback actually causes the > StopProcessing? > - If StopProcessing is optional (conditions changed on the device) then should > callback be called only if StopProcessing is not invoked? > - Can this class process multiple requests or is ti one URL per 'processing'? Please check updated wording on Start/Stop. Btw, the first sentence of Start does already say "one or more" so think I have last point covered so if not clear please suggest how to make more clear. https://codereview.chromium.org/1922533003/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1922533003/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:13: class RequestCoordinator : public KeyedService { On 2016/04/26 00:56:13, Pete Williamson wrote: > On 2016/04/26 00:52:19, Pete Williamson wrote: > > KeyedService -> BrowserContextKeyedService > > > > Once we have a KeyedService, we will also need a factory (maybe better to not > > inherit from KeyedService yet, and add that with a future change including the > > factory) > > Oops, my bad, it looks like it is KeyedService after all. (The Factory is > BrowserContextKeyedServiceFactory). I'm happy to take out KeyedService for this CL or leave in if sufficient as is and seems helpful. https://codereview.chromium.org/1922533003/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:24: virtual void Shutdown() = 0; On 2016/04/26 00:52:19, Pete Williamson wrote: > Why make these pure virtual? We don't really need an abstract base class for an > interface, I think a regular .h file would work as well (we could always > separate out an implementation class later if we want one, but until then, we > could have the implementation in the RequestCoordinator class). Done.
https://codereview.chromium.org/1922533003/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1922533003/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:13: class RequestCoordinator : public KeyedService { On 2016/04/26 15:50:38, dougarnett wrote: > On 2016/04/26 00:56:13, Pete Williamson wrote: > > On 2016/04/26 00:52:19, Pete Williamson wrote: > > > KeyedService -> BrowserContextKeyedService > > > > > > Once we have a KeyedService, we will also need a factory (maybe better to > not > > > inherit from KeyedService yet, and add that with a future change including > the > > > factory) > > > > Oops, my bad, it looks like it is KeyedService after all. (The Factory is > > BrowserContextKeyedServiceFactory). > > I'm happy to take out KeyedService for this CL or leave in if sufficient as is > and seems helpful. Let's take out the KeyedService, and add it back when we add a factory.
On 2016/04/26 16:40:59, Pete Williamson wrote: > https://codereview.chromium.org/1922533003/diff/60001/components/offline_page... > File components/offline_pages/background/request_coordinator.h (right): > > https://codereview.chromium.org/1922533003/diff/60001/components/offline_page... > components/offline_pages/background/request_coordinator.h:13: class > RequestCoordinator : public KeyedService { > On 2016/04/26 15:50:38, dougarnett wrote: > > On 2016/04/26 00:56:13, Pete Williamson wrote: > > > On 2016/04/26 00:52:19, Pete Williamson wrote: > > > > KeyedService -> BrowserContextKeyedService > > > > > > > > Once we have a KeyedService, we will also need a factory (maybe better to > > not > > > > inherit from KeyedService yet, and add that with a future change including > > the > > > > factory) > > > > > > Oops, my bad, it looks like it is KeyedService after all. (The Factory is > > > BrowserContextKeyedServiceFactory). > > > > I'm happy to take out KeyedService for this CL or leave in if sufficient as is > > and seems helpful. > > Let's take out the KeyedService, and add it back when we add a factory. Done
lgtm
lgtm
dewittj@chromium.org changed reviewers: + dewittj@chromium.org
no BUILD.gn or gyp changes? https://codereview.chromium.org/1922533003/diff/80001/components/offline_page... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/1922533003/diff/80001/components/offline_page... components/offline_pages/background/offliner.h:28: typedef base::Callback<void(const SavePageRequest& request, CompletionStatus)> I think that technically a forward declaration of SavePageRequest is insufficient to make this header compile, since you're using a const ref.
lgtm
On 2016/04/26 18:28:00, dewittj wrote: > no BUILD.gn or gyp changes? > > https://codereview.chromium.org/1922533003/diff/80001/components/offline_page... > File components/offline_pages/background/offliner.h (right): > > https://codereview.chromium.org/1922533003/diff/80001/components/offline_page... > components/offline_pages/background/offliner.h:28: typedef > base::Callback<void(const SavePageRequest& request, CompletionStatus)> > I think that technically a forward declaration of SavePageRequest is > insufficient to make this header compile, since you're using a const ref. Pete will be adding that once this lands.
The CQ bit was checked by dougarnett@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1922533003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1922533003/100001
The CQ bit was unchecked by dougarnett@chromium.org
Pete will follow up with gn/gypi updates. Committing ... https://codereview.chromium.org/1922533003/diff/80001/components/offline_page... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/1922533003/diff/80001/components/offline_page... components/offline_pages/background/offliner.h:28: typedef base::Callback<void(const SavePageRequest& request, CompletionStatus)> On 2016/04/26 18:28:00, dewittj wrote: > I think that technically a forward declaration of SavePageRequest is > insufficient to make this header compile, since you're using a const ref. replaced with struct for this CL/iteration
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, petewil@chromium.org, dimich@chromium.org Link to the patchset: https://codereview.chromium.org/1922533003/#ps120001 (title: "Changed forward reference of SavePageRequest to empty stuct for this CL per dewittj feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1922533003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1922533003/120001
Message was sent while issue was closed.
Description was changed from ========== Defines initial inner circle of interfaces for background offlining. BUG= ========== to ========== Defines initial inner circle of interfaces for background offlining. BUG= ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Defines initial inner circle of interfaces for background offlining. BUG= ========== to ========== Defines initial inner circle of interfaces for background offlining. BUG= Committed: https://crrev.com/7ccea9c5ace7e2e11dfcce807707b1962341a784 Cr-Commit-Position: refs/heads/master@{#389846} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7ccea9c5ace7e2e11dfcce807707b1962341a784 Cr-Commit-Position: refs/heads/master@{#389846} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
