Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2196273002:
Zine: support multiple sections in the ui (Closed)
|
Created:
4 years, 4 months ago by Bernhard Bauer Modified:
4 years, 4 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org, Michael van Ouwerkerk Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionZine: support multiple sections in the ui
* The adapter now manages groups of items.
* A SuggestionsSection is a group that holds suggestions, a header, and a status card.
* Add @SuggestionsCategory and @SuggestionsCategoryStatus annotations.
* SnippetsObserver now specifies what category it is talking about.
Based on http://codereview.chromium.org/2194433002#ps20001 by mvanouwerkerk@chromium.org.
BUG=616090
Committed: https://crrev.com/57a839dc61aece5b2138586dd0ffaa500b7ccc7f
Cr-Commit-Position: refs/heads/master@{#410075}
Patch Set 1 #
Total comments: 37
Patch Set 2 : rebase #Patch Set 3 : rebase #Patch Set 4 : review #
Total comments: 6
Patch Set 5 : rebase #Patch Set 6 : fix test #Patch Set 7 : fix junit test #Patch Set 8 : fix junit test #
Total comments: 29
Patch Set 9 : review #
Total comments: 2
Patch Set 10 : review #Patch Set 11 : fix #Patch Set 12 : rebase #Messages
Total messages: 72 (47 generated)
The CQ bit was checked by bauerb@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
bauerb@chromium.org changed reviewers: + peconn@chromium.org, pke@google.com
Please review!
https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:361: List<NewTabPageListItem> getItems() { Not sure if this is relevant, but this method is called a lot because all the methods for the Adapter interface depend on it. So performance on low-end devices might be bad (not sure if that actually matters for the number of suggestions we usually display, namely <= 10*#sections https://codereview.chromium.org/2196273002/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/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:31: @SuggestionsCategory int category, List<SnippetArticleListItem> suggestions); ContentSuggestionsCategory is now simply called Category. We can leave @SuggestionsCategory as is, or maybe find a new name? Is there a convention for naming these? I'd say it would be convenient if there was a deterministic way to derive the annotation name from the enum name. "Suggestions" as a term is technically already taken and different from "content suggestions". https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:139: private static void addSuggestion(List<SnippetArticleListItem> suggestions, String id, Have you seen this solution (also looks a bit hacky): https://codereview.chromium.org/2166523002/diff/1/chrome/browser/android/ntp/... Not sure if it's preferable. I can always change this when I refactor the snippets bridge for separate sections.
Haven't rebased this on https://codereview.chromium.org/2187233002 yet, just replying to comments. https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:361: List<NewTabPageListItem> getItems() { On 2016/08/01 15:28:57, Philipp Keck wrote: > Not sure if this is relevant, but this method is called a lot because all the > methods for the Adapter interface depend on it. So performance on low-end > devices might be bad (not sure if that actually matters for the number of > suggestions we usually display, namely <= 10*#sections > Acknowledged. This is the area where I'm planning to do more refactorings :) https://codereview.chromium.org/2196273002/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/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:31: @SuggestionsCategory int category, List<SnippetArticleListItem> suggestions); On 2016/08/01 15:28:57, Philipp Keck wrote: > ContentSuggestionsCategory is now simply called Category. > We can leave @SuggestionsCategory as is, or maybe find a new name? Is there a > convention for naming these? I'd say it would be convenient if there was a > deterministic way to derive the annotation name from the enum name. > "Suggestions" as a term is technically already taken and different from "content > suggestions". Yeah, I wasn't super happy about the name either, but given that https://codereview.chromium.org/1932183002/ is going to land Very Soon Nowâ„¢, this will be moot and we can remove the manual @IntDef completely once that happens. https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:139: private static void addSuggestion(List<SnippetArticleListItem> suggestions, String id, On 2016/08/01 15:28:57, Philipp Keck wrote: > Have you seen this solution (also looks a bit hacky): > https://codereview.chromium.org/2166523002/diff/1/chrome/browser/android/ntp/... > Not sure if it's preferable. I can always change this when I refactor the > snippets bridge for separate sections. Yes, I considered that, but I didn't really want to hardcode the class name as a string. I then considered getting the class from Java, but at that point figured I could just have Java create the array for me :)
The CQ bit was checked by bauerb@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:48: private final SnippetsBridge mSnippetsBridge; Just a heads up, when you merge this will be changed to SnippetsSource (if my CL passes...) https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:125: mGroups = new ArrayList<>(); Why not initialize these inline where they are declared? https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:184: setSuggestions(category, Collections.<SnippetArticleListItem>emptyList(), status); Do we want to pull the rug out from under the user? If they have suggestions then something goes wrong in the back end, why not let them continue to see their suggestions? https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:194: if (sectionItems.get(i) instanceof StatusListItem) { Won't the StatusListItem only ever be the first item in a section? https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:291: for (@SuggestionsCategory Integer key : mSections.keySet()) { Integer -> int ? https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageItemGroup.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageItemGroup.java:13: nit: Random newlines? And lacking Javadoc. https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageSingleItemGroup.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageSingleItemGroup.java:16: public NewTabPageSingleItemGroup() { mItems = Arrays.asList(this); ? https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:24: mItems = new ArrayList<>(); Why ArrayList? The list is going to be kept fairly small (about 10 items) but we're going to be doing random removals. Mindyou, the data locality will probably make moving all the items up one position in an ArrayList faster than following the links of a linked list. https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:47: for (NewTabPageListItem item : mItems) { Why do we need to search for the first SnippetArticleListItem? Can't we just check whether the first item is a SnippetHeaderListItem or a StatusListItem? https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleListItem.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleListItem.java:25: public final int mPosition; What is this position? Position in the whole list or in the section? Can we add a comment here and one to the javadoc of the constructor about it? https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsCategory.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsCategory.java:15: public @interface SuggestionsCategory {} My CL has now landed, so we should be able to remove this and SuggestionsCategoryStatus (providing my CL doesn't get reverted). https://codereview.chromium.org/2196273002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2196273002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:154: Java_SnippetsBridge_addSuggestion( I do like this approach but is there any overhead to flitting back and forth between Java and native code? I don't think there should be, but there could be some annoying conversion logic eating the CPU.
treib@chromium.org changed reviewers: + treib@chromium.org
https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:184: setSuggestions(category, Collections.<SnippetArticleListItem>emptyList(), status); On 2016/08/01 16:26:51, PEConn1 wrote: > Do we want to pull the rug out from under the user? If they have suggestions > then something goes wrong in the back end, why not let them continue to see > their suggestions? Drive-by: In some cases, yes, namely when the suggestions actually become unavailable. This can happen for offline pages, if an offline item actually gets deleted. Then it's better to pull it out from under the user, rather than having them click on something that will then fail to load. Note that this will not happen for regular background updates, only when a provider explicitly says "all my stuff must be removed right now".
The CQ bit was checked by bauerb@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: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by bauerb@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 for the review. Rebased on a bunch of CLs, PTAL! :) https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:48: private final SnippetsBridge mSnippetsBridge; On 2016/08/01 16:26:51, PEConn1 wrote: > Just a heads up, when you merge this will be changed to SnippetsSource (if my CL > passes...) Acknowledged. https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:125: mGroups = new ArrayList<>(); On 2016/08/01 16:26:50, PEConn1 wrote: > Why not initialize these inline where they are declared? Done. https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:194: if (sectionItems.get(i) instanceof StatusListItem) { On 2016/08/01 16:26:51, PEConn1 wrote: > Won't the StatusListItem only ever be the first item in a section? Done. https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:291: for (@SuggestionsCategory Integer key : mSections.keySet()) { On 2016/08/01 16:26:51, PEConn1 wrote: > Integer -> int ? Hm, the key type for the Map is a boxed Integer, so it would have to be boxed again when indexing the map on the next line. I was able to get around that though by just directly getting a collection of the map values (which is going to be ordered by the key). One other thing I could do is use a SparseArray instead of TreeMap, which would have unboxed ints as keys and is supposedly pretty efficient for small numbers of items. WDYT? https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageItemGroup.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageItemGroup.java:13: Done. https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageSingleItemGroup.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageSingleItemGroup.java:16: public NewTabPageSingleItemGroup() { On 2016/08/01 16:26:51, PEConn1 wrote: > mItems = Arrays.asList(this); > ? Used a singleton list, as no one should modify the list anyway. https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:24: mItems = new ArrayList<>(); On 2016/08/01 16:26:51, PEConn1 wrote: > Why ArrayList? The list is going to be kept fairly small (about 10 items) but > we're going to be doing random removals. > > Mindyou, the data locality will probably make moving all the items up one > position in an ArrayList faster than following the links of a linked list. Plus, purely algorithmically, traversing a linked list is also going to take O(n). There isn't really a good data structure in the standard Java libraries that allows insertion/deletion at random indices :-/ https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:47: for (NewTabPageListItem item : mItems) { On 2016/08/01 16:26:51, PEConn1 wrote: > Why do we need to search for the first SnippetArticleListItem? Can't we just > check whether the first item is a SnippetHeaderListItem or a StatusListItem? I slightly prefer this, as it reflects the intent of the method more clearly (we have suggestions if we have at least one SnippetArticleListItem). And performance-wise it shouldn't make a big difference, because we should get a result by the second item anyway. https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleListItem.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleListItem.java:25: public final int mPosition; On 2016/08/01 16:26:51, PEConn1 wrote: > What is this position? Position in the whole list or in the section? As the comment says, it's the position in _the_ list of snippets 😉 I.e. it refers to the old state of the world. It looks like this is only used for tracking though, so I'd be tempted to define it as the position in the whole list (assuming that suggestions further down are clicked less often or something). > Can we add > a comment here and one to the javadoc of the constructor about it? I moved the comments here from the constructor (I'd rather have them in a single place than duplicate them). https://codereview.chromium.org/2196273002/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/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:31: @SuggestionsCategory int category, List<SnippetArticleListItem> suggestions); On 2016/08/01 15:37:24, Bernhard Bauer wrote: > On 2016/08/01 15:28:57, Philipp Keck wrote: > > ContentSuggestionsCategory is now simply called Category. > > We can leave @SuggestionsCategory as is, or maybe find a new name? Is there a > > convention for naming these? I'd say it would be convenient if there was a > > deterministic way to derive the annotation name from the enum name. > > "Suggestions" as a term is technically already taken and different from > "content > > suggestions". > > Yeah, I wasn't super happy about the name either, but given that > https://codereview.chromium.org/1932183002/ is going to land Very Soon Now™, > this will be moot and we can remove the manual @IntDef completely once that > happens. And in fact that happened about five minutes after I wrote this comment :) So, changed now. (I realize that at some point in the future I might have to remove a bunch of these annotations again, when we're going to have _not_ well-known categories, i.e. purely server-side defined ones). https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsCategory.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsCategory.java:15: public @interface SuggestionsCategory {} On 2016/08/01 16:26:51, PEConn1 wrote: > My CL has now landed, so we should be able to remove this and > SuggestionsCategoryStatus (providing my CL doesn't get reverted). Indeed! Done. https://codereview.chromium.org/2196273002/diff/1/chrome/browser/android/ntp/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2196273002/diff/1/chrome/browser/android/ntp/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:154: Java_SnippetsBridge_addSuggestion( On 2016/08/01 16:26:51, PEConn1 wrote: > I do like this approach but is there any overhead to flitting back and forth > between Java and native code? I don't think there should be, but there could be > some annoying conversion logic eating the CPU. My gut feeling would be a lot of the time would be spent in UTF conversions, which we are going to have to do either way. The other thing is that things like ToJavaArrayOfStrings also have to call into Java for each item, so I think the total overhead is comparable both ways.
https://codereview.chromium.org/2196273002/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/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:31: @SuggestionsCategory int category, List<SnippetArticleListItem> suggestions); On 2016/08/02 13:03:27, Bernhard Bauer wrote: > On 2016/08/01 15:37:24, Bernhard Bauer wrote: > > On 2016/08/01 15:28:57, Philipp Keck wrote: > > > ContentSuggestionsCategory is now simply called Category. > > > We can leave @SuggestionsCategory as is, or maybe find a new name? Is there > a > > > convention for naming these? I'd say it would be convenient if there was a > > > deterministic way to derive the annotation name from the enum name. > > > "Suggestions" as a term is technically already taken and different from > > "content > > > suggestions". > > > > Yeah, I wasn't super happy about the name either, but given that > > https://codereview.chromium.org/1932183002/ is going to land Very Soon Nowâ„¢, > > this will be moot and we can remove the manual @IntDef completely once that > > happens. > > And in fact that happened about five minutes after I wrote this comment :) So, > changed now. > > (I realize that at some point in the future I might have to remove a bunch of > these annotations again, when we're going to have _not_ well-known categories, > i.e. purely server-side defined ones). That also happened a couple of minutes after your comment -- so we now do allow any nonnegative int for categories. "[ContentSuggestions]Category" also isn't an enum anymore, it's now a class. So the @KnownCategoriesEnum shouldn't be applied there. Admittedly, we currently don't have a provider that makes use of that possibility. But the ContentSuggestionsService interface already allows it.
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...)
https://codereview.chromium.org/2196273002/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/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:31: @SuggestionsCategory int category, List<SnippetArticleListItem> suggestions); On 2016/08/02 13:26:21, Philipp Keck wrote: > On 2016/08/02 13:03:27, Bernhard Bauer wrote: > > On 2016/08/01 15:37:24, Bernhard Bauer wrote: > > > On 2016/08/01 15:28:57, Philipp Keck wrote: > > > > ContentSuggestionsCategory is now simply called Category. > > > > We can leave @SuggestionsCategory as is, or maybe find a new name? Is > there > > a > > > > convention for naming these? I'd say it would be convenient if there was a > > > > deterministic way to derive the annotation name from the enum name. > > > > "Suggestions" as a term is technically already taken and different from > > > "content > > > > suggestions". > > > > > > Yeah, I wasn't super happy about the name either, but given that > > > https://codereview.chromium.org/1932183002/ is going to land Very Soon Nowâ„¢, > > > this will be moot and we can remove the manual @IntDef completely once that > > > happens. > > > > And in fact that happened about five minutes after I wrote this comment :) So, > > changed now. > > > > (I realize that at some point in the future I might have to remove a bunch of > > these annotations again, when we're going to have _not_ well-known categories, > > i.e. purely server-side defined ones). > > That also happened a couple of minutes after your comment -- so we now do allow > any nonnegative int for categories. Well, yes, but in practice the category values we have right now are in fact well-known and part of the KnownCategories enum. > "[ContentSuggestions]Category" also isn't an > enum anymore, it's now a class. So the @KnownCategoriesEnum shouldn't be applied > there. Admittedly, we currently don't have a provider that makes use of that > possibility. But the ContentSuggestionsService interface already allows it. KnownCategories is still an enum (class), no?
On 2016/08/02 14:38:40, Bernhard Bauer wrote: > https://codereview.chromium.org/2196273002/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/2196273002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:31: > @SuggestionsCategory int category, List<SnippetArticleListItem> suggestions); > On 2016/08/02 13:26:21, Philipp Keck wrote: > > On 2016/08/02 13:03:27, Bernhard Bauer wrote: > > > On 2016/08/01 15:37:24, Bernhard Bauer wrote: > > > > On 2016/08/01 15:28:57, Philipp Keck wrote: > > > > > ContentSuggestionsCategory is now simply called Category. > > > > > We can leave @SuggestionsCategory as is, or maybe find a new name? Is > > there > > > a > > > > > convention for naming these? I'd say it would be convenient if there was > a > > > > > deterministic way to derive the annotation name from the enum name. > > > > > "Suggestions" as a term is technically already taken and different from > > > > "content > > > > > suggestions". > > > > > > > > Yeah, I wasn't super happy about the name either, but given that > > > > https://codereview.chromium.org/1932183002/ is going to land Very Soon > Nowâ„¢, > > > > this will be moot and we can remove the manual @IntDef completely once > that > > > > happens. > > > > > > And in fact that happened about five minutes after I wrote this comment :) > So, > > > changed now. > > > > > > (I realize that at some point in the future I might have to remove a bunch > of > > > these annotations again, when we're going to have _not_ well-known > categories, > > > i.e. purely server-side defined ones). > > > > That also happened a couple of minutes after your comment -- so we now do > allow > > any nonnegative int for categories. > > Well, yes, but in practice the category values we have right now are in fact > well-known and part of the KnownCategories enum. > > > "[ContentSuggestions]Category" also isn't an > > enum anymore, it's now a class. So the @KnownCategoriesEnum shouldn't be > applied > > there. Admittedly, we currently don't have a provider that makes use of that > > possibility. But the ContentSuggestionsService interface already allows it. > > KnownCategories is still an enum (class), no? Yes and yes.
https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:291: for (@SuggestionsCategory Integer key : mSections.keySet()) { On 2016/08/02 13:03:26, Bernhard Bauer wrote: > On 2016/08/01 16:26:51, PEConn1 wrote: > > Integer -> int ? > > Hm, the key type for the Map is a boxed Integer, so it would have to be boxed > again when indexing the map on the next line. I was able to get around that > though by just directly getting a collection of the map values (which is going > to be ordered by the key). > > One other thing I could do is use a SparseArray instead of TreeMap, which would > have unboxed ints as keys and is supposedly pretty efficient for small numbers > of items. WDYT? Given we're probably going to have a handful of categories, it may be better to just stick to the simpler data structure (though I'm not sure which on that is!). https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:24: mItems = new ArrayList<>(); On 2016/08/02 13:03:27, Bernhard Bauer wrote: > On 2016/08/01 16:26:51, PEConn1 wrote: > > Why ArrayList? The list is going to be kept fairly small (about 10 items) but > > we're going to be doing random removals. > > > > Mindyou, the data locality will probably make moving all the items up one > > position in an ArrayList faster than following the links of a linked list. > > Plus, purely algorithmically, traversing a linked list is also going to take > O(n). There isn't really a good data structure in the standard Java libraries > that allows insertion/deletion at random indices :-/ Acknowledged. https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:47: for (NewTabPageListItem item : mItems) { On 2016/08/02 13:03:27, Bernhard Bauer wrote: > On 2016/08/01 16:26:51, PEConn1 wrote: > > Why do we need to search for the first SnippetArticleListItem? Can't we just > > check whether the first item is a SnippetHeaderListItem or a StatusListItem? > > I slightly prefer this, as it reflects the intent of the method more clearly (we > have suggestions if we have at least one SnippetArticleListItem). And > performance-wise it shouldn't make a big difference, because we should get a > result by the second item anyway. Acknowledged. https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleListItem.java (right): https://codereview.chromium.org/2196273002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleListItem.java:25: public final int mPosition; On 2016/08/02 13:03:27, Bernhard Bauer wrote: > On 2016/08/01 16:26:51, PEConn1 wrote: > > What is this position? Position in the whole list or in the section? > > As the comment says, it's the position in _the_ list of snippets 😉 I.e. it > refers to the old state of the world. It looks like this is only used for > tracking though, so I'd be tempted to define it as the position in the whole > list (assuming that suggestions further down are clicked less often or > something). > > > Can we add > > a comment here and one to the javadoc of the constructor about it? > > I moved the comments here from the constructor (I'd rather have them in a single > place than duplicate them). Acknowledged. https://codereview.chromium.org/2196273002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2196273002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:140: private void onSuggestionsAvailable(/* @KnownCategoriesEnum */ int category, Why is this commented out? https://codereview.chromium.org/2196273002/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2196273002/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:160: ConvertUTF16ToJavaString(env, suggestion.title()).obj(), So we have a mix of two different string representations based on whether the string is user visible? I guess it's necessary, but how come we don't have some sort of overloading that picks the correct one, eg: ConvertToJavaString(..., std::string); ConvertToJavaString(..., base::string16);
The CQ bit was checked by bauerb@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...
Rebased again :) PTAL! https://codereview.chromium.org/2196273002/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2196273002/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:160: ConvertUTF16ToJavaString(env, suggestion.title()).obj(), On 2016/08/02 15:45:15, PEConn1 wrote: > So we have a mix of two different string representations based on whether the > string is user visible? I guess it's necessary, but how come we don't have some > sort of overloading that picks the correct one, eg: > > ConvertToJavaString(..., std::string); > ConvertToJavaString(..., base::string16); Because overloads are discouraged by the style guide :) Also, the names make it clear that these strings are actually encoded in very different ways.
https://codereview.chromium.org/2196273002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2196273002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:140: private void onSuggestionsAvailable(/* @KnownCategoriesEnum */ int category, On 2016/08/02 15:45:14, PEConn1 wrote: > Why is this commented out? ^^^
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
https://codereview.chromium.org/2196273002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2196273002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:140: private void onSuggestionsAvailable(/* @KnownCategoriesEnum */ int category, On 2016/08/03 16:34:23, PEConn1 wrote: > On 2016/08/02 15:45:14, PEConn1 wrote: > > Why is this commented out? > > ^^^ Oops, forgot. The JNI generator doesn't seem to like annotations and produces non-compiling stubs...
LGTM https://codereview.chromium.org/2196273002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java (right): https://codereview.chromium.org/2196273002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:140: private void onSuggestionsAvailable(/* @KnownCategoriesEnum */ int category, On 2016/08/03 16:44:59, Bernhard Bauer wrote: > On 2016/08/03 16:34:23, PEConn1 wrote: > > On 2016/08/02 15:45:14, PEConn1 wrote: > > > Why is this commented out? > > > > ^^^ > > Oops, forgot. The JNI generator doesn't seem to like annotations and produces > non-compiling stubs... Acknowledged.
The CQ bit was checked by bauerb@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: 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 bauerb@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by bauerb@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...
bauerb@chromium.org changed reviewers: + dgn@chromium.org
Nicolas, could you take an extra look at the JUnit test changes? I had to rewrite it a bit, as the adapter now asks the snippets source for the status of a category.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
NTPATest looks good to me, but I have comments on the rest :/ https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1678: Log.e(TAG, "onDeferredStartup exception:"); Adding the throwable as last argument will print the trace and prepend the tag for each line, so line 1679 would not be needed. Also, why do we just catch it? This can happen in regular usage? Can we have a comment to explain why that's okay? https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:54: private final List<NewTabPageItemGroup> mGroups = new ArrayList<>(); Document this? Especially the difference with mSections below. AFAICT, this is only used to cache a complete list of the items. Then name it itemCache or something? https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:58: // Maps suggestion categories to sections, with stable iteration ordering. nit: why not a javadoc comment? https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:179: setSuggestions(category, Collections.<SnippetArticleListItem>emptyList(), status); Isn't that still doing the same thing as the else branch? Except without clearing and rebuilding mGroups, but it looks like the same end result. (except for notifyDataSetChanged vs more granular notifications) in the original code the goal of the else branch was to have a smaller version of loadSnippets(), but sections.setSuggestions() is the equivalent now. https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:198: return getItems().get(position).getType(); We're going to create and throw away lots of lists here. Can't we sum group sizes to get a position without doing that? But that's premature optimization I guess. https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:244: return getItems().indexOf(mAboveTheFold); using getGroupPositionOffset would avoid building a list, and looking through it, same for the bottomspacer. https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:259: public int getLastCardPosition() { The name is not technically correct anymore... the progress indicator is not a card and its position would be returned here. But that's something I should have corrected in my CL, sorry :( https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:274: copyThumbnails(category, suggestions); Items won't move across categories, right? Shouldn't this be implemented in SuggestionsSection and run at the section's discretion when it runs setSuggestions()? https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageItemGroup.java (right): https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageItemGroup.java:12: public interface NewTabPageItemGroup { nit: Do we need to have NewTabPage prepended to the name? it's already in the ntp package. nit2: do we need a group/section distinction? https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:16: * A group of suggestions, with a header and a status card. and a progress indicator. https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:24: SuggestionsSection(List<SnippetArticleListItem> suggestions, Is it using the default visibility on purpose? Why not set it on the class then? https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:46: for (NewTabPageListItem item : mItems) { I thought the point of having detailed sections like that was to be able to avoid going through the list of items to know what's going on. What about keeping mItems purely a list of suggestions, and getItems would return a new list with the other header and status items as appropriate? Would we need to recreate the list so often that it's a reason not to do it? https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:59: mStatus = StatusListItem.create(status, adapter); Maybe for another CL: Would it make sense to have a factory we give the adapter to, and that produces the statuslistitem without having to pass around an adapter just for that purpose?
https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:54: private final List<NewTabPageItemGroup> mGroups = new ArrayList<>(); On 2016/08/04 16:17:53, dgn wrote: > Document this? Especially the difference with mSections below. AFAICT, this is > only used to cache a complete list of the items. Then name it itemCache or > something? s/cache a complete list of the items/cache the order of the groups https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:198: return getItems().get(position).getType(); On 2016/08/04 16:17:53, dgn wrote: > We're going to create and throw away lots of lists here. Can't we sum group > sizes to get a position without doing that? But that's premature optimization I > guess. nvm, I see you mentioned planning to refactor this.
The CQ bit was checked by bauerb@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/2196273002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1678: Log.e(TAG, "onDeferredStartup exception:"); On 2016/08/04 16:17:53, dgn wrote: > Adding the throwable as last argument will print the trace and prepend the tag > for each line, so line 1679 would not be needed. > > Also, why do we just catch it? This can happen in regular usage? Can we have a > comment to explain why that's okay? Sorry, that slipped in while I was trying to debug the stack overflow (that you actually fixed) the other day. https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:54: private final List<NewTabPageItemGroup> mGroups = new ArrayList<>(); On 2016/08/04 16:21:40, dgn wrote: > On 2016/08/04 16:17:53, dgn wrote: > > Document this? Especially the difference with mSections below. AFAICT, this is > > only used to cache a complete list of the items. Then name it itemCache or > > something? > > s/cache a complete list of the items/cache the order of the groups It's a full list of all the items, whereas |mSections| below is only for card sections. https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:58: // Maps suggestion categories to sections, with stable iteration ordering. On 2016/08/04 16:17:53, dgn wrote: > nit: why not a javadoc comment? Done. https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:179: setSuggestions(category, Collections.<SnippetArticleListItem>emptyList(), status); On 2016/08/04 16:17:53, dgn wrote: > Isn't that still doing the same thing as the else branch? Except without > clearing and rebuilding mGroups, but it looks like the same end result. (except > for notifyDataSetChanged vs more granular notifications) > > in the original code the goal of the else branch was to have a smaller version > of loadSnippets(), but sections.setSuggestions() is the equivalent now. Changed to only use setSuggestions(). https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:244: return getItems().indexOf(mAboveTheFold); On 2016/08/04 16:17:53, dgn wrote: > using getGroupPositionOffset would avoid building a list, and looking through > it, same for the bottomspacer. Done. https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:259: public int getLastCardPosition() { On 2016/08/04 16:17:53, dgn wrote: > The name is not technically correct anymore... the progress indicator is not a > card and its position would be returned here. But that's something I should have > corrected in my CL, sorry :( Renamed to getLastContentItemPosition(). https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:274: copyThumbnails(category, suggestions); On 2016/08/04 16:17:53, dgn wrote: > Items won't move across categories, right? Shouldn't this be implemented in > SuggestionsSection and run at the section's discretion when it runs > setSuggestions()? Moved to SuggestionsSection. Really, now that we have Promises, we should just store the images as Promises in the items themselves. https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageItemGroup.java (right): https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageItemGroup.java:12: public interface NewTabPageItemGroup { On 2016/08/04 16:17:53, dgn wrote: > nit: Do we need to have NewTabPage prepended to the name? it's already in the > ntp package. Renamed. > nit2: do we need a group/section distinction? Right now a group is just a container for a list of items, and all the items are flattened into a big list. I think what I would like to do is merge the ListItem and ItemGroup interfaces into a single tree-like interface and have classes for the various individual items and sections. Stay tuned :) https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:16: * A group of suggestions, with a header and a status card. On 2016/08/04 16:17:53, dgn wrote: > and a progress indicator. :) Done. https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:24: SuggestionsSection(List<SnippetArticleListItem> suggestions, On 2016/08/04 16:17:53, dgn wrote: > Is it using the default visibility on purpose? Why not set it on the class then? Made public for now. We can look into restricting visibility later. https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:46: for (NewTabPageListItem item : mItems) { On 2016/08/04 16:17:53, dgn wrote: > I thought the point of having detailed sections like that was to be able to > avoid going through the list of items to know what's going on. What about > keeping mItems purely a list of suggestions, and getItems would return a new > list with the other header and status items as appropriate? Would we need to > recreate the list so often that it's a reason not to do it? Done! That's much closer to what I eventually want to do anyway. https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:59: mStatus = StatusListItem.create(status, adapter); On 2016/08/04 16:17:53, dgn wrote: > Maybe for another CL: Would it make sense to have a factory we give the adapter > to, and that produces the statuslistitem without having to pass around an > adapter just for that purpose? Or we keep the object and have it update itself with the new status value?
Thanks! lgtm, when the 2 things I commented are fixed :) https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1678: Log.e(TAG, "onDeferredStartup exception:"); On 2016/08/05 10:13:24, Bernhard Bauer wrote: > On 2016/08/04 16:17:53, dgn wrote: > > Adding the throwable as last argument will print the trace and prepend the tag > > for each line, so line 1679 would not be needed. > > > > Also, why do we just catch it? This can happen in regular usage? Can we have a > > comment to explain why that's okay? > > Sorry, that slipped in while I was trying to debug the stack overflow (that you > actually fixed) the other day. Still in the latest patch :D https://codereview.chromium.org/2196273002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2196273002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:186: if (!sectionItems.isEmpty()) { setSuggestions() already calls notifyDataSetChanged(), the notifications below are unnecessary
The CQ bit was checked by bauerb@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/2196273002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2196273002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1678: Log.e(TAG, "onDeferredStartup exception:"); On 2016/08/05 10:47:52, dgn wrote: > On 2016/08/05 10:13:24, Bernhard Bauer wrote: > > On 2016/08/04 16:17:53, dgn wrote: > > > Adding the throwable as last argument will print the trace and prepend the > tag > > > for each line, so line 1679 would not be needed. > > > > > > Also, why do we just catch it? This can happen in regular usage? Can we have > a > > > comment to explain why that's okay? > > > > Sorry, that slipped in while I was trying to debug the stack overflow (that > you > > actually fixed) the other day. > > Still in the latest patch :D Argh :) Done. https://codereview.chromium.org/2196273002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/2196273002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:186: if (!sectionItems.isEmpty()) { On 2016/08/05 10:47:52, dgn wrote: > setSuggestions() already calls notifyDataSetChanged(), the notifications below > are unnecessary Removed (but added a TODO to use more granular notifications).
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...)
The CQ bit was checked by bauerb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org, dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2196273002/#ps200001 (title: "fix")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bauerb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org, dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2196273002/#ps220001 (title: "rebase")
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.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Zine: support multiple sections in the ui * The adapter now manages groups of items. * A SuggestionsSection is a group that holds suggestions, a header, and a status card. * Add @SuggestionsCategory and @SuggestionsCategoryStatus annotations. * SnippetsObserver now specifies what category it is talking about. Based on http://codereview.chromium.org/2194433002#ps20001 by mvanouwerkerk@chromium.org. BUG=616090 ========== to ========== Zine: support multiple sections in the ui * The adapter now manages groups of items. * A SuggestionsSection is a group that holds suggestions, a header, and a status card. * Add @SuggestionsCategory and @SuggestionsCategoryStatus annotations. * SnippetsObserver now specifies what category it is talking about. Based on http://codereview.chromium.org/2194433002#ps20001 by mvanouwerkerk@chromium.org. BUG=616090 Committed: https://crrev.com/57a839dc61aece5b2138586dd0ffaa500b7ccc7f Cr-Commit-Position: refs/heads/master@{#410075} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/57a839dc61aece5b2138586dd0ffaa500b7ccc7f Cr-Commit-Position: refs/heads/master@{#410075} |
