Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2235463002:
Change Snippet layout based on Category. (Closed)
DescriptionChange Snippet layout based on Category.
BUG=634275, 631479
Committed: https://crrev.com/9ada493805a5a7abc38a38a706ba5db7f627c4db
Cr-Commit-Position: refs/heads/master@{#411671}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Rebased and nits. #
Total comments: 5
Patch Set 3 : Pass card layout at construction. #Patch Set 4 : Pass card layout at construction. #
Total comments: 8
Patch Set 5 : Crash when category info is not found. #Patch Set 6 : Update tests. #Patch Set 7 : Merge in master. #Messages
Total messages: 40 (23 generated)
peconn@chromium.org changed reviewers: + bauerb@chromium.org, mvanouwerkerk@chromium.org, pke@google.com
PTAL - there are basically two different parts to this CL: The work to change the layout, in: chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java chrome/android/java/res/values/dimens.xml chrome/android/java/res/layout/new_tab_page_snippets_card.xml Getting the layout type to the SnippetArticleViewHolder, in: chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleListItem.java chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java chrome/browser/android/ntp/ntp_snippets_bridge.cc The first part shouldn't be controversial, the second part goes into the discussion we had earlier about where the layout type should live.
lgtm with nit https://codereview.chromium.org/2235463002/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/2235463002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleListItem.java:44: @KnownCategories.KnownCategoriesEnum This isn't always going to be a known category is it? I don't think we have an annotation for just "Category" yet. See https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr... for how we'll just pass a plain int for the time being.
https://codereview.chromium.org/2235463002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2235463002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:227: return mArticle.mCategory == KnownCategories.BOOKMARKS; So how would you hook this up to the category-info? mArticle.mCardLayout instead of mArticle.mCategory? Or snippetsSource.getCategoryInfo(mArticle.mCategory).getCardLayout()?
https://codereview.chromium.org/2235463002/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/2235463002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleListItem.java:44: @KnownCategories.KnownCategoriesEnum On 2016/08/10 09:52:31, Michael van Ouwerkerk wrote: > This isn't always going to be a known category is it? I don't think we have an > annotation for just "Category" yet. See > https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr... > for how we'll just pass a plain int for the time being. I'm also working on that CL now to use a separate annotation :) https://codereview.chromium.org/2235463002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2235463002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:227: return mArticle.mCategory == KnownCategories.BOOKMARKS; Now that https://codereview.chromium.org/2207493002/ landed, we should be able to get that information from the SuggestionsService. https://codereview.chromium.org/2235463002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:235: mArticleSnippetTextView.setVisibility(visibility); Is this going to work if we start out in the NARROW display style? (It might be good to extract this into a single method that sets the visibility based on both the article type and the display style.) https://codereview.chromium.org/2235463002/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/2235463002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:137: long timestamp, float score, int category) { FYI, this will have to be rebased on https://codereview.chromium.org/2230473002, which will make storing the category inside the suggestion a bit redundant (as we will only fetch suggestions by category anyway), but we can refactor that later, I guess.
PTAL! https://codereview.chromium.org/2235463002/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/2235463002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleListItem.java:44: @KnownCategories.KnownCategoriesEnum On 2016/08/10 10:11:53, Bernhard Bauer wrote: > On 2016/08/10 09:52:31, Michael van Ouwerkerk wrote: > > This isn't always going to be a known category is it? I don't think we have an > > annotation for just "Category" yet. See > > > https://codereview.chromium.org/2230473002/diff/180001/chrome/android/java/sr... > > for how we'll just pass a plain int for the time being. > > I'm also working on that CL now to use a separate annotation :) This isn't needed anymore. https://codereview.chromium.org/2235463002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2235463002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:227: return mArticle.mCategory == KnownCategories.BOOKMARKS; On 2016/08/10 10:11:53, Bernhard Bauer wrote: > Now that https://codereview.chromium.org/2207493002/ landed, we should be able > to get that information from the SuggestionsService. I decided to create a back link from the Article to the SuggestionsSection. To me this makes more sense as all article's in a SuggestionsSection share a layout (instead of having each article hold it's own layout). https://codereview.chromium.org/2235463002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:235: mArticleSnippetTextView.setVisibility(visibility); On 2016/08/10 10:11:53, Bernhard Bauer wrote: > Is this going to work if we start out in the NARROW display style? > > (It might be good to extract this into a single method that sets the visibility > based on both the article type and the display style.) Done. https://codereview.chromium.org/2235463002/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/2235463002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java:137: long timestamp, float score, int category) { On 2016/08/10 10:11:53, Bernhard Bauer wrote: > FYI, this will have to be rebased on https://codereview.chromium.org/2230473002, > which will make storing the category inside the suggestion a bit redundant (as > we will only fetch suggestions by category anyway), but we can refactor that > later, I guess. This isn't needed anymore.
https://codereview.chromium.org/2235463002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2235463002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:238: // If we aren't showing the article snippet, reduce the top margin for publisher text. The margin is only reduced if we are in minimal layout (so not when we have full snippets but in narrow layout). I'm assuming since we weren't modifying the margin in that case that the presence of a thumbnail makes this unnecessary.
Description was changed from ========== Change Snippet layout based on Category. BUG=634275 ========== to ========== Change Snippet layout based on Category. BUG=634275, 631479 ==========
lgtm https://codereview.chromium.org/2235463002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2235463002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:229: // If the screen in narrow, increase the number of lines in the header. Nit: "is narrow"
One nit to consider for refinement, but still lgtm :-) https://codereview.chromium.org/2235463002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2235463002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:80: suggestion.setSection(this); nit: The suggestion currently doesn't need to know the section it is in, just the category info. You could set that when the SnippetArticleListItem is instantiated, so there is no mSuggestionSection member that is temporarily null, and needs to be initialized in a place other than where the SnippetArticleListItem is instantiated.
Could you please take a quick look at the changes to snippets bridge? https://codereview.chromium.org/2235463002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2235463002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:80: suggestion.setSection(this); On 2016/08/11 09:22:52, Michael van Ouwerkerk wrote: > nit: The suggestion currently doesn't need to know the section it is in, just > the category info. You could set that when the SnippetArticleListItem is > instantiated, so there is no mSuggestionSection member that is temporarily null, > and needs to be initialized in a place other than where the > SnippetArticleListItem is instantiated. Done. https://codereview.chromium.org/2235463002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2235463002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:229: // If the screen in narrow, increase the number of lines in the header. On 2016/08/11 09:07:27, Bernhard Bauer wrote: > Nit: "is narrow" Done.
https://codereview.chromium.org/2235463002/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2235463002/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:156: int layout = static_cast<int>(info ? info->card_layout() I'm not sure we need to deal with the category info missing here -- it would be a bug if we didn't have it.
https://codereview.chromium.org/2235463002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2235463002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:30: mInfo = info; Actually, does this need to be stored as a member at all? It is private and the info is only used in the constructor. https://codereview.chromium.org/2235463002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:37: public SuggestionsCategoryInfo getCategoryInfo() { Is this still needed? https://codereview.chromium.org/2235463002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleListItem.java (right): https://codereview.chromium.org/2235463002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleListItem.java:61: String url, String ampUrl, long timestamp, float score, int position, int cardLayout) { Add @ContentSuggestionsCardLayoutEnum ?
https://codereview.chromium.org/2235463002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2235463002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:30: mInfo = info; On 2016/08/11 15:25:48, Michael van Ouwerkerk wrote: > Actually, does this need to be stored as a member at all? It is private and the > info is only used in the constructor. Done. https://codereview.chromium.org/2235463002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:37: public SuggestionsCategoryInfo getCategoryInfo() { On 2016/08/11 15:25:48, Michael van Ouwerkerk wrote: > Is this still needed? Done. https://codereview.chromium.org/2235463002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleListItem.java (right): https://codereview.chromium.org/2235463002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleListItem.java:61: String url, String ampUrl, long timestamp, float score, int position, int cardLayout) { On 2016/08/11 15:25:48, Michael van Ouwerkerk wrote: > Add @ContentSuggestionsCardLayoutEnum ? Done. https://codereview.chromium.org/2235463002/diff/60001/chrome/browser/android/... File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right): https://codereview.chromium.org/2235463002/diff/60001/chrome/browser/android/... chrome/browser/android/ntp/ntp_snippets_bridge.cc:156: int layout = static_cast<int>(info ? info->card_layout() On 2016/08/11 15:22:24, Bernhard Bauer wrote: > I'm not sure we need to deal with the category info missing here -- it would be > a bug if we didn't have it. Done.
lgtm
Thanks Peter! Still good.
The CQ bit was checked by peconn@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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by peconn@chromium.org to run a CQ dry run
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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by peconn@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...)
The CQ bit was checked by peconn@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 peconn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2235463002/#ps120001 (title: "Merge in master.")
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 ========== Change Snippet layout based on Category. BUG=634275, 631479 ========== to ========== Change Snippet layout based on Category. BUG=634275, 631479 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Change Snippet layout based on Category. BUG=634275, 631479 ========== to ========== Change Snippet layout based on Category. BUG=634275, 631479 Committed: https://crrev.com/9ada493805a5a7abc38a38a706ba5db7f627c4db Cr-Commit-Position: refs/heads/master@{#411671} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/9ada493805a5a7abc38a38a706ba5db7f627c4db Cr-Commit-Position: refs/heads/master@{#411671} |
