Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1982483002:
New Tab Page: Work around potentially long delays if model is not loaded. (Closed)
|
Created:
4 years, 7 months ago by dewittj Modified:
4 years, 7 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, mcwilliams+watch_chromium.org, dgn+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNew Tab Page: Work around potentially long delays if model is not loaded.
This avoids querying for offline pages if the model is not loaded, so
that the maximum possible delay is minimal (once the model is loaded,
all pages are stored in memory).
Additionally checks for null when obtaining the offline page model.
BUG=607573, 613359
Committed: https://crrev.com/9a12ce07eb8c36407ae27beba7b2082229cf5678
Cr-Commit-Position: refs/heads/master@{#395096}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : Address nit and add null check. #Patch Set 3 : Reformat a touch. #Messages
Total messages: 21 (10 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== New Tab Page: Work around potentially long delays if model is not loaded. This avoids querying for offline pages if the model is not loaded, so that the maximum possible delay is minimal (once the model is loaded, all pages are stored in memory). BUG=607573 ========== to ========== New Tab Page: Work around potentially long delays if model is not loaded. This avoids querying for offline pages if the model is not loaded, so that the maximum possible delay is minimal (once the model is loaded, all pages are stored in memory). BUG=607573 ==========
dewittj@chromium.org changed reviewers: + bauerb@chromium.org
ptal. Feel free to +CQ in order to land this before branch
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/patch-status/1982483002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1982483002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1982483002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1982483002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:486: if (!offlinePageBridge.isOfflinePageModelLoaded()) { Can't you move this into OfflinePageBridge?
https://codereview.chromium.org/1982483002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1982483002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:486: if (!offlinePageBridge.isOfflinePageModelLoaded()) { On 2016/05/19 08:58:56, Bernhard Bauer wrote: > Can't you move this into OfflinePageBridge? I'd prefer to leave it here.. This behavior isn't intended to be the way OfflinePageBridge#checkPagesExistOffline works - it is intended to reliably check the model. So this workaround is here in order to make that clear. Also then when the NTP is able to badge pages after load we can just remove this NTP code instead of having to change the offline page bridge. I can change it if you still feel this should be in the model.
https://codereview.chromium.org/1982483002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1982483002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:484: // TODO(dewittj): Remove this page by making the NTP badging available after the NTP is s/page/code/? https://codereview.chromium.org/1982483002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:486: if (!offlinePageBridge.isOfflinePageModelLoaded()) { On 2016/05/19 15:40:41, dewittj wrote: > On 2016/05/19 08:58:56, Bernhard Bauer wrote: > > Can't you move this into OfflinePageBridge? > I'd prefer to leave it here.. > > This behavior isn't intended to be the way > OfflinePageBridge#checkPagesExistOffline works - it is intended to reliably > check the model. So this workaround is here in order to make that clear. Also > then when the NTP is able to badge pages after load we can just remove this NTP > code instead of having to change the offline page bridge. > > I can change it if you still feel this should be in the model. OK, I guess you do have a TODO there already.
LGTM I meant...
Description was changed from ========== New Tab Page: Work around potentially long delays if model is not loaded. This avoids querying for offline pages if the model is not loaded, so that the maximum possible delay is minimal (once the model is loaded, all pages are stored in memory). BUG=607573 ========== to ========== New Tab Page: Work around potentially long delays if model is not loaded. This avoids querying for offline pages if the model is not loaded, so that the maximum possible delay is minimal (once the model is loaded, all pages are stored in memory). Additionally checks for null when obtaining the offline page model. BUG=607573,613359 ==========
The CQ bit was checked by dewittj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/1982483002/#ps60001 (title: "Reformat a touch.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1982483002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1982483002/60001
Message was sent while issue was closed.
Description was changed from ========== New Tab Page: Work around potentially long delays if model is not loaded. This avoids querying for offline pages if the model is not loaded, so that the maximum possible delay is minimal (once the model is loaded, all pages are stored in memory). Additionally checks for null when obtaining the offline page model. BUG=607573,613359 ========== to ========== New Tab Page: Work around potentially long delays if model is not loaded. This avoids querying for offline pages if the model is not loaded, so that the maximum possible delay is minimal (once the model is loaded, all pages are stored in memory). Additionally checks for null when obtaining the offline page model. BUG=607573,613359 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== New Tab Page: Work around potentially long delays if model is not loaded. This avoids querying for offline pages if the model is not loaded, so that the maximum possible delay is minimal (once the model is loaded, all pages are stored in memory). Additionally checks for null when obtaining the offline page model. BUG=607573,613359 ========== to ========== New Tab Page: Work around potentially long delays if model is not loaded. This avoids querying for offline pages if the model is not loaded, so that the maximum possible delay is minimal (once the model is loaded, all pages are stored in memory). Additionally checks for null when obtaining the offline page model. BUG=607573,613359 Committed: https://crrev.com/9a12ce07eb8c36407ae27beba7b2082229cf5678 Cr-Commit-Position: refs/heads/master@{#395096} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9a12ce07eb8c36407ae27beba7b2082229cf5678 Cr-Commit-Position: refs/heads/master@{#395096}
Message was sent while issue was closed.
https://codereview.chromium.org/1982483002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/1982483002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:484: // TODO(dewittj): Remove this page by making the NTP badging available after the NTP is On 2016/05/20 10:27:39, Bernhard Bauer wrote: > s/page/code/? Done. https://codereview.chromium.org/1982483002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:486: if (!offlinePageBridge.isOfflinePageModelLoaded()) { On 2016/05/20 10:27:39, Bernhard Bauer wrote: > On 2016/05/19 15:40:41, dewittj wrote: > > On 2016/05/19 08:58:56, Bernhard Bauer wrote: > > > Can't you move this into OfflinePageBridge? > > I'd prefer to leave it here.. > > > > This behavior isn't intended to be the way > > OfflinePageBridge#checkPagesExistOffline works - it is intended to reliably > > check the model. So this workaround is here in order to make that clear. > Also > > then when the NTP is able to badge pages after load we can just remove this > NTP > > code instead of having to change the offline page bridge. > > > > I can change it if you still feel this should be in the model. > > OK, I guess you do have a TODO there already. Acknowledged. |
