Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2274293002:
📰 Keep Suggestion sections in declaration order (Closed)
|
Created:
4 years, 4 months ago by dgn Modified:
4 years, 3 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, ntp-dev+reviews_chromium.org, Philipp Keck Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NTP Client] Keep Suggestion sections in declaration order
When a new NTP is created, it queries the list of registered categories
with the backend. The returned category order will be kept for the
visible categories.
Suggestions received from categories that are after that point will
be ignored.
BUG=640568
Committed: https://crrev.com/b6a7058d9f131ac191f59f9044646c7a2a98da0d
Cr-Commit-Position: refs/heads/master@{#414696}
Patch Set 1 #
Total comments: 5
Patch Set 2 : address comments #Patch Set 3 : fix comments #
Total comments: 2
Patch Set 4 : more comment fixes #Patch Set 5 : fix render test #
Messages
Total messages: 38 (26 generated)
Description was changed from ========== [NTP Client] Keep Suggestion sections in declaration order When a new NTP is created, it queries the list of registered categories with the backend. The returned category order will be kept for the visible categories. Categories subsequently added to a live NTP will just be added at the end BUG=640568 ========== to ========== [NTP Client] Keep Suggestion sections in declaration order When a new NTP is created, it queries the list of registered categories with the backend. The returned category order will be kept for the visible categories. Categories subsequently added to a live NTP will just be added at the end BUG=640568 ==========
The CQ bit was checked by dgn@chromium.org to run a CQ dry run
dgn@chromium.org changed reviewers: + bauerb@chromium.org
PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> Categories subsequently added to a live NTP will > just be added at the end Is that really the desired behaviour from the product standpoint? If not, can you add a TODO to make it work?
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...)
Description was changed from ========== [NTP Client] Keep Suggestion sections in declaration order When a new NTP is created, it queries the list of registered categories with the backend. The returned category order will be kept for the visible categories. Categories subsequently added to a live NTP will just be added at the end BUG=640568 ========== to ========== [NTP Client] Keep Suggestion sections in declaration order When a new NTP is created, it queries the list of registered categories with the backend. The returned category order will be kept for the visible categories. Suggestions received from categories that are unknown at that point will be ignored. BUG=640568 ==========
Description was changed from ========== [NTP Client] Keep Suggestion sections in declaration order When a new NTP is created, it queries the list of registered categories with the backend. The returned category order will be kept for the visible categories. Suggestions received from categories that are unknown at that point will be ignored. BUG=640568 ========== to ========== [NTP Client] Keep Suggestion sections in declaration order When a new NTP is created, it queries the list of registered categories with the backend. The returned category order will be kept for the visible categories. Suggestions received from categories that are after that point will be ignored. BUG=640568 ==========
On 2016/08/24 21:35:54, Bernhard Bauer wrote: > > Categories subsequently added to a live NTP will > > just be added at the end > > Is that really the desired behaviour from the product standpoint? If not, can > you add a TODO to make it work? Clarified with nepper@: we don't add new sections over the lifetime of a NTP. Updated the CL to reflect that.
The CQ bit was checked by dgn@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...
https://codereview.chromium.org/2274293002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2274293002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:536: // Use available since the Adapter does not add fake suggestion source does not return Nit: This sentence does not parse. https://codereview.chromium.org/2274293002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:539: // Important: showIfEmptry flag to true. Nit: showIfEmpty
The CQ bit was checked by dgn@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...
PTAL https://codereview.chromium.org/2274293002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2274293002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:536: // Use available since the Adapter does not add fake suggestion source does not return On 2016/08/25 13:23:45, Bernhard Bauer wrote: > Nit: This sentence does not parse. Done. I have no memory of writing that. I was probably writing a comment and Oh! Piece of candy! https://codereview.chromium.org/2274293002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:539: // Important: showIfEmptry flag to true. On 2016/08/25 13:23:45, Bernhard Bauer wrote: > Nit: showIfEmpty Done.
LGTM https://codereview.chromium.org/2274293002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2274293002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:536: // Use available since the Adapter does not add fake suggestion source does not return On 2016/08/25 15:09:40, dgn wrote: > On 2016/08/25 13:23:45, Bernhard Bauer wrote: > > Nit: This sentence does not parse. > > Done. I have no memory of writing that. I was probably writing a comment and Oh! > Piece of candy! https://www.youtube.com/watch?v=SSUXXzN26zg https://codereview.chromium.org/2274293002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2274293002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:542: // The adapter is already initialised, it will not only accept new categories "not accept new categories anymore"
The CQ bit was checked by dgn@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...
Thanks! https://codereview.chromium.org/2274293002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java (right): https://codereview.chromium.org/2274293002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:542: // The adapter is already initialised, it will not only accept new categories On 2016/08/25 15:20:31, Bernhard Bauer wrote: > "not accept new categories anymore" Done.
The CQ bit was unchecked by dgn@chromium.org
The CQ bit was checked by dgn@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/2274293002/#ps60001 (title: "more comment fixes")
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 dgn@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 dgn@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/2274293002/#ps80001 (title: "fix render test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [NTP Client] Keep Suggestion sections in declaration order When a new NTP is created, it queries the list of registered categories with the backend. The returned category order will be kept for the visible categories. Suggestions received from categories that are after that point will be ignored. BUG=640568 ========== to ========== [NTP Client] Keep Suggestion sections in declaration order When a new NTP is created, it queries the list of registered categories with the backend. The returned category order will be kept for the visible categories. Suggestions received from categories that are after that point will be ignored. BUG=640568 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [NTP Client] Keep Suggestion sections in declaration order When a new NTP is created, it queries the list of registered categories with the backend. The returned category order will be kept for the visible categories. Suggestions received from categories that are after that point will be ignored. BUG=640568 ========== to ========== [NTP Client] Keep Suggestion sections in declaration order When a new NTP is created, it queries the list of registered categories with the backend. The returned category order will be kept for the visible categories. Suggestions received from categories that are after that point will be ignored. BUG=640568 Committed: https://crrev.com/b6a7058d9f131ac191f59f9044646c7a2a98da0d Cr-Commit-Position: refs/heads/master@{#414696} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b6a7058d9f131ac191f59f9044646c7a2a98da0d Cr-Commit-Position: refs/heads/master@{#414696} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
