Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1968593002:
PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle (Closed)
|
Created:
4 years, 7 months ago by dougarnett Modified:
4 years, 6 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrerenderingLoader initial integration with PrerenderManager/PrerenderHandle and make it unit-testable.
It proved challenging to write unittests wrt mocking PrerenderManager and/or PrerenderHandler and be able to create a stub PrerenderHandler without touching the prerender stack code so this CL takes the approach of adding an adapter between the Loader and prerender stack calls so that they can be intercepted for testing.
BTW, PrerenderingLoader is actually the "BackgroundLoader" of go/chrome-background-loading
BUG=601173
Committed: https://crrev.com/8ed74fd7c401a1ff9a5a57c270409e0f66f6d3ce
Cr-Commit-Position: refs/heads/master@{#395485}
Patch Set 1 #
Total comments: 47
Patch Set 2 : Uploaded new unittest file that was missing and addressed some petewil feedback #
Total comments: 4
Patch Set 3 : Address some feedback (some remains for followup) #
Total comments: 10
Patch Set 4 : Split out Adapter into separate class/file and addressed some pasko feedback #Patch Set 5 : Some fixes for trybot failure #Patch Set 6 : Removed OnPrerenderCreatedMatchCompleteReplacement() override as the interface method looks to be d… #Patch Set 7 : Add PrerenderingLoader initialization that was missing #Patch Set 8 : An initialization order fix #
Total comments: 30
Patch Set 9 : Changes per pasko feedback #
Total comments: 43
Patch Set 10 : Address some gabadie feedback #
Total comments: 26
Patch Set 11 : Address fgorski feedback #Patch Set 12 : Missing overrides caught by trybot #Patch Set 13 : Add some Loader state_ unit test checks #Patch Set 14 : Added use of Observer delegate to keep Loader event handling methods private (also merge to Coordin… #Patch Set 15 : Fixe components unit tests from merge and update to scoped enumerator for status #Patch Set 16 : uninlined ObserverDelegate impl #
Total comments: 59
Patch Set 17 : Address fgorski & pasko feedback (except for additional unit tests) #Patch Set 18 : Added more unit tests for PrerenderingLoader #
Total comments: 14
Patch Set 19 : Moves PrerenderHandler Observer into the PrerenderAdapter and add support for 2nd callback after LO… #Patch Set 20 : virtual => override nits #
Total comments: 30
Patch Set 21 : More updates for pasko #
Total comments: 11
Patch Set 22 : Tweaks per pasko feedback #Dependent Patchsets: Messages
Total messages: 54 (11 generated)
Description was changed from ========== PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle and make unit-testable. It proved challenging to write unittests wrt mocking PrerenderManager and/or PrerenderHandler and be able to create a stub PrerenderHandler without touching the prerender stack code so this CL takes the approach of adding an adapter between the Loader and prerender stack calls so that they can be intercepted for testing. BUG=601173 ========== to ========== PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle and make it unit-testable. It proved challenging to write unittests wrt mocking PrerenderManager and/or PrerenderHandler and be able to create a stub PrerenderHandler without touching the prerender stack code so this CL takes the approach of adding an adapter between the Loader and prerender stack calls so that they can be intercepted for testing. BUG=601173 ==========
dougarnett@chromium.org changed reviewers: + fgorski@chromium.org, pasko@chromium.org, petewil@chromium.org
Description was changed from ========== PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle and make it unit-testable. It proved challenging to write unittests wrt mocking PrerenderManager and/or PrerenderHandler and be able to create a stub PrerenderHandler without touching the prerender stack code so this CL takes the approach of adding an adapter between the Loader and prerender stack calls so that they can be intercepted for testing. BUG=601173 ========== to ========== PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle and make it unit-testable. It proved challenging to write unittests wrt mocking PrerenderManager and/or PrerenderHandler and be able to create a stub PrerenderHandler without touching the prerender stack code so this CL takes the approach of adding an adapter between the Loader and prerender stack calls so that they can be intercepted for testing. BTW, PrerenderingLoader is actually the "BackgroundLoader" of go/chrome-background-loading BUG=601173 ==========
https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:21: adapter_.reset(new PrerenderingAdapter()); If we pass in the adapter as an argument, we won't need the override for testing method. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:64: // TODO(dougarnett): Create separate namespace from default. A comment should say *why* we use a separate session storage namespace. (Avoid contaminating user history, etc). https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:67: ->GetController() I don't think we need a newline here https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:74: ->GetContainerBounds() I think I normally see -> at the end of the line above instead of on the continuation line. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:75: .size(); I don't think we need a newline here (before .size();) https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:99: void PrerenderingLoader::ReleaseCurrentLoad() { Naming suggestion: CancelPrerender() https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:115: ReportLoaded(); So we seem to return "loaded" for both stop loading and com content loaded, does that mean ReportLoaded is designed to be called twice? https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:159: active_handle_.reset( // Start the prerender manager doing the render of the target url. A comment here might help maintainers later, since this seems to be the important handoff point. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:179: return active_handle_->contents()->prerender_contents(); Maybe add a comment about ownership of the web contents, and how it must be freed? https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.h:35: class PrerenderingAdapter { Would this be better in its own file? It is starting to seem a bit heavyweight for a nested class. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.h:109: // request. IsAvailable() may be a better name than IsIdle(). https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.h:144: enum class State { kIdle, kPending, kLoading, kLoaded }; I don't think this is wrong or bad as is, but as I recall I normally see one per line, and a end of line comment explaining it https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.h:147: // Not owned. Suggestion to avoid ambiguity: browser_context_ is an unowned pointer. https://codereview.chromium.org/1968593002/diff/1/chrome/chrome_tests_unit.gypi File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/1968593002/diff/1/chrome/chrome_tests_unit.gy... chrome/chrome_tests_unit.gypi:1641: 'browser/android/offline_pages/prerendering_loader_unittest.cc', I don't see this file in the changelist. I sometimes see this when I forget to do a git add on it.
https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:21: adapter_.reset(new PrerenderingAdapter()); On 2016/05/10 20:00:27, Pete Williamson wrote: > If we pass in the adapter as an argument, we won't need the override for testing > method. Maybe use factory methods then to inject appropriate adapter? https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:64: // TODO(dougarnett): Create separate namespace from default. On 2016/05/10 20:00:27, Pete Williamson wrote: > A comment should say *why* we use a separate session storage namespace. (Avoid > contaminating user history, etc). Done. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:67: ->GetController() On 2016/05/10 20:00:27, Pete Williamson wrote: > I don't think we need a newline here not my original edit - brought to you by "git cl format" https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:74: ->GetContainerBounds() On 2016/05/10 20:00:27, Pete Williamson wrote: > I think I normally see -> at the end of the line above instead of on the > continuation line. ditto https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:75: .size(); On 2016/05/10 20:00:27, Pete Williamson wrote: > I don't think we need a newline here (before .size();) ditto https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:99: void PrerenderingLoader::ReleaseCurrentLoad() { On 2016/05/10 20:00:27, Pete Williamson wrote: > Naming suggestion: CancelPrerender() Done. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:115: ReportLoaded(); On 2016/05/10 20:00:27, Pete Williamson wrote: > So we seem to return "loaded" for both stop loading and com content loaded, does > that mean ReportLoaded is designed to be called twice? Not initially, the state change from kLoading to kLoaded is meant to limit to once (need/todo to add test for this). When timeouts are implemented, then it will be combination of the event plus its timeout after it as to which will report loaded. We may revisit this with some experience - that is, if worth trying two snapshots based from the two events (if second one happens before overall time limit). https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:159: active_handle_.reset( On 2016/05/10 20:00:27, Pete Williamson wrote: > // Start the prerender manager doing the render of the target url. > > A comment here might help maintainers later, since this seems to be the > important handoff point. Done. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:179: return active_handle_->contents()->prerender_contents(); On 2016/05/10 20:00:27, Pete Williamson wrote: > Maybe add a comment about ownership of the web contents, and how it must be > freed? Beefed up .h comment and added .cc comment as well. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.h:35: class PrerenderingAdapter { On 2016/05/10 20:00:27, Pete Williamson wrote: > Would this be better in its own file? It is starting to seem a bit heavyweight > for a nested class. Yes, maybe so. Would like to get some review from Filip and Egor that this adapter approach is acceptable and then will do it. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.h:109: // request. On 2016/05/10 20:00:27, Pete Williamson wrote: > IsAvailable() may be a better name than IsIdle(). Maybe, or IsReady https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.h:144: enum class State { kIdle, kPending, kLoading, kLoaded }; On 2016/05/10 20:00:27, Pete Williamson wrote: > I don't think this is wrong or bad as is, but as I recall I normally see one per > line, and a end of line comment explaining it Think I saw this style from recent dimich CL so tried it out. The other style will scale better. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.h:147: // Not owned. On 2016/05/10 20:00:27, Pete Williamson wrote: > Suggestion to avoid ambiguity: browser_context_ is an unowned pointer. Moved to be a suffix comment. I don't want to set up the comment for inconsistency on renaming member. https://codereview.chromium.org/1968593002/diff/1/chrome/chrome_tests_unit.gypi File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/1968593002/diff/1/chrome/chrome_tests_unit.gy... chrome/chrome_tests_unit.gypi:1641: 'browser/android/offline_pages/prerendering_loader_unittest.cc', On 2016/05/10 20:00:27, Pete Williamson wrote: > I don't see this file in the changelist. I sometimes see this when I forget to > do a git add on it. thx!
https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:21: adapter_.reset(new PrerenderingAdapter()); On 2016/05/10 20:00:27, Pete Williamson wrote: > If we pass in the adapter as an argument, we won't need the override for testing > method. also, if you are initializing in constructor, please use the initializer list. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:31: LOG(WARNING) << "Existing request pending"; use DVLOG LOG is too heavy. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:67: ->GetController() On 2016/05/10 20:00:27, Pete Williamson wrote: > I don't think we need a newline here Make sure to run git cl format https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:95: callback_.Run(false, nullptr); add a todo, that you want to post it to the thread instead. This prevents a retry when you have this method still on stack. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:106: if (adapter_->IsActive(handle) && state_ == State::kPending) { nit: skip {} Also consider if this should be DCHECK() of some conditions in addition to if(). What if the criteria are not met? Is it normal? https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:135: LOG(WARNING) << "Offlining prerender should never do replacement"; dvlog https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:190: return active_handle_ ? true : false; return active_handle_; https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.h:35: class PrerenderingAdapter { On 2016/05/10 20:00:27, Pete Williamson wrote: > Would this be better in its own file? It is starting to seem a bit heavyweight > for a nested class. +1 what to Pete suggested, and also, PrerenderAdapter seems like a good name too. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.h:87: virtual ~PrerenderingLoader(); scratch the virtual, add override;
Changes you have made so far look good, I'll take another look when the others go in. https://codereview.chromium.org/1968593002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_loader_unittest.cc (right): https://codereview.chromium.org/1968593002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:194: nullptr /* handle */); Handle is not very helpful here. handle to PrerenderManager, perhaps? https://codereview.chromium.org/1968593002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:197: EXPECT_FALSE(did_load()); // Per no WebContents provided in callback. // Since we didn't provide a web contents to the test adapter, it did not send one with the callback, so the load fails. Maybe a more explicit comment like the one above will make the test code easier for a maintenance programmer to understand.
Still pending on where adapter lives and if passed in (and it right approach). I agree on spirit of moving it out but at same time it is a very tightly, private-ly related to Loader at this point. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:21: adapter_.reset(new PrerenderingAdapter()); On 2016/05/10 21:36:27, fgorski wrote: > On 2016/05/10 20:00:27, Pete Williamson wrote: > > If we pass in the adapter as an argument, we won't need the override for > testing > > method. > > also, if you are initializing in constructor, please use the initializer list. Revised to use initializer list for now - but haven't yet addressed passing in. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:31: LOG(WARNING) << "Existing request pending"; On 2016/05/10 21:36:27, fgorski wrote: > use DVLOG > > LOG is too heavy. Done. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:95: callback_.Run(false, nullptr); On 2016/05/10 21:36:27, fgorski wrote: > add a todo, that you want to post it to the thread instead. > This prevents a retry when you have this method still on stack. Done. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:106: if (adapter_->IsActive(handle) && state_ == State::kPending) { On 2016/05/10 21:36:27, fgorski wrote: > nit: skip {} > > Also consider if this should be DCHECK() of some conditions in addition to if(). > What if the criteria are not met? Is it normal? I've been wondering if there is some different project style guideline for braces? Since this is what I found in the style guide: "In general, curly braces are not required for single-line statements, but they are allowed if you like them" I generally anticipate more code could be added within these clauses in this family of callbacks - vs. simple one-liner return on the condition. <Java guy struggling with no-brace-guilt> https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:135: LOG(WARNING) << "Offlining prerender should never do replacement"; On 2016/05/10 21:36:27, fgorski wrote: > dvlog Done https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:190: return active_handle_ ? true : false; On 2016/05/10 21:36:27, fgorski wrote: > return active_handle_; Compiler not happy with that - but maybe return active_handle_.get(); is better https://codereview.chromium.org/1968593002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_loader_unittest.cc (right): https://codereview.chromium.org/1968593002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:194: nullptr /* handle */); On 2016/05/10 22:13:15, Pete Williamson wrote: > Handle is not very helpful here. handle to PrerenderManager, perhaps? Done. https://codereview.chromium.org/1968593002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:197: EXPECT_FALSE(did_load()); // Per no WebContents provided in callback. On 2016/05/10 22:13:15, Pete Williamson wrote: > // Since we didn't provide a web contents to the test adapter, it did not send > one with the callback, so the load fails. > > Maybe a more explicit comment like the one above will make the test code easier > for a maintenance programmer to understand. Done.
LGTM with nits - After thinking about it, some of the formatting really does bother me, even though git cl format did it. IMHO, it doesn't always do the best thing, even though you will never get dinged for using it.
Great to see this! I am starting by looking at the use of the Prerender API, will take a look at everything else after we resolve these questions. Plus some bonus questions about the toplevel classes. https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.cc:63: // TODO(dougarnett): Create separate namespace from default (to better my reading says that there is a 1:1 mapping between WebContentsImpl and its NavigationControllerImpl. The latter associates session_storage_namespace_map_[""] with a SessionStorageNamespace on the first call to GetDefaultSessionStorageNamespace(). So, if we avoid reusing the WebContents, the session storage namespace won't be reused. Also, I am not very comfortable having this TODO for more than a week, because reusing the namespace with another (such as customtabs), would crash the browser. https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.cc:65: return content::WebContents::Create( This would leak WebContents, right? I think we need to keep this tiny WebContents around until the prerendered WebContents is destroyed. https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.cc:83: // TODO(dougarnett): Post callback on the thread. which thread? https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.h:30: // the page loading in the background. what are the threading-related guarantees of this class? does it live on a single thread? which one? https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.h:64: virtual content::WebContents* GetPrerenderContents() const; probably should be named as GetWebContents() because it does not return PrerenderContents.
https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.cc:63: // TODO(dougarnett): Create separate namespace from default (to better On 2016/05/11 12:18:34, pasko wrote: > my reading says that there is a 1:1 mapping between WebContentsImpl and its > NavigationControllerImpl. > > The latter associates session_storage_namespace_map_[""] with a > SessionStorageNamespace on the first call to > GetDefaultSessionStorageNamespace(). > > So, if we avoid reusing the WebContents, the session storage namespace won't be > reused. > > Also, I am not very comfortable having this TODO for more than a week, because > reusing the namespace with another (such as customtabs), would crash the > browser. Nice https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.cc:65: return content::WebContents::Create( On 2016/05/11 12:18:34, pasko wrote: > This would leak WebContents, right? I think we need to keep this tiny > WebContents around until the prerendered WebContents is destroyed. Now holding onto this WebContents instance (in session_contents_) and releasing in CancelPrerender() https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.cc:83: // TODO(dougarnett): Post callback on the thread. On 2016/05/11 12:18:34, pasko wrote: > which thread? Current thread. Went ahead and added the PostTask's https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.h:30: // the page loading in the background. On 2016/05/11 12:18:34, pasko wrote: > what are the threading-related guarantees of this class? does it live on a > single thread? which one? Updated comment. Single thread which needs to be UI thread if/when actually integrating with the PrerenderManager. I'm not sure if it preferred to make those DCHECKs here or just let PrerenderManager DCHECKs catch it. https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.h:64: virtual content::WebContents* GetPrerenderContents() const; On 2016/05/11 12:18:34, pasko wrote: > probably should be named as GetWebContents() because it does not return > PrerenderContents. Done.
On 2016/05/10 23:19:55, Pete Williamson wrote: > LGTM with nits - After thinking about it, some of the formatting really does > bother me, even though git cl format did it. IMHO, it doesn't always do the best > thing, even though you will never get dinged for using it. Btw, the two ugly ones have had some refactoring that had the side effect of removing ugly format outcome.
On 2016/05/11 21:10:15, dougarnett wrote: > https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... > File chrome/browser/android/offline_pages/prerendering_loader.cc (right): > > https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... > chrome/browser/android/offline_pages/prerendering_loader.cc:63: // > TODO(dougarnett): Create separate namespace from default (to better > On 2016/05/11 12:18:34, pasko wrote: > > my reading says that there is a 1:1 mapping between WebContentsImpl and its > > NavigationControllerImpl. > > > > The latter associates session_storage_namespace_map_[""] with a > > SessionStorageNamespace on the first call to > > GetDefaultSessionStorageNamespace(). > > > > So, if we avoid reusing the WebContents, the session storage namespace won't > be > > reused. > > > > Also, I am not very comfortable having this TODO for more than a week, because > > reusing the namespace with another (such as customtabs), would crash the > > browser. > > Nice > > https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... > chrome/browser/android/offline_pages/prerendering_loader.cc:65: return > content::WebContents::Create( > On 2016/05/11 12:18:34, pasko wrote: > > This would leak WebContents, right? I think we need to keep this tiny > > WebContents around until the prerendered WebContents is destroyed. > > Now holding onto this WebContents instance (in session_contents_) and releasing > in CancelPrerender() > > https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... > chrome/browser/android/offline_pages/prerendering_loader.cc:83: // > TODO(dougarnett): Post callback on the thread. > On 2016/05/11 12:18:34, pasko wrote: > > which thread? > > Current thread. Went ahead and added the PostTask's > > https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... > File chrome/browser/android/offline_pages/prerendering_loader.h (right): > > https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... > chrome/browser/android/offline_pages/prerendering_loader.h:30: // the page > loading in the background. > On 2016/05/11 12:18:34, pasko wrote: > > what are the threading-related guarantees of this class? does it live on a > > single thread? which one? > > Updated comment. Single thread which needs to be UI thread if/when actually > integrating with the PrerenderManager. I'm not sure if it preferred to make > those DCHECKs here or just let PrerenderManager DCHECKs catch it. > > https://codereview.chromium.org/1968593002/diff/40001/chrome/browser/android/... > chrome/browser/android/offline_pages/prerendering_loader.h:64: virtual > content::WebContents* GetPrerenderContents() const; > On 2016/05/11 12:18:34, pasko wrote: > > probably should be named as GetWebContents() because it does not return > > PrerenderContents. > > Done. Also, this CL is now merged with the PrerenderingOffliner CL that landed yesterday.
https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.cc (right): https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:84: active_handle_.reset(nullptr); PrerenderHandle destructor has this comment: // Before calling the destructor, the caller must invalidate the handle by // calling either OnNavigateAway or OnCancel. We should either provide a similar comment for the adapter or move the OnCancel body here. I would prefer the latter as less error prone. https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:32: class PrerenderAdapter { looks closer in meaning to something like PrerenderHandleAdapter https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:72: virtual bool IsActive(prerender::PrerenderHandle* handle) const; would handle->IsPrerendering() be sufficient to replace this method? https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:28: const LoadPageCallback& callback) { assert that runs on the correct thread for every method please https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:42: adapter_->AddPrerenderForOffline( does not check return value, which can be false in case Prerender gave up early (due to many concurrent prerenders or other reasons). https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:51: state_ = State::kLoading; Though the Google C++ Style Guide now says to use kConstantNaming for enums, Chromium was written using MACRO_STYLE naming. In enums that are actually enumerations (i.e. have multiple values), continue to use this style for consistency. Use kConstantNaming when using the "enum hack" to define a single constant, as you would for a const int or the like. from: http://www.chromium.org/developers/coding-style#Naming Also, see SHOUTY_CASE here: https://chromium-cpp.appspot.com/ https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:75: return contents->GetContainerBounds().size(); I suspect the size would be not what we want here, we'll probably need to set the size from outsize the loader. https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:83: base::ThreadTaskRunnerHandle::Get()->PostTask( why ThreadTaskRunnerHandle? Can this just call the callback? https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:100: base::Bind(callback_, Offliner::CompletionStatus::FAILED_CONSIDER_RETRY, should we assume non-retriable by default and retry based on a whitelist of final statuses? Feels safer against wasted retries this way. https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:110: session_contents_.reset(nullptr); we should run the callback here https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:141: void PrerenderingLoader::SetAdapterForTesting( the order of methods in the .cc file should match the order in the .h file https://codereview.chromium.org/1968593002/diff/140001/components/offline_pag... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/1968593002/diff/140001/components/offline_pag... components/offline_pages/background/offliner.h:19: enum CompletionStatus { since there are values like "LOADED" with indication that the request is not completed yet, having "Completion" in the name is misleading. Suggesting: offline_pages::OfflineRequestStatus offline_pages::Offliner::RequestProgressStatus https://codereview.chromium.org/1968593002/diff/140001/components/offline_pag... components/offline_pages/background/offliner.h:20: UNKNOWN = 0, // No completion status determined/reported yet. this value is not used in the code, use it in a test or remove. https://codereview.chromium.org/1968593002/diff/140001/components/offline_pag... components/offline_pages/background/offliner.h:25: FAILED_DO_NOT_RETRY = 5, do we need numerical constants here? Scoped enums (i.e. "enum class") are preferable in this case.
https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.cc (right): https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:84: active_handle_.reset(nullptr); On 2016/05/12 19:10:07, pasko wrote: > PrerenderHandle destructor has this comment: > // Before calling the destructor, the caller must invalidate the handle by > // calling either OnNavigateAway or OnCancel. > > We should either provide a similar comment for the adapter or move the OnCancel > body here. I would prefer the latter as less error prone. Done. https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:32: class PrerenderAdapter { On 2016/05/12 19:10:07, pasko wrote: > looks closer in meaning to something like PrerenderHandleAdapter It has ended up close to that with just 2 PrerenderManager calls. But from the point of view of the Loader it is taking care of all of the calls to the prerender:: namespace so I'd rather not tie its name specifically to the Handle .... unless we refactor from here. https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:72: virtual bool IsActive(prerender::PrerenderHandle* handle) const; On 2016/05/12 19:10:07, pasko wrote: > would handle->IsPrerendering() be sufficient to replace this method? Maybe for IsActive() which is worried about whether we still have a handle or not while PrerenderHandler::IsPrerendering() is worried about whether the handle is still connected to the PrerenderManager it seems. But this one with Handle arg is trying to guard against a callback from a previous operation being used - not sure if this is a real concern though yet. https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:28: const LoadPageCallback& callback) { On 2016/05/12 19:10:07, pasko wrote: > assert that runs on the correct thread for every method please Done. https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:42: adapter_->AddPrerenderForOffline( On 2016/05/12 19:10:07, pasko wrote: > does not check return value, which can be false in case Prerender gave up early > (due to many concurrent prerenders or other reasons). Done. https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:51: state_ = State::kLoading; On 2016/05/12 19:10:07, pasko wrote: > Though the Google C++ Style Guide now says to use kConstantNaming for enums, > Chromium was written using MACRO_STYLE naming. In enums that are actually > enumerations (i.e. have multiple values), continue to use this style for > consistency. Use kConstantNaming when using the "enum hack" to define a single > constant, as you would for a const int or the like. > > from: http://www.chromium.org/developers/coding-style#Naming > > Also, see SHOUTY_CASE here: https://chromium-cpp.appspot.com/ Done. https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:75: return contents->GetContainerBounds().size(); On 2016/05/12 19:10:07, pasko wrote: > I suspect the size would be not what we want here, we'll probably need to set > the size from outsize the loader. Any idea on how to determine it if no Tab context available? https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:83: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/05/12 19:10:07, pasko wrote: > why ThreadTaskRunnerHandle? Can this just call the callback? This one probably could. The PostTask was added per feedback from fgorski as a safer pattern (eg, so that callback's associated object can be checked if weakptr still good). He recently had a nasty crash by calling where the object was deleted. https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:100: base::Bind(callback_, Offliner::CompletionStatus::FAILED_CONSIDER_RETRY, On 2016/05/12 19:10:07, pasko wrote: > should we assume non-retriable by default and retry based on a whitelist of > final statuses? Feels safer against wasted retries this way. Done. https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:110: session_contents_.reset(nullptr); On 2016/05/12 19:10:07, pasko wrote: > we should run the callback here Done. https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:141: void PrerenderingLoader::SetAdapterForTesting( On 2016/05/12 19:10:07, pasko wrote: > the order of methods in the .cc file should match the order in the .h file Done. https://codereview.chromium.org/1968593002/diff/140001/components/offline_pag... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/1968593002/diff/140001/components/offline_pag... components/offline_pages/background/offliner.h:19: enum CompletionStatus { On 2016/05/12 19:10:07, pasko wrote: > since there are values like "LOADED" with indication that the request is not > completed yet, having "Completion" in the name is misleading. > > Suggesting: > > offline_pages::OfflineRequestStatus > offline_pages::Offliner::RequestProgressStatus Done. https://codereview.chromium.org/1968593002/diff/140001/components/offline_pag... components/offline_pages/background/offliner.h:20: UNKNOWN = 0, // No completion status determined/reported yet. On 2016/05/12 19:10:07, pasko wrote: > this value is not used in the code, use it in a test or remove. Actually meant to use in unit tests so added initialization. Or is there decent way to test for undefined value? https://codereview.chromium.org/1968593002/diff/140001/components/offline_pag... components/offline_pages/background/offliner.h:25: FAILED_DO_NOT_RETRY = 5, On 2016/05/12 19:10:07, pasko wrote: > do we need numerical constants here? > > Scoped enums (i.e. "enum class") are preferable in this case. Done.
gabadie@chromium.org changed reviewers: + gabadie@chromium.org
Fly by comments. =) https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.cc (right): https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:34: DCHECK(!IsActive()); DCHECK(CanPrerender()) ? https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:77: bool PrerenderAdapter::IsActive(prerender::PrerenderHandle* handle) const { The name of this method is miss leading with what it does. CompareHandle()? https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:82: if (IsActive()) { Why not asserting like other methods? https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:30: // prerendering request for offlining (ORIGIN_OFFLINE). This provides virtual ORIGIN_OFFLINE is prerender internal, please remove. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:41: virtual bool AddPrerenderForOffline( The name of this methods looks weird because this API assumes only one prerender. StartPrerender()? https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:48: virtual void SetObserver(prerender::PrerenderHandle::Observer* observer); You can remove this method and passdown observer as a parameter of AddPrerenderForOffline(). That would remove the state of having started a prerender without an attached observer. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:69: virtual bool IsActive(prerender::PrerenderHandle* handle) const; I think chromium style guideline don't allow overloaded methods. CompareHandle()? https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:90: if (adapter_->IsActive(handle)) { I think asserts here would be more appropriate that a call that does nothing. Unless I would be wrong and the Prerender API sends some different handles here? https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:99: if (adapter_->IsActive(handle)) { Please assert. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:101: ReportLoaded(); To me it looks weird that there is 2 ways we ReportLoaded(). Is there any particular reason? Yeah, the second call of ReportLoaded() would simply early return... https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:107: if (adapter_->IsActive(handle)) { Please assert. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:113: PrerenderingLoader::GetSessionStorageNamespace(content::WebContents* contents) { Must be in an anonymous namespace https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:117: const gfx::Size PrerenderingLoader::GetSize(content::WebContents* contents) { Must be in an anonymous namespace https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:122: if (state_ == State::LOADING) { Why not asserting? https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:136: if (state_ != State::LOADED && state_ != State::IDLE) { DCHECK(state_ == State::LOADING || state_ == State::PENDING) instead of the IF? https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:140: state_ = State::IDLE; Looks like here your state tracking is bugged because you are missing a session_contents_.reset(nullptr); and a adapter_->DestroyActive(); as you do in CancelPrerender(). Or may be do a LOAD_FAILED state? https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:33: class PrerenderingLoader : public prerender::PrerenderHandle::Observer { I think the inheritance can be private here. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:72: void OnPrerenderStop(prerender::PrerenderHandle* handle) override; I think PrerenderHandle should not be exposed to this API. I would make this methods private. https://codereview.chromium.org/1968593002/diff/160001/components/offline_pag... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/1968593002/diff/160001/components/offline_pag... components/offline_pages/background/offliner.h:24: FAILED_CONSIDER_RETRY, // Failed in a way that is subject to retry. Sorry, but where is this guy used?
Guillaume, thanks for the review! I did not understand your comments about anonymous namespace though - so please clarify here or email. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.cc (right): https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:34: DCHECK(!IsActive()); On 2016/05/13 07:55:37, gabadie wrote: > DCHECK(CanPrerender()) ? Done. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:77: bool PrerenderAdapter::IsActive(prerender::PrerenderHandle* handle) const { On 2016/05/13 07:55:37, gabadie wrote: > The name of this method is miss leading with what it does. CompareHandle()? Done. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:82: if (IsActive()) { On 2016/05/13 07:55:37, gabadie wrote: > Why not asserting like other methods? Primarily so it can be simply called in the dtor. It is meant to be an idempotent operation. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:30: // prerendering request for offlining (ORIGIN_OFFLINE). This provides virtual On 2016/05/13 07:55:37, gabadie wrote: > ORIGIN_OFFLINE is prerender internal, please remove. Done. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:41: virtual bool AddPrerenderForOffline( On 2016/05/13 07:55:37, gabadie wrote: > The name of this methods looks weird because this API assumes only one > prerender. StartPrerender()? Good idea. Yes, unfortunately this class has ended up with state rather than the straight across calls I was hoping to keep it to. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:48: virtual void SetObserver(prerender::PrerenderHandle::Observer* observer); On 2016/05/13 07:55:37, gabadie wrote: > You can remove this method and passdown observer as a parameter of > AddPrerenderForOffline(). That would remove the state of having started a > prerender without an attached observer. Thanks https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:69: virtual bool IsActive(prerender::PrerenderHandle* handle) const; On 2016/05/13 07:55:37, gabadie wrote: > I think chromium style guideline don't allow overloaded methods. > CompareHandle()? Trying HasHandle() https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:90: if (adapter_->IsActive(handle)) { On 2016/05/13 07:55:37, gabadie wrote: > I think asserts here would be more appropriate that a call that does nothing. > Unless I would be wrong and the Prerender API sends some different handles here? Yes, sounds good https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:99: if (adapter_->IsActive(handle)) { On 2016/05/13 07:55:37, gabadie wrote: > Please assert. Done. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:101: ReportLoaded(); On 2016/05/13 07:55:38, gabadie wrote: > To me it looks weird that there is 2 ways we ReportLoaded(). Is there any > particular reason? Yeah, the second call of ReportLoaded() would simply early > return... Not complete yet. Expect to have two tunable timeout values for the two events - whichever event plus its timeout occurs first will trigger the load callback. Eg, DomContentLoaded + 10 seconds and OnLoad/StopLoading + 2 seconds. This would be just an initial approach to decide when to snapshot. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:107: if (adapter_->IsActive(handle)) { On 2016/05/13 07:55:37, gabadie wrote: > Please assert. Done. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:113: PrerenderingLoader::GetSessionStorageNamespace(content::WebContents* contents) { On 2016/05/13 07:55:37, gabadie wrote: > Must be in an anonymous namespace Are you saying the function must be in C++ anonymous namespace or the WebContents must be in anonymous/incognito namespace? https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:122: if (state_ == State::LOADING) { On 2016/05/13 07:55:37, gabadie wrote: > Why not asserting? Took stab at a comment. If/when we have timeout triggered load events, either one could happen before the other or a StopLoading might have happened. This method is meant to be the one copy of logic checking on whether to issue the load callback or not. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:136: if (state_ != State::LOADED && state_ != State::IDLE) { On 2016/05/13 07:55:37, gabadie wrote: > DCHECK(state_ == State::LOADING || state_ == State::PENDING) instead of the IF? In my prototype I recall getting an OnPrerenderStop() callback after a successful load which is why this ended up here. I was trying to collect cases and callback management into these couple methods. Let me try revising their names anyway. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:140: state_ = State::IDLE; On 2016/05/13 07:55:37, gabadie wrote: > Looks like here your state tracking is bugged because you are missing a > session_contents_.reset(nullptr); and a adapter_->DestroyActive(); as you do in > CancelPrerender(). Or may be do a LOAD_FAILED state? Done. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:33: class PrerenderingLoader : public prerender::PrerenderHandle::Observer { On 2016/05/13 07:55:38, gabadie wrote: > I think the inheritance can be private here. Done, thanks https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:72: void OnPrerenderStop(prerender::PrerenderHandle* handle) override; On 2016/05/13 07:55:38, gabadie wrote: > I think PrerenderHandle should not be exposed to this API. I would make this > methods private. Done.
https://codereview.chromium.org/1968593002/diff/160001/components/offline_pag... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/1968593002/diff/160001/components/offline_pag... components/offline_pages/background/offliner.h:24: FAILED_CONSIDER_RETRY, // Failed in a way that is subject to retry. On 2016/05/13 07:55:38, gabadie wrote: > Sorry, but where is this guy used? The RequestCoordinator (not part of this CL) uses this to decide whether to drop the offlining request from the RequestQueue or to keep it for a future attempt (subject to retry count). The idea is for the PrerenderingLoader to bucket the prerender::FinalStatus codes into whether the operation might be subject to retry or not (TODO in this CL). If/when we add a server-rendering Offliner implementation, it would map its result codes into these buckets also to give the RequestCoordinator the same type of recommendation.
https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.cc (right): https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:10: #include "chrome/browser/prerender/prerender_manager.h" already included from header, but if you can forward declare there, you can keep it. https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:11: #include "chrome/browser/prerender/prerender_handle.h" Can any of these be forward declared? https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:31: if (state_ != State::IDLE) { if (!IsIdle()) https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:44: bool accepted = adapter_->StartPrerender( This code does not try to setobserver. Please make sure to try if the private inheritance does not prevent it from working. https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:64: // First check if prerendering is enabled. Is this comment relevant? https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:81: DCHECK(adapter_->HasHandle(handle)); Is there any state that is incorrect when this event is received. https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:110: PrerenderingLoader::GetSessionStorageNamespace(content::WebContents* contents) { since this is a private method and you own the contents passed in, why not simply refer to it here? https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:114: const gfx::Size PrerenderingLoader::GetSize(content::WebContents* contents) { ditto https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:11: #include "chrome/browser/android/offline_pages/prerender_adapter.h" Same here, perhaps you can forward declare some of these? https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:93: enum class State { define at the beginning of the private block https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:105: }; Consider DISALLOW_COPY_AND_ASSIGN...
https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.cc (right): https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:10: #include "chrome/browser/prerender/prerender_manager.h" On 2016/05/13 17:39:58, fgorski wrote: > already included from header, but if you can forward declare there, you can keep > it. Done. https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:11: #include "chrome/browser/prerender/prerender_handle.h" On 2016/05/13 17:39:58, fgorski wrote: > Can any of these be forward declared? I was trying to follow my interpretation of the style guide here, wrt: Con: "Forward declarations can hide a dependency, allowing user code to skip necessary recompilation when headers change." Decision: "Try to avoid forward declarations of entities defined in another project." https://google.github.io/styleguide/cppguide.html#Forward_Declarations https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:31: if (state_ != State::IDLE) { On 2016/05/13 17:39:58, fgorski wrote: > if (!IsIdle()) Done. https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:44: bool accepted = adapter_->StartPrerender( On 2016/05/13 17:39:58, fgorski wrote: > This code does not try to setobserver. Please make sure to try if the private > inheritance does not prevent it from working. Not sure what you mean. It used to SetObserver here but now observer is passed as StartPrerender arg and SetObserver done there (per gabadie comment). Do you mean to just try this locally to prove the inheritance is working? https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:64: // First check if prerendering is enabled. On 2016/05/13 17:39:58, fgorski wrote: > Is this comment relevant? Done. https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:81: DCHECK(adapter_->HasHandle(handle)); On 2016/05/13 17:39:58, fgorski wrote: > Is there any state that is incorrect when this event is received. Done. https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:110: PrerenderingLoader::GetSessionStorageNamespace(content::WebContents* contents) { On 2016/05/13 17:39:58, fgorski wrote: > since this is a private method and you own the contents passed in, why not > simply refer to it here? Done. https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:114: const gfx::Size PrerenderingLoader::GetSize(content::WebContents* contents) { On 2016/05/13 17:39:58, fgorski wrote: > ditto Done. https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:11: #include "chrome/browser/android/offline_pages/prerender_adapter.h" On 2016/05/13 17:39:58, fgorski wrote: > Same here, perhaps you can forward declare some of these? Acknowledged. https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:93: enum class State { On 2016/05/13 17:39:58, fgorski wrote: > define at the beginning of the private block Done. https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:105: }; On 2016/05/13 17:39:58, fgorski wrote: > Consider DISALLOW_COPY_AND_ASSIGN... Done.
https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:72: void OnPrerenderStop(prerender::PrerenderHandle* handle) override; On 2016/05/13 16:49:46, dougarnett wrote: > On 2016/05/13 07:55:38, gabadie wrote: > > I think PrerenderHandle should not be exposed to this API. I would make this > > methods private. > > Done. Actually, doesn't work to make private with using inheritance. So could try composition with inner Observer implementation.
PTAL - added delegate observer instead of inheritance to hide PrerenderHandler::Observer methods at Loader's public interface. Also merged to recent RequestCoordinator changes (wrt CompletionStatus -> RequestStatus and some lint).
Went back through the comments and responded to ones missing responses or needing some update. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.h:35: class PrerenderingAdapter { On 2016/05/10 21:36:27, fgorski wrote: > On 2016/05/10 20:00:27, Pete Williamson wrote: > > Would this be better in its own file? It is starting to seem a bit > heavyweight > > for a nested class. > > +1 what to Pete suggested, and also, PrerenderAdapter seems like a good name > too. Done. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.h:87: virtual ~PrerenderingLoader(); On 2016/05/10 21:36:27, fgorski wrote: > scratch the virtual, add override; Done. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.h:109: // request. On 2016/05/10 21:33:12, dougarnett wrote: > On 2016/05/10 20:00:27, Pete Williamson wrote: > > IsAvailable() may be a better name than IsIdle(). > > Maybe, or IsReady Kept as IsIdle(). Available and Ready seem more potentially confused wrt whether it is the Loader or some loaded contents that is Available/Ready. https://codereview.chromium.org/1968593002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.h:144: enum class State { kIdle, kPending, kLoading, kLoaded }; On 2016/05/10 21:33:12, dougarnett wrote: > On 2016/05/10 20:00:27, Pete Williamson wrote: > > I don't think this is wrong or bad as is, but as I recall I normally see one > per > > line, and a end of line comment explaining it > > Think I saw this style from recent dimich CL so tried it out. The other style > will scale better. Revised to other style. https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:75: return contents->GetContainerBounds().size(); On 2016/05/13 00:00:24, dougarnett wrote: > On 2016/05/12 19:10:07, pasko wrote: > > I suspect the size would be not what we want here, we'll probably need to set > > the size from outsize the loader. > > Any idea on how to determine it if no Tab context available? Egor, still open question for alternative. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:117: const gfx::Size PrerenderingLoader::GetSize(content::WebContents* contents) { On 2016/05/13 07:55:37, gabadie wrote: > Must be in an anonymous namespace Guillaume, can you explain further? I don't understand what you mean (C++ namespace scope?). https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:72: void OnPrerenderStop(prerender::PrerenderHandle* handle) override; On 2016/05/13 23:48:48, dougarnett wrote: > On 2016/05/13 16:49:46, dougarnett wrote: > > On 2016/05/13 07:55:38, gabadie wrote: > > > I think PrerenderHandle should not be exposed to this API. I would make this > > > methods private. > > > > Done. > > Actually, doesn't work to make private with using inheritance. So could try > composition with inner Observer implementation. Done - achieved hiding these methods with composition of an ObserverAdapter class
Looks better and better. Notice I left a couple of comments in Patch 10. https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:11: #include "chrome/browser/prerender/prerender_handle.h" On 2016/05/13 20:58:32, dougarnett wrote: > On 2016/05/13 17:39:58, fgorski wrote: > > Can any of these be forward declared? > > I was trying to follow my interpretation of the style guide here, wrt: > > Con: "Forward declarations can hide a dependency, allowing user code to skip > necessary recompilation when headers change." > > Decision: "Try to avoid forward declarations of entities defined in another > project." > > https://google.github.io/styleguide/cppguide.html#Forward_Declarations OK. Please follow chromium's code style above all others ;) http://www.chromium.org/developers/coding-style#TOC-Forward-declarations-vs.-... https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:44: bool accepted = adapter_->StartPrerender( On 2016/05/13 20:58:32, dougarnett wrote: > On 2016/05/13 17:39:58, fgorski wrote: > > This code does not try to setobserver. Please make sure to try if the private > > inheritance does not prevent it from working. > > Not sure what you mean. It used to SetObserver here but now observer is passed > as StartPrerender arg and SetObserver done there (per gabadie comment). > > Do you mean to just try this locally to prove the inheritance is working? Yes, getting the proof is what I was after. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.cc (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:53: DCHECK(IsActive()); I am wondering why you decided on a DCHECK here. why won't this be: return IsActive() && active_handle_->IsPrerendering(); ? https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:70: DCHECK(active_handle_->contents()); why is there a difference between GetWebContents and GetFinalStatus, with respect to how they fail when active_handle_->contents() == nullptr? https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:29: // An adapter for making calls into the prerender stack for managing an for managing a prerendering... https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:30: // prerendering request for offlining. This provides virtual methods This provides... you are explaining what interfaces are for in general terms. I don't think this description is needed here. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:50: // Returns whether actively prerendering. nit: whether adapter is actively? https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:57: // to report when it no longer needs the contents. the web contents. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:63: // Returns whether there is an active prerendering operation. Compare to documentation of IsPrerendering() and try to explain how these are different. Perhaps one of these should be private/protected. Also, I made a comment about implementation of IsPrerendering() that is relevant. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:40: DCHECK(!session_contents_.get()); don't need the .get() with ! https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:43: if (!observer_.get()) ditto https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:79: PrerenderAdapter* prerender_adapter) { std::unique_ptr<PrerenderAdapter> Please tell the compiler you are taking ownership of the adapter. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:174: : loader_(loader) {} DCHECK(loader); ? you are calling methods on it later, without checking. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:65: // Returns whether the loader has successfully loaded contents. nit: web contents. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:66: // Note that StopLoading() should be used to clear this state once nit: |StopLoading()| https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:67: // the loaded contents are no longer needed. the loaded web contents is https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:78: class ObserverDelegate : public prerender::PrerenderHandle::Observer { Can you name it PrerenderHandleObserver or HandleObserver. ObserverDelegate feel a bit like calling something HandlerManager or ManagerHandler and thinking of the two interchangeably. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:94: // State of the loader (only one request may be active at a time). the comment in () should be in the documentation of the class I think. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:127: std::unique_ptr<PrerenderAdapter> adapter_; More documentation on each one, please. This is important to understanding this class a bit better. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader_unittest.cc (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:23: class TestAdapter : public PrerenderAdapter { define in anonymous namespace, please can be inside of offline_pages namespace. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:228: Please add test, where LoadPage happens before the previous loading concluded. Please add test, where LoadPage is called when CanPrerender is false. Is it possible to test other calls to the observer? https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:43: void PrerenderingOffliner::SetLoaderForTesting( why did you decide to move it up? https://codereview.chromium.org/1968593002/diff/300001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1968593002/diff/300001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:80: LOG(ERROR) << "Unable to create Offliner. " change to DVLOG(), please, while you are at it.
apologies for long response times, we had a long weekend in France. https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.cc (right): https://codereview.chromium.org/1968593002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:20: PrerenderAdapter::~PrerenderAdapter() {} nit: empty line above https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:113: PrerenderingLoader::GetSessionStorageNamespace(content::WebContents* contents) { On 2016/05/13 16:49:45, dougarnett wrote: > On 2016/05/13 07:55:37, gabadie wrote: > > Must be in an anonymous namespace > > Are you saying the function must be in C++ anonymous namespace or the > WebContents must be in anonymous/incognito namespace? I think gabadie@ means that this function does not need the PrerenderingLoader state, and does not need to be exported, so it is better to limit its visibility further by putting it into the anonymous namespace (as in c++) in this .cc file. Same with GetSize() below. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.cc (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:53: DCHECK(IsActive()); On 2016/05/17 05:09:51, fgorski wrote: > I am wondering why you decided on a DCHECK here. > why won't this be: > return IsActive() && active_handle_->IsPrerendering(); ? I would support the DCHECK because the adapter is supposed to be 'just a mockable adapter' talking to the prerender stack. All cleverness about the prerendering state should sit on levels above. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:83: if (IsActive()) { We are checking for the same condition in prerendering_loader. I understand that it's probably for moar safety, but it makes the contract between the two classes less clear. For simplicity we should have DCHECK(IsActive()) here, and move all the cleverness away from the PrerenderAdapter. It is supposed to be the minimal possible surface for mocking. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:58: virtual content::WebContents* GetWebContents() const; Since Prerendering can destroy WebContents behind the scenes (via timeout, for example), we should have OnWebContentsDestroyed in the adapter to properly wrap up the uses of this instance of WebContents (such as serialization). I think the best way to observe it is by attaching a WebContentsObserver with ::WebContentsDestroyed() immediately after we successfully AddPrerenderForOffline(). With that, IsPrerendering() could be implemented via the observer you expose here, and not needed in the Adapter. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:63: // Returns whether there is an active prerendering operation. On 2016/05/17 05:09:52, fgorski wrote: > Compare to documentation of IsPrerendering() and try to explain how these are > different. Perhaps one of these should be private/protected. > > Also, I made a comment about implementation of IsPrerendering() that is > relevant. Comments to both methods are indeed confusing here. My today's reading of the prerender code suggests that: 1. PrerenderHandle is owned by the client of Prerendering, hence we control when we are IsActive() here. 2. IsPrerendering(), in contrary, can change behind the scenes. It is controlled by lifetime of PrerenderData in two lists: PrerenderManager::active_prerenders_ and PrerenderManager::to_delete_prerenders_; https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:40: DCHECK(!session_contents_.get()); On 2016/05/17 05:09:52, fgorski wrote: > don't need the .get() with ! FWIW, I would prefer to refer to session_contents in all contexts uniformly with .get(). It seems it is a preferable way in prerendering code. Not sure if it is covered by any style guide though.
Responded to Egor's comments. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.cc (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:53: DCHECK(IsActive()); On 2016/05/17 15:34:20, pasko wrote: > On 2016/05/17 05:09:51, fgorski wrote: > > I am wondering why you decided on a DCHECK here. > > why won't this be: > > return IsActive() && active_handle_->IsPrerendering(); ? > > I would support the DCHECK because the adapter is supposed to be 'just a > mockable adapter' talking to the prerender stack. All cleverness about the > prerendering state should sit on levels above. IsActive() is not a fall through but statement of internal state of the adapter. The call site is forced to do checks like this: if (IsActive() && IsPrerendering()) That is why I have a problem with DCHECK in this method. It does not affect the state, it is const and yet it may DCHECK. If we choose to keep the DCHECK I'll be fine with that, but a) let's document it b) let's stay aware of the fact that this is asking for trouble :) https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:63: // Returns whether there is an active prerendering operation. On 2016/05/17 15:34:20, pasko wrote: > On 2016/05/17 05:09:52, fgorski wrote: > > Compare to documentation of IsPrerendering() and try to explain how these are > > different. Perhaps one of these should be private/protected. > > > > Also, I made a comment about implementation of IsPrerendering() that is > > relevant. > > Comments to both methods are indeed confusing here. My today's reading of the > prerender code suggests that: > > 1. PrerenderHandle is owned by the client of Prerendering, hence we control when > we are IsActive() here. > > 2. IsPrerendering(), in contrary, can change behind the scenes. It is controlled > by lifetime of PrerenderData in two lists: PrerenderManager::active_prerenders_ > and PrerenderManager::to_delete_prerenders_; As long as it is documented I am OK with having both of course. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:40: DCHECK(!session_contents_.get()); On 2016/05/17 15:34:20, pasko wrote: > On 2016/05/17 05:09:52, fgorski wrote: > > don't need the .get() with ! > > FWIW, I would prefer to refer to session_contents in all contexts uniformly with > .get(). It seems it is a preferable way in prerendering code. Not sure if it is > covered by any style guide though. That sounds great, as long as we are consistent everywhere.
response to fgorski and some thoughts about toplevel design https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.cc (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:53: DCHECK(IsActive()); On 2016/05/17 15:54:54, fgorski wrote: > On 2016/05/17 15:34:20, pasko wrote: > > On 2016/05/17 05:09:51, fgorski wrote: > > > I am wondering why you decided on a DCHECK here. > > > why won't this be: > > > return IsActive() && active_handle_->IsPrerendering(); ? > > > > I would support the DCHECK because the adapter is supposed to be 'just a > > mockable adapter' talking to the prerender stack. All cleverness about the > > prerendering state should sit on levels above. > > IsActive() is not a fall through but statement of internal state of the adapter. > The call site is forced to do checks like this: > if (IsActive() && IsPrerendering()) There are only two methods that can affect this bit of state (StartPrerender() and DestroyActive()), and their behavior towards IsActive() is part of the contract between the Adapter and its clients. This means one can omit checks to IsActive() in many reasonable codepaths. I understand that this might be not the easiest contract. Alternatively the client could manage PrerenderHandleAdapter, created by injectable factory, and matching the lifetime of the PrerenderHandle. Created roughly by something like StartPrerender(), destroyed when necessary (before creating another prerender). Checks for IsActive() are not necessary then. Same mockability. WDYT? https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:106: void OnPrerenderStop(prerender::PrerenderHandle* handle); why should PrerenderingLoader know about the handle? I think management of the handle and events on it is logically closer to the adapter. The handle is used only for a DCHECK, and via a call to Adapter anyway. Looks out of place to me. https://codereview.chromium.org/1968593002/diff/300001/components/offline_pag... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/1968593002/diff/300001/components/offline_pag... components/offline_pages/background/offliner.h:24: FAILED_CONSIDER_RETRY, // Failed in a way that is subject to retry. We should do our best to avoid dead code, let's introduce new enum values not earlier than a use of them is introduced. This way it would be easier to review the changes to follow.
https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.cc (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:53: DCHECK(IsActive()); On 2016/05/17 16:26:31, pasko wrote: > On 2016/05/17 15:54:54, fgorski wrote: > > On 2016/05/17 15:34:20, pasko wrote: > > > On 2016/05/17 05:09:51, fgorski wrote: > > > > I am wondering why you decided on a DCHECK here. > > > > why won't this be: > > > > return IsActive() && active_handle_->IsPrerendering(); ? > > > > > > I would support the DCHECK because the adapter is supposed to be 'just a > > > mockable adapter' talking to the prerender stack. All cleverness about the > > > prerendering state should sit on levels above. > > > > IsActive() is not a fall through but statement of internal state of the > adapter. > > The call site is forced to do checks like this: > > if (IsActive() && IsPrerendering()) > > There are only two methods that can affect this bit of state (StartPrerender() > and DestroyActive()), and their behavior towards IsActive() is part of the > contract between the Adapter and its clients. This means one can omit checks to > IsActive() in many reasonable codepaths. > > I understand that this might be not the easiest contract. Alternatively the > client could manage PrerenderHandleAdapter, created by injectable factory, and > matching the lifetime of the PrerenderHandle. Created roughly by something like > StartPrerender(), destroyed when necessary (before creating another prerender). > Checks for IsActive() are not necessary then. Same mockability. WDYT? > Let's have Doug digest it and try to come up with necessary changes. I think this could be a sound approach, but anything with a solid contract between adapter and callers will work for me. I am looking for consistency in both design and implementation.
Addressed today's feedback and added more unit tests. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:113: PrerenderingLoader::GetSessionStorageNamespace(content::WebContents* contents) { On 2016/05/17 15:34:20, pasko wrote: > On 2016/05/13 16:49:45, dougarnett wrote: > > On 2016/05/13 07:55:37, gabadie wrote: > > > Must be in an anonymous namespace > > > > Are you saying the function must be in C++ anonymous namespace or the > > WebContents must be in anonymous/incognito namespace? > > I think gabadie@ means that this function does not need the PrerenderingLoader > state, and does not need to be exported, so it is better to limit its visibility > further by putting it into the anonymous namespace (as in c++) in this .cc file. > > Same with GetSize() below. Thx, makes sense. I have since modified to use member variable per other feedback so now not sure which feedback should win. https://codereview.chromium.org/1968593002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:117: const gfx::Size PrerenderingLoader::GetSize(content::WebContents* contents) { On 2016/05/16 23:51:38, dougarnett wrote: > On 2016/05/13 07:55:37, gabadie wrote: > > Must be in an anonymous namespace > > Guillaume, can you explain further? I don't understand what you mean (C++ > namespace scope?). Egor clarified. https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:11: #include "chrome/browser/prerender/prerender_handle.h" On 2016/05/17 05:09:51, fgorski wrote: > On 2016/05/13 20:58:32, dougarnett wrote: > > On 2016/05/13 17:39:58, fgorski wrote: > > > Can any of these be forward declared? > > > > I was trying to follow my interpretation of the style guide here, wrt: > > > > Con: "Forward declarations can hide a dependency, allowing user code to skip > > necessary recompilation when headers change." > > > > Decision: "Try to avoid forward declarations of entities defined in another > > project." > > > > https://google.github.io/styleguide/cppguide.html#Forward_Declarations > > OK. Please follow chromium's code style above all others ;) > > http://www.chromium.org/developers/coding-style#TOC-Forward-declarations-vs.-... ah, thanks for the pointer! Wasn't able to drop this one but clean up others. https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:44: bool accepted = adapter_->StartPrerender( On 2016/05/17 05:09:51, fgorski wrote: > On 2016/05/13 20:58:32, dougarnett wrote: > > On 2016/05/13 17:39:58, fgorski wrote: > > > This code does not try to setobserver. Please make sure to try if the > private > > > inheritance does not prevent it from working. > > > > Not sure what you mean. It used to SetObserver here but now observer is passed > > as StartPrerender arg and SetObserver done there (per gabadie comment). > > > > Do you mean to just try this locally to prove the inheritance is working? > > Yes, getting the proof is what I was after. Done (it did not work so dropped inheritance altogether). https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.cc (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:53: DCHECK(IsActive()); On 2016/05/17 17:06:19, fgorski wrote: > On 2016/05/17 16:26:31, pasko wrote: > > On 2016/05/17 15:54:54, fgorski wrote: > > > On 2016/05/17 15:34:20, pasko wrote: > > > > On 2016/05/17 05:09:51, fgorski wrote: > > > > > I am wondering why you decided on a DCHECK here. > > > > > why won't this be: > > > > > return IsActive() && active_handle_->IsPrerendering(); ? > > > > > > > > I would support the DCHECK because the adapter is supposed to be 'just a > > > > mockable adapter' talking to the prerender stack. All cleverness about the > > > > prerendering state should sit on levels above. > > > > > > IsActive() is not a fall through but statement of internal state of the > > adapter. > > > The call site is forced to do checks like this: > > > if (IsActive() && IsPrerendering()) > > > > There are only two methods that can affect this bit of state (StartPrerender() > > and DestroyActive()), and their behavior towards IsActive() is part of the > > contract between the Adapter and its clients. This means one can omit checks > to > > IsActive() in many reasonable codepaths. > > > > I understand that this might be not the easiest contract. Alternatively the > > client could manage PrerenderHandleAdapter, created by injectable factory, and > > matching the lifetime of the PrerenderHandle. Created roughly by something > like > > StartPrerender(), destroyed when necessary (before creating another > prerender). > > Checks for IsActive() are not necessary then. Same mockability. WDYT? > > > > Let's have Doug digest it and try to come up with necessary changes. > I think this could be a sound approach, but anything with a solid contract > between adapter and callers will work for me. I am looking for consistency in > both design and implementation. Simplifying by dropping IsPrerendering() altogether! [I have ended up using the OnPrerenderStart() observer call instead of this method to know when loading is in progress] https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:70: DCHECK(active_handle_->contents()); On 2016/05/17 05:09:51, fgorski wrote: > why is there a difference between GetWebContents and GetFinalStatus, with > respect to how they fail when active_handle_->contents() == nullptr? Put DCHECK in other one https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.cc:83: if (IsActive()) { On 2016/05/17 15:34:20, pasko wrote: > We are checking for the same condition in prerendering_loader. I understand that > it's probably for moar safety, but it makes the contract between the two classes > less clear. > > For simplicity we should have DCHECK(IsActive()) here, and move all the > cleverness away from the PrerenderAdapter. It is supposed to be the minimal > possible surface for mocking. Ok, I will move this IsActive() condition into the destructor as that is the usage it is covering. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:29: // An adapter for making calls into the prerender stack for managing an On 2016/05/17 05:09:52, fgorski wrote: > for managing a prerendering... Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:30: // prerendering request for offlining. This provides virtual methods On 2016/05/17 05:09:52, fgorski wrote: > This provides... you are explaining what interfaces are for in general terms. I > don't think this description is needed here. Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:50: // Returns whether actively prerendering. On 2016/05/17 05:09:52, fgorski wrote: > nit: whether adapter is actively? dropped method https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:57: // to report when it no longer needs the contents. On 2016/05/17 05:09:52, fgorski wrote: > the web contents. Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:58: virtual content::WebContents* GetWebContents() const; On 2016/05/17 15:34:20, pasko wrote: > Since Prerendering can destroy WebContents behind the scenes (via timeout, for > example), we should have OnWebContentsDestroyed in the adapter to properly wrap > up the uses of this instance of WebContents (such as serialization). > > I think the best way to observe it is by attaching a WebContentsObserver with > ::WebContentsDestroyed() immediately after we successfully > AddPrerenderForOffline(). > > With that, IsPrerendering() could be implemented via the observer you expose > here, and not needed in the Adapter. As discussed OnPrerenderStop() seems to be the better signal. Added some note about that in the comment. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:63: // Returns whether there is an active prerendering operation. On 2016/05/17 15:54:54, fgorski wrote: > On 2016/05/17 15:34:20, pasko wrote: > > On 2016/05/17 05:09:52, fgorski wrote: > > > Compare to documentation of IsPrerendering() and try to explain how these > are > > > different. Perhaps one of these should be private/protected. > > > > > > Also, I made a comment about implementation of IsPrerendering() that is > > > relevant. > > > > Comments to both methods are indeed confusing here. My today's reading of the > > prerender code suggests that: > > > > 1. PrerenderHandle is owned by the client of Prerendering, hence we control > when > > we are IsActive() here. > > > > 2. IsPrerendering(), in contrary, can change behind the scenes. It is > controlled > > by lifetime of PrerenderData in two lists: > PrerenderManager::active_prerenders_ > > and PrerenderManager::to_delete_prerenders_; > > As long as it is documented I am OK with having both of course. Please see if new wording is clear enough. Dropped IsPrerendering as not currently used anyway. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:40: DCHECK(!session_contents_.get()); On 2016/05/17 05:09:52, fgorski wrote: > don't need the .get() with ! Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:40: DCHECK(!session_contents_.get()); On 2016/05/17 15:34:20, pasko wrote: > On 2016/05/17 05:09:52, fgorski wrote: > > don't need the .get() with ! > > FWIW, I would prefer to refer to session_contents in all contexts uniformly with > .get(). It seems it is a preferable way in prerendering code. Not sure if it is > covered by any style guide though. Going without the .get() for now. PrerenderManager does not use .get() for DCHECK(contents_) but PrerenderContents does DCHECK(prerender_contents_.get()) https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:43: if (!observer_.get()) On 2016/05/17 05:09:52, fgorski wrote: > ditto Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:79: PrerenderAdapter* prerender_adapter) { On 2016/05/17 05:09:52, fgorski wrote: > std::unique_ptr<PrerenderAdapter> > Please tell the compiler you are taking ownership of the adapter. Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:174: : loader_(loader) {} On 2016/05/17 05:09:52, fgorski wrote: > DCHECK(loader); ? > > you are calling methods on it later, without checking. Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:65: // Returns whether the loader has successfully loaded contents. On 2016/05/17 05:09:52, fgorski wrote: > nit: web contents. Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:66: // Note that StopLoading() should be used to clear this state once On 2016/05/17 05:09:52, fgorski wrote: > nit: |StopLoading()| Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:67: // the loaded contents are no longer needed. On 2016/05/17 05:09:52, fgorski wrote: > the loaded web contents is Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:78: class ObserverDelegate : public prerender::PrerenderHandle::Observer { On 2016/05/17 05:09:52, fgorski wrote: > Can you name it PrerenderHandleObserver or HandleObserver. > > ObserverDelegate feel a bit like calling something HandlerManager or > ManagerHandler and thinking of the two interchangeably. Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:94: // State of the loader (only one request may be active at a time). On 2016/05/17 05:09:52, fgorski wrote: > the comment in () should be in the documentation of the class I think. Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:106: void OnPrerenderStop(prerender::PrerenderHandle* handle); On 2016/05/17 16:26:31, pasko wrote: > why should PrerenderingLoader know about the handle? I think management of the > handle and events on it is logically closer to the adapter. > > The handle is used only for a DCHECK, and via a call to Adapter anyway. Looks > out of place to me. Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:127: std::unique_ptr<PrerenderAdapter> adapter_; On 2016/05/17 05:09:52, fgorski wrote: > More documentation on each one, please. This is important to understanding this > class a bit better. Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader_unittest.cc (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:23: class TestAdapter : public PrerenderAdapter { On 2016/05/17 05:09:52, fgorski wrote: > define in anonymous namespace, please can be inside of offline_pages namespace. Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:228: On 2016/05/17 05:09:52, fgorski wrote: > Please add test, where LoadPage happens before the previous loading concluded. > > Please add test, where LoadPage is called when CanPrerender is false. > > Is it possible to test other calls to the observer? Done. https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/1968593002/diff/300001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:43: void PrerenderingOffliner::SetLoaderForTesting( On 2016/05/17 05:09:52, fgorski wrote: > why did you decide to move it up? To match ordering in .h file (feedback I got for another method). https://codereview.chromium.org/1968593002/diff/300001/components/offline_pag... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/1968593002/diff/300001/components/offline_pag... components/offline_pages/background/offliner.h:24: FAILED_CONSIDER_RETRY, // Failed in a way that is subject to retry. On 2016/05/17 16:26:32, pasko wrote: > We should do our best to avoid dead code, let's introduce new enum values not > earlier than a use of them is introduced. This way it would be easier to review > the changes to follow. Done. https://codereview.chromium.org/1968593002/diff/300001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/1968593002/diff/300001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:80: LOG(ERROR) << "Unable to create Offliner. " On 2016/05/17 05:09:52, fgorski wrote: > change to DVLOG(), please, while you are at it. Done.
looks cleaner, thank you https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:22: adapter_(new PrerenderAdapter()) {} If you are not planning to reset adapter_ in the PrerenderingLoader, making the adapter a plain PrerenderAdapter member (instead of unique_ptr of it) would make it easier to reason about the lifetime of the object, by only looking at the header. Please do. https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:43: if (!observer_.get()) It would be simpler to have the observer class as a member, and initialized in the PrerenderingLoader constructor. Less state. Lifetimes match. .. but I proposed above to make it a part of the adapter, that would be preferable. https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:86: void OnPrerenderStart(prerender::PrerenderHandle* handle) override; A cleaner design IMO is PrerenderHandleObserver being part of the adapter (probably under a different name, exposing all the same hooks, but without the handle as a parameter, and performing the DCHECK directly in the overridden methods. https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:90: void OnPrerenderStop(prerender::PrerenderHandle* handle) override; please dispatch OnPrerenderStop in tests https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader_unittest.cc (right): https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:154: std::unique_ptr<PrerenderAdapter>(test_adapter_)); base::WrapUnique(test_adapter_) allows to infer the type https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:164: void PrerenderingLoaderTest::PumpLoop() { looks like a static method
lgtm. Really nice. https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:22: adapter_(new PrerenderAdapter()) {} On 2016/05/18 11:47:26, pasko wrote: > If you are not planning to reset adapter_ in the PrerenderingLoader, making the > adapter a plain PrerenderAdapter member (instead of unique_ptr of it) would make > it easier to reason about the lifetime of the object, by only looking at the > header. Please do. There is setadapterfortesting(). I requested it to take the ownership. https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:82: explicit PrerenderHandleObserver(PrerenderingLoader* loader, no need for explicit, when number of arguments is different than 1.
https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:43: if (!observer_.get()) On 2016/05/18 11:47:26, pasko wrote: > It would be simpler to have the observer class as a member, and initialized in > the PrerenderingLoader constructor. Less state. Lifetimes match. > > .. but I proposed above to make it a part of the adapter, that would be > preferable. Done. https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:82: explicit PrerenderHandleObserver(PrerenderingLoader* loader, On 2016/05/19 04:29:39, fgorski wrote: > no need for explicit, when number of arguments is different than 1. Acknowledged. https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:86: void OnPrerenderStart(prerender::PrerenderHandle* handle) override; On 2016/05/18 11:47:26, pasko wrote: > A cleaner design IMO is PrerenderHandleObserver being part of the adapter > (probably under a different name, exposing all the same hooks, but without the > handle as a parameter, and performing the DCHECK directly in the overridden > methods. Done. https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:90: void OnPrerenderStop(prerender::PrerenderHandle* handle) override; On 2016/05/18 11:47:26, pasko wrote: > please dispatch OnPrerenderStop in tests Yes, good point. I need to report a Stop (perhaps CANCELED) after the LOADED has already been reported. [Done] https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader_unittest.cc (right): https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:154: std::unique_ptr<PrerenderAdapter>(test_adapter_)); On 2016/05/18 11:47:26, pasko wrote: > base::WrapUnique(test_adapter_) allows to infer the type nice https://codereview.chromium.org/1968593002/diff/340001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:164: void PrerenderingLoaderTest::PumpLoop() { On 2016/05/18 11:47:26, pasko wrote: > looks like a static method Done.
The CQ bit was checked by dougarnett@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/1968593002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1968593002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:10: #include "chrome/browser/prerender/prerender_handle.h" not needed https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:42: // Signals that the prerender has stopped running. let's also mention in the comment that it signals about invalidation of all previously returned GetWebContents() https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:31: DVLOG(1) << "WARNING: Existing request pending"; it might be LOADED and LOADING, not just pending https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:103: PrerenderingLoader::GetSessionStorageNamespace() { called only once, does this really need to be a separate method? https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:108: const gfx::Size PrerenderingLoader::GetSize() { this method is used only once, and the implementation is one line, but still nontrivial, so forces the reader of the callsite to lookup for definition. => inlining it makes it easier to read. https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:113: void PrerenderingLoader::ReportLoaded() { it does not only Report, but also performs state transitioning, making sure that calling it twice is possible. This makes the state transitioning scattered around non-trivially, and hence I think the code should rather primarily emphasize how the state transitioning is done, and what happens in between the transitions is secondary. I'm wondering how we could make it more obvious. One way is to mandate the transitioning functions to return the new state, and have the code that checks for the state be close to the lines of code that modify the state. This way it would be easy to see, for example, that ReportLoadingStopped() is only executed once and transitions from LOADING to LOADED. In the current code it requires a bit of reading to conclude that. let me know if the above is unclear :) https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:116: if (!IsLoaded() && !IsIdle()) { We are not supposed to call ReportLoaded with state_ == IDLE, hence this should be a DCHECK. Same for PENDING. AFAICT this should be a DCHECK(state_ == LOADING || state_ == LOADED) https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:120: base::ThreadTaskRunnerHandle::Get()->PostTask( I still do not understand the ThreadTaskRunnerHandle thing. First, why not just BrowserThread::PostTask(BrowserThread::UI, ...) ? Second, you were mentioning some crashes you could avoid by doing this. Can you provide concrete examples of a callbacks that would crash if called here and work fine if they are re-posted to the UI thread? https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:131: // Check if request is still active to see if reporting is applicable. "Applicable" is not concrete. It would be clearer if we say that we report it only once per load request. https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:76: // Prerender event handling logic: // PrerenderAdapter::Observer overrides. https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:95: const gfx::Size GetSize(); did you mean: gfx::Size GetSize() const; ? https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:46: loader_ = std::move(loader); std::move is a cast to an rvalue, and avoiding a cast seems beneficial, so how about: loader_.swap(loader) ?
https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:10: #include "chrome/browser/prerender/prerender_handle.h" On 2016/05/20 19:04:53, pasko wrote: > not needed It is here for the inheritance of PrerenderHandle::Observer. Do you have an alternative? https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:42: // Signals that the prerender has stopped running. On 2016/05/20 19:04:53, pasko wrote: > let's also mention in the comment that it signals about invalidation of all > previously returned GetWebContents() Done. https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:31: DVLOG(1) << "WARNING: Existing request pending"; On 2016/05/20 19:04:53, pasko wrote: > it might be LOADED and LOADING, not just pending Done. https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:103: PrerenderingLoader::GetSessionStorageNamespace() { On 2016/05/20 19:04:53, pasko wrote: > called only once, does this really need to be a separate method? Done. https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:108: const gfx::Size PrerenderingLoader::GetSize() { On 2016/05/20 19:04:53, pasko wrote: > this method is used only once, and the implementation is one line, but still > nontrivial, so forces the reader of the callsite to lookup for definition. => > inlining it makes it easier to read. Done. https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:113: void PrerenderingLoader::ReportLoaded() { On 2016/05/20 19:04:53, pasko wrote: > it does not only Report, but also performs state transitioning, making sure that > calling it twice is possible. > > This makes the state transitioning scattered around non-trivially, and hence I > think the code should rather primarily emphasize how the state transitioning is > done, and what happens in between the transitions is secondary. > > I'm wondering how we could make it more obvious. One way is to mandate the > transitioning functions to return the new state, and have the code that checks > for the state be close to the lines of code that modify the state. This way it > would be easy to see, for example, that ReportLoadingStopped() is only executed > once and transitions from LOADING to LOADED. In the current code it requires a > bit of reading to conclude that. > > let me know if the above is unclear :) Tried improved names and comments for these methods for their actual responsibilities. https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:116: if (!IsLoaded() && !IsIdle()) { On 2016/05/20 19:04:53, pasko wrote: > We are not supposed to call ReportLoaded with state_ == IDLE, hence this should > be a DCHECK. Same for PENDING. > > AFAICT this should be a DCHECK(state_ == LOADING || state_ == LOADED) see preceding comment https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:120: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/05/20 19:04:53, pasko wrote: > I still do not understand the ThreadTaskRunnerHandle thing. > > First, why not just BrowserThread::PostTask(BrowserThread::UI, ...) ? > > Second, you were mentioning some crashes you could avoid by doing this. Can you > provide concrete examples of a callbacks that would crash if called here and > work fine if they are re-posted to the UI thread? I don't know which type of PostTask is preferred. Although seems nicer to ensure posting on same thread (for single threaded code) instead of possibly posting on different, specific thread. The crash was doing direct callback call (instead of posting and returning up the call stack before the callback runs). As I understand it, this direct call end up call an object that was in the process of being deleted. https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:131: // Check if request is still active to see if reporting is applicable. On 2016/05/20 19:04:53, pasko wrote: > "Applicable" is not concrete. It would be clearer if we say that we report it > only once per load request. reworded https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:76: // Prerender event handling logic: On 2016/05/20 19:04:53, pasko wrote: > // PrerenderAdapter::Observer overrides. Done. https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:95: const gfx::Size GetSize(); On 2016/05/20 19:04:53, pasko wrote: > did you mean: > gfx::Size GetSize() const; > > ? Done. https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:46: loader_ = std::move(loader); On 2016/05/20 19:04:53, pasko wrote: > std::move is a cast to an rvalue, and avoiding a cast seems beneficial, so how > about: > > loader_.swap(loader) > > ? I don't know about that. This is the pattern I see for move-ing ownership and swap seems confusing to me wrt that intent. It is not hard to find examples of this pattern in online C++ guides. Do you have examples or doc pointers on swap being preferred?
prerender_adapter and prerendering_loader lgtm (though please answer my remaining non-critical questions) thank you, Doug! https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... File chrome/browser/android/offline_pages/prerender_adapter.h (right): https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerender_adapter.h:10: #include "chrome/browser/prerender/prerender_handle.h" On 2016/05/20 22:21:12, dougarnett wrote: > On 2016/05/20 19:04:53, pasko wrote: > > not needed > > It is here for the inheritance of PrerenderHandle::Observer. Do you have an > alternative? ah, you are right, I overlooked this fact, please ignore https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:113: void PrerenderingLoader::ReportLoaded() { On 2016/05/20 22:21:12, dougarnett wrote: > On 2016/05/20 19:04:53, pasko wrote: > > it does not only Report, but also performs state transitioning, making sure > that > > calling it twice is possible. > > > > This makes the state transitioning scattered around non-trivially, and hence I > > think the code should rather primarily emphasize how the state transitioning > is > > done, and what happens in between the transitions is secondary. > > > > I'm wondering how we could make it more obvious. One way is to mandate the > > transitioning functions to return the new state, and have the code that checks > > for the state be close to the lines of code that modify the state. This way it > > would be easy to see, for example, that ReportLoadingStopped() is only > executed > > once and transitions from LOADING to LOADED. In the current code it requires a > > bit of reading to conclude that. > > > > let me know if the above is unclear :) > > Tried improved names and comments for these methods for their actual > responsibilities. I still find the state transitions here to be complicated (like: re-entering HandleLoadingStopped() from different codepaths? is it when we asked to cancel, but prerender crashed at the same time?) - easy to forget to update the state_, etc. But since the code is relatively small, and me not having concrete better suggestions, unblocking the review is the best I can do. Thank you for clarifications, they helped. https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:120: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/05/20 22:21:12, dougarnett wrote: > On 2016/05/20 19:04:53, pasko wrote: > > I still do not understand the ThreadTaskRunnerHandle thing. > > > > First, why not just BrowserThread::PostTask(BrowserThread::UI, ...) ? > > > > Second, you were mentioning some crashes you could avoid by doing this. Can > you > > provide concrete examples of a callbacks that would crash if called here and > > work fine if they are re-posted to the UI thread? > > I don't know which type of PostTask is preferred. Although seems nicer to ensure > posting on same thread (for single threaded code) instead of possibly posting on > different, specific thread. OK, using ThreadTaskRunnerHandle to post to the same thread makes sense, and it seems commonly used. > The crash was doing direct callback call (instead of posting and returning up > the call stack before the callback runs). As I understand it, this direct call > end up call an object that was in the process of being deleted. This scenario does not seem possible. OnPrerenderStopLoading() and OnPrerenderDomContentLoaded() are arriving from the prerender stack and are delivered by posting tasks or sending IPCs from the renderer (or both). I cannot see a codepath that would trigger one of these events from one of the offline-related destructors. This could happen only if the destructor called this method on the same stack, right? Now there is a tradeoff: be on the safe side by posting a task, or save 10ms or sometimes 100ms by running the callback earlier. On the large scale of prerender-related events these savings probably do not matter, WDYT? https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:46: loader_ = std::move(loader); On 2016/05/20 22:21:12, dougarnett wrote: > On 2016/05/20 19:04:53, pasko wrote: > > std::move is a cast to an rvalue, and avoiding a cast seems beneficial, so how > > about: > > > > loader_.swap(loader) > > > > ? > > I don't know about that. This is the pattern I see for move-ing ownership and > swap seems confusing to me wrt that intent. It is not hard to find examples of > this pattern in online C++ guides. Do you have examples or doc pointers on swap > being preferred? There is no official recommendation to use one or the other. My intuition leans towards .swap(), but now that I think about it, this may be slightly more expensive than std::move, so I would understand why std::move would be preferable now. I saw examples of both in the code. If the code around .../android/offline_pages prefers std::move, then it is preferable to be consistent and use std::move here as well. Thank you for background! https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:113: if (!IsLoaded() && !IsIdle()) { Less nesting an less negation seems more readable: if (IsLoaded() || IsIdle()) return; content::WebContents* contents = ... https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:49: virtual bool LoadPage(const GURL& url, const LoadPageCallback& callback); A question related to adding virtual methods here. Does it mean that there is a plan to inherit from this class (in which case I am worried), or this area of code has a convention to default to virtual methods (in which case I am less worried, but still those ~1.5MiB of relocations to perform on every startup, and a policy like this would not be helpful to shrink this amount)? https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:54: // which case the callback will not be called). nit: s/called/run/ https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:97: // failue, cancel, or stop request). It may be due to some asynchronous typo: s/failue/failure/
Thanks for all the great feedback Egor! https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:113: void PrerenderingLoader::ReportLoaded() { On 2016/05/23 20:05:41, pasko wrote: > On 2016/05/20 22:21:12, dougarnett wrote: > > On 2016/05/20 19:04:53, pasko wrote: > > > it does not only Report, but also performs state transitioning, making sure > > that > > > calling it twice is possible. > > > > > > This makes the state transitioning scattered around non-trivially, and hence > I > > > think the code should rather primarily emphasize how the state transitioning > > is > > > done, and what happens in between the transitions is secondary. > > > > > > I'm wondering how we could make it more obvious. One way is to mandate the > > > transitioning functions to return the new state, and have the code that > checks > > > for the state be close to the lines of code that modify the state. This way > it > > > would be easy to see, for example, that ReportLoadingStopped() is only > > executed > > > once and transitions from LOADING to LOADED. In the current code it requires > a > > > bit of reading to conclude that. > > > > > > let me know if the above is unclear :) > > > > Tried improved names and comments for these methods for their actual > > responsibilities. > > I still find the state transitions here to be complicated (like: re-entering > HandleLoadingStopped() from different codepaths? is it when we asked to cancel, > but prerender crashed at the same time?) - easy to forget to update the state_, > etc. > > But since the code is relatively small, and me not having concrete better > suggestions, unblocking the review is the best I can do. Thank you for > clarifications, they helped. Will keep this feedback in mind going forward as I add timeouts to the load events for determining when to snapshot and a timeout from above that will cancel operation after some time budget and see if I can make clearer. https://codereview.chromium.org/1968593002/diff/380001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:120: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/05/23 20:05:41, pasko wrote: > On 2016/05/20 22:21:12, dougarnett wrote: > > On 2016/05/20 19:04:53, pasko wrote: > > > I still do not understand the ThreadTaskRunnerHandle thing. > > > > > > First, why not just BrowserThread::PostTask(BrowserThread::UI, ...) ? > > > > > > Second, you were mentioning some crashes you could avoid by doing this. Can > > you > > > provide concrete examples of a callbacks that would crash if called here and > > > work fine if they are re-posted to the UI thread? > > > > I don't know which type of PostTask is preferred. Although seems nicer to > ensure > > posting on same thread (for single threaded code) instead of possibly posting > on > > different, specific thread. > > OK, using ThreadTaskRunnerHandle to post to the same thread makes sense, and it > seems commonly used. > > > The crash was doing direct callback call (instead of posting and returning up > > the call stack before the callback runs). As I understand it, this direct call > > end up call an object that was in the process of being deleted. > > This scenario does not seem possible. OnPrerenderStopLoading() and > OnPrerenderDomContentLoaded() are arriving from the prerender stack and are > delivered by posting tasks or sending IPCs from the renderer (or both). I cannot > see a codepath that would trigger one of these events from one of the > offline-related destructors. This could happen only if the destructor called > this method on the same stack, right? > > Now there is a tradeoff: be on the safe side by posting a task, or save 10ms or > sometimes 100ms by running the callback earlier. On the large scale of > prerender-related events these savings probably do not matter, WDYT? I don't have a strong opinion here. I agree does not look possible so Run() is simpler/faster but also see Filip's point that it is on boundary of another subsystem that may change beneath us so good to be conservative. I was inclined to revisit with Filip and push to make Run()s but instead at this moment I think I want to leave as is for: 1. getting CL landed to move forward; and 2. it is not a hot code path (in fact, could be really cold code path wrt system testing) so maybe conservative approach slightly better than the optimization. https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:113: if (!IsLoaded() && !IsIdle()) { On 2016/05/23 20:05:41, pasko wrote: > Less nesting an less negation seems more readable: > > if (IsLoaded() || IsIdle()) > return; > > content::WebContents* contents = ... Done. https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:49: virtual bool LoadPage(const GURL& url, const LoadPageCallback& callback); On 2016/05/23 20:05:41, pasko wrote: > A question related to adding virtual methods here. > > Does it mean that there is a plan to inherit from this class (in which case I am > worried), or this area of code has a convention to default to virtual methods > (in which case I am less worried, but still those ~1.5MiB of relocations to > perform on every startup, and a policy like this would not be helpful to shrink > this amount)? The virtual methods here are to be able to mock it for testing. Specifically, there is a MockPrerenderingLoader in prerendering_offliner_unittests.cc https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:54: // which case the callback will not be called). On 2016/05/23 20:05:41, pasko wrote: > nit: s/called/run/ Done. https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:97: // failue, cancel, or stop request). It may be due to some asynchronous On 2016/05/23 20:05:41, pasko wrote: > typo: s/failue/failure/ Done.
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, fgorski@chromium.org, pasko@chromium.org Link to the patchset: https://codereview.chromium.org/1968593002/#ps420001 (title: "Tweaks per pasko feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1968593002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1968593002/420001
Message was sent while issue was closed.
Description was changed from ========== PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle and make it unit-testable. It proved challenging to write unittests wrt mocking PrerenderManager and/or PrerenderHandler and be able to create a stub PrerenderHandler without touching the prerender stack code so this CL takes the approach of adding an adapter between the Loader and prerender stack calls so that they can be intercepted for testing. BTW, PrerenderingLoader is actually the "BackgroundLoader" of go/chrome-background-loading BUG=601173 ========== to ========== PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle and make it unit-testable. It proved challenging to write unittests wrt mocking PrerenderManager and/or PrerenderHandler and be able to create a stub PrerenderHandler without touching the prerender stack code so this CL takes the approach of adding an adapter between the Loader and prerender stack calls so that they can be intercepted for testing. BTW, PrerenderingLoader is actually the "BackgroundLoader" of go/chrome-background-loading BUG=601173 ==========
Message was sent while issue was closed.
Committed patchset #22 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle and make it unit-testable. It proved challenging to write unittests wrt mocking PrerenderManager and/or PrerenderHandler and be able to create a stub PrerenderHandler without touching the prerender stack code so this CL takes the approach of adding an adapter between the Loader and prerender stack calls so that they can be intercepted for testing. BTW, PrerenderingLoader is actually the "BackgroundLoader" of go/chrome-background-loading BUG=601173 ========== to ========== PrerenderingLoader initial integration with PrerenderManager/PrerenderHandle and make it unit-testable. It proved challenging to write unittests wrt mocking PrerenderManager and/or PrerenderHandler and be able to create a stub PrerenderHandler without touching the prerender stack code so this CL takes the approach of adding an adapter between the Loader and prerender stack calls so that they can be intercepted for testing. BTW, PrerenderingLoader is actually the "BackgroundLoader" of go/chrome-background-loading BUG=601173 Committed: https://crrev.com/8ed74fd7c401a1ff9a5a57c270409e0f66f6d3ce Cr-Commit-Position: refs/heads/master@{#395485} ==========
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/8ed74fd7c401a1ff9a5a57c270409e0f66f6d3ce Cr-Commit-Position: refs/heads/master@{#395485}
Message was sent while issue was closed.
A revert of this CL (patchset #22 id:420001) has been created in https://codereview.chromium.org/2011913002/ by mvanouwerkerk@chromium.org. The reason for reverting is: The unit tests are failing on asan-clang-phone, I'm sorry. https://bugs.chromium.org/p/chromium/issues/detail?id=614699.
Message was sent while issue was closed.
https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:49: virtual bool LoadPage(const GURL& url, const LoadPageCallback& callback); On 2016/05/23 21:43:54, dougarnett wrote: > On 2016/05/23 20:05:41, pasko wrote: > > A question related to adding virtual methods here. > > > > Does it mean that there is a plan to inherit from this class (in which case I > am > > worried), or this area of code has a convention to default to virtual methods > > (in which case I am less worried, but still those ~1.5MiB of relocations to > > perform on every startup, and a policy like this would not be helpful to > shrink > > this amount)? > > The virtual methods here are to be able to mock it for testing. Specifically, > there is a MockPrerenderingLoader in prerendering_offliner_unittests.cc Makes sense, I did not know the mock already exists. So the only method not mocked out is CanPrerender(). Is it a TODO? Can the layers above be tested by reusing the TestAdapter or it is more difficult?
Message was sent while issue was closed.
dougarnett@chromium.org changed reviewers: - fgorski@chromium.org, gabadie@chromium.org, petewil@chromium.org
Message was sent while issue was closed.
Oh, just noticed I had this draft comment laying around ... https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:49: virtual bool LoadPage(const GURL& url, const LoadPageCallback& callback); On 2016/05/26 11:00:28, pasko wrote: > On 2016/05/23 21:43:54, dougarnett wrote: > > On 2016/05/23 20:05:41, pasko wrote: > > > A question related to adding virtual methods here. > > > > > > Does it mean that there is a plan to inherit from this class (in which case > I > > am > > > worried), or this area of code has a convention to default to virtual > methods > > > (in which case I am less worried, but still those ~1.5MiB of relocations to > > > perform on every startup, and a policy like this would not be helpful to > > shrink > > > this amount)? > > > > The virtual methods here are to be able to mock it for testing. Specifically, > > there is a MockPrerenderingLoader in prerendering_offliner_unittests.cc > > Makes sense, I did not know the mock already exists. So the only method not > mocked out is CanPrerender(). Is it a TODO? > > Can the layers above be tested by reusing the TestAdapter or it is more > difficult? Yes, I do need to mock CanPrerender as well, thanks! I think it is good idea to revisit merging Loader and Offliner and writing Offliner tests that use Loader and just mock the Adapter would be good way to start a merge. The Adapter has turned out to be a bit more comprehensive layer than when I first wrote the shells of Offliner and Loader so seems like it would work.
Message was sent while issue was closed.
https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.h (right): https://codereview.chromium.org/1968593002/diff/400001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.h:49: virtual bool LoadPage(const GURL& url, const LoadPageCallback& callback); On 2016/06/01 15:05:48, dougarnett wrote: > On 2016/05/26 11:00:28, pasko wrote: > > On 2016/05/23 21:43:54, dougarnett wrote: > > > On 2016/05/23 20:05:41, pasko wrote: > > > > A question related to adding virtual methods here. > > > > > > > > Does it mean that there is a plan to inherit from this class (in which > case > > I > > > am > > > > worried), or this area of code has a convention to default to virtual > > methods > > > > (in which case I am less worried, but still those ~1.5MiB of relocations > to > > > > perform on every startup, and a policy like this would not be helpful to > > > shrink > > > > this amount)? > > > > > > The virtual methods here are to be able to mock it for testing. > Specifically, > > > there is a MockPrerenderingLoader in prerendering_offliner_unittests.cc > > > > Makes sense, I did not know the mock already exists. So the only method not > > mocked out is CanPrerender(). Is it a TODO? > > > > Can the layers above be tested by reusing the TestAdapter or it is more > > difficult? > > Yes, I do need to mock CanPrerender as well, thanks! > > I think it is good idea to revisit merging Loader and Offliner and writing > Offliner tests that use Loader and just mock the Adapter would be good way to > start a merge. The Adapter has turned out to be a bit more comprehensive layer > than when I first wrote the shells of Offliner and Loader so seems like it would > work. nice, +1 here, thanks |
