Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1947263003:
Pin the snippets to the bottom of the page and introduce a peeking card to indicate to the user the… (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, PEConn Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPeeking cards
Pin the snippets to the bottom of the page and introduce a peeking card to indicate to the user there is something at the bottom of the page. When scrolling up transition the peeking card into a full bleed card.
BUG=586126, 609487
Committed: https://crrev.com/1c4d6b032de178f2a2a6f9beff8d7067c07970b8
Cr-Commit-Position: refs/heads/master@{#391955}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 56
Patch Set 6 : #
Total comments: 4
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 2
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Messages
Total messages: 45 (22 generated)
Description was changed from ========== Pin the snippets to the bottom of the page and introduce a peeking card to indicate to the user there is something at the bottom of the page. When scrolling up transition the peeking card into a full bleed card. BUG=586126 ========== to ========== Pin the snippets to the bottom of the page and introduce a peeking card to indicate to the user there is something at the bottom of the page. When scrolling up transition the peeking card into a full bleed card. BUG=586126, 609487 ==========
mcwilliams@chromium.org changed reviewers: + bauerb@chromium.org, maybelle@chromium.org
peconn@chromium.org changed reviewers: + peconn@chromium.org
https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:26: private NewTabPageLayout mNewTabPageLayout; Do we ever read this?
https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:84: Log.i("NewTabPageLayout", "set mParentScrollViewportHeight: %s", height); Remove log https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:80: private static boolean USE_CARDS_UI; ALL_CAPS only for static final, otherwise use sAbcXyz http://source.android.com/source/code-style.html#follow-field-naming-conventions https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:123: private int peekingCardVerticalScrollStart; Unused variable? https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:397: mRecyclerView.setNewTabPageLayout(mNewTabPageLayout); I don't think this is necessary anymore https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:456: int recyclerViewSize = mNewTabPageAdapter.getItemCount(); Not really the recyclerViewSize, it's the adapterSize https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:530: snapToMostLikely && currentScroll < mNewTabPageLayout.getHeight() * 2 / 3) { unnecessary line break https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1150: mNewTabPageLayout.setParentScrollViewportHeight(MeasureSpec.getSize(heightMeasureSpec)); If we're doing this here, do we still need the mNewTabPageLayout.setParentScrollViewportHeight() in NewTabPageScrollView? https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:108: // Remove this for now as we need to come up with a better way to visibly not affect the add a //TODO https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:81: public void setNewTabPageLayout(NewTabPageLayout newTabPageLayout) { Don't think we need this anymore
https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:84: Log.i("NewTabPageLayout", "set mParentScrollViewportHeight: %s", height); I think we can remove this? https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:468: int maxPadding = mNewTabPageLayout.getContext().getResources().getDimensionPixelSize( mNewTabPageLayout.getContext().getResources() can be replaced with just: getResources() (the view has access to the Context and the Resources) https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:482: maxPadding - bleed, maxPadding, maxPadding - bleed, maxPadding); Maybe add a comment that the 2nd and 4th arguments (the top and bottom padding) happen to be equal to maxPadding, and we're not actually changing them. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:493: * Sets up scrolling when snippets are enabled. It adds scroll listeners and touch listeners to I'd argue this comment is a bit too indepth - the first part is OK, but the 'adds scroll listeners and touch listeners' is a bit too close to implementation. But this is probably just personal preference. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:578: * Sets up scrolling when snippets are disabled. It ads scroll and touch listeners to the scroll *adds https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1168: return USE_CARDS_UI; What if the NewTabPageView hasn't been initialized? I don't think we'd come across this in this CL, but 'isUseCardsUiEnabled' sounds pretty generic and the fact its a static function implies it should work regardless of whether an instance of NewTabPageView has had a certain function run on it. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:111: // mNewTabPageListItems.add(new SnippetHeaderListItem()); Change this to a TODO (and get rid of the commented out line of code).
Nice! https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:262: <dimen name="snippets_vertical_space">2dp</dimen> Is this used? https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:84: Log.i("NewTabPageLayout", "set mParentScrollViewportHeight: %s", height); Make this Log.d(), so it can be removed in Debug builds? Also, the log tag should be a constant. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:96: if (NewTabPageView.isUseCardsUiEnabled()) { Aaaand now I see why you wanted to make this static 😃 I think it would be nicer not to work around the fact that this class doesn't have a reference to the NewTabPageView. We could make this flag a member and pass it in via a method that is called from NewTabPageView.initialize() after it has inflated the NewTabPageLayout. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:102: setLayoutParams(params); If you need to trigger a layout after changing the LayoutParams, you can just call requestLayout(). https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:105: // TODO(https://crbug.com/609487): Implement push snippets below the fold when most likely Nit: s/Implement // https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:80: private static boolean USE_CARDS_UI; SHOUTING_CASE is only used for constants (which are final). It also feels a bit weird to assign a static variable from a constructor argument. We could keep it as a member variable, but make it final? https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:123: private int peekingCardVerticalScrollStart; Member variables should start with "m" (and continue in CamelCase, i.e. mPeekingCardVerticalScrollStart). https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:453: private void updatePeekingCard() { updatePeekingCardOnScroll()? https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:578: * Sets up scrolling when snippets are disabled. It ads scroll and touch listeners to the scroll Nit: "adds" https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:110: // visibility is set to GONE. Please add a TODO to address 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/patch-status/1947263003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1947263003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
PTAL https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:262: <dimen name="snippets_vertical_space">2dp</dimen> On 2016/05/05 16:46:04, Bernhard Bauer wrote: > Is this used? Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:84: Log.i("NewTabPageLayout", "set mParentScrollViewportHeight: %s", height); On 2016/05/05 16:31:28, PEConn1 wrote: > I think we can remove this? Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:84: Log.i("NewTabPageLayout", "set mParentScrollViewportHeight: %s", height); On 2016/05/05 16:28:21, May wrote: > Remove log Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:84: Log.i("NewTabPageLayout", "set mParentScrollViewportHeight: %s", height); On 2016/05/05 16:46:04, Bernhard Bauer wrote: > Make this Log.d(), so it can be removed in Debug builds? Also, the log tag > should be a constant. Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:96: if (NewTabPageView.isUseCardsUiEnabled()) { On 2016/05/05 16:46:04, Bernhard Bauer wrote: > Aaaand now I see why you wanted to make this static 😃 I think it would be nicer > not to work around the fact that this class doesn't have a reference to the > NewTabPageView. We could make this flag a member and pass it in via a method > that is called from NewTabPageView.initialize() after it has inflated the > NewTabPageLayout. Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:102: setLayoutParams(params); On 2016/05/05 16:46:04, Bernhard Bauer wrote: > If you need to trigger a layout after changing the LayoutParams, you can just > call requestLayout(). Removed as not needed https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:105: // TODO(https://crbug.com/609487): Implement push snippets below the fold when most likely On 2016/05/05 16:46:04, Bernhard Bauer wrote: > Nit: s/Implement // Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:80: private static boolean USE_CARDS_UI; On 2016/05/05 16:28:22, May wrote: > ALL_CAPS only for static final, otherwise use sAbcXyz > > http://source.android.com/source/code-style.html#follow-field-naming-conventions Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:80: private static boolean USE_CARDS_UI; On 2016/05/05 16:46:04, Bernhard Bauer wrote: > SHOUTING_CASE is only used for constants (which are final). It also feels a bit > weird to assign a static variable from a constructor argument. We could keep it > as a member variable, but make it final? Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:123: private int peekingCardVerticalScrollStart; On 2016/05/05 16:28:22, May wrote: > Unused variable? Very much used https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:123: private int peekingCardVerticalScrollStart; On 2016/05/05 16:46:04, Bernhard Bauer wrote: > Member variables should start with "m" (and continue in CamelCase, i.e. > mPeekingCardVerticalScrollStart). Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:397: mRecyclerView.setNewTabPageLayout(mNewTabPageLayout); On 2016/05/05 16:28:22, May wrote: > I don't think this is necessary anymore Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:453: private void updatePeekingCard() { On 2016/05/05 16:46:04, Bernhard Bauer wrote: > updatePeekingCardOnScroll()? It is not just on scroll but also on initialize https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:456: int recyclerViewSize = mNewTabPageAdapter.getItemCount(); On 2016/05/05 16:28:22, May wrote: > Not really the recyclerViewSize, it's the adapterSize Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:468: int maxPadding = mNewTabPageLayout.getContext().getResources().getDimensionPixelSize( On 2016/05/05 16:31:28, PEConn1 wrote: > mNewTabPageLayout.getContext().getResources() > > can be replaced with just: > > getResources() > > (the view has access to the Context and the Resources) Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:482: maxPadding - bleed, maxPadding, maxPadding - bleed, maxPadding); On 2016/05/05 16:31:28, PEConn1 wrote: > Maybe add a comment that the 2nd and 4th arguments (the top and bottom padding) > happen to be equal to maxPadding, and we're not actually changing them. Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:493: * Sets up scrolling when snippets are enabled. It adds scroll listeners and touch listeners to On 2016/05/05 16:31:28, PEConn1 wrote: > I'd argue this comment is a bit too indepth - the first part is OK, but the > 'adds scroll listeners and touch listeners' is a bit too close to > implementation. But this is probably just personal preference. Acknowledged. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:530: snapToMostLikely && currentScroll < mNewTabPageLayout.getHeight() * 2 / 3) { On 2016/05/05 16:28:22, May wrote: > unnecessary line break If I don't line break it is over 100 chars? https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:578: * Sets up scrolling when snippets are disabled. It ads scroll and touch listeners to the scroll On 2016/05/05 16:31:28, PEConn1 wrote: > *adds Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:578: * Sets up scrolling when snippets are disabled. It ads scroll and touch listeners to the scroll On 2016/05/05 16:46:04, Bernhard Bauer wrote: > Nit: "adds" Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1150: mNewTabPageLayout.setParentScrollViewportHeight(MeasureSpec.getSize(heightMeasureSpec)); On 2016/05/05 16:28:22, May wrote: > If we're doing this here, do we still need the > mNewTabPageLayout.setParentScrollViewportHeight() in NewTabPageScrollView? No, removed https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1168: return USE_CARDS_UI; On 2016/05/05 16:31:28, PEConn1 wrote: > What if the NewTabPageView hasn't been initialized? > > I don't think we'd come across this in this CL, but 'isUseCardsUiEnabled' sounds > pretty generic and the fact its a static function implies it should work > regardless of whether an instance of NewTabPageView has had a certain function > run on it. Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:108: // Remove this for now as we need to come up with a better way to visibly not affect the On 2016/05/05 16:28:22, May wrote: > add a //TODO Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:110: // visibility is set to GONE. On 2016/05/05 16:46:04, Bernhard Bauer wrote: > Please add a TODO to address this. Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java:111: // mNewTabPageListItems.add(new SnippetHeaderListItem()); On 2016/05/05 16:31:28, PEConn1 wrote: > Change this to a TODO (and get rid of the commented out line of code). Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:26: private NewTabPageLayout mNewTabPageLayout; On 2016/05/05 16:17:47, PEConn1 wrote: > Do we ever read this? Done. https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java:81: public void setNewTabPageLayout(NewTabPageLayout newTabPageLayout) { On 2016/05/05 16:28:22, May wrote: > Don't think we need this anymore Done.
Description nit: Use a short title (first paragraph) for the commit message, with more explanation in subsequent paragraphs: http://chris.beams.io/posts/git-commit/ :) https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:530: snapToMostLikely && currentScroll < mNewTabPageLayout.getHeight() * 2 / 3) { On 2016/05/05 17:39:47, mcwilliams wrote: > On 2016/05/05 16:28:22, May wrote: > > unnecessary line break > > If I don't line break it is over 100 chars? I think what May meant was that breaking right after the opening parenthesis is a bit weird. In particular as there are multiple conditions, you should be able to break after |snapToMostLikely|, for example. https://codereview.chromium.org/1947263003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1947263003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:38: private boolean mUseCardsUiEnabled; Nit: I would call this either |mUseCardsUi| or |mCardsUiEnabled|. https://codereview.chromium.org/1947263003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:88: * Javadoc please.
PTAL https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1947263003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:530: snapToMostLikely && currentScroll < mNewTabPageLayout.getHeight() * 2 / 3) { On 2016/05/05 18:04:40, Bernhard Bauer wrote: > On 2016/05/05 17:39:47, mcwilliams wrote: > > On 2016/05/05 16:28:22, May wrote: > > > unnecessary line break > > > > If I don't line break it is over 100 chars? > > I think what May meant was that breaking right after the opening parenthesis is > a bit weird. In particular as there are multiple conditions, you should be able > to break after |snapToMostLikely|, for example. Done. https://codereview.chromium.org/1947263003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1947263003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:38: private boolean mUseCardsUiEnabled; On 2016/05/05 18:04:40, Bernhard Bauer wrote: > Nit: I would call this either |mUseCardsUi| or |mCardsUiEnabled|. Done. https://codereview.chromium.org/1947263003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:88: * On 2016/05/05 18:04:40, Bernhard Bauer wrote: > Javadoc please. :)
LGTM with the description formatted more nicely and a final nit below: https://codereview.chromium.org/1947263003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1947263003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:88: * Is the cards ui enabled. Nit: "Sets whether the cards UI is enabled." :)
https://codereview.chromium.org/1947263003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1947263003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:88: * Is the cards ui enabled. On 2016/05/05 18:23:58, Bernhard Bauer wrote: > Nit: "Sets whether the cards UI is enabled." :) Done.
Description was changed from ========== Pin the snippets to the bottom of the page and introduce a peeking card to indicate to the user there is something at the bottom of the page. When scrolling up transition the peeking card into a full bleed card. BUG=586126, 609487 ========== to ========== Peeking cards Pin the snippets to the bottom of the page and introduce a peeking card to indicate to the user there is something at the bottom of the page. When scrolling up transition the peeking card into a full bleed card. BUG=586126, 609487 ==========
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 Link to the patchset: https://codereview.chromium.org/1947263003/#ps180001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1947263003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1947263003/180001
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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1947263003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1947263003/180001
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 Link to the patchset: https://codereview.chromium.org/1947263003/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1947263003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1947263003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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 Link to the patchset: https://codereview.chromium.org/1947263003/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1947263003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1947263003/220001
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 Link to the patchset: https://codereview.chromium.org/1947263003/#ps150009 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1947263003/150009 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1947263003/150009
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
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/1947263003/150009 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1947263003/150009
Message was sent while issue was closed.
Description was changed from ========== Peeking cards Pin the snippets to the bottom of the page and introduce a peeking card to indicate to the user there is something at the bottom of the page. When scrolling up transition the peeking card into a full bleed card. BUG=586126, 609487 ========== to ========== Peeking cards Pin the snippets to the bottom of the page and introduce a peeking card to indicate to the user there is something at the bottom of the page. When scrolling up transition the peeking card into a full bleed card. BUG=586126, 609487 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:150009)
Message was sent while issue was closed.
Description was changed from ========== Peeking cards Pin the snippets to the bottom of the page and introduce a peeking card to indicate to the user there is something at the bottom of the page. When scrolling up transition the peeking card into a full bleed card. BUG=586126, 609487 ========== to ========== Peeking cards Pin the snippets to the bottom of the page and introduce a peeking card to indicate to the user there is something at the bottom of the page. When scrolling up transition the peeking card into a full bleed card. BUG=586126, 609487 Committed: https://crrev.com/1c4d6b032de178f2a2a6f9beff8d7067c07970b8 Cr-Commit-Position: refs/heads/master@{#391955} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/1c4d6b032de178f2a2a6f9beff8d7067c07970b8 Cr-Commit-Position: refs/heads/master@{#391955} |
