Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714733003:
[Offline Pages] Add a timer to BackgroundLoaderOffliner to delay SavePage by 2 seconds. (Closed)
|
Created:
3 years, 10 months ago by chili Modified:
3 years, 9 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, 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] Add a timer to BackgroundLoaderOffliner to delay SavePage by 2 seconds.
BUG=695915
Review-Url: https://codereview.chromium.org/2714733003
Cr-Commit-Position: refs/heads/master@{#453381}
Committed: https://chromium.googlesource.com/chromium/src/+/632b3bc8fe84af8ed8f69adbfa181e025957bf6a
Patch Set 1 #
Total comments: 4
Patch Set 2 : add additional pointer invalidation after possible navigation commit #
Total comments: 2
Patch Set 3 : unittests #Patch Set 4 : fix tests and last comment #
Messages
Total messages: 26 (18 generated)
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...
chili@chromium.org changed reviewers: + fgorski@chromium.org, petewil@chromium.org
https://codereview.chromium.org/2714733003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.cc (left): https://codereview.chromium.org/2714733003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:132: SavePageRequest request(*pending_request_.get()); How about leaving this code where it is instead of moving into the delayed save page? If we had a navigation error, might as well take care of it now instead of two seconds later. https://codereview.chromium.org/2714733003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2714733003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:22: long kOfflinePageDelayMs = 2000; As a follow up change, let's put this into OfflinerPolicy, and use the constant in both offliners.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Per offline discussion with Filip, I added an additional check in DidFinishNavigation to invalidate ongoing saves if we detect a URL change. There are 4 Navigation-related events, but according to https://cs.chromium.org/chromium/src/content/public/browser/web_contents_obse..., DidFinishNavigation is called right after navigation commits. Please take a look and see if you agree https://codereview.chromium.org/2714733003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.cc (left): https://codereview.chromium.org/2714733003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:132: SavePageRequest request(*pending_request_.get()); On 2017/02/24 17:31:45, Pete Williamson wrote: > How about leaving this code where it is instead of moving into the delayed save > page? If we had a navigation error, might as well take care of it now instead > of two seconds later. I think we still need this check in SavePage because a redirect can also result in an error page. While we do our best to ensure that pointers are invalidated, there may be some race we're not aware of that results in an error page being loaded. https://codereview.chromium.org/2714733003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2714733003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:22: long kOfflinePageDelayMs = 2000; On 2017/02/24 17:31:45, Pete Williamson wrote: > As a follow up change, let's put this into OfflinerPolicy, and use the constant > in both offliners. Acknowledged.
Patchset #2 (id:20001) has been deleted
lgtm, but that test might need more work in case pump loop does not work for you. https://codereview.chromium.org/2714733003/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2714733003/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:223: if (navigation_handle->HasCommitted() && !navigation_handle->IsSamePage()) { int: drop {}
lgtm
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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...
Added an ability to override the delay for tests. There doesn't seem to be an easy way to both use a TestMockTimeTaskRunner and a TestBrowserThreadBundle and have them interact the way you want them to. Tests now passing locally https://codereview.chromium.org/2714733003/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2714733003/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:223: if (navigation_handle->HasCommitted() && !navigation_handle->IsSamePage()) { On 2017/02/24 22:12:07, fgorski wrote: > int: drop {} Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2714733003/#ps80001 (title: "fix tests and last comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488236916176890,
"parent_rev": "b5377a06bac448cdb7eb9999823e673e7cb20e3c", "commit_rev":
"632b3bc8fe84af8ed8f69adbfa181e025957bf6a"}
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Add a timer to BackgroundLoaderOffliner to delay SavePage by 2 seconds. BUG=695915 ========== to ========== [Offline Pages] Add a timer to BackgroundLoaderOffliner to delay SavePage by 2 seconds. BUG=695915 Review-Url: https://codereview.chromium.org/2714733003 Cr-Commit-Position: refs/heads/master@{#453381} Committed: https://chromium.googlesource.com/chromium/src/+/632b3bc8fe84af8ed8f69adbfa18... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/632b3bc8fe84af8ed8f69adbfa18... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
