[NTP::Downloads] Move attribution higher if description is empty.

[NTP::Downloads] Move attribution higher if description is empty.

Previously, the attribution was moved higher only for layouts which do
not show description. In this CL, this is also done for cards with empty
description. This is needed to make Downloads look better (they have
thumbnails and do not have descriptions).

BUG=690420
Review-Url: https://codereview.chromium.org/2698833004
Cr-Commit-Position: refs/heads/master@{#450959}
Committed: https://chromium.googlesource.com/chromium/src/+/9b5580df750abd8c3354d7c63e8a7d38c479ea39

[vitaliii, 2017-02-16 13:03:51]

Patchset #1 (id:1) has been deleted

[vitaliii, 2017-02-16 13:04:26]

vitaliii@chromium.org changed reviewers:
+ dgn@chromium.org

[vitaliii, 2017-02-16 13:04:26]

Hi dgn@,

Could you have a look please?

[dgn, 2017-02-16 13:37:51]

https://codereview.chromium.org/2698833004/diff/10001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2698833004/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:224:
boolean showsThumbnail = shouldShowThumbnail(horizontalStyle, verticalStyle,
layout);
IMO present tense is inappropriate here because this variable does not describe
the current state but the state we want to be in. So imperative?

https://codereview.chromium.org/2698833004/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:249:
return true;
check the text length here instead.

[dgn, 2017-02-16 13:40:50]

https://codereview.chromium.org/2698833004/diff/10001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2698833004/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:223:
&& mArticle != null && mArticle.mPreviewText.length() != 0;
Prefer TextUtils#isEmpty(), as it would also handle the string being null.

(Additional remark: String#isEmpty() is preferred to checking the string length)

[vitaliii, 2017-02-16 14:02:02]

The CQ bit was checked by vitaliii@chromium.org to run a CQ dry run

[vitaliii, 2017-02-16 14:02:13]

Hi dgn@,

I addressed your comments, PTAL.

https://codereview.chromium.org/2698833004/diff/10001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2698833004/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:223:
&& mArticle != null && mArticle.mPreviewText.length() != 0;
On 2017/02/16 13:40:50, dgn wrote:
> Prefer TextUtils#isEmpty(), as it would also handle the string being null.
> 
> (Additional remark: String#isEmpty() is preferred to checking the string
length)

Done.

https://codereview.chromium.org/2698833004/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:224:
boolean showsThumbnail = shouldShowThumbnail(horizontalStyle, verticalStyle,
layout);
On 2017/02/16 13:37:51, dgn wrote:
> IMO present tense is inappropriate here because this variable does not
describe
> the current state but the state we want to be in. So imperative?

Done.

https://codereview.chromium.org/2698833004/diff/10001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:249:
return true;
On 2017/02/16 13:37:51, dgn wrote:
> check the text length here instead.

Done.

[commit-bot: I haz the power, 2017-02-16 14:02:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgn, 2017-02-16 14:03:41]

lgtm

[vitaliii, 2017-02-16 14:10:56]

The CQ bit was unchecked by vitaliii@chromium.org

[vitaliii, 2017-02-16 14:10:59]

The CQ bit was checked by vitaliii@chromium.org

[commit-bot: I haz the power, 2017-02-16 14:11:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 14:42:48]

CQ is committing da patch.

Bot data: {"patchset_id": 30001, "attempt_start_ts": 1487254259848260,
"parent_rev": "3d1a8d1c963cae739551639bcec3e4b2e15b9b10", "commit_rev":
"9b5580df750abd8c3354d7c63e8a7d38c479ea39"}

[commit-bot: I haz the power, 2017-02-16 14:43:25]

Description was changed from

==========
[NTP::Downloads] Move attribution higher if description is empty.

Previously, the attribution was moved higher only for layouts which do
not show description. In this CL, this is also done for cards with empty
description. This is needed to make Downloads look better (they have
thumbnails and do not have descriptions).

BUG=690420
==========

to

==========
[NTP::Downloads] Move attribution higher if description is empty.

Previously, the attribution was moved higher only for layouts which do
not show description. In this CL, this is also done for cards with empty
description. This is needed to make Downloads look better (they have
thumbnails and do not have descriptions).

BUG=690420

Review-Url: https://codereview.chromium.org/2698833004
Cr-Commit-Position: refs/heads/master@{#450959}
Committed:
https://chromium.googlesource.com/chromium/src/+/9b5580df750abd8c3354d7c63e8a...
==========

[commit-bot: I haz the power, 2017-02-16 14:43:27]

Committed patchset #2 (id:30001) as
https://chromium.googlesource.com/chromium/src/+/9b5580df750abd8c3354d7c63e8a...