Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2592053004:
Call the Scheduler when opening the NTP. (Closed)
DescriptionCall the Scheduler when opening the NTP.
When a NTP is created, the scheduler is informed and can consider this
event in scheduling the fetching of new snippets.
BUG=676308
Committed: https://crrev.com/b29ba3730a14e6ee4f7836802caf1853aabf3772
Cr-Commit-Position: refs/heads/master@{#441129}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fixed the compile errors #Patch Set 3 : Renamed to OnNTPInitialized. #
Total comments: 2
Patch Set 4 : CamelCase'd initilialism. #
Messages
Total messages: 30 (18 generated)
fhorschig@chromium.org changed reviewers: + bauerb@chromium.org
Hi Bernhard, could you please take a look if this is the right place for notifying the scheduler?
Description was changed from ========== Call the Scheduler when opening the NTP. When a NTP is created, the scheduler is informed and can consider this event in scheduling the fetching of new snippets. BUG=676308 ========== to ========== Call the Scheduler when opening the NTP. When a NTP is created, the scheduler is informed and can consider this event in scheduling the fetching of new snippets. BUG=676308 ==========
tschumann@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
Bernhard is OOO but I can try :-) https://codereview.chromium.org/2592053004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2592053004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:750: mSnippetsBridge.onNTPOpened(); Might this do something synchronously and call back to the NTP? Maybe it's better to call this after the mNewTabPageView.initialize call. https://codereview.chromium.org/2592053004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2592053004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:175: public void onNTPOpened() { Maybe onNTPInitialized, if we wait for initialization? https://codereview.chromium.org/2592053004/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2592053004/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:417: content_suggestions_service_->remote_suggestions_scheduler()-> OnNTPOpened(); There seems to be a space in "-> OnNTPOpened"
The CQ bit was checked by mvanouwerkerk@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...
Thank you for stepping in Michael! It's unlikely that we intend to update the NTP synchronously but I cc'ed jkrcal@ to correct me if I am wrong. (I moved the call anyway) https://codereview.chromium.org/2592053004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2592053004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:750: mSnippetsBridge.onNTPOpened(); On 2016/12/22 11:38:33, Michael van Ouwerkerk wrote: > Might this do something synchronously and call back to the NTP? Maybe it's > better to call this after the mNewTabPageView.initialize call. Moved, although it's unlikely. https://codereview.chromium.org/2592053004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2592053004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:175: public void onNTPOpened() { On 2016/12/22 11:38:33, Michael van Ouwerkerk wrote: > Maybe onNTPInitialized, if we wait for initialization? Done. https://codereview.chromium.org/2592053004/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2592053004/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:417: content_suggestions_service_->remote_suggestions_scheduler()-> OnNTPOpened(); On 2016/12/22 11:38:33, Michael van Ouwerkerk wrote: > There seems to be a space in "-> OnNTPOpened" Done.
On 2016/12/22 12:26:50, fhorschig wrote: > Thank you for stepping in Michael! > It's unlikely that we intend to update the NTP synchronously but I cc'ed jkrcal@ > to correct me if I am wrong. > (I moved the call anyway) > > https://codereview.chromium.org/2592053004/diff/1/chrome/android/java/src/org... > File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java > (right): > > https://codereview.chromium.org/2592053004/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:750: > mSnippetsBridge.onNTPOpened(); > On 2016/12/22 11:38:33, Michael van Ouwerkerk wrote: > > Might this do something synchronously and call back to the NTP? Maybe it's > > better to call this after the mNewTabPageView.initialize call. > > Moved, although it's unlikely. > > https://codereview.chromium.org/2592053004/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java > (right): > > https://codereview.chromium.org/2592053004/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:175: > public void onNTPOpened() { > On 2016/12/22 11:38:33, Michael van Ouwerkerk wrote: > > Maybe onNTPInitialized, if we wait for initialization? > > Done. > > https://codereview.chromium.org/2592053004/diff/1/chrome/browser/android/ntp/... > File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): > > https://codereview.chromium.org/2592053004/diff/1/chrome/browser/android/ntp/... > chrome/browser/android/ntp/ntp_snippets_bridge.cc:417: > content_suggestions_service_->remote_suggestions_scheduler()-> OnNTPOpened(); > On 2016/12/22 11:38:33, Michael van Ouwerkerk wrote: > > There seems to be a space in "-> OnNTPOpened" > > Done. No intends to interact with NTP synchronously from here.
This change lgtm. Do we have test coverage for the scheduler somewhere? It seems like a tricky thing to test.
fhorschig@chromium.org changed reviewers: - bauerb@chromium.org
fhorschig@chromium.org changed reviewers: + treib@chromium.org
treib@chromium.org: Please review changes in snippets_bridge.* @Michael: Yes, testing will be tricky. Test could be introduced as soon as we know what happens exactly after calling the scheduler. (But extending and testing were reasons to make the scheduler an own class)
fhorschig@chromium.org changed reviewers: - treib@chromium.org
fhorschig@chromium.org changed reviewers: + bauerb@chromium.org
Hi Bernhard, would you please take a look?
LGTM with a nit: https://codereview.chromium.org/2592053004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2592053004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:758: mSnippetsBridge.onNTPInitialized(); Nit: The Java/Android convention is to treat initialisms as words, i.e. onNtpInitialized.
The CQ bit was checked by fhorschig@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...
Thank you for the quick reaction! https://codereview.chromium.org/2592053004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java (right): https://codereview.chromium.org/2592053004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:758: mSnippetsBridge.onNTPInitialized(); On 2017/01/03 15:05:11, Bernhard (just came back) wrote: > Nit: The Java/Android convention is to treat initialisms as words, i.e. > onNtpInitialized. Thanks.
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 fhorschig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2592053004/#ps60001 (title: "CamelCase'd initilialism.")
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": 60001, "attempt_start_ts": 1483458604306470,
"parent_rev": "23d7a11ab561715ee564ef9b19da94dfe8092c41", "commit_rev":
"1b806923de9b00bee539a9e06ae6c2fa5eaa3fc9"}
Message was sent while issue was closed.
Description was changed from ========== Call the Scheduler when opening the NTP. When a NTP is created, the scheduler is informed and can consider this event in scheduling the fetching of new snippets. BUG=676308 ========== to ========== Call the Scheduler when opening the NTP. When a NTP is created, the scheduler is informed and can consider this event in scheduling the fetching of new snippets. BUG=676308 Review-Url: https://codereview.chromium.org/2592053004 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Call the Scheduler when opening the NTP. When a NTP is created, the scheduler is informed and can consider this event in scheduling the fetching of new snippets. BUG=676308 Review-Url: https://codereview.chromium.org/2592053004 ========== to ========== Call the Scheduler when opening the NTP. When a NTP is created, the scheduler is informed and can consider this event in scheduling the fetching of new snippets. BUG=676308 Committed: https://crrev.com/b29ba3730a14e6ee4f7836802caf1853aabf3772 Cr-Commit-Position: refs/heads/master@{#441129} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b29ba3730a14e6ee4f7836802caf1853aabf3772 Cr-Commit-Position: refs/heads/master@{#441129} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
