Optimizing for small screen display on android devices

Optimizing for small screen display on android devices

BUG=258034
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211384

[anitawoodruff, 2013-07-08 14:30:50]

Hi Joao,

Sorry not to include screenshots, I broke the apk you sent me when I rebased
against master (at least, policy values don't show up now due to the changes to
policy_ui.cc). Please could you send me the apk you create from this patch set
so I can double check it is all displaying OK now.

Thanks,

Anita

[anitawoodruff, 2013-07-08 16:26:15]

The odd indentation issues and small font size should be fixed now.

[Joao da Silva, 2013-07-08 16:44:51]

lgtm after fixing the nits

I looked at this on a Galaxy Nexus, Nexus 7 and Nexus 10, and they all look good
on both orientations. Nice!

https://codereview.chromium.org/18615004/diff/2001/chrome/browser/resources/p...
File chrome/browser/resources/policy.js (right):

https://codereview.chromium.org/18615004/diff/2001/chrome/browser/resources/p...
chrome/browser/resources/policy.js:13:
document.querySelector('.scope-column')).display == 'none' &&
nit: indent

https://codereview.chromium.org/18615004/diff/2001/chrome/browser/resources/p...
chrome/browser/resources/policy.js:15:
document.querySelector('.level-column')).display == 'none';
nit: indent

Also, why two tests? Don't they always have the same true/false value?

[anitawoodruff, 2013-07-09 07:41:27]

https://codereview.chromium.org/18615004/diff/2001/chrome/browser/resources/p...
File chrome/browser/resources/policy.js (right):

https://codereview.chromium.org/18615004/diff/2001/chrome/browser/resources/p...
chrome/browser/resources/policy.js:13:
document.querySelector('.scope-column')).display == 'none' &&
On 2013/07/08 16:44:51, Joao da Silva wrote:
> nit: indent

Can you explain what is wrong with the indent here? I thought when function
calls are wrapped then 4 spaces of indentation was required
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls).
Or is it because I should wrap at the 2nd parenthesis rather than the 1st?
(which I've done in the latest patch).

https://codereview.chromium.org/18615004/diff/2001/chrome/browser/resources/p...
chrome/browser/resources/policy.js:15:
document.querySelector('.level-column')).display == 'none';
On 2013/07/08 16:44:51, Joao da Silva wrote:
> nit: indent
> 
> Also, why two tests? Don't they always have the same true/false value?

Er, I guess I was vaguely thinking I should check both in case one gets disabled
for some other reason in future, but you're right, querying one is enough. Fixed
now.

[Joao da Silva, 2013-07-09 07:44:17]

lgtm! You need a review from a resources/OWNER; I suggest James.

[anitawoodruff, 2013-07-09 07:50:46]

*** Adding James as a reviewer ***

Hi James, please could you review this change to use an alternative css file for
the chrome://policy page for android devices. Please see screenshots attached to
bug for how it looks now on phone & tablet.

[James Hawkins, 2013-07-09 23:20:32]

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/policy.html (right):

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/policy.html:5: <meta name="viewport"
content="width=device-width, user-scalable=no" />
s/ \/\>/>/

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/policy.js (right):

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/policy.js:10: * @return {boolean} True if this is the
mobile version. */
nit: */ should be on the next line.

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/policy.js:142: expandedValue.setAttribute('colspan',
expandedValue.colSpan - 2);
What does the '2' represent?

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/policy.js:497: var tableHeadings = ['Scope', 'Level',
'Name', 'Value', 'Status'];
Localization?

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/policy_android.css (right):

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/policy_android.css:13: min-width: 0;
Does this do anything?

[anitawoodruff, 2013-07-10 13:54:18]

Thanks for the review James :) A couple of questions for you in the comments..

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/policy.html (right):

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/policy.html:5: <meta name="viewport"
content="width=device-width, user-scalable=no" />
On 2013/07/09 23:20:32, James Hawkins wrote:
> s/ \/\>/>/

Er.. I've removed the '/' from the end of this tag now, is that what you were
after?

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/policy.js (right):

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/policy.js:10: * @return {boolean} True if this is the
mobile version. */
On 2013/07/09 23:20:32, James Hawkins wrote:
> nit: */ should be on the next line.

Done.

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/policy.js:142: expandedValue.setAttribute('colspan',
expandedValue.colSpan - 2);
On 2013/07/09 23:20:32, James Hawkins wrote:
> What does the '2' represent?

The number of columns which are hidden in the android version (hence we need to
reduce the width of the expanded-value cell by this many). Would it be better if
I added a comment explaining this here, or should I declare the number of
columns which are hidden for android as a global constant with a name to this
effect?

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/policy.js:497: var tableHeadings = ['Scope', 'Level',
'Name', 'Value', 'Status'];
On 2013/07/09 23:20:32, James Hawkins wrote:
> Localization?

I'm still sending exactly the same strings to the loadTimeData.getString method
4 lines below, which I assumed was where translations get substituted - is this
not the case? (I've just done it like this so I can reuse the stems for the
class names).

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/policy_android.css (right):

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/policy_android.css:13: min-width: 0;
On 2013/07/09 23:20:32, James Hawkins wrote:
> Does this do anything?

