Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2849523004:
Removed whitespace when no headline is set in a snippet article. (Closed)
DescriptionRemoved whitespace when no headline is set in a snippet article.
Screenshots at
https://drive.google.com/drive/folders/0B7eRCSITD4qmelN0aEFMQXNwNTA?usp=sharing
BUG=716022
Review-Url: https://codereview.chromium.org/2849523004
Cr-Commit-Position: refs/heads/master@{#468007}
Committed: https://chromium.googlesource.com/chromium/src/+/e04941a0e5810c69846b52f664efae23431b1458
Patch Set 1 #
Total comments: 6
Patch Set 2 : Removed unnecessary checking for null in shouldShowHeadline() #
Total comments: 15
Patch Set 3 : Renamed variables. #Patch Set 4 : Removed changes to the snippetTextView. Fixed javadoc comment. Removed extraneous string creation. #
Total comments: 2
Patch Set 5 : Resolved patchset dependency. #Patch Set 6 : Rearrange the big if statement. #Messages
Total messages: 24 (10 generated)
Description was changed from ========== Removed whitespace when no headline is set in a snippet article. BUG=716022 ========== to ========== Removed whitespace when no headline is set in a snippet article. Screenshots at https://drive.google.com/corp/drive/folders/0B7eRCSITD4qmelN0aEFMQXNwNTA BUG=716022 ==========
galinap@google.com changed reviewers: + bauerb@chromium.org, dgn@chromium.org, mvanouwerkerk@chromium.org
PTAL
https://codereview.chromium.org/2849523004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2849523004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:251: ViewGroup.MarginLayoutParams ps = Can you use a more descriptive variable name than "ps"? https://codereview.chromium.org/2849523004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:271: private boolean shouldShowHeadline(SnippetArticle article) { This methods gets passed one parameter and gets the other from a member. Would it make more sense to either pass in both or neither? https://codereview.chromium.org/2849523004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:275: String nakedHeadline = headline.replaceAll("\\s", ""); I would just just trim().isEmpty().
On 2017/04/28 10:31:56, Galia wrote: > PTAL Drive-by nit: I guess it is enough to have screenshots attached in the bug. You may get multiple request access emails if you have a link to drive in the description.
Description was changed from ========== Removed whitespace when no headline is set in a snippet article. Screenshots at https://drive.google.com/corp/drive/folders/0B7eRCSITD4qmelN0aEFMQXNwNTA BUG=716022 ========== to ========== Removed whitespace when no headline is set in a snippet article. Screenshots at https://drive.google.com/drive/folders/0B7eRCSITD4qmelN0aEFMQXNwNTA?usp=sharing BUG=716022 ==========
https://codereview.chromium.org/2849523004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2849523004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:251: ViewGroup.MarginLayoutParams ps = On 2017/04/28 10:57:24, Bernhard Bauer wrote: > Can you use a more descriptive variable name than "ps"? Yes, sorry. I also renamed the publisher bar layout params name, as it makes it more clear. Thanks! https://codereview.chromium.org/2849523004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:271: private boolean shouldShowHeadline(SnippetArticle article) { On 2017/04/28 10:57:24, Bernhard Bauer wrote: > This methods gets passed one parameter and gets the other from a member. Would > it make more sense to either pass in both or neither? There is actually no need for any of this. I think I corrected this in patch set 2. I didn't see there was a member article at the top :) https://codereview.chromium.org/2849523004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:275: String nakedHeadline = headline.replaceAll("\\s", ""); On 2017/04/28 10:57:24, Bernhard Bauer wrote: > I would just just trim().isEmpty(). Handy method. Thanks!
https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:244: ViewGroup.MarginLayoutParams params = nit: rename to publisherParams or publisherBarParams? to distinguish with the descriptionParams https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:251: ViewGroup.MarginLayoutParams ps = nit: avoid abbreviated variable names. descriptionParams or snippetTextParams? https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:253: ps.topMargin = 0; you don't need to do that if there is no description, since the view will be marked GONE, right? https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:256: params.topMargin = 0; you should do that only if there is no description. Otherwise it would squash it with the publisher bar. https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:270: // If the title is empty (or contains only whitespace characters), we do not show it. Use javadoc style (/** */) for method documentation https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:272: String headline = mArticle.mTitle.toString(); title is already a string https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:275: String nakedHeadline = headline.replaceAll("\\s", ""); return mArticle.mTitle.trim().isEmpty() https://docs.oracle.com/javase/7/docs/api/java/lang/String.html#trim()
Sent the comments way too late, some of them are already fixed u_u https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:244: ViewGroup.MarginLayoutParams params = On 2017/04/28 13:05:12, dgn wrote: > nit: rename to publisherParams or publisherBarParams? to distinguish with the > descriptionParams Obsolete. https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:251: ViewGroup.MarginLayoutParams ps = On 2017/04/28 13:05:12, dgn wrote: > nit: avoid abbreviated variable names. descriptionParams or snippetTextParams? Obsolete. https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:275: String nakedHeadline = headline.replaceAll("\\s", ""); On 2017/04/28 13:05:12, dgn wrote: > return mArticle.mTitle.trim().isEmpty() > > https://docs.oracle.com/javase/7/docs/api/java/lang/String.html#trim() Obsolete.
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:253: ps.topMargin = 0; On 2017/04/28 13:05:12, dgn wrote: > you don't need to do that if there is no description, since the view will be > marked GONE, right? Good point. It's not needed. https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:256: params.topMargin = 0; On 2017/04/28 13:05:12, dgn wrote: > you should do that only if there is no description. Otherwise it would squash it > with the publisher bar. That's true. I don't think that case is ever possible (a description without a title?), but it is the correct thing to do. I've added an 'if' there. https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:270: // If the title is empty (or contains only whitespace characters), we do not show it. On 2017/04/28 13:05:12, dgn wrote: > Use javadoc style (/** */) for method documentation Done. https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:272: String headline = mArticle.mTitle.toString(); On 2017/04/28 13:05:12, dgn wrote: > title is already a string Done.
https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2849523004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:256: params.topMargin = 0; On 2017/04/28 13:59:46, Galia wrote: > On 2017/04/28 13:05:12, dgn wrote: > > you should do that only if there is no description. Otherwise it would squash > it > > with the publisher bar. > > That's true. I don't think that case is ever possible (a description without a > title?), but it is the correct thing to do. I've added an 'if' there. Ignore the previous comment. It's not relevant to the code. No changes to the snippetTextView is needed. I've removed it all.
lgtm!
The CQ bit was checked by galinap@google.com
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2831053004 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
lgtm https://codereview.chromium.org/2849523004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2849523004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:256: publisherBarParams.topMargin = mPublisherBar.getResources().getDimensionPixelSize( Nit: I would put this branch first with `if (showDescription)`, then the middle one with `else if (showHeadline)`, then the first one with `else`.
https://codereview.chromium.org/2849523004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/2849523004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:256: publisherBarParams.topMargin = mPublisherBar.getResources().getDimensionPixelSize( On 2017/04/28 14:20:25, Bernhard Bauer wrote: > Nit: I would put this branch first with `if (showDescription)`, then the middle > one with `else if (showHeadline)`, then the first one with `else`. I knew there was a better way of doing this, thanks!
The CQ bit was checked by galinap@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2849523004/#ps120001 (title: "Rearrange the big if statement.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493390227649170,
"parent_rev": "4990bf6e0264a539cf76b09243dffc27e6546c52", "commit_rev":
"e04941a0e5810c69846b52f664efae23431b1458"}
Message was sent while issue was closed.
Description was changed from ========== Removed whitespace when no headline is set in a snippet article. Screenshots at https://drive.google.com/drive/folders/0B7eRCSITD4qmelN0aEFMQXNwNTA?usp=sharing BUG=716022 ========== to ========== Removed whitespace when no headline is set in a snippet article. Screenshots at https://drive.google.com/drive/folders/0B7eRCSITD4qmelN0aEFMQXNwNTA?usp=sharing BUG=716022 Review-Url: https://codereview.chromium.org/2849523004 Cr-Commit-Position: refs/heads/master@{#468007} Committed: https://chromium.googlesource.com/chromium/src/+/e04941a0e5810c69846b52f664ef... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/e04941a0e5810c69846b52f664ef... |
