Made the Android PageInfo dialog appear correctly on tablets

Made the Android PageInfo dialog appear correctly on tablets

Modified the layout logic for the PageInfo dialog so it appears
correctly on larger screens such as tablets.

BUG=365528
Committed: https://crrev.com/05faf6fb0d0a483266b86dd8df16fc831a5e6ff0
Cr-Commit-Position: refs/heads/master@{#301002}

[sashab, 2014-10-17 04:39:21]

sashab@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Ted C, 2014-10-17 19:35:26]

https://codereview.chromium.org/663733002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/663733002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:336:
LinearLayout.LayoutParams.MATCH_PARENT,
Instead of passing MATCH_PARENT here, what happens if you pass an explicit
number value (i.e. 800 just to test).

I wonder if potentially doing something like doing WRAP_CONTENT here and doing
some onMeasure magic in the scroll view could allow what Rebecca wants.

Here's my back of napkin guess

ScrollView scrollView = new ScrollView(mContext) {
    @Override
    protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
        int specWidth = MeasureSpec.getSize(widthMeasureSpec);
        if (specWidth > (400 * density)) {
            widthMeasureSpec = MeasureSpec.makeMeasureSpec(400 * density,
EXACTLY);
        }
        super.onMeasure(widthMeasureSpec, heightMeasureSpec);
    }
};

Granted, I have no idea what the measure spec passed in would be and whether the
width actually is something useful (it might be 0 or unspecified).  But that is
what I would at least play around with to see if it is easy to bound it.

[sashab, 2014-10-20 04:44:28]

https://codereview.chromium.org/663733002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/663733002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:336:
LinearLayout.LayoutParams.MATCH_PARENT,
On 2014/10/17 19:35:25, Ted C wrote:
> Instead of passing MATCH_PARENT here, what happens if you pass an explicit
> number value (i.e. 800 just to test).
> 
> I wonder if potentially doing something like doing WRAP_CONTENT here and doing
> some onMeasure magic in the scroll view could allow what Rebecca wants.
> 
> Here's my back of napkin guess
> 
> ScrollView scrollView = new ScrollView(mContext) {
>     @Override
>     protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
>         int specWidth = MeasureSpec.getSize(widthMeasureSpec);
>         if (specWidth > (400 * density)) {
>             widthMeasureSpec = MeasureSpec.makeMeasureSpec(400 * density,
> EXACTLY);
>         }
>         super.onMeasure(widthMeasureSpec, heightMeasureSpec);
>     }
> };
> 
> Granted, I have no idea what the measure spec passed in would be and whether
the
> width actually is something useful (it might be 0 or unspecified).  But that
is
> what I would at least play around with to see if it is easy to bound it.

Worked and looks great :) Thanks for your help Ted - sent you a screenshot!

[Ted C, 2014-10-20 15:15:42]

https://codereview.chromium.org/663733002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/663733002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:337:
int widthInPx = (int) (400 *
mContext.getResources().getDisplayMetrics().density);
make 400 a constant

also, isn't all of this logic specific to tablets?

https://codereview.chromium.org/663733002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:348:
LinearLayout.LayoutParams.WRAP_CONTENT,
Does this also only apply to tablets?

If so, we might want a top level if/else for phone and tablet make this easier
to follow.

[Ted C, 2014-10-23 22:37:25]

lgtm with a couple little nits

https://codereview.chromium.org/663733002/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/663733002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:344:
ViewGroup.LayoutParams.WRAP_CONTENT);
+1 indent as this is only seven.

Actually, it looks like the line above and the one above that are actually
indented 1 too many and don't align with the comment.

https://codereview.chromium.org/663733002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:350:
final int maxDialogWidthInDp = 400;
I would put this as a

private static final int MAX_TABLET_DIALOG_WIDTH_DP = 400;

at the top of the file.

[sashab, 2014-10-23 23:08:52]

https://codereview.chromium.org/663733002/diff/40001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
(right):

https://codereview.chromium.org/663733002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:344:
ViewGroup.LayoutParams.WRAP_CONTENT);
On 2014/10/23 22:37:24, Ted C wrote:
> +1 indent as this is only seven.
> 
> Actually, it looks like the line above and the one above that are actually
> indented 1 too many and don't align with the comment.

Ahh, you're right. Nice find, done.

https://codereview.chromium.org/663733002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:350:
final int maxDialogWidthInDp = 400;
On 2014/10/23 22:37:24, Ted C wrote:
> I would put this as a
> 
> private static final int MAX_TABLET_DIALOG_WIDTH_DP = 400;
> 
> at the top of the file.

Done.

[sashab, 2014-10-23 23:08:55]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-10-23 23:12:54]

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

[commit-bot: I haz the power, 2014-10-24 00:26:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-24 00:27:11]

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