Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1961073002:
Add the snippet header back into the code and show the snippet header when the user scrolls the sni… (Closed)
|
Created:
4 years, 7 months ago by mcwilliams Modified:
4 years, 7 months ago CC:
chromium-reviews, mcwilliams+watch_chromium.org, dgn+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd the snippet header back into the code and show the snippet header
Show the header when the user scrolls the snippets towards the top of the viewport.
videos:
Nexus5: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE
Nexus6: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE
BUG=596418, 608918
Committed: https://crrev.com/8481607348529fd6085c243257f4b547cd5e5bef
Cr-Commit-Position: refs/heads/master@{#392882}
Patch Set 1 #
Total comments: 29
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 30 (15 generated)
Description was changed from ========== Add the snippet header back into the code and show the snippet header when the user scrolls the snippets towards the top of the viewport BUG=596418,608918 ========== to ========== Add the snippet header back into the code and show the snippet header when the user scrolls the snippets towards the top of the viewport. videos: Nexus5: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE Nexus6:https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE BUG=596418,608918 ==========
mcwilliams@chromium.org changed reviewers: + bauerb@chromium.org, maybelle@chromium.org
In general: it feels like we are doing too much in NewTabPageView. Can we achieve the same thing by passing in the necessary parameters to the header view instead? (And the card view, for the peeking card) https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:125: private int mViewPortHeight; We don't seem to use this in this file. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:500: for (int i = 0; i < adapterSize; i++) { Can you pull out this section here into a private helper function and change updatePeekingCard() to use that? https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:513: int numberHeaderHeight = 2; I have no idea what this is. Can you clarify? Also if it's never going to change, make it final. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:517: int heightOmnibox = mNewTabPageLayout.getPaddingTop(); Can you get the omnibox height directly rather than using the fact that the padding happens to be the same height? https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:519: // Check if snippet header top - minus the height of the omnibox is less than value to The first line of the comment is confusing to me. It's describing the calculations rather than saying what you're trying to achieve (e.g., "Check if the top of the snippets is getting to within x of the omnibox, and if to start showing the header") https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:550: private void setSnippetHeaderDisplay(View snippetHeader, int height, float alpha) { This can be a static function.
Description was changed from ========== Add the snippet header back into the code and show the snippet header when the user scrolls the snippets towards the top of the viewport. videos: Nexus5: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE Nexus6:https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE BUG=596418,608918 ========== to ========== Add the snippet header back into the code and show the snippet header when the user scrolls the snippets towards the top of the viewport. videos: Nexus5: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE Nexus6: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE BUG=596418,608918 ==========
https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/new_tab_page_snippets_header.xml (right): https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/new_tab_page_snippets_header.xml:15: android:gravity="start|center" /> "center" is basically a shortcut for "center_vertical|center_horizontal", and given that you're already specifying a horizontal gravity, use "start|center_vertical"? https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:508: if (snippetHeader != null && snippetHeader.isShown()) { Return early otherwise? https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:517: int heightOmnibox = mNewTabPageLayout.getPaddingTop(); Nit: |omniboxHeight|? https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:524: // heightToStartChangingHeader. Use |pipe| symbols for variable names. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:547: * @param snippetHeader Parameters don't match. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:550: private void setSnippetHeaderDisplay(View snippetHeader, int height, float alpha) { On 2016/05/09 16:15:19, May wrote: > This can be a static function. Or just inline this and move the call sites out of the if-statement?
Description was changed from ========== Add the snippet header back into the code and show the snippet header when the user scrolls the snippets towards the top of the viewport. videos: Nexus5: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE Nexus6: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE BUG=596418,608918 ========== to ========== Add the snippet header back into the code and show the snippet header Show the header when the user scrolls the snippets towards the top of the viewport. videos: Nexus5: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE Nexus6: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE BUG=596418,608918 ==========
https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:125: private int mViewPortHeight; On 2016/05/09 16:15:19, May wrote: > We don't seem to use this in this file. Done. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:500: for (int i = 0; i < adapterSize; i++) { On 2016/05/09 16:15:19, May wrote: > Can you pull out this section here into a private helper function and change > updatePeekingCard() to use that? Done. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:508: if (snippetHeader != null && snippetHeader.isShown()) { On 2016/05/09 17:47:41, Bernhard Bauer wrote: > Return early otherwise? Seems a little verbose to return when this is all the function does and will exit now naturally https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:513: int numberHeaderHeight = 2; On 2016/05/09 16:15:19, May wrote: > I have no idea what this is. Can you clarify? Also if it's never going to > change, make it final. Done. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:513: int numberHeaderHeight = 2; On 2016/05/09 16:15:19, May wrote: > I have no idea what this is. Can you clarify? Also if it's never going to > change, make it final. Done. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:517: int heightOmnibox = mNewTabPageLayout.getPaddingTop(); On 2016/05/09 17:47:41, Bernhard Bauer wrote: > Nit: |omniboxHeight|? Done. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:517: int heightOmnibox = mNewTabPageLayout.getPaddingTop(); On 2016/05/09 16:15:19, May wrote: > Can you get the omnibox height directly rather than using the fact that the > padding happens to be the same height? 1. This is already being done here: initializeSearchBoxRecyclerViewScrollHandling line 574 2. Unfortunately I do not have access to the ToolbarPhone from NTPLayout. The padding is set for the layout in the xml as: android:paddingTop="@dimen/toolbar_height_no_shadow" Any suggestions? https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:519: // Check if snippet header top - minus the height of the omnibox is less than value to On 2016/05/09 16:15:19, May wrote: > The first line of the comment is confusing to me. It's describing the > calculations rather than saying what you're trying to achieve (e.g., "Check if > the top of the snippets is getting to within x of the omnibox, and if to start > showing the header") Done. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:524: // heightToStartChangingHeader. On 2016/05/09 17:47:41, Bernhard Bauer wrote: > Use |pipe| symbols for variable names. Done. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:547: * @param snippetHeader On 2016/05/09 17:47:41, Bernhard Bauer wrote: > Parameters don't match. Done. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:550: private void setSnippetHeaderDisplay(View snippetHeader, int height, float alpha) { On 2016/05/09 16:15:19, May wrote: > This can be a static function. Acknowledged. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:550: private void setSnippetHeaderDisplay(View snippetHeader, int height, float alpha) { On 2016/05/09 17:47:40, Bernhard Bauer wrote: > On 2016/05/09 16:15:19, May wrote: > > This can be a static function. > > Or just inline this and move the call sites out of the if-statement? Agree, it as more complex when messing with margins but figured I could simplify and never removed this
lgtm Can you file a tracking bug to move the peeking card and header logic out of here and into its own class? Once we get done with MVP we should tackle that to avoid even more creep. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:517: int heightOmnibox = mNewTabPageLayout.getPaddingTop(); On 2016/05/10 09:26:05, mcwilliams wrote: > On 2016/05/09 16:15:19, May wrote: > > Can you get the omnibox height directly rather than using the fact that the > > padding happens to be the same height? > > 1. This is already being done here: > initializeSearchBoxRecyclerViewScrollHandling line 574 > 2. Unfortunately I do not have access to the ToolbarPhone from NTPLayout. The > padding is set for the layout in the xml as: > android:paddingTop="@dimen/toolbar_height_no_shadow" > > Any suggestions? Yeah, seems like there's precedent for using this. I don't see any clear alternatives, let's leave it as is.
crbug://610692 https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/new_tab_page_snippets_header.xml (right): https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/new_tab_page_snippets_header.xml:15: android:gravity="start|center" /> On 2016/05/09 17:47:40, Bernhard Bauer wrote: > "center" is basically a shortcut for "center_vertical|center_horizontal", and > given that you're already specifying a horizontal gravity, use > "start|center_vertical"? Done. https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:517: int heightOmnibox = mNewTabPageLayout.getPaddingTop(); On 2016/05/10 13:46:09, May wrote: > On 2016/05/10 09:26:05, mcwilliams wrote: > > On 2016/05/09 16:15:19, May wrote: > > > Can you get the omnibox height directly rather than using the fact that the > > > padding happens to be the same height? > > > > 1. This is already being done here: > > initializeSearchBoxRecyclerViewScrollHandling line 574 > > 2. Unfortunately I do not have access to the ToolbarPhone from NTPLayout. The > > padding is set for the layout in the xml as: > > android:paddingTop="@dimen/toolbar_height_no_shadow" > > > > Any suggestions? > > Yeah, seems like there's precedent for using this. I don't see any clear > alternatives, let's leave it as is. Acknowledged.
On 2016/05/10 13:59:09, mcwilliams wrote: > crbug://610692 That's not a valid URL scheme 😉 Most common is crbug.com/610692 (or https://crbug.com/610692, because HTTPS FTW 😃). https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:508: if (snippetHeader != null && snippetHeader.isShown()) { On 2016/05/10 09:26:05, mcwilliams wrote: > On 2016/05/09 17:47:41, Bernhard Bauer wrote: > > Return early otherwise? > > Seems a little verbose to return when this is all the function does and will > exit now naturally Early returns are the idiomatic style in Chrome/Android code, as it prevents indentation "pyramids", and in particular with the Android style four-space indent, that can happen pretty fast.
PTAL https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1961073002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:508: if (snippetHeader != null && snippetHeader.isShown()) { On 2016/05/10 14:14:19, Bernhard Bauer wrote: > On 2016/05/10 09:26:05, mcwilliams wrote: > > On 2016/05/09 17:47:41, Bernhard Bauer wrote: > > > Return early otherwise? > > > > Seems a little verbose to return when this is all the function does and will > > exit now naturally > > Early returns are the idiomatic style in Chrome/Android code, as it prevents > indentation "pyramids", and in particular with the Android style four-space > indent, that can happen pretty fast. Makes sense :) thanks
LGTM!
The CQ bit was checked by mcwilliams@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maybelle@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/1961073002/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1961073002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1961073002/60001
The CQ bit was checked by mcwilliams@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, maybelle@chromium.org Link to the patchset: https://codereview.chromium.org/1961073002/#ps80001 (title: " ")
The CQ bit was unchecked by mcwilliams@chromium.org
The CQ bit was checked by mcwilliams@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1961073002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1961073002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by mcwilliams@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, maybelle@chromium.org Link to the patchset: https://codereview.chromium.org/1961073002/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1961073002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1961073002/100001
Message was sent while issue was closed.
Description was changed from ========== Add the snippet header back into the code and show the snippet header Show the header when the user scrolls the snippets towards the top of the viewport. videos: Nexus5: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE Nexus6: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE BUG=596418,608918 ========== to ========== Add the snippet header back into the code and show the snippet header Show the header when the user scrolls the snippets towards the top of the viewport. videos: Nexus5: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE Nexus6: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE BUG=596418,608918 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add the snippet header back into the code and show the snippet header Show the header when the user scrolls the snippets towards the top of the viewport. videos: Nexus5: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE Nexus6: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE BUG=596418,608918 ========== to ========== Add the snippet header back into the code and show the snippet header Show the header when the user scrolls the snippets towards the top of the viewport. videos: Nexus5: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE Nexus6: https://drive.google.com/open?id=0B1IgAIJ9cgizZ3h5QUVPaEhmMUE BUG=596418,608918 Committed: https://crrev.com/8481607348529fd6085c243257f4b547cd5e5bef Cr-Commit-Position: refs/heads/master@{#392882} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8481607348529fd6085c243257f4b547cd5e5bef Cr-Commit-Position: refs/heads/master@{#392882} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