Yep, it's required to override the 'min-width: 576px' attribute (which is set on
a bunch of things at '(index):26', according to the remote inspector). Without
it the page becomes much wider than can fit on a mobile screen.

[James Hawkins, 2013-07-10 16:20:57]

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/policy.html (right):

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/policy.html:5: <meta name="viewport"
content="width=device-width, user-scalable=no" />
On 2013/07/10 13:54:18, anitawoodruff wrote:
> On 2013/07/09 23:20:32, James Hawkins wrote:
> > s/ \/\>/>/
> 
> Er.. I've removed the '/' from the end of this tag now, is that what you were
> after?

Yep!

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/policy.js (right):

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/policy.js:142: expandedValue.setAttribute('colspan',
expandedValue.colSpan - 2);
On 2013/07/10 13:54:18, anitawoodruff wrote:
> On 2013/07/09 23:20:32, James Hawkins wrote:
> > What does the '2' represent?
> 
> The number of columns which are hidden in the android version (hence we need
to
> reduce the width of the expanded-value cell by this many). Would it be better
if
> I added a comment explaining this here, or should I declare the number of
> columns which are hidden for android as a global constant with a name to this
> effect?

Pull it out in a well-named, potentially documented const var.

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/policy.js:497: var tableHeadings = ['Scope', 'Level',
'Name', 'Value', 'Status'];
On 2013/07/10 13:54:18, anitawoodruff wrote:
> On 2013/07/09 23:20:32, James Hawkins wrote:
> > Localization?
> 
> I'm still sending exactly the same strings to the loadTimeData.getString
method
> 4 lines below, which I assumed was where translations get substituted - is
this
> not the case? (I've just done it like this so I can reuse the stems for the
> class names).

I meant to cancel this comment after reading the code; I guess it snuck back in.

[anitawoodruff, 2013-07-11 08:19:06]

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/policy.js (right):

https://codereview.chromium.org/18615004/diff/18001/chrome/browser/resources/...
chrome/browser/resources/policy.js:142: expandedValue.setAttribute('colspan',
expandedValue.colSpan - 2);
On 2013/07/10 16:20:58, James Hawkins wrote:
> On 2013/07/10 13:54:18, anitawoodruff wrote:
> > On 2013/07/09 23:20:32, James Hawkins wrote:
> > > What does the '2' represent?
> > 
> > The number of columns which are hidden in the android version (hence we need
> to
> > reduce the width of the expanded-value cell by this many). Would it be
better
> if
> > I added a comment explaining this here, or should I declare the number of
> > columns which are hidden for android as a global constant with a name to
this
> > effect?
> 
> Pull it out in a well-named, potentially documented const var.

Done. Though I'm not sure if I've put it in the right place (at the top but
within the main function).

[James Hawkins, 2013-07-11 23:07:35]

LGTM

https://codereview.chromium.org/18615004/diff/27001/chrome/browser/resources/...
File chrome/browser/resources/policy.js (right):

https://codereview.chromium.org/18615004/diff/27001/chrome/browser/resources/...
chrome/browser/resources/policy.js:8: * (Android) version of this page.
@const

https://codereview.chromium.org/18615004/diff/27001/chrome/browser/resources/...
chrome/browser/resources/policy.js:10: var
NO_OF_HIDDEN_COLUMNS_IN_MOBILE_VERSION = 2;
Move this down to where it's used.

https://codereview.chromium.org/18615004/diff/27001/chrome/browser/resources/...
chrome/browser/resources/policy.js:10: var
NO_OF_HIDDEN_COLUMNS_IN_MOBILE_VERSION = 2;
The name is a bit verbose, but whatever.

[anitawoodruff, 2013-07-12 08:53:02]

Fixed the nits in policy.js and rebased in preparation for committing (hence why
it looks like I changed other files in the latest patch set). Thanks for all the
reviews guys.

https://codereview.chromium.org/18615004/diff/27001/chrome/browser/resources/...
File chrome/browser/resources/policy.js (right):

https://codereview.chromium.org/18615004/diff/27001/chrome/browser/resources/...
chrome/browser/resources/policy.js:8: * (Android) version of this page.
On 2013/07/11 23:07:35, James Hawkins wrote:
> @const

Done.

https://codereview.chromium.org/18615004/diff/27001/chrome/browser/resources/...
chrome/browser/resources/policy.js:10: var
NO_OF_HIDDEN_COLUMNS_IN_MOBILE_VERSION = 2;
On 2013/07/11 23:07:35, James Hawkins wrote:
> Move this down to where it's used.

Done.

https://codereview.chromium.org/18615004/diff/27001/chrome/browser/resources/...
chrome/browser/resources/policy.js:10: var
NO_OF_HIDDEN_COLUMNS_IN_MOBILE_VERSION = 2;
On 2013/07/11 23:07:35, James Hawkins wrote:
> The name is a bit verbose, but whatever.

Made it slightly less verbose.

[commit-bot: I haz the power, 2013-07-12 08:53:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/18615004/37001

[commit-bot: I haz the power, 2013-07-12 12:38:02]

Change committed as 211384