Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1965363002:
Set the parent viewport on the NewTabPageScroll (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. |
DescriptionSet the parent viewport on the NewTabPageScroll
Introduced a bug on new tab page scroll where on measure is slightly different when recycler view is enabled. Reverting this change and cleaning it up slightly
BUG=610262
Committed: https://crrev.com/436aed1d712016d6653e913f634ced168eddd8d5
Cr-Commit-Position: refs/heads/master@{#392953}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #Messages
Total messages: 22 (9 generated)
mcwilliams@chromium.org changed reviewers: + bauerb@chromium.org, maybelle@chromium.org
The CQ bit was checked by mcwilliams@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/1965363002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1965363002/1
https://codereview.chromium.org/1965363002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageScrollView.java (right): https://codereview.chromium.org/1965363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageScrollView.java:84: if (child instanceof NewTabPageLayout) mNewTabPageLayout = (NewTabPageLayout) child; We don't need this check -- the first child should always be a NewTabPageLayout (and if it isn't, we'll get a crash so we will learn about it via crash reports). https://codereview.chromium.org/1965363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageScrollView.java:89: if (mNewTabPageLayout != null) { This check should also be unnecessary.
PTAL https://codereview.chromium.org/1965363002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageScrollView.java (right): https://codereview.chromium.org/1965363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageScrollView.java:84: if (child instanceof NewTabPageLayout) mNewTabPageLayout = (NewTabPageLayout) child; On 2016/05/11 13:53:20, Bernhard Bauer wrote: > We don't need this check -- the first child should always be a NewTabPageLayout > (and if it isn't, we'll get a crash so we will learn about it via crash > reports). Done. https://codereview.chromium.org/1965363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageScrollView.java:89: if (mNewTabPageLayout != null) { On 2016/05/11 13:53:20, Bernhard Bauer wrote: > This check should also be unnecessary. Done.
The CQ bit was checked by mcwilliams@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/1965363002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1965363002/20001
Also, please pay attention to the CL description and try to follow e.g. http://chris.beams.io/posts/git-commit/. In this particular case, the CL title should be in the imperative mood ("NewTabPageScroll calculation on measure" doesn't really say what this CL changes). Also, the context on the body is already on the linked bug. It might seem nitpicky, but commit messages will show up in a lot of places, and following standard conventions will make it a lot easier to glance over a large number of commits.
Description was changed from ========== NewTabPageScroll calculation on measure Introduced a bug on new tab page scroll where on measure is slightly different when recycler view is enabled BUG=610262 ========== to ========== Set the parent viewport on the NewTabPageScroll Introduced a bug on new tab page scroll where on measure is slightly different when recycler view is enabled. Reverting this change and cleaning it up slightly BUG=610262 ==========
PTAL
LGTM, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
https://codereview.chromium.org/1965363002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageScrollView.java (right): https://codereview.chromium.org/1965363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageScrollView.java:84: if (child instanceof NewTabPageLayout) mNewTabPageLayout = (NewTabPageLayout) child; On 2016/05/11 14:00:04, mcwilliams wrote: > On 2016/05/11 13:53:20, Bernhard Bauer wrote: > > We don't need this check -- the first child should always be a > NewTabPageLayout > > (and if it isn't, we'll get a crash so we will learn about it via crash > > reports). > > Done. Changed this to find by id as safer - incognito child(0) is not always the layout and they share the view but they do not share the same xml so the id will return null as expected. Added a comment for this in the code https://codereview.chromium.org/1965363002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageScrollView.java:89: if (mNewTabPageLayout != null) { On 2016/05/11 14:00:04, mcwilliams wrote: > On 2016/05/11 13:53:20, Bernhard Bauer wrote: > > This check should also be unnecessary. > > Done. Put this back as the NewTabPageLayout can be null
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/1965363002/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1965363002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1965363002/40001
Message was sent while issue was closed.
Description was changed from ========== Set the parent viewport on the NewTabPageScroll Introduced a bug on new tab page scroll where on measure is slightly different when recycler view is enabled. Reverting this change and cleaning it up slightly BUG=610262 ========== to ========== Set the parent viewport on the NewTabPageScroll Introduced a bug on new tab page scroll where on measure is slightly different when recycler view is enabled. Reverting this change and cleaning it up slightly BUG=610262 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Set the parent viewport on the NewTabPageScroll Introduced a bug on new tab page scroll where on measure is slightly different when recycler view is enabled. Reverting this change and cleaning it up slightly BUG=610262 ========== to ========== Set the parent viewport on the NewTabPageScroll Introduced a bug on new tab page scroll where on measure is slightly different when recycler view is enabled. Reverting this change and cleaning it up slightly BUG=610262 Committed: https://crrev.com/436aed1d712016d6653e913f634ced168eddd8d5 Cr-Commit-Position: refs/heads/master@{#392953} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/436aed1d712016d6653e913f634ced168eddd8d5 Cr-Commit-Position: refs/heads/master@{#392953} |
