Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2797013002:
[Offline pages] Update background loader to override the WebContentsObserver methods expected (Closed)
|
Created:
3 years, 8 months ago by chili Modified:
3 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, romax+watch_chromium.org, tburkard+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, gavinp+prer_chromium.org, dimich+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages] Update background loader to override the WebContentsObserver methods expected by the SnapshotController.
Note that while the same issue exists for prerendering loader, we are not going to update that.
BUG=708798
Review-Url: https://codereview.chromium.org/2797013002
Cr-Commit-Position: refs/heads/master@{#463782}
Committed: https://chromium.googlesource.com/chromium/src/+/c92ef59a640cf2dacabef34f8cf798e39c1e61f5
Patch Set 1 #
Total comments: 2
Patch Set 2 : Change signals that background loader is passing. Ignore prerender mismatch #Patch Set 3 : fix offliner tests #
Messages
Total messages: 34 (20 generated)
The CQ bit was checked by chili@chromium.org to run a CQ dry run
chili@chromium.org changed reviewers: + fgorski@chromium.org, petewil@chromium.org
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.
mostly looks good. One question. https://codereview.chromium.org/2797013002/diff/1/components/offline_pages/co... File components/offline_pages/core/snapshot_controller.cc (right): https://codereview.chromium.org/2797013002/diff/1/components/offline_pages/co... components/offline_pages/core/snapshot_controller.cc:21: const int64_t kDelayAfterLoadCompletedMs = 1000; Shouldn't this be 2000? I was thinking all along it was 2000, and the extra time helps us to not be busy rearranging the DOM when we snapshot.
https://codereview.chromium.org/2797013002/diff/1/components/offline_pages/co... File components/offline_pages/core/snapshot_controller.cc (right): https://codereview.chromium.org/2797013002/diff/1/components/offline_pages/co... components/offline_pages/core/snapshot_controller.cc:21: const int64_t kDelayAfterLoadCompletedMs = 1000; On 2017/04/05 00:26:21, Pete Williamson wrote: > Shouldn't this be 2000? > > I was thinking all along it was 2000, and the extra time helps us to not be busy > rearranging the DOM when we snapshot. This is the default number. Both offliners pass in 2000. I think the RecentTabHelper is using 1000, but RTH is also using DocumentOnLoadCompleted rather than DidStopLoading.
lgtm
dimich@chromium.org changed reviewers: + dimich@chromium.org
drive-by: It is unclear why this better matches what they actually correspond to... I remember a discussion of how DidStopLoading is an artificial signal that doesn't tell much about what actually happens. Is there a bug/design doc?
Description was changed from ========== [Offline pages] Update snapshot controller methods to better match what they actually correspond to. BUG= ========== to ========== [Offline pages] Update background loader to override the WebContentsObserver methods expected by the SnapshotController. Note that while the same issue exists for prerendering loader, we are not going to update that. BUG=708798 ==========
Per offline discussion with dimich@, we are no longer going to introduce new methods inside snapshot controller. We will instead update the BackgroundLoader to pass on expected signals. The prerenderer will still have a mismatch, but we think that's ok for now. Updated subject and description to match current change
lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
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...
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/2797013002/#ps40001 (title: "fix offliner tests")
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: + holte@chromium.org
+holte for histogram changes
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491944852098080,
"parent_rev": "9b4e899d21ec602a49cd050b800085cd22130c69", "commit_rev":
"c92ef59a640cf2dacabef34f8cf798e39c1e61f5"}
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Update background loader to override the WebContentsObserver methods expected by the SnapshotController. Note that while the same issue exists for prerendering loader, we are not going to update that. BUG=708798 ========== to ========== [Offline pages] Update background loader to override the WebContentsObserver methods expected by the SnapshotController. Note that while the same issue exists for prerendering loader, we are not going to update that. BUG=708798 Review-Url: https://codereview.chromium.org/2797013002 Cr-Commit-Position: refs/heads/master@{#463782} Committed: https://chromium.googlesource.com/chromium/src/+/c92ef59a640cf2dacabef34f8cf7... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c92ef59a640cf2dacabef34f8cf7...
Message was sent while issue was closed.
what histogram changes?
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Update background loader to override the WebContentsObserver methods expected by the SnapshotController. Note that while the same issue exists for prerendering loader, we are not going to update that. BUG=708798 Review-Url: https://codereview.chromium.org/2797013002 Cr-Commit-Position: refs/heads/master@{#463782} Committed: https://chromium.googlesource.com/chromium/src/+/c92ef59a640cf2dacabef34f8cf7... ========== to ========== [Offline pages] Update background loader to override the WebContentsObserver methods expected by the SnapshotController. Note that while the same issue exists for prerendering loader, we are not going to update that. BUG=708798 Review-Url: https://codereview.chromium.org/2797013002 Cr-Commit-Position: refs/heads/master@{#463782} Committed: https://chromium.googlesource.com/chromium/src/+/c92ef59a640cf2dacabef34f8cf7... ==========
Message was sent while issue was closed.
chili@chromium.org changed reviewers: - holte@chromium.org |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
