Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2149333003:
📰 Adjust the card display depending on the screen width. (Closed)
Description[NTP Snippets] Adjust the card display depending on the screen width.
Changes the lines to go from always 2 to at most 2 by default, so that
we don't show empty lines on very large screens.
For smaller screens, the title can go up to 4 lines, and we then hide
the description.
On bigger screens, we add space on the side of the cards
Measures used:
< 360dp: Narrow -> 4 lines title
>= 360dp: Regular -> 2 + 2 lines
>= 600dp: Wide -> 2 + 2 lines, 48dp gutters around the cards
Preview: https://goo.gl/photos/prJ42tvP4jzwiCn3A
BUG=625628, 624333
Committed: https://crrev.com/7c430bb62b6269b9f1bce082083e00324a26daee
Cr-Commit-Position: refs/heads/master@{#406923}
Patch Set 1 #
Total comments: 2
Patch Set 2 : update parent branch #
Total comments: 2
Patch Set 3 : add doc, fix nits #
Total comments: 28
Patch Set 4 : address comments #
Total comments: 8
Patch Set 5 : address comments #
Total comments: 6
Patch Set 6 : address comments #Patch Set 7 : rebase #
Total comments: 10
Patch Set 8 : address comments #Messages
Total messages: 55 (36 generated)
dgn@chromium.org changed reviewers: + bauerb@chromium.org, mvanouwerkerk@chromium.org
PTAL. [Not much doc currently, sorry.]
The CQ bit was checked by mvanouwerkerk@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...
https://codereview.chromium.org/2149333003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java (right): https://codereview.chromium.org/2149333003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java:11: public abstract class AbstractDisplayStyleChangeListener nit: just DisplayStyleChangeListener, it's cleaner.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dgn@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...
Current bug I'm looking in: When hiding the app away by closing its pane in MW mode, it does not refresh the display style when brought back to foreground. https://codereview.chromium.org/2149333003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java (right): https://codereview.chromium.org/2149333003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java:11: public abstract class AbstractDisplayStyleChangeListener On 2016/07/15 16:13:39, Michael van Ouwerkerk wrote: > nit: just DisplayStyleChangeListener, it's cleaner. DisplayStyleChangeListener is an inner class in UiConfig. Should I still do it? https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1101: protected void onConfigurationChanged(Configuration newConfig) { Not too happy about requiring this here since the UiConfig is created and owned somewhere else. Maybe I should just leave UiConfig entirely in here and pass it directly instead of through NTPManager? Thoughts?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
peconn@chromium.org changed reviewers: + peconn@chromium.org
https://codereview.chromium.org/2149333003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java (right): https://codereview.chromium.org/2149333003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:33: public static final int NARROW_CARD_MAX_WIDTH_DP = 359; Why public? https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1101: protected void onConfigurationChanged(Configuration newConfig) { On 2016/07/15 17:40:05, dgn wrote: > Not too happy about requiring this here since the UiConfig is created and owned > somewhere else. Maybe I should just leave UiConfig entirely in here and pass it > directly instead of through NTPManager? Thoughts? I thought the only reason for putting he UiConfig in the NTPManager was so that the SnippetArticleViewHolder could get access to it without changing it's constructor (since it already takes a NTPManager). I'd say if that choice is making the code ugly elsewhere, then just go ahead and put the UiConfig in the NewTabPageView.
Description was changed from ========== [NTP Snippets] Adjust the card display depending on the screen width. Changes the lines to go from always 2 to at most 2 by default, so that we don't show empty lines on very large screens. For smaller screens, the title can go up to 4 lines, and we then hide the description. On bigger screens, we add space on the side of the cards Measures used: < 360dp: Narrow -> 4 lines title >= 360dp: Regular -> 2 + 2 lines >= 600dp: Wide -> 2 + 2 lines, 48dp gutters around the cards Preview: https://goo.gl/photos/WbVcdjyAQG4kHY8J6 BUG=625628, 624333 ========== to ========== [NTP Snippets] Adjust the card display depending on the screen width. Changes the lines to go from always 2 to at most 2 by default, so that we don't show empty lines on very large screens. For smaller screens, the title can go up to 4 lines, and we then hide the description. On bigger screens, we add space on the side of the cards Measures used: < 360dp: Narrow -> 4 lines title >= 360dp: Regular -> 2 + 2 lines >= 600dp: Wide -> 2 + 2 lines, 48dp gutters around the cards Preview: https://goo.gl/photos/WbVcdjyAQG4kHY8J6 BUG=625628, 624333 ==========
Description was changed from ========== [NTP Snippets] Adjust the card display depending on the screen width. Changes the lines to go from always 2 to at most 2 by default, so that we don't show empty lines on very large screens. For smaller screens, the title can go up to 4 lines, and we then hide the description. On bigger screens, we add space on the side of the cards Measures used: < 360dp: Narrow -> 4 lines title >= 360dp: Regular -> 2 + 2 lines >= 600dp: Wide -> 2 + 2 lines, 48dp gutters around the cards Preview: https://goo.gl/photos/WbVcdjyAQG4kHY8J6 BUG=625628, 624333 ========== to ========== [NTP Snippets] Adjust the card display depending on the screen width. Changes the lines to go from always 2 to at most 2 by default, so that we don't show empty lines on very large screens. For smaller screens, the title can go up to 4 lines, and we then hide the description. On bigger screens, we add space on the side of the cards Measures used: < 360dp: Narrow -> 4 lines title >= 360dp: Regular -> 2 + 2 lines >= 600dp: Wide -> 2 + 2 lines, 48dp gutters around the cards Preview: https://goo.gl/photos/prJ42tvP4jzwiCn3A BUG=625628, 624333 ==========
Updated the preview video https://codereview.chromium.org/2149333003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java (right): https://codereview.chromium.org/2149333003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:33: public static final int NARROW_CARD_MAX_WIDTH_DP = 359; On 2016/07/18 07:24:54, PEConn1 wrote: > Why public? Fixed in PS3
https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java (right): https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java:13: * associated view is not attached to the window Nit: end the sentence with a period. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java:19: * Makes the associated view to have lateral margins added when the display style is Nit: "Adds lateral margins to the view [...]". https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java:22: public static class MarginResizer extends AbstractDisplayStyleChangeListener{ Should we make this a top-level class? https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java:51: // TODO(dgn) getParent() is not a good way to test that, but isAttachedToWindow() Nit: colon after TODO(): and period at the end. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java:67: public abstract void applyNewDisplayStyle(@UiConfig.DisplayStyle int newDisplayStyle); Hm... I'm thinking this class should be an adapter that takes an existing DisplayStyleObserver instead of using inheritance. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java (right): https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:30: public interface DisplayStyleChangeListener { DisplayStyleObserver would be a bit more idiomatic, I think. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:44: private static final int NARROW_CARD_MAX_WIDTH_DP = 359; Making the value 360 would be a bit less magic, you'd just have to rename the constant to something like REGULAR_CARD_MIN_WIDTH_DP. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:50: private Context mContext; Make this final? https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:53: private List<DisplayStyleChangeListener> mListeners; Make this final and initialize here? https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:85: Nit: remove this empty line. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:95: mMarginResizer = new AbstractDisplayStyleChangeListener.MarginResizer(itemView, config); This class never uses this field again. Do we need to call destroy() on it at some point? https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:167: int lateralPadding = mMaxPeekPadding; I would not initialize these variables here, but instead use both branches of the if-statement below to assign values. That way the compiler will check that both branches assign a value.
The CQ bit was checked by dgn@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...
PTAL https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java (right): https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java:13: * associated view is not attached to the window On 2016/07/18 14:26:23, Bernhard Bauer wrote: > Nit: end the sentence with a period. Done. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java:19: * Makes the associated view to have lateral margins added when the display style is On 2016/07/18 14:26:23, Bernhard Bauer wrote: > Nit: "Adds lateral margins to the view [...]". Done. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java:22: public static class MarginResizer extends AbstractDisplayStyleChangeListener{ On 2016/07/18 14:26:23, Bernhard Bauer wrote: > Should we make this a top-level class? Done. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java:51: // TODO(dgn) getParent() is not a good way to test that, but isAttachedToWindow() On 2016/07/18 14:26:24, Bernhard Bauer wrote: > Nit: colon after TODO(): and period at the end. Done. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/AbstractDisplayStyleChangeListener.java:67: public abstract void applyNewDisplayStyle(@UiConfig.DisplayStyle int newDisplayStyle); On 2016/07/18 14:26:23, Bernhard Bauer wrote: > Hm... I'm thinking this class should be an adapter that takes an existing > DisplayStyleObserver instead of using inheritance. Done. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1101: protected void onConfigurationChanged(Configuration newConfig) { On 2016/07/18 07:24:54, PEConn1 wrote: > On 2016/07/15 17:40:05, dgn wrote: > > Not too happy about requiring this here since the UiConfig is created and > owned > > somewhere else. Maybe I should just leave UiConfig entirely in here and pass > it > > directly instead of through NTPManager? Thoughts? > > I thought the only reason for putting he UiConfig in the NTPManager was so that > the SnippetArticleViewHolder could get access to it without changing it's > constructor (since it already takes a NTPManager). I'd say if that choice is > making the code ugly elsewhere, then just go ahead and put the UiConfig in the > NewTabPageView. Fixed, I used OnLayoutChangedListener. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java (right): https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:30: public interface DisplayStyleChangeListener { On 2016/07/18 14:26:24, Bernhard Bauer wrote: > DisplayStyleObserver would be a bit more idiomatic, I think. Done. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:44: private static final int NARROW_CARD_MAX_WIDTH_DP = 359; On 2016/07/18 14:26:24, Bernhard Bauer wrote: > Making the value 360 would be a bit less magic, you'd just have to rename the > constant to something like REGULAR_CARD_MIN_WIDTH_DP. Done. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:50: private Context mContext; On 2016/07/18 14:26:24, Bernhard Bauer wrote: > Make this final? Done. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:53: private List<DisplayStyleChangeListener> mListeners; On 2016/07/18 14:26:24, Bernhard Bauer wrote: > Make this final and initialize here? Done. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:85: On 2016/07/18 14:26:24, Bernhard Bauer wrote: > Nit: remove this empty line. Done. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:95: mMarginResizer = new AbstractDisplayStyleChangeListener.MarginResizer(itemView, config); On 2016/07/18 14:26:24, Bernhard Bauer wrote: > This class never uses this field again. Do we need to call destroy() on it at > some point? No we don't really need it. removed the field. https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:167: int lateralPadding = mMaxPeekPadding; On 2016/07/18 14:26:24, Bernhard Bauer wrote: > I would not initialize these variables here, but instead use both branches of > the if-statement below to assign values. That way the compiler will check that > both branches assign a value. Done.
https://codereview.chromium.org/2149333003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/DisplayStyleObserver.java (right): https://codereview.chromium.org/2149333003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/DisplayStyleObserver.java:22: public static class ViewAdapter Pull this out into a separate class? https://codereview.chromium.org/2149333003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/DisplayStyleObserver.java:48: mConfig.addListener(this); You could just have the caller attached the returned object, then you don't to pass it in here. Actually, that would mean you wouldn't have a constructor call with the return value ignored anymore, so you could remove the static method as well. https://codereview.chromium.org/2149333003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java (right): https://codereview.chromium.org/2149333003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:84: public void addListener(DisplayStyleObserver listener) { Nit: It's an observer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2149333003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1101: protected void onConfigurationChanged(Configuration newConfig) { On 2016/07/19 14:31:47, dgn wrote: > On 2016/07/18 07:24:54, PEConn1 wrote: > > On 2016/07/15 17:40:05, dgn wrote: > > > Not too happy about requiring this here since the UiConfig is created and > > owned > > > somewhere else. Maybe I should just leave UiConfig entirely in here and pass > > it > > > directly instead of through NTPManager? Thoughts? > > > > I thought the only reason for putting he UiConfig in the NTPManager was so > that > > the SnippetArticleViewHolder could get access to it without changing it's > > constructor (since it already takes a NTPManager). I'd say if that choice is > > making the code ugly elsewhere, then just go ahead and put the UiConfig in the > > NewTabPageView. > > Fixed, I used OnLayoutChangedListener. Actually, OnLayoutChangedListener does not work. Moved UiConfig to NTPView. https://codereview.chromium.org/2149333003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/DisplayStyleObserver.java (right): https://codereview.chromium.org/2149333003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/DisplayStyleObserver.java:22: public static class ViewAdapter On 2016/07/19 14:46:52, Bernhard Bauer wrote: > Pull this out into a separate class? Done. https://codereview.chromium.org/2149333003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/DisplayStyleObserver.java:48: mConfig.addListener(this); On 2016/07/19 14:46:52, Bernhard Bauer wrote: > You could just have the caller attached the returned object, then you don't to > pass it in here. Actually, that would mean you wouldn't have a constructor call > with the return value ignored anymore, so you could remove the static method as > well. I use it in refreshDisplayStyle() to pull the current display style. https://codereview.chromium.org/2149333003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java (right): https://codereview.chromium.org/2149333003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:84: public void addListener(DisplayStyleObserver listener) { On 2016/07/19 14:46:53, Bernhard Bauer wrote: > Nit: It's an observer. Done. https://codereview.chromium.org/2149333003/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/new_tab_page_snippets_header.xml (right): https://codereview.chromium.org/2149333003/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/new_tab_page_snippets_header.xml:14: android:paddingStart="@dimen/snippets_padding_and_peeking_card_height" Changed because I now modify margins and it was glitching when they are also set here. If we need other margins on cards at some point it could be an issue but so far the current implementation works perfectly. https://codereview.chromium.org/2149333003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2149333003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:72: mWideMarginSizePixels = itemView.getResources().getDimensionPixelSize( I could also pull the value from MarginResizer instead, not sure which is better.
The CQ bit was checked by dgn@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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
https://codereview.chromium.org/2149333003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/DisplayStyleObserver.java (right): https://codereview.chromium.org/2149333003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/DisplayStyleObserver.java:48: mConfig.addListener(this); On 2016/07/19 23:33:56, dgn wrote: > On 2016/07/19 14:46:52, Bernhard Bauer wrote: > > You could just have the caller attached the returned object, then you don't to > > pass it in here. Actually, that would mean you wouldn't have a constructor > call > > with the return value ignored anymore, so you could remove the static method > as > > well. > > I use it in refreshDisplayStyle() to pull the current display style. Hm... you could have UiConfig.addObserver() immediately notify the new observer, and cache the last style in this class. Then you could immediately notify your observer when we attach back to a window. This would effectively make everything push-based. It would also have the side effect of avoiding notifications if all we did was detach and reattach without the display style actually changing. https://codereview.chromium.org/2149333003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java (right): https://codereview.chromium.org/2149333003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:40: private int mCurrentDisplayStyle = DISPLAY_STYLE_UNDEFINED; Hm... having UNDEFINED as a valid style is a bit inelegant, in particular as it's only used right at the beginning. If you extract a method that returns the current display style, you could initialize it right away. https://codereview.chromium.org/2149333003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/MarginResizer.java (right): https://codereview.chromium.org/2149333003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/MarginResizer.java:27: org.chromium.chrome.R.dimen.ntp_wide_card_lateral_margins); You can't import R?
The CQ bit was checked by dgn@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...
PTAL https://codereview.chromium.org/2149333003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/DisplayStyleObserver.java (right): https://codereview.chromium.org/2149333003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/DisplayStyleObserver.java:48: mConfig.addListener(this); On 2016/07/20 08:56:24, Bernhard Bauer wrote: > On 2016/07/19 23:33:56, dgn wrote: > > On 2016/07/19 14:46:52, Bernhard Bauer wrote: > > > You could just have the caller attached the returned object, then you don't > to > > > pass it in here. Actually, that would mean you wouldn't have a constructor > > call > > > with the return value ignored anymore, so you could remove the static method > > as > > > well. > > > > I use it in refreshDisplayStyle() to pull the current display style. > > Hm... you could have UiConfig.addObserver() immediately notify the new observer, > and cache the last style in this class. Then you could immediately notify your > observer when we attach back to a window. This would effectively make everything > push-based. It would also have the side effect of avoiding notifications if all > we did was detach and reattach without the display style actually changing. Done. https://codereview.chromium.org/2149333003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java (right): https://codereview.chromium.org/2149333003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:40: private int mCurrentDisplayStyle = DISPLAY_STYLE_UNDEFINED; On 2016/07/20 08:56:24, Bernhard Bauer wrote: > Hm... having UNDEFINED as a valid style is a bit inelegant, in particular as > it's only used right at the beginning. If you extract a method that returns the > current display style, you could initialize it right away. Done. https://codereview.chromium.org/2149333003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/MarginResizer.java (right): https://codereview.chromium.org/2149333003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/MarginResizer.java:27: org.chromium.chrome.R.dimen.ntp_wide_card_lateral_margins); On 2016/07/20 08:56:24, Bernhard Bauer wrote: > You can't import R? Ah I didn't notice it. Still need to get used to strange Android Studio auto-completions. Fixed.
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-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by dgn@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
lgtm with nits, looks good! https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:99: private UiConfig mUiConfig; nit: move this below mManager as it is assigned after mManager in initialize() https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java (right): https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:37: private static final boolean DEBUG = false; I'm not sure I understand your intent for this one... do you mean to commit the DEBUG code? https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:82: for (DisplayStyleObserver listener : mObservers) { nit:s/listener/observer/ https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:97: mConfig = config; nit: I'd prefer mUiConfig for the member and uiConfig for the param https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/DisplayStyleObserverAdapter.java (right): https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/DisplayStyleObserverAdapter.java:42: mListener = listener; nit: it's an observer now
The CQ bit was checked by dgn@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...
PTAL https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:99: private UiConfig mUiConfig; On 2016/07/20 17:26:57, Michael van Ouwerkerk wrote: > nit: move this below mManager as it is assigned after mManager in initialize() Done. https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java (right): https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:37: private static final boolean DEBUG = false; On 2016/07/20 17:26:57, Michael van Ouwerkerk wrote: > I'm not sure I understand your intent for this one... do you mean to commit the > DEBUG code? Yes, I suppose it can be useful, esp when sending APKs to be checked by UX, they can try it on various devices and have dp values displayed with toasts. https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/UiConfig.java:82: for (DisplayStyleObserver listener : mObservers) { On 2016/07/20 17:26:57, Michael van Ouwerkerk wrote: > nit:s/listener/observer/ Done. https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java (right): https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java:97: mConfig = config; On 2016/07/20 17:26:57, Michael van Ouwerkerk wrote: > nit: I'd prefer mUiConfig for the member and uiConfig for the param Done. https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/DisplayStyleObserverAdapter.java (right): https://codereview.chromium.org/2149333003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/DisplayStyleObserverAdapter.java:42: mListener = listener; On 2016/07/20 17:26:57, Michael van Ouwerkerk wrote: > nit: it's an observer now Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM!
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mvanouwerkerk@chromium.org Link to the patchset: https://codereview.chromium.org/2149333003/#ps140001 (title: "address comments")
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 ========== [NTP Snippets] Adjust the card display depending on the screen width. Changes the lines to go from always 2 to at most 2 by default, so that we don't show empty lines on very large screens. For smaller screens, the title can go up to 4 lines, and we then hide the description. On bigger screens, we add space on the side of the cards Measures used: < 360dp: Narrow -> 4 lines title >= 360dp: Regular -> 2 + 2 lines >= 600dp: Wide -> 2 + 2 lines, 48dp gutters around the cards Preview: https://goo.gl/photos/prJ42tvP4jzwiCn3A BUG=625628, 624333 ========== to ========== [NTP Snippets] Adjust the card display depending on the screen width. Changes the lines to go from always 2 to at most 2 by default, so that we don't show empty lines on very large screens. For smaller screens, the title can go up to 4 lines, and we then hide the description. On bigger screens, we add space on the side of the cards Measures used: < 360dp: Narrow -> 4 lines title >= 360dp: Regular -> 2 + 2 lines >= 600dp: Wide -> 2 + 2 lines, 48dp gutters around the cards Preview: https://goo.gl/photos/prJ42tvP4jzwiCn3A BUG=625628, 624333 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [NTP Snippets] Adjust the card display depending on the screen width. Changes the lines to go from always 2 to at most 2 by default, so that we don't show empty lines on very large screens. For smaller screens, the title can go up to 4 lines, and we then hide the description. On bigger screens, we add space on the side of the cards Measures used: < 360dp: Narrow -> 4 lines title >= 360dp: Regular -> 2 + 2 lines >= 600dp: Wide -> 2 + 2 lines, 48dp gutters around the cards Preview: https://goo.gl/photos/prJ42tvP4jzwiCn3A BUG=625628, 624333 ========== to ========== [NTP Snippets] Adjust the card display depending on the screen width. Changes the lines to go from always 2 to at most 2 by default, so that we don't show empty lines on very large screens. For smaller screens, the title can go up to 4 lines, and we then hide the description. On bigger screens, we add space on the side of the cards Measures used: < 360dp: Narrow -> 4 lines title >= 360dp: Regular -> 2 + 2 lines >= 600dp: Wide -> 2 + 2 lines, 48dp gutters around the cards Preview: https://goo.gl/photos/prJ42tvP4jzwiCn3A BUG=625628, 624333 Committed: https://crrev.com/7c430bb62b6269b9f1bce082083e00324a26daee Cr-Commit-Position: refs/heads/master@{#406923} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7c430bb62b6269b9f1bce082083e00324a26daee Cr-Commit-Position: refs/heads/master@{#406923}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2167973004/ by dbeam@chromium.org. The reason for reverting is: Broke Android compile: util.build_utils.CalledProcessError: Command failed: ( cd /mnt/data/b/c/b/Android_Arm64_Builder__dbg_/src/out/Debug; javac -g -encoding UTF-8 -classpath lib.java/chrome/android/chrome_java.interface.jar:lib.java/base/base_java.interface.jar:lib.java/base/base_java_test_support.interface.jar:lib.java/base/base_junit_test_support.interface.jar:lib.java/components/bookmarks/common/android/bookmarks_java.interface.jar:lib.java/components/invalidation/impl/java.interface.jar:lib.java/components/web_restrictions/web_restrictions_java.interface.jar:lib.java/content/public/android/content_java.interface.jar:lib.java/net/android/net_java.interface.jar:lib.java/sync/sync_java_test_support.interface.jar:lib.java/sync/android/sync_java.interface.jar:lib.java/third_party/WebKit/public/blink_headers_java.interface.jar:lib.java/third_party/android_tools/android_support_v7_mediarouter_java__jar_1.interface.jar:lib.java/third_party/android_tools/android_support_v7_mediarouter_java__jar_2.interface.jar:lib.java/third_party/android_tools/android_support_v7_recyclerview_java__jar_1.interface.jar:lib.java/third_party/cacheinvalidation/cacheinvalidation_javalib.interface.jar:lib.java/third_party/junit/hamcrest.interface.jar:lib.java/ui/android/ui_java.interface.jar:lib.java/third_party/android_tools/google_play_services_default_java.interface.jar:lib.java/testing/android/junit/junit_test_support.interface.jar:lib.java/third_party/junit/junit.interface.jar:lib.java/third_party/mockito/mockito_java.interface.jar:lib.java/third_party/robolectric/android-all-4.3_r2-robolectric-0.interface.jar:lib.java/third_party/robolectric/robolectric_java.interface.jar -sourcepath '' -Xlint:unchecked -Xlint:deprecation -d /tmp/tmphznxR6/classes ../../chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiterTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/ShortcutHelperTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/SSLClientCertificateRequestTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionControllerTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/cookies/CanonicalCookieTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/crash/LogcatExtractionCallableTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/externalauth/ExternalAuthUtilsTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/firstrun/FirstRunFlowSequencerTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/gcore/GoogleApiClientHelperTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/invalidation/InvalidationControllerTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/media/remote/MediaUrlResolverTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/media/remote/RemoteVideoInfoTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java ../../chrome/android/junit/src/org/chromium/chrome/browser/media/router/cast/CastMessageHandlerTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallbackTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/media/router/cast/JSONTestUtils.java ../../chrome/android/junit/src/org/chromium/chrome/browser/media/router/cast/MediaSourceTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/media/router/cast/TestUtils.java ../../chrome/android/junit/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/ntp/NativePageFactoryTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/ntp/TitleUtilTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ClientIdTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java ../../chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubBackgroundSchedulerProcessor.java ../../chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPackerTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/omaha/ResponseParserTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/omaha/VersionNumberTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/payments/AutofillContactTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/payments/CurrencyStringFormatterTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/snackbar/SnackbarCollectionUnitTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProviderUnitTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/tabstate/TabStateUnitTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/util/NonThreadSafeTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappDataStorageTest.java ../../chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebappRegistryTest.java /tmp/tmphznxR6/java/android/support/v7/recyclerview/R.java /tmp/tmphznxR6/java/android/support/v7/appcompat/R.java /tmp/tmphznxR6/java/android/support/v7/mediarouter/R.java /tmp/tmphznxR6/java/android/support/design/R.java /tmp/tmphznxR6/java/com/google/android/gms/R.java /tmp/tmphznxR6/java/org/chromium/ui/R.java /tmp/tmphznxR6/java/org/chromium/content/R.java /tmp/tmphznxR6/java/org/chromium/components/web_contents_delegate_android/R.java /tmp/tmphznxR6/java/org/chromium/chrome/R.java /tmp/tmphznxR6/java/org/chromium/third_party/android/R.java /tmp/tmphznxR6/java/org/chromium/third_party/android/media/R.java ) ../../chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java:162: error: constructor NewTabPageAdapter in class NewTabPageAdapter cannot be applied to given types; NewTabPageAdapter ntpa = new NewTabPageAdapter(mNewTabPageManager, null, mSnippetsBridge); ^ required: NewTabPageManager,NewTabPageLayout,SnippetsBridge,UiConfig found: NewTabPageManager,<null>,SnippetsBridge reason: actual and formal argument lists differ in length https://build.chromium.org/p/chromium.linux/builders/Android%20Builder/builds....
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2171933002/ by jiayl@chromium.org. The reason for reverting is: https://build.chromium.org/p/chromium.linux/builders/Android%20Builder%20%28d.... |
