Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2534673002:
[Offline pages] Create offliner that uses background loader (Closed)
|
Created:
4 years ago by chili Modified:
4 years ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages] Create offliner that uses background loader
BUG=665242
Committed: https://crrev.com/1ae92b9ebd0682f1c2abd530a6d191c779d9ce71
Cr-Commit-Position: refs/heads/master@{#439580}
Patch Set 1 #Patch Set 2 : test update #
Total comments: 12
Patch Set 3 : fooooo #Patch Set 4 : test and format #
Total comments: 47
Patch Set 5 : Code review responses #
Total comments: 4
Patch Set 6 : bleh it works now #Dependent Patchsets: Messages
Total messages: 52 (26 generated)
chili@chromium.org changed reviewers: + dougarnett@chromium.org, petewil@chromium.org
Notice that I have not finished the tests yet. Not fully happy with the test setup, so comments/suggestions requested. Working on some basic functionality tests right now Thought I'd send this out so we can get conversation started about the offliner. I've a separate CL for attaching the offliner to RC that is going to be sent soon as well. Thanks!
Looks like you have added tests - are those ready for review now then? https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:116: if (pending_request_ && is_low_end_device_ && Btw, I hope we can drop this for new loader (but may take some more experimenting with low end devices and/or A/B testing immediate processing on low end devices).
The test file you see is more of a setup (and it compiles! :D) There are no test cases written yet. I'm working on the set of scenarios and ironing that out right now https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:116: if (pending_request_ && is_low_end_device_ && On 2016/12/01 00:10:10, dougarnett wrote: > Btw, I hope we can drop this for new loader (but may take some more > experimenting with low end devices and/or A/B testing immediate processing on > low end devices). I was doing comparison between what I had and what the prerendering offliner had, and this was in there that wasn't here. I figured we can add it for now, and A/B test/remove it when we have more concrete data?
I haven't looked at the unit tests yet, but here is some preliminary feedback. https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:52: app_listener_.reset(new base::android::ApplicationStatusListener( Does the listener need to be set before we start loading, in case we start getting events right away? (For instance, a fast fail). https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:67: if (!pending_request_.get()) Maybe we should log this, it indicates a problem. https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:70: SavePageRequest request(*pending_request_.get()); Can DidStopLoading() also get called for a page that failed to load? I expected to see a check that it succeeded before calling SavePage. https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.h (right): https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.h:69: // Whether we are on a low-end device. We needed to track this for the prerenderer, because we know it had problems on svelte devices. Do you think your offliner will have problems on svelte devices, or are you just allowing for the possibility?
Looking good, lemme know when you add tests https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:109: loader_.reset( this is a bit interesting here so might be good to add a comment and might be a better place for the TODO comment than the .h file
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by chili@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by chili@chromium.org to run a CQ dry run
Tests are ready to take a look at. Thanks for taking a look! https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:52: app_listener_.reset(new base::android::ApplicationStatusListener( On 2016/12/01 02:03:38, Pete Williamson wrote: > Does the listener need to be set before we start loading, in case we start > getting events right away? (For instance, a fast fail). Done. https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:67: if (!pending_request_.get()) On 2016/12/01 02:03:38, Pete Williamson wrote: > Maybe we should log this, it indicates a problem. done. https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:70: SavePageRequest request(*pending_request_.get()); On 2016/12/01 02:03:38, Pete Williamson wrote: > Can DidStopLoading() also get called for a page that failed to load? I expected > to see a check that it succeeded before calling SavePage. Yes. I've updated the original doc with what to do about each of the statuses, minus the ones that apply at a much lower level, i.e. handled by the webcontentsdelegate. It is https://docs.google.com/document/d/1Qz6h3hsPWhLg9WcaHguQNoy_3OOuNyfS8X6NJO6MX... To summarize, most of the load failures prerendering suffers either don't quite apply or we need intervention at a much higher level to make sure we do the right thing. For example, prerender has a clause in chrome_content_browser_client (https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_cl...) to send a signal when there is an SSL error rather than be handled by the normal chrome UI that appears telling you about SSL. I will send out another CL for the ones we can do really easily, and let's have a discussion about what we can do for the ones that need special intervention? https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:109: loader_.reset( On 2016/12/02 19:12:31, dougarnett wrote: > this is a bit interesting here so might be good to add a comment > and might be a better place for the TODO comment than the .h file Done. https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.h (right): https://codereview.chromium.org/2534673002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.h:69: // Whether we are on a low-end device. On 2016/12/01 02:03:38, Pete Williamson wrote: > We needed to track this for the prerenderer, because we know it had problems on > svelte devices. Do you think your offliner will have problems on svelte > devices, or are you just allowing for the possibility? I'm allowing for possibility. We can remove later after more testing
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your fixes to my previous comments all look good. A few new comments on the tests. DougArnett may have better insight on the tests, since they seem similar to the existing prerendering offliner tests. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc (right): https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:30: class MockOfflinePageModel : public StubOfflinePageModel { I wonder if this could be split out into a separate class, and re-used by the prerendering offliner unit tests. (Perhaps it was copied from there originally?). https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:118: TestBackgroundLoaderOffliner* offliner() const { return offliner_.get(); } Can this return the BackgroundLoaderOffliner instead? If so, that will make life easier when we get rid of the TestBackgroundLoaderOffliner. It looks like the call to reset is done on the "offliner" member variable, so maybe we don't need to access any of the special test members from the unit tests themselves. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:147: : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP), Why use TestBrowserThreadBundle instead of ThreadTaskRunnerHandle? (I'm not sure of the difference, I just know that most of our tests seem to use the other thread runner instead of a thread bundle.) https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:187: // Save still in progress because does not support canceling. So what happens on a call to cancel when save is in progress? Is the saved file deleted by the offliner after the save operation returns, or is that the responsibility of the request coordinator to check for the file and remove it? (If so, let's add a TODO to the RC code.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
drive by https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:20: : WebContentsObserver(), Is this line needed? https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:22: offline_page_model_(offline_page_model), why do you need both browser_context_ and offline_page_model_? (latter can be retrieved using the former) Also, do you intend to DCHECK these? https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:23: pending_request_(nullptr), not needed. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:24: app_listener_(nullptr), not needed. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:31: const CompletionCallback& callback) { dcheck completion_callback_ please. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:35: DVLOG(1) << "Already have pending request"; DCHECK above prevents this DVLOG https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:63: pending_request_.reset(nullptr); will using just: reset() work? https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:64: ResetState(); This usage of ResetState contradicts line 49. This will create a new loader_ is that what you intend to do here? Wouldn't it be more appropriate to use ResetState to reset() all pointers (pending_request_, app_listener_, loader_, ...) ? https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:69: if (!pending_request_.get()) { Is there a chance Cancel and DidStopLoading are queued in message loop one after another? https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:81: DCHECK(offline_page_model_); this dcheck comes a little late. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:99: pending_request_.reset(nullptr); one more reason to put this in ResetState() also you can drop nullptr from here too, I think. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:103: if (save_result == SavePageResult::SUCCESS) { {} not needed. remove, please. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.h (right): https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.h:14: #include "components/offline_pages/core/offline_page_model.h" nit: this likely can be forward declared. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.h:32: explicit BackgroundLoaderOffliner(content::BrowserContext* browser_context, nit: no need for explicit if number of arguments != 1 https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.h:37: // Offliner implementation nit: Put colon or period at the end, please. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.h:42: // WebContentsObserver implementation Ditto. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.h:50: friend class TestBackgroundLoaderOffliner; nit: add empty line, please.
https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:63: pending_request_.reset(nullptr); On 2016/12/12 17:59:16, fgorski wrote: > will using just: reset() work? Interesting, I didn't know reset() was defined on unique_ptr. We have this in many places (and many more in some other parts of chrome). Seems good to clean these up. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:64: ResetState(); Also consider comment/TODO about not currently able to cancel a pending OfflinePageModel::SavePage() operation https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc (right): https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:147: : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP), On 2016/12/10 01:52:54, Pete Williamson wrote: > Why use TestBrowserThreadBundle instead of ThreadTaskRunnerHandle? (I'm not > sure of the difference, I just know that most of our tests seem to use the other > thread runner instead of a thread bundle.) Not sure but I have faint recollection of needing to do something different to make the Prerenderer code happy (ie, get past some thread DCHECKs) - but that might only apply now to PrerenderingAdapter tests at this point. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:187: // Save still in progress because does not support canceling. On 2016/12/10 01:52:54, Pete Williamson wrote: > So what happens on a call to cancel when save is in progress? Is the saved file > deleted by the offliner after the save operation returns, or is that the > responsibility of the request coordinator to check for the file and remove it? > (If so, let's add a TODO to the RC code.) This is a hole. We should have some bug opened or TODO. Ideally, we would be able to cancel the SavePage() operation but if not, then good question on how to clean it up. Can we have Offliner clean it up? Might be more complicated if Offliner gets destroyed (in future iteration). For RC to clean up, we need to pass it the proposed_offline_id I expect. How bad is it if not cleaned up? Shows up in Downloads?
https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc (right): https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:187: // Save still in progress because does not support canceling. On 2016/12/12 19:24:17, dougarnett wrote: > On 2016/12/10 01:52:54, Pete Williamson wrote: > > So what happens on a call to cancel when save is in progress? Is the saved > file > > deleted by the offliner after the save operation returns, or is that the > > responsibility of the request coordinator to check for the file and remove it? > > > (If so, let's add a TODO to the RC code.) > > This is a hole. We should have some bug opened or TODO. Ideally, we would be > able to cancel the SavePage() operation but if not, then good question on how to > clean it up. Can we have Offliner clean it up? Might be more complicated if > Offliner gets destroyed (in future iteration). For RC to clean up, we need to > pass it the proposed_offline_id I expect. How bad is it if not cleaned up? Shows > up in Downloads? Actually, what happens to WebContents when ResetState() destroys current loader? Can archiver keep using that WebContents?
I broke something over the last day regarding cancel and resetting. It appears that cancel() isn't resetting properly. Still trying to fix, but please advise if you see something obvious Thanks! https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:20: : WebContentsObserver(), On 2016/12/12 17:59:16, fgorski wrote: > Is this line needed? Done. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:22: offline_page_model_(offline_page_model), On 2016/12/12 17:59:16, fgorski wrote: > why do you need both browser_context_ and offline_page_model_? (latter can be > retrieved using the former) > > Also, do you intend to DCHECK these? following example for the prerendering_offliner here. Also lets us pass in a mock/stub offlinepagemodel for testing here. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:23: pending_request_(nullptr), On 2016/12/12 17:59:16, fgorski wrote: > not needed. Done. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:24: app_listener_(nullptr), On 2016/12/12 17:59:16, fgorski wrote: > not needed. Done. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:31: const CompletionCallback& callback) { On 2016/12/12 17:59:16, fgorski wrote: > dcheck completion_callback_ please. Done. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:35: DVLOG(1) << "Already have pending request"; On 2016/12/12 17:59:16, fgorski wrote: > DCHECK above prevents this DVLOG Done. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:63: pending_request_.reset(nullptr); On 2016/12/12 19:24:17, dougarnett wrote: > On 2016/12/12 17:59:16, fgorski wrote: > > will using just: reset() work? > > Interesting, I didn't know reset() was defined on unique_ptr. We have this in > many places (and many more in some other parts of chrome). Seems good to clean > these up. yes it does. I always thought we passed in nullptr in places to make it more explicit... I should assume less :D https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:64: ResetState(); On 2016/12/12 19:24:17, dougarnett wrote: > Also consider comment/TODO about not currently able to cancel a pending > OfflinePageModel::SavePage() operation Done. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:64: ResetState(); On 2016/12/12 17:59:16, fgorski wrote: > This usage of ResetState contradicts line 49. > > This will create a new loader_ is that what you intend to do here? > Wouldn't it be more appropriate to use ResetState to reset() all pointers > (pending_request_, app_listener_, loader_, ...) ? Line 49 is mainly there because I can't call ResetState from constructor if I want to override it for test. Ideally, ResetState is not there at all, and callbacks/cancel options will lead to the destruction of the object, hence I wanted to limit the scope of ResetState to only the loader_ and resetting the observer. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:69: if (!pending_request_.get()) { On 2016/12/12 17:59:16, fgorski wrote: > Is there a chance Cancel and DidStopLoading are queued in message loop one after > another? I don't think so. The DidStopLoading() is called synchonously by the WebContentsDelegate when it receives a message about loading. If Cancel() is called first, it would de-register the observer from the WCD, and WCD will therefore not end up calling this method when it cycles through its list of observers. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:81: DCHECK(offline_page_model_); On 2016/12/12 17:59:16, fgorski wrote: > this dcheck comes a little late. Done. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:99: pending_request_.reset(nullptr); On 2016/12/12 17:59:16, fgorski wrote: > one more reason to put this in ResetState() > also you can drop nullptr from here too, I think. Done. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:103: if (save_result == SavePageResult::SUCCESS) { On 2016/12/12 17:59:16, fgorski wrote: > {} not needed. remove, please. for some reason i thought it's necessary for if/else https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.h (right): https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.h:14: #include "components/offline_pages/core/offline_page_model.h" On 2016/12/12 17:59:16, fgorski wrote: > nit: this likely can be forward declared. Done. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.h:32: explicit BackgroundLoaderOffliner(content::BrowserContext* browser_context, On 2016/12/12 17:59:16, fgorski wrote: > nit: no need for explicit if number of arguments != 1 Done. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.h:37: // Offliner implementation On 2016/12/12 17:59:16, fgorski wrote: > nit: Put colon or period at the end, please. Done. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.h:42: // WebContentsObserver implementation On 2016/12/12 17:59:16, fgorski wrote: > Ditto. Done. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.h:50: friend class TestBackgroundLoaderOffliner; On 2016/12/12 17:59:16, fgorski wrote: > nit: add empty line, please. Done. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc (right): https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:118: TestBackgroundLoaderOffliner* offliner() const { return offliner_.get(); } On 2016/12/10 01:52:54, Pete Williamson wrote: > Can this return the BackgroundLoaderOffliner instead? If so, that will make > life easier when we get rid of the TestBackgroundLoaderOffliner. It looks like > the call to reset is done on the "offliner" member variable, so maybe we don't > need to access any of the special test members from the unit tests themselves. The Test has methods to call is_loading() (returns whether a loading call has been called) on the stub contents for testing purposes. I can imagine that going away once we don't need to reset state, though it's still useful right now. https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:147: : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP), On 2016/12/12 19:24:17, dougarnett wrote: > On 2016/12/10 01:52:54, Pete Williamson wrote: > > Why use TestBrowserThreadBundle instead of ThreadTaskRunnerHandle? (I'm not > > sure of the difference, I just know that most of our tests seem to use the > other > > thread runner instead of a thread bundle.) > > Not sure but I have faint recollection of needing to do something different to > make the Prerenderer code happy (ie, get past some thread DCHECKs) - but that > might only apply now to PrerenderingAdapter tests at this point. thread_task_runner_handle.cc fails with a "Must be called on Chrome_UIThread; actually called on Unknown Thread" error when i used ThreadTaskRunnerHandle https://codereview.chromium.org/2534673002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner_unittest.cc:187: // Save still in progress because does not support canceling. On 2016/12/12 19:33:28, dougarnett wrote: > On 2016/12/12 19:24:17, dougarnett wrote: > > On 2016/12/10 01:52:54, Pete Williamson wrote: > > > So what happens on a call to cancel when save is in progress? Is the saved > > file > > > deleted by the offliner after the save operation returns, or is that the > > > responsibility of the request coordinator to check for the file and remove > it? > > > > > (If so, let's add a TODO to the RC code.) > > > > This is a hole. We should have some bug opened or TODO. Ideally, we would be > > able to cancel the SavePage() operation but if not, then good question on how > to > > clean it up. Can we have Offliner clean it up? Might be more complicated if > > Offliner gets destroyed (in future iteration). For RC to clean up, we need to > > pass it the proposed_offline_id I expect. How bad is it if not cleaned up? > Shows > > up in Downloads? > > Actually, what happens to WebContents when ResetState() destroys current loader? > Can archiver keep using that WebContents? This is a good point. ResetState would delete the WebContents, and we could have some potential crashing going on. Added a safety guard to delete the loader & WebContents after save if cancel() was called during the middle of save
did you forget to upload your most recent patchset?
On 2016/12/15 17:47:40, dewittj wrote: > did you forget to upload your most recent patchset? Oops. It hung on "Title describing this patch set". Uploaded now
OK, my comments have been addressed, lgtm when you get it working.
lgtm
lgtm, but let's talk about save_state_ in implementation. https://codereview.chromium.org/2534673002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2534673002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/background_loader_offliner.cc:66: if (save_state_ != NONE) { I don't understand the logic here. Can we chat about this? It is likely correct, but hard to follow.
https://codereview.chromium.org/2534673002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2534673002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/background_loader_offliner.cc:65: if (pending_request_) { how about? if (!pending_request_) return; if (save_state_ != NONE) { save_state_ = DELETE_AFTER_SAVE; return; } ResetState();
By assuming that calling StartLoading (via web_contents()->TestSetIsLoading(true)) will eventually trigger DidStopLoading, all the tests are passing... Made a note to come back to this later if I have time to make it more right Thanks for taking a look! https://codereview.chromium.org/2534673002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2534673002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/background_loader_offliner.cc:65: if (pending_request_) { On 2016/12/15 22:01:13, fgorski wrote: > how about? > > if (!pending_request_) > return; > > if (save_state_ != NONE) { > save_state_ = DELETE_AFTER_SAVE; > return; > } > > ResetState(); Done. https://codereview.chromium.org/2534673002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/background_loader_offliner.cc:66: if (save_state_ != NONE) { On 2016/12/15 21:59:07, fgorski wrote: > I don't understand the logic here. Can we chat about this? > > It is likely correct, but hard to follow. Done.
The CQ bit was checked by chili@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, dougarnett@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2534673002/#ps120001 (title: "bleh it works now")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
chili@chromium.org changed reviewers: + sky@chromium.org
+sky for deps on content/public/test, used by background_loader_contents_stub for WebContentsTester
sky@chromium.org changed reviewers: + jam@chromium.org
sky->jam
On 2016/12/16 20:33:25, sky (OOO) wrote: > sky->jam lgtm
The CQ bit was checked by chili@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482174337349920,
"parent_rev": "115cab6893e4a342f9a8202ea703d3219e0fa722", "commit_rev":
"001dacd63b3d78514790a3ad70626e7a6ef8ba6b"}
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Create offliner that uses background loader BUG=665242 ========== to ========== [Offline pages] Create offliner that uses background loader BUG=665242 Review-Url: https://codereview.chromium.org/2534673002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Create offliner that uses background loader BUG=665242 Review-Url: https://codereview.chromium.org/2534673002 ========== to ========== [Offline pages] Create offliner that uses background loader BUG=665242 Committed: https://crrev.com/1ae92b9ebd0682f1c2abd530a6d191c779d9ce71 Cr-Commit-Position: refs/heads/master@{#439580} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1ae92b9ebd0682f1c2abd530a6d191c779d9ce71 Cr-Commit-Position: refs/heads/master@{#439580} |
