Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2769043002:
[Offline pages] Integrate snapshot controller fully into background loader. (Closed)
|
Created:
3 years, 9 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/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages] Integrate snapshot controller fully into background loader.
BUG=705753
Review-Url: https://codereview.chromium.org/2769043002
Cr-Commit-Position: refs/heads/master@{#459967}
Committed: https://chromium.googlesource.com/chromium/src/+/e101d81bbdb680bd24286fab8d799c5116257744
Patch Set 1 #
Total comments: 4
Patch Set 2 : added comment and reset #
Messages
Total messages: 21 (14 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chili@chromium.org changed reviewers: + fgorski@chromium.org, petewil@chromium.org
lgtm with nit. https://codereview.chromium.org/2769043002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2769043002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:192: if (!render_host->GetParent()) Let's add a comment here: // If this was for the main frame, inform the snapshot controller.
lgtm with 1 code comment and request: Please add bug and update CL description to explain the changes. https://codereview.chromium.org/2769043002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2769043002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:379: void BackgroundLoaderOffliner::ResetState() { did you consider resetting the snapshot controller?
Description was changed from ========== [Offline pages] Integrate snapshot controller fully into background loader. BUG= ========== to ========== [Offline pages] Integrate snapshot controller fully into background loader. BUG=705753 ==========
Added bug. Thanks! https://codereview.chromium.org/2769043002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2769043002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:192: if (!render_host->GetParent()) On 2017/03/27 21:45:45, Pete Williamson wrote: > Let's add a comment here: > // If this was for the main frame, inform the snapshot controller. Done. https://codereview.chromium.org/2769043002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:379: void BackgroundLoaderOffliner::ResetState() { On 2017/03/27 22:51:54, fgorski wrote: > did you consider resetting the snapshot controller? Added now. At first I didn't think it mattered as much, since we cleared the pending_request_ and LoadAndSave resets the snapshot controller, and consistency with prerenderer code. However, it's probably nicer to make this explicit
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: 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/2769043002/#ps20001 (title: "added comment and reset")
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": 20001, "attempt_start_ts": 1490663870689620,
"parent_rev": "4e149500c5c4a5a944e5761cb4aedb9ef627a14b", "commit_rev":
"e101d81bbdb680bd24286fab8d799c5116257744"}
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Integrate snapshot controller fully into background loader. BUG=705753 ========== to ========== [Offline pages] Integrate snapshot controller fully into background loader. BUG=705753 Review-Url: https://codereview.chromium.org/2769043002 Cr-Commit-Position: refs/heads/master@{#459967} Committed: https://chromium.googlesource.com/chromium/src/+/e101d81bbdb680bd24286fab8d79... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/e101d81bbdb680bd24286fab8d79...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2789503002/ by chili@chromium.org. The reason for reverting is: Causes crash, crbug/706279. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
