Set article spacing in dp, not px.

Set article spacing in dp, not px.

The article separator is now explicitly drawn as a rectangle of the given colour (previously the effect was achieved by increasing the distance between items as the separator was the background colour).

BUG=607805
Committed: https://crrev.com/413fff1d0206229797cad8eba00d24911d580715
Cr-Commit-Position: refs/heads/master@{#392326}

[PEConn, 2016-05-06 16:24:51]

Description was changed from

==========
Set article spacing in dp, not px.

BUG=607805
==========

to

==========
Set article spacing in dp, not px.

This change reverts half of the changes in
https://codereview.chromium.org/1943893002/.

BUG=607805
==========

[PEConn, 2016-05-06 16:25:55]

peconn@chromium.org changed reviewers:
+ bauerb@chromium.org, maybelle@chromium.org, mcwilliams@chromium.org

[PEConn, 2016-05-06 16:25:56]

I've prepared this in-case UI decide to use dp for article separation.

[Bernhard Bauer, 2016-05-06 17:03:39]

LGTM

[PEConn, 2016-05-09 11:13:34]

Description was changed from

==========
Set article spacing in dp, not px.

This change reverts half of the changes in
https://codereview.chromium.org/1943893002/.

BUG=607805
==========

to

==========
Set article spacing in dp, not px.

The article separator is now explicitly drawn as a rectangle of the given colour
(previously the effect was achieved by increasing the distance between items as
the separator was the background colour).

BUG=607805
==========

[PEConn, 2016-05-09 11:13:49]

PTAL, some of the logic has changed.

[Bernhard Bauer, 2016-05-09 11:21:56]

lgtm

https://codereview.chromium.org/1948223006/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java
(right):

https://codereview.chromium.org/1948223006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:34:
* Add space between items to show the separator.
Nit: I would make this an implementation (non-Javadoc) comment (same for onDraw
below).

[May, 2016-05-09 11:47:53]

https://codereview.chromium.org/1948223006/diff/20001/chrome/android/java/res...
File chrome/android/java/res/values/dimens.xml (right):

https://codereview.chromium.org/1948223006/diff/20001/chrome/android/java/res...
chrome/android/java/res/values/dimens.xml:318: <dimen
name="payments_section_largest_spacing">32dp</dimen>
Why is this here? Bad merge?

https://codereview.chromium.org/1948223006/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java
(right):

https://codereview.chromium.org/1948223006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:52:
final int left = parent.getPaddingLeft();
Can you just create this as a drawable shape xml or do we really need it drawn
directly on the canvas?

[PEConn, 2016-05-09 13:33:39]

I moved the graphic part into a drawable (Rietveld is going a bit funny showing
the diff on that file though, I think it's trying to compare it with a similar
file it thinks I renamed).

https://codereview.chromium.org/1948223006/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java
(right):

https://codereview.chromium.org/1948223006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:34:
* Add space between items to show the separator.
On 2016/05/09 11:21:56, Bernhard Bauer wrote:
> Nit: I would make this an implementation (non-Javadoc) comment (same for
onDraw
> below).

Done.

https://codereview.chromium.org/1948223006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetItemDecoration.java:52:
final int left = parent.getPaddingLeft();
On 2016/05/09 11:47:53, May wrote:
> Can you just create this as a drawable shape xml or do we really need it drawn
> directly on the canvas?

Done.

[May, 2016-05-09 15:01:08]

lgtm

[PEConn, 2016-05-09 15:04:11]

The CQ bit was checked by peconn@chromium.org

[PEConn, 2016-05-09 15:04:12]

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/1948223006/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-05-09 15:04:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1948223006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1948223006/40001

[PEConn, 2016-05-09 15:21:25]

The CQ bit was unchecked by peconn@chromium.org

[PEConn, 2016-05-09 15:23:40]

The CQ bit was checked by peconn@chromium.org

[PEConn, 2016-05-09 15:23:40]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
maybelle@chromium.org
Link to the patchset: https://codereview.chromium.org/1948223006/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-05-09 15:24:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1948223006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1948223006/60001

[commit-bot: I haz the power, 2016-05-09 16:04:21]

Description was changed from

==========
Set article spacing in dp, not px.

The article separator is now explicitly drawn as a rectangle of the given colour
(previously the effect was achieved by increasing the distance between items as
the separator was the background colour).

BUG=607805
==========

to

==========
Set article spacing in dp, not px.

The article separator is now explicitly drawn as a rectangle of the given colour
(previously the effect was achieved by increasing the distance between items as
the separator was the background colour).

BUG=607805
==========

[commit-bot: I haz the power, 2016-05-09 16:04:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-09 16:06:00]

Description was changed from

==========
Set article spacing in dp, not px.

The article separator is now explicitly drawn as a rectangle of the given colour
(previously the effect was achieved by increasing the distance between items as
the separator was the background colour).

BUG=607805
==========

to

==========
Set article spacing in dp, not px.

The article separator is now explicitly drawn as a rectangle of the given colour
(previously the effect was achieved by increasing the distance between items as
the separator was the background colour).

BUG=607805

Committed: https://crrev.com/413fff1d0206229797cad8eba00d24911d580715
Cr-Commit-Position: refs/heads/master@{#392326}
==========

[commit-bot: I haz the power, 2016-05-09 16:06:02]

Patchset 4 (id:??) landed as
https://crrev.com/413fff1d0206229797cad8eba00d24911d580715
Cr-Commit-Position: refs/heads/master@{#392326}

[mcwilliams, 2016-05-10 11:00:13]

https://codereview.chromium.org/1948223006/diff/60001/chrome/android/java/res...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/1948223006/diff/60001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:111: <color
name="snippets_separator">#19000000</color>
Is this a valid hex color? They are normally only 6 characters long

[Bernhard Bauer, 2016-05-10 13:08:04]

https://codereview.chromium.org/1948223006/diff/60001/chrome/android/java/res...
File chrome/android/java/res/values/colors.xml (right):

https://codereview.chromium.org/1948223006/diff/60001/chrome/android/java/res...
chrome/android/java/res/values/colors.xml:111: <color
name="snippets_separator">#19000000</color>
On 2016/05/10 11:00:13, mcwilliams wrote:
> Is this a valid hex color? They are normally only 6 characters long

Yes, it's ARGB (see http://goo.gl/YEMhFL).