Add landscape specs for Data Reduction Proxy promo

Add landscape specs for Data Reduction Proxy promo

BUG=529429
Committed: https://crrev.com/09d95755367c62b40fa6145c1a9079d7a8406fb4
Cr-Commit-Position: refs/heads/master@{#349998}

[megjablon, 2015-09-14 18:16:46]

megjablon@chromium.org changed reviewers:
+ newt@chromium.org

[newt (away), 2015-09-14 18:41:42]

Please upload screenshots to the bug so I can see what this looks like. Thanks!

[newt (away), 2015-09-15 02:35:16]

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreenView.java
(right):

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreenView.java:16:
* View that handles orientation changes for the Data Reduction Proxy promo.
How does it handle it? I'd add a short explanation so developers don't have to
read through onMeasure() to understand what's going on.

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreenView.java:18:
public class DataReductionPromoScreenView extends PaddedFrameLayout {
You could probably remove "Screen" from this class name to make it a bit less
unwieldy (more "wieldy"?).

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreenView.java:28:
LinearLayout content = (LinearLayout)
findViewById(R.id.data_reduction_promo_content);
findViewById() and Resources.getDimension() are relatively expensive calls. You
should call these once in the constructor and cache their return values.

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreenView.java:37:
int paddingInPixels = (int) (24 * scale + 0.5f);
Don't hard-code "24". Either make this a class constant
("HORIZONTAL_PADDING_DP"), or add it to dimens.xml.

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreenView.java:38:
findViewById(R.id.data_saver_illustration).setPadding(0, 0, paddingInPixels, 0);
setPadding() doesn't handle RTL locales correctly (you'll end up with padding on
the right side of the image, but the text will be to the left of the image).
Instead use ApiCompatibilityUtils.setPaddingRelative().

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreenView.java:47:
findViewById(R.id.data_saver_illustration).setPadding(0, 0, 0, paddingBottom);
(It's OK to use setPadding() here since you're setting the right and left
padding to the same value.)

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/PaddedFrameLayout.java
(right):

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/PaddedFrameLayout.java:29:
protected int mMaxChildWidth;
Protected member variables are generally discouraged because they make it harder
to see who's reading and who's writing the values. Use protected (or public)
setter methods instead.

[megjablon, 2015-09-15 22:43:10]

Patchset #2 (id:20001) has been deleted

[megjablon, 2015-09-15 22:44:02]

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreenView.java
(right):

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreenView.java:16:
* View that handles orientation changes for the Data Reduction Proxy promo.
On 2015/09/15 02:35:16, newt wrote:
> How does it handle it? I'd add a short explanation so developers don't have to
> read through onMeasure() to understand what's going on.

Done.

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreenView.java:18:
public class DataReductionPromoScreenView extends PaddedFrameLayout {
On 2015/09/15 02:35:16, newt wrote:
> You could probably remove "Screen" from this class name to make it a bit less
> unwieldy (more "wieldy"?).

Done.

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreenView.java:28:
LinearLayout content = (LinearLayout)
findViewById(R.id.data_reduction_promo_content);
On 2015/09/15 02:35:16, newt wrote:
> findViewById() and Resources.getDimension() are relatively expensive calls.
You
> should call these once in the constructor and cache their return values.

Done.

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreenView.java:37:
int paddingInPixels = (int) (24 * scale + 0.5f);
On 2015/09/15 02:35:16, newt wrote:
> Don't hard-code "24". Either make this a class constant
> ("HORIZONTAL_PADDING_DP"), or add it to dimens.xml.

Done.

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreenView.java:38:
findViewById(R.id.data_saver_illustration).setPadding(0, 0, paddingInPixels, 0);
On 2015/09/15 02:35:15, newt wrote:
> setPadding() doesn't handle RTL locales correctly (you'll end up with padding
on
> the right side of the image, but the text will be to the left of the image).
> Instead use ApiCompatibilityUtils.setPaddingRelative().

Done.

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoScreenView.java:47:
findViewById(R.id.data_saver_illustration).setPadding(0, 0, 0, paddingBottom);
On 2015/09/15 02:35:16, newt wrote:
> (It's OK to use setPadding() here since you're setting the right and left
> padding to the same value.)

Acknowledged.

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/PaddedFrameLayout.java
(right):

https://codereview.chromium.org/1343433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/widget/PaddedFrameLayout.java:29:
protected int mMaxChildWidth;
On 2015/09/15 02:35:16, newt wrote:
> Protected member variables are generally discouraged because they make it
harder
> to see who's reading and who's writing the values. Use protected (or public)
> setter methods instead.

Done.

[newt (away), 2015-09-15 23:08:44]

lgtm after comments

https://codereview.chromium.org/1343433002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoView.java
(right):

https://codereview.chromium.org/1343433002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoView.java:25:
private View mDataReductionIllustration;
suggestion: Since these private variables inside a DataReductionXyz class, you
can omit "DataReduction" from the name without losing any clarity.

https://codereview.chromium.org/1343433002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoView.java:32:
private float mScale;
Save mPaddingSide as a member variable instead of mScale.

https://codereview.chromium.org/1343433002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoView.java:39:
.getDimension(R.dimen.data_reduction_promo_screen_height);
use getDimensionPixelSize(), which returns an int

https://codereview.chromium.org/1343433002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoView.java:60:
int paddingInPixels = (int) (HORIZONTAL_PADDING_DP * mScale + 0.5f);
Customarily, all dimension variables are in pixels unless otherwise specified.
So, "paddingSide" or "paddingEnd" is a fine name for this.

[megjablon, 2015-09-16 19:02:48]

https://codereview.chromium.org/1343433002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoView.java
(right):

https://codereview.chromium.org/1343433002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoView.java:25:
private View mDataReductionIllustration;
On 2015/09/15 23:08:43, newt wrote:
> suggestion: Since these private variables inside a DataReductionXyz class, you
> can omit "DataReduction" from the name without losing any clarity.

Done.

https://codereview.chromium.org/1343433002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoView.java:32:
private float mScale;
On 2015/09/15 23:08:43, newt wrote:
> Save mPaddingSide as a member variable instead of mScale.

Done.

https://codereview.chromium.org/1343433002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoView.java:39:
.getDimension(R.dimen.data_reduction_promo_screen_height);
On 2015/09/15 23:08:43, newt wrote:
> use getDimensionPixelSize(), which returns an int

Done.

https://codereview.chromium.org/1343433002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionPromoView.java:60:
int paddingInPixels = (int) (HORIZONTAL_PADDING_DP * mScale + 0.5f);
On 2015/09/15 23:08:43, newt wrote:
> Customarily, all dimension variables are in pixels unless otherwise specified.
> So, "paddingSide" or "paddingEnd" is a fine name for this.

Done.

[newt (away), 2015-09-16 20:58:52]

still lgtm

[megjablon, 2015-09-17 21:10:16]

On 2015/09/16 20:58:52, newt wrote:
> still lgtm

Had to move findViewById calls to onFinishInflate because the views are null in
the constructor.

[megjablon, 2015-09-17 21:37:02]

On 2015/09/17 21:10:16, megjablon wrote:
> On 2015/09/16 20:58:52, newt wrote:
> > still lgtm
> 
> Had to move findViewById calls to onFinishInflate because the views are null
in
> the constructor.

Also, I'm going to fix the squished promo bug crbug.com/532199 before landing
this one. That bug will require merge and have side effects on the landscape
view.

[newt (away), 2015-09-17 21:38:52]

On 2015/09/17 21:10:16, megjablon wrote:
> On 2015/09/16 20:58:52, newt wrote:
> > still lgtm
> 
> Had to move findViewById calls to onFinishInflate because the views are null
in
> the constructor.

Doh. Yeah, onFinishInflate() is the right place :)

[megjablon, 2015-09-19 01:00:10]

On 2015/09/17 21:38:52, newt wrote:
> On 2015/09/17 21:10:16, megjablon wrote:
> > On 2015/09/16 20:58:52, newt wrote:
> > > still lgtm
> > 
> > Had to move findViewById calls to onFinishInflate because the views are null
> in
> > the constructor.
> 
> Doh. Yeah, onFinishInflate() is the right place :)

I rebased this cl so it has the dialog dimensions fix and changed it so that the
maxChildHeight is set to the heightMeasureSpec with 15 dp of margin on top and
bottom. Want to take another look?

[newt (away), 2015-09-21 17:29:36]

lgtm

[megjablon, 2015-09-21 19:51:08]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2015-09-21 19:52:27]

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

[commit-bot: I haz the power, 2015-09-21 19:59:18]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-09-21 20:00:28]

Patchset 5 (id:??) landed as
https://crrev.com/09d95755367c62b40fa6145c1a9079d7a8406fb4
Cr-Commit-Position: refs/heads/master@{#349998}