Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1964423002:
Show peeking card when scrolled to Most Likely (Closed)
DescriptionShow peeking card when scrolled to Most Likely.
BUG=609487
Committed: https://crrev.com/1af1e4f3b5c2f04ffa56108eb36244fb39227aef
Cr-Commit-Position: refs/heads/master@{#393523}
Patch Set 1 : Refactor NewTabPageLayout #
Total comments: 21
Patch Set 2 : #Patch Set 3 : #
Total comments: 21
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Patch Set 6 : #
Total comments: 4
Patch Set 7 : #
Messages
Total messages: 26 (6 generated)
peconn@chromium.org changed reviewers: + bauerb@chromium.org, maybelle@chromium.org, mcwilliams@chromium.org
I refactored NewTabPageLayout's onMeasure to make it (hopefully) a bit easier to understand before working on it. I'm going to work on https://crbug.com/609487 on this patch, but in it's current state, could check that my refactoring preserves the logic.
https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:41: private View mScrollCompensationSpacer; Would be nice to add a comment as to what these various spacers are for. https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:74: * Specifies the height of the scroll viewport for the container view of this View. Update the comments as well https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:164: Math.round(extraHeight * (TOP_SPACER_HEIGHT_DP / TOTAL_SPACER_HEIGHT_DP)); Remove extra spaces between * and (TOP_SPACER_HEIGHT_DP https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1160: // Set the parent scroll view port height in the layout. Change the comment as well
https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:104: if (mParentHeight >= getMeasuredHeight() + peekingCardHeight) { You could rewrite this condition as `getMeasuredHeight() <= mParentHeight - peekingCardHeight`, so it will maybe be clearer that we're ensuring a minimum height? https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:111: distributeExtraSpace(mTopSpacer.getMeasuredHeight()); We're only ever calling this with mTopSpacer.getMeasuredHeight(), so we could just get the value in the method? https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:186: * Convenience method to call measure on the given view with MeasureSpecs converted from the Nit: measure() to make it a bit clearer you are talking about the method. Also, |view|?
https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:35: private int mParentHeight; Rename back and add a comment that it is the viewPort Height minus the tab strip as discussed with May https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:95: mScrollCompensationSpacer.setVisibility(View.GONE); naming does not really tell me what this is. https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:129: setMeasure(mSearchBoxView, width, mSearchBoxView.getMeasuredHeight()); Remove extra spacing before width https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:179: setMeasure(mTopSpacer, 0, topSpacerHeight); Remove extra spacing
Description was changed from ========== Refactored NewTabPageLayout. BUG=609487 ========== to ========== Show peeking card when scrolled to Most Likely. BUG=609487 ==========
Please take another look, I think the control flow is a bit simpler now. https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:35: private int mParentHeight; On 2016/05/11 13:20:31, mcwilliams wrote: > Rename back and add a comment that it is the viewPort Height minus the tab strip > as discussed with May Done. https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:41: private View mScrollCompensationSpacer; On 2016/05/11 09:54:59, May wrote: > Would be nice to add a comment as to what these various spacers are for. Done. https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:74: * Specifies the height of the scroll viewport for the container view of this View. On 2016/05/11 09:54:59, May wrote: > Update the comments as well Done. https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:95: mScrollCompensationSpacer.setVisibility(View.GONE); On 2016/05/11 13:20:31, mcwilliams wrote: > naming does not really tell me what this is. Acknowledged. https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:111: distributeExtraSpace(mTopSpacer.getMeasuredHeight()); On 2016/05/11 11:46:49, Bernhard Bauer wrote: > We're only ever calling this with mTopSpacer.getMeasuredHeight(), so we could > just get the value in the method? Acknowledged. https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:129: setMeasure(mSearchBoxView, width, mSearchBoxView.getMeasuredHeight()); On 2016/05/11 13:20:31, mcwilliams wrote: > Remove extra spacing before width Done. https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:164: Math.round(extraHeight * (TOP_SPACER_HEIGHT_DP / TOTAL_SPACER_HEIGHT_DP)); On 2016/05/11 09:54:59, May wrote: > Remove extra spaces between * and (TOP_SPACER_HEIGHT_DP Done. https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:179: setMeasure(mTopSpacer, 0, topSpacerHeight); On 2016/05/11 13:20:31, mcwilliams wrote: > Remove extra spacing Done. https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:186: * Convenience method to call measure on the given view with MeasureSpecs converted from the On 2016/05/11 11:46:49, Bernhard Bauer wrote: > Nit: measure() to make it a bit clearer you are talking about the method. Also, > |view|? Done. https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/1964423002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1160: // Set the parent scroll view port height in the layout. On 2016/05/11 09:54:59, May wrote: > Change the comment as well Acknowledged.
https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:36: private int mParentViewportHeight; Add a comment that this includes tab height for tablet https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:39: private View mTopSpacer; // Spacer above search logo. Remove extra spacings https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:43: // at the top of the screen. Big confused here as above you have said mBottomSpacer is spacer below most likely and you are saying the same thing here but they are different? https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:149: View child = getChildAt(i); What is child in this case? https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:168: Can you please add some comments to below code
https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:43: // at the top of the screen. On 2016/05/11 16:49:45, mcwilliams wrote: > Big confused here as above you have said mBottomSpacer is spacer below most > likely and you are saying the same thing here but they are different? Could we merge these spacers into one? (Would that work with the no_search_logo_spacer, which is currently in between them?) https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:177: topSpacerHeight = mTopSpacerHeight + (extraHeight - mTotalSpacerHeight) / 2; Why not use `+=`?
https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:177: topSpacerHeight = mTopSpacerHeight + (extraHeight - mTotalSpacerHeight) / 2; On 2016/05/11 18:06:28, Bernhard Bauer wrote: > Why not use `+=`? NVM, I'm blind.
https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:36: private int mParentViewportHeight; On 2016/05/11 16:49:45, mcwilliams wrote: > Add a comment that this includes tab height for tablet Does that comment really belong here? The NewTabPageLayout shouldn't care about what the mParentViewportHeight is, it should just try to fill it. https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:39: private View mTopSpacer; // Spacer above search logo. On 2016/05/11 16:49:45, mcwilliams wrote: > Remove extra spacings Done. https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:43: // at the top of the screen. On 2016/05/11 18:06:28, Bernhard Bauer wrote: > On 2016/05/11 16:49:45, mcwilliams wrote: > > Big confused here as above you have said mBottomSpacer is spacer below most > > likely and you are saying the same thing here but they are different? > > Could we merge these spacers into one? (Would that work with the > no_search_logo_spacer, which is currently in between them?) I'd say we should keep them separate: - Having 1 additional View isn't really much overhead. - They have different purposes, so keeping them separate simplifies the code. - The code seems a bit more robust if we don't reuse elements (we don't need to remember to reset everything). https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:43: // at the top of the screen. On 2016/05/11 16:49:45, mcwilliams wrote: > Big confused here as above you have said mBottomSpacer is spacer below most > likely and you are saying the same thing here but they are different? They're in the same place, but they have different purposes. https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:149: View child = getChildAt(i); On 2016/05/11 16:49:45, mcwilliams wrote: > What is child in this case? A child of the layout (eg, the top spacer, the logo, the search box). https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:168: On 2016/05/11 16:49:45, mcwilliams wrote: > Can you please add some comments to below code Done. https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:177: topSpacerHeight = mTopSpacerHeight + (extraHeight - mTotalSpacerHeight) / 2; On 2016/05/11 18:06:28, Bernhard Bauer wrote: > Why not use `+=`? Acknowledged. https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:177: topSpacerHeight = mTopSpacerHeight + (extraHeight - mTotalSpacerHeight) / 2; On 2016/05/12 09:20:51, Bernhard Bauer wrote: > On 2016/05/11 18:06:28, Bernhard Bauer wrote: > > Why not use `+=`? > > NVM, I'm blind. Acknowledged.
https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:43: // at the top of the screen. On 2016/05/12 09:39:07, PEConn1 wrote: > On 2016/05/11 18:06:28, Bernhard Bauer wrote: > > On 2016/05/11 16:49:45, mcwilliams wrote: > > > Big confused here as above you have said mBottomSpacer is spacer below most > > > likely and you are saying the same thing here but they are different? > > > > Could we merge these spacers into one? (Would that work with the > > no_search_logo_spacer, which is currently in between them?) > > I'd say we should keep them separate: > - Having 1 additional View isn't really much overhead. > - They have different purposes, so keeping them separate simplifies the code. > - The code seems a bit more robust if we don't reuse elements (we don't need to > remember to reset everything). I guess my issue with this is the name needs to then change. It is also a bottom spacer but the use for this is different and only used at some times. When I read mScrollCompensationSpacer having not read the code does not tell me what it is for. I agree alternatively merge into 1 https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:149: View child = getChildAt(i); On 2016/05/12 09:39:07, PEConn1 wrote: > On 2016/05/11 16:49:45, mcwilliams wrote: > > What is child in this case? > > A child of the layout (eg, the top spacer, the logo, the search box). Yes I get that it is a child, but what child are you expecting - most likely, snippet header, etc https://codereview.chromium.org/1964423002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:171: // The spacers will be less than their ideal height, shrink them proportionally. Why will they be less than their ideal height?
https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:149: View child = getChildAt(i); On 2016/05/12 14:27:08, mcwilliams wrote: > On 2016/05/12 09:39:07, PEConn1 wrote: > > On 2016/05/11 16:49:45, mcwilliams wrote: > > > What is child in this case? > > > > A child of the layout (eg, the top spacer, the logo, the search box). > > Yes I get that it is a child, but what child are you expecting - most likely, > snippet header, etc All children above mostVisited.
https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:149: View child = getChildAt(i); On 2016/05/12 14:34:55, PEConn1 wrote: > On 2016/05/12 14:27:08, mcwilliams wrote: > > On 2016/05/12 09:39:07, PEConn1 wrote: > > > On 2016/05/11 16:49:45, mcwilliams wrote: > > > > What is child in this case? > > > > > > A child of the layout (eg, the top spacer, the logo, the search box). > > > > Yes I get that it is a child, but what child are you expecting - most likely, > > snippet header, etc > > All children above mostVisited. (The point of it being that we don't actually care which child it is -- we just aggregate all the children until we reach Most Visited.)
lgtm https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:149: View child = getChildAt(i); On 2016/05/12 14:39:28, Bernhard Bauer wrote: > On 2016/05/12 14:34:55, PEConn1 wrote: > > On 2016/05/12 14:27:08, mcwilliams wrote: > > > On 2016/05/12 09:39:07, PEConn1 wrote: > > > > On 2016/05/11 16:49:45, mcwilliams wrote: > > > > > What is child in this case? > > > > > > > > A child of the layout (eg, the top spacer, the logo, the search box). > > > > > > Yes I get that it is a child, but what child are you expecting - most > likely, > > > snippet header, etc > > > > All children above mostVisited. > > (The point of it being that we don't actually care which child it is -- we just > aggregate all the children until we reach Most Visited.) My point was that we should not call it child - child of what - I am referring to the naming of 'child'. I feel it should be mostLikelyChild for example as when I read the code I know exactly what I am looking at then.
https://codereview.chromium.org/1964423002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:171: // The spacers will be less than their ideal height, shrink them proportionally. On 2016/05/12 14:27:09, mcwilliams wrote: > Why will they be less than their ideal height? Because the space that we have to fill (extraHeight) is less than the space required for the spacers to be their ideal height (given in the static final floats at the top of the class).
https://codereview.chromium.org/1964423002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:40: private View mTopSpacer; // Spacer above search logo. Nit: two spaces before the comment. https://codereview.chromium.org/1964423002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:43: // Separate spacer below Most Likely to add enough space so the user can scroll with Most Likely Add an empty line before the comment?
https://codereview.chromium.org/1964423002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java (right): https://codereview.chromium.org/1964423002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:40: private View mTopSpacer; // Spacer above search logo. On 2016/05/13 09:05:22, Bernhard Bauer wrote: > Nit: two spaces before the comment. Done. https://codereview.chromium.org/1964423002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageLayout.java:43: // Separate spacer below Most Likely to add enough space so the user can scroll with Most Likely On 2016/05/13 09:05:22, Bernhard Bauer wrote: > Add an empty line before the comment? Done.
lgtm
The CQ bit was checked by peconn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcwilliams@chromium.org Link to the patchset: https://codereview.chromium.org/1964423002/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1964423002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1964423002/120001
Message was sent while issue was closed.
Description was changed from ========== Show peeking card when scrolled to Most Likely. BUG=609487 ========== to ========== Show peeking card when scrolled to Most Likely. BUG=609487 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Show peeking card when scrolled to Most Likely. BUG=609487 ========== to ========== Show peeking card when scrolled to Most Likely. BUG=609487 Committed: https://crrev.com/1af1e4f3b5c2f04ffa56108eb36244fb39227aef Cr-Commit-Position: refs/heads/master@{#393523} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1af1e4f3b5c2f04ffa56108eb36244fb39227aef Cr-Commit-Position: refs/heads/master@{#393523} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
