Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10796054:
Adapt the linux about_memory page to better render on Android. (Closed)
|
Created:
8 years, 5 months ago by Ted C Modified:
8 years, 4 months ago CC:
chromium-reviews, arv (Not doing code reviews) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdapt the linux about_memory page to better render on Android.
Updates kSyncAutoStarts in defaults.cc to match the android version as well.
BUG=138245
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148637
Patch Set 1 #Patch Set 2 : Use grd syntax for specifying android. #
Total comments: 9
Patch Set 3 : Using grd summary descriptions. #
Total comments: 8
Patch Set 4 : Rebased and addressed comments. #
Total comments: 2
Patch Set 5 : Resolve nits #
Total comments: 1
Messages
Total messages: 22 (0 generated)
lgtm (including change to kSyncAutoStarts although you should update cl description)
On 2012/07/20 01:25:30, Yaron wrote:
> lgtm
>
> (including change to kSyncAutoStarts although you should update cl
description)
Hold the phone...looking at about_version.html, I think they support grd like
syntax.
I see the following in about_version.
<if expr="pp_ifdef('chromeos')">
I'm going to see if I can convert this to use those instead.
Updated with the grd syntax and removed the nasty is_android flag from about_ui.cc. PTAL
lgtm
@arv or @estade for OWNERS approval
http://codereview.chromium.org/10796054/diff/3002/chrome/browser/resources/ab... File chrome/browser/resources/about_memory_linux.html (right): http://codereview.chromium.org/10796054/diff/3002/chrome/browser/resources/ab... chrome/browser/resources/about_memory_linux.html:14: font-size: 22pt; you should set the font-size on <body> like <body i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize"> http://codereview.chromium.org/10796054/diff/3002/chrome/browser/resources/ab... chrome/browser/resources/about_memory_linux.html:62: Summary of memory used by Chromium. Since Chromium uses why did you change the string? The difference seems immaterial. Also, references to "Chromium" here are incorrect on Google Chrome builds.
http://codereview.chromium.org/10796054/diff/3002/chrome/browser/resources/ab... File chrome/browser/resources/about_memory_linux.html (right): http://codereview.chromium.org/10796054/diff/3002/chrome/browser/resources/ab... chrome/browser/resources/about_memory_linux.html:14: font-size: 22pt; On 2012/07/23 22:03:01, Evan Stade wrote: > you should set the font-size on <body> like > > <body i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize"> I'm just overriding the values defined in about_memory.css, which is shared in all the platforms. i.e. this definition. body { font-size: 84%; margin: 0; min-width: 45em; padding: 0.75em; } This font size is also larger than the android default font size of 16, which was too small for this to be readable. Is this ever overridden per page, or does it always use the call in ChromeURLDataManager::DataSource::SetFontAndTextDirection? Changing the fontFamily also seems like something that would change the behavior of this page, which is not my intention for other platforms. http://codereview.chromium.org/10796054/diff/3002/chrome/browser/resources/ab... chrome/browser/resources/about_memory_linux.html:62: Summary of memory used by Chromium. Since Chromium uses On 2012/07/23 22:03:01, Evan Stade wrote: > why did you change the string? The difference seems immaterial. > > Also, references to "Chromium" here are incorrect on Google Chrome builds. The main reason is that this was what we had in the android specific version of this file before I started trying to join them (not a good reason) But in it's defense, the above seems like it should only be true if show_other_browsers == true, which this one was meant to be specific to just Chromium/Google Chrome. I think the second one is nicer, but it's not a sticking point for me and can get rid of it if need be.
http://codereview.chromium.org/10796054/diff/3002/chrome/browser/resources/ab... File chrome/browser/resources/about_memory_linux.html (right): http://codereview.chromium.org/10796054/diff/3002/chrome/browser/resources/ab... chrome/browser/resources/about_memory_linux.html:14: font-size: 22pt; On 2012/07/23 22:18:00, Ted C wrote: > On 2012/07/23 22:03:01, Evan Stade wrote: > > you should set the font-size on <body> like > > > > <body i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize"> > > I'm just overriding the values defined in about_memory.css, which is shared in > all the platforms. > > i.e. this definition. > body { > font-size: 84%; > margin: 0; > min-width: 45em; > padding: 0.75em; > } > > This font size is also larger than the android default font size of 16, which > was too small for this to be readable. > > Is this ever overridden per page, or does it always use the call in > ChromeURLDataManager::DataSource::SetFontAndTextDirection? > > Changing the fontFamily also seems like something that would change the behavior > of this page, which is not my intention for other platforms. it can be overridden by the page but usually we'd still use SetFontAndTextDirection, then grow it by an em factor like 1.5em. font-family should not be here either. if there is an about_memory.css, why is there css here? http://codereview.chromium.org/10796054/diff/3002/chrome/browser/resources/ab... chrome/browser/resources/about_memory_linux.html:62: Summary of memory used by Chromium. Since Chromium uses On 2012/07/23 22:18:00, Ted C wrote: > On 2012/07/23 22:03:01, Evan Stade wrote: > > why did you change the string? The difference seems immaterial. > > > > Also, references to "Chromium" here are incorrect on Google Chrome builds. > > The main reason is that this was what we had in the android specific version of > this file before I started trying to join them (not a good reason) > > But in it's defense, the above seems like it should only be true if > show_other_browsers == true, which this one was meant to be specific to just > Chromium/Google Chrome. > > I think the second one is nicer, but it's not a sticking point for me and can > get rid of it if need be. OK, now I understand the difference. Still, I think that you should pull this string out into a grd file because of the product name problem. It's also noteworthy that the <p> tags aren't matched.
https://chromiumcodereview.appspot.com/10796054/diff/3002/chrome/browser/reso... File chrome/browser/resources/about_memory_linux.html (right): https://chromiumcodereview.appspot.com/10796054/diff/3002/chrome/browser/reso... chrome/browser/resources/about_memory_linux.html:14: font-size: 22pt; On 2012/07/23 22:44:27, Evan Stade wrote: > On 2012/07/23 22:18:00, Ted C wrote: > > On 2012/07/23 22:03:01, Evan Stade wrote: > > > you should set the font-size on <body> like > > > > > > <body i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize"> > > > > I'm just overriding the values defined in about_memory.css, which is shared in > > all the platforms. > > > > i.e. this definition. > > body { > > font-size: 84%; > > margin: 0; > > min-width: 45em; > > padding: 0.75em; > > } > > > > This font size is also larger than the android default font size of 16, which > > was too small for this to be readable. > > > > Is this ever overridden per page, or does it always use the call in > > ChromeURLDataManager::DataSource::SetFontAndTextDirection? > > > > Changing the fontFamily also seems like something that would change the > behavior > > of this page, which is not my intention for other platforms. > > it can be overridden by the page but usually we'd still use > SetFontAndTextDirection, then grow it by an em factor like 1.5em. > > font-family should not be here either. > > if there is an about_memory.css, why is there css here? The css file is shared across all platforms, so this section contains any subsequent styles needed. No idea what font-family is here, but on linux Helvetica is not included in the default, which is 'Arial, sans-serif'. I'm wary to change the global font size/family as I don't know why they were there in the first place. https://chromiumcodereview.appspot.com/10796054/diff/3002/chrome/browser/reso... chrome/browser/resources/about_memory_linux.html:62: Summary of memory used by Chromium. Since Chromium uses On 2012/07/23 22:44:27, Evan Stade wrote: > On 2012/07/23 22:18:00, Ted C wrote: > > On 2012/07/23 22:03:01, Evan Stade wrote: > > > why did you change the string? The difference seems immaterial. > > > > > > Also, references to "Chromium" here are incorrect on Google Chrome builds. > > > > The main reason is that this was what we had in the android specific version > of > > this file before I started trying to join them (not a good reason) > > > > But in it's defense, the above seems like it should only be true if > > show_other_browsers == true, which this one was meant to be specific to just > > Chromium/Google Chrome. > > > > I think the second one is nicer, but it's not a sticking point for me and can > > get rid of it if need be. > > OK, now I understand the difference. Still, I think that you should pull this > string out into a grd file because of the product name problem. It's also > noteworthy that the <p> tags aren't matched. Done...but it's worth nothing that Chromium is used throughout the file other places. I guess it doesn't hurt to start.
https://chromiumcodereview.appspot.com/10796054/diff/3002/chrome/browser/reso... File chrome/browser/resources/about_memory_linux.html (right): https://chromiumcodereview.appspot.com/10796054/diff/3002/chrome/browser/reso... chrome/browser/resources/about_memory_linux.html:14: font-size: 22pt; On 2012/07/24 20:51:50, Ted C wrote: > On 2012/07/23 22:44:27, Evan Stade wrote: > > On 2012/07/23 22:18:00, Ted C wrote: > > > On 2012/07/23 22:03:01, Evan Stade wrote: > > > > you should set the font-size on <body> like > > > > > > > > <body i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize"> > > > > > > I'm just overriding the values defined in about_memory.css, which is shared > in > > > all the platforms. > > > > > > i.e. this definition. > > > body { > > > font-size: 84%; > > > margin: 0; > > > min-width: 45em; > > > padding: 0.75em; > > > } > > > > > > This font size is also larger than the android default font size of 16, > which > > > was too small for this to be readable. > > > > > > Is this ever overridden per page, or does it always use the call in > > > ChromeURLDataManager::DataSource::SetFontAndTextDirection? > > > > > > Changing the fontFamily also seems like something that would change the > > behavior > > > of this page, which is not my intention for other platforms. > > > > it can be overridden by the page but usually we'd still use > > SetFontAndTextDirection, then grow it by an em factor like 1.5em. > > > > font-family should not be here either. > > > > if there is an about_memory.css, why is there css here? > > The css file is shared across all platforms, so this section contains any > subsequent styles needed. it's not your fault, but that is not a valid reason to have css in an html file. There is actually no valid reason to have css in an html file. Please don't add any more. Refactor it if you can. > > No idea what font-family is here, but on linux Helvetica is not included in the > default, which is 'Arial, sans-serif'. on every platform the default for webui:// pages is your system default. For my linux desktop, that happens to be 'DejaVu sans'. This page disregards that because it predated SetFontAndTextDirection, and no one thought to update it. > > I'm wary to change the global font size/family as I don't know why they were > there in the first place. because they were added before the appearance of webui was standardized. This page is obviously not really intended to be user-facing because the text is not even translated (although as far as I'm aware, this page is less for debugging and more for helping users analyze the browsers they have on their system, so it should probably be translated). The page as a whole has such an oddball UI that I wouldn't worry too much about messing with the appearance; it's obviously been neglected for many years. https://chromiumcodereview.appspot.com/10796054/diff/10001/chrome/app/google_... File chrome/app/google_chrome_strings.grd (right): https://chromiumcodereview.appspot.com/10796054/diff/10001/chrome/app/google_... chrome/app/google_chrome_strings.grd:570: <message name="IDS_MEMORY_USAGE_SUMMARY_DESC" desc="Describes the browser summary table in the about memory page, which shows memory usage for Google Chrome."> for now, you can put translateable="false" https://chromiumcodereview.appspot.com/10796054/diff/10001/chrome/app/google_... chrome/app/google_chrome_strings.grd:571: Summary of memory used by Google Chrome. Since Google Chrome uses multiple processes, memory reflects aggregate memory used across all browser processes.<ph name="BEGIN_PARAGRAPH"><p></ph>For Google Chrome, processes used to display diagnostics information (such as this "about:memory") are excluded.<ph name="END_PARAGRAPH"></p></ph> instead of cooking html into the grd, I think it's better to just put a couple line returns along with white-space: pre-line https://chromiumcodereview.appspot.com/10796054/diff/10001/chrome/browser/res... File chrome/browser/resources/about_memory_linux.html (right): https://chromiumcodereview.appspot.com/10796054/diff/10001/chrome/browser/res... chrome/browser/resources/about_memory_linux.html:55: Summary of memory used by currently active browsers. I meant for you to pull both summaries out of here, defining the same string resource differently on android and desktop. https://chromiumcodereview.appspot.com/10796054/diff/10001/chrome/browser/res... chrome/browser/resources/about_memory_linux.html:63: <p jsvalues=".innerHTML:summary_desc"></p> usually we use i18n-values:
http://codereview.chromium.org/10796054/diff/10001/chrome/app/google_chrome_s... File chrome/app/google_chrome_strings.grd (right): http://codereview.chromium.org/10796054/diff/10001/chrome/app/google_chrome_s... chrome/app/google_chrome_strings.grd:570: <message name="IDS_MEMORY_USAGE_SUMMARY_DESC" desc="Describes the browser summary table in the about memory page, which shows memory usage for Google Chrome."> On 2012/07/24 21:37:04, Evan Stade wrote: > for now, you can put translateable="false" Done. http://codereview.chromium.org/10796054/diff/10001/chrome/app/google_chrome_s... chrome/app/google_chrome_strings.grd:571: Summary of memory used by Google Chrome. Since Google Chrome uses multiple processes, memory reflects aggregate memory used across all browser processes.<ph name="BEGIN_PARAGRAPH"><p></ph>For Google Chrome, processes used to display diagnostics information (such as this "about:memory") are excluded.<ph name="END_PARAGRAPH"></p></ph> On 2012/07/24 21:37:04, Evan Stade wrote: > instead of cooking html into the grd, I think it's better to just put a couple > line returns along with white-space: pre-line Done. http://codereview.chromium.org/10796054/diff/10001/chrome/browser/resources/a... File chrome/browser/resources/about_memory_linux.html (right): http://codereview.chromium.org/10796054/diff/10001/chrome/browser/resources/a... chrome/browser/resources/about_memory_linux.html:55: Summary of memory used by currently active browsers. On 2012/07/24 21:37:04, Evan Stade wrote: > I meant for you to pull both summaries out of here, defining the same string > resource differently on android and desktop. Done. http://codereview.chromium.org/10796054/diff/10001/chrome/browser/resources/a... chrome/browser/resources/about_memory_linux.html:63: <p jsvalues=".innerHTML:summary_desc"></p> On 2012/07/24 21:37:04, Evan Stade wrote: > usually we use i18n-values: Done.
lgtm!
(forgot to publish) http://codereview.chromium.org/10796054/diff/13002/chrome/app/google_chrome_s... File chrome/app/google_chrome_strings.grd (right): http://codereview.chromium.org/10796054/diff/13002/chrome/app/google_chrome_s... chrome/app/google_chrome_strings.grd:577: <message translateable="false" name="IDS_MEMORY_USAGE_SUMMARY_DESC" desc="Describes the browser summary table in the about memory page, which shows memory usage for Google Chrome."> nit: put translateable at the end of the attributes.
http://codereview.chromium.org/10796054/diff/13002/chrome/app/google_chrome_s... File chrome/app/google_chrome_strings.grd (right): http://codereview.chromium.org/10796054/diff/13002/chrome/app/google_chrome_s... chrome/app/google_chrome_strings.grd:577: <message translateable="false" name="IDS_MEMORY_USAGE_SUMMARY_DESC" desc="Describes the browser summary table in the about memory page, which shows memory usage for Google Chrome."> On 2012/07/26 04:01:19, Evan Stade wrote: > nit: put translateable at the end of the attributes. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tedchoc@chromium.org/10796054/18001
Presubmit check for 10796054-18001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
chrome/app
Presubmit checks took 1.1s to calculate.
+thakis for OWNERS of chrome/app chrome/browser bits
LGTM Please pull a mac build from the build waterfall (build.chromium.org -> continuous -> mac) after this has landed and check that about:memory still works on mac with this (it should). http://codereview.chromium.org/10796054/diff/18001/chrome/browser/ui/webui/ab... File chrome/browser/ui/webui/about_ui.cc (right): http://codereview.chromium.org/10796054/diff/18001/chrome/browser/ui/webui/ab... chrome/browser/ui/webui/about_ui.cc:1243: l10n_util::GetStringUTF16(IDS_MEMORY_USAGE_SUMMARY_DESC)); fwiw, on mac we completely forked the about:memory html. I suppose the mac html file won't get confused by this unused value.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tedchoc@chromium.org/10796054/18001
Change committed as 148637
On 2012/07/26 22:40:09, I haz the power (commit-bot) wrote: > Change committed as 148637 @thakis, I downloaded the latest mac build this morning and tested about:memory and about:version (both I changed yesterday) and they are looking the same as before. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
