separate about page into its own page (included in chrome://chrome)

separate about page into its own page (included in chrome://chrome)

for now it's only enabled in cros, but we plan to have a version for desktop chrome as well, which is why I took it out of the chromeos namespace.

BUG=109203
TEST=manual


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=117136

[Dan Beam, 2012-01-09 23:05:51]

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/browser_resou...
File chrome/browser/browser_resources.grd (right):

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/browser_resou...
chrome/browser/browser_resources.grd:136: <include name="IDR_ABOUT_PAGE_HTML"
file="resources\about_page\about_page.html" flattenhtml="true" type="BINDATA" />
When are going to evaluate whether we should be flattening or not?

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extensions.js (left):

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extensions.js:185: }
Can you add a ; at the end of the function expression while you're here?

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/resources/ube...
File chrome/browser/resources/uber/uber.html (right):

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/resources/ube...
chrome/browser/resources/uber/uber.html:40: <iframe
src="chrome://about-page-frame/"></iframe></div>
Why isn't </div> on following line?

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/ui/webui/abou...
File chrome/browser/ui/webui/about_page/about_page_handler.cc (right):

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/ui/webui/abou...
chrome/browser/ui/webui/about_page/about_page_handler.cc:110: { "pageTitle",
IDS_ABOUT_TAB_TITLE },
Do you think it would be worth alpha sorting these to find one easier?

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/ui/webui/abou...
chrome/browser/ui/webui/about_page/about_page_handler.cc:133: for (size_t i = 0;
i < ARRAYSIZE_UNSAFE(resources); ++i) {
^ didn't know about this macro, cool!

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/ui/webui/abou...
chrome/browser/ui/webui/about_page/about_page_handler.cc:135:
l10n_util::GetStringUTF16(resources[i].ids));
Should we both DCHECK()ing that the key doesn't exist here?  Seems like it'd be
pretty easy to write over an existing key accidentally (which may be a bug or
feature, depending on how you look at it, but I think bug).

http://codereview.chromium.org/9124028/diff/9001/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/9124028/diff/9001/chrome/common/url_constants....
chrome/common/url_constants.h:23: extern const char
kChromeUIAboutPageFrameURL[];
alpha here and below if appropriate (seems to be)

[Evan Stade, 2012-01-09 23:31:16]

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/browser_resou...
File chrome/browser/browser_resources.grd (right):

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/browser_resou...
chrome/browser/browser_resources.grd:136: <include name="IDR_ABOUT_PAGE_HTML"
file="resources\about_page\about_page.html" flattenhtml="true" type="BINDATA" />
On 2012/01/09 23:05:51, Dan Beam wrote:
> When are going to evaluate whether we should be flattening or not?

in the future

I did notice some jiggle when the uber page loads, I think due to loading css
and/or fonts after the initial render. So I will be looking into that soon. As
far as size/speed tradeoff in general, I think we should optimize for size.
flattenhtml="true" does not necessarily flatten everything, it just allows you
to use flattening (via relative paths). Absolute paths (e.g.
chrome://aboutPage/about.js) are still not flattened.

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extensions.js (left):

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extensions.js:185: }
On 2012/01/09 23:05:51, Dan Beam wrote:
> Can you add a ; at the end of the function expression while you're here?

Done.

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/resources/ube...
File chrome/browser/resources/uber/uber.html (right):

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/resources/ube...
chrome/browser/resources/uber/uber.html:40: <iframe
src="chrome://about-page-frame/"></iframe></div>
On 2012/01/09 23:05:51, Dan Beam wrote:
> Why isn't </div> on following line?

I haven't actually looked at an html style guide, if one even exists, however
I've seen this done a lot and I don't think it hurts readability. Do you?

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/ui/webui/abou...
File chrome/browser/ui/webui/about_page/about_page_handler.cc (right):

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/ui/webui/abou...
chrome/browser/ui/webui/about_page/about_page_handler.cc:110: { "pageTitle",
IDS_ABOUT_TAB_TITLE },
On 2012/01/09 23:05:51, Dan Beam wrote:
> Do you think it would be worth alpha sorting these to find one easier?

no I prefer to keep the related ones together, e.g. stable near beta and os near
platform. Anyway it's not my call as I don't own this code.

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/ui/webui/abou...
chrome/browser/ui/webui/about_page/about_page_handler.cc:133: for (size_t i = 0;
i < ARRAYSIZE_UNSAFE(resources); ++i) {
On 2012/01/09 23:05:51, Dan Beam wrote:
> ^ didn't know about this macro, cool!

fyi, better to use arraysize() where possible

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/ui/webui/abou...
chrome/browser/ui/webui/about_page/about_page_handler.cc:135:
l10n_util::GetStringUTF16(resources[i].ids));
On 2012/01/09 23:05:51, Dan Beam wrote:
> Should we both DCHECK()ing that the key doesn't exist here?  Seems like it'd
be
> pretty easy to write over an existing key accidentally (which may be a bug or
> feature, depending on how you look at it, but I think bug).

I don't think so, it seems a little overly cautious. I can't recall ever
encountering a bug where a string was overwritten. This function is the only
place where any strings should be written to this dictionary so it's not like a
code change in another file could affect this list.

http://codereview.chromium.org/9124028/diff/9001/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

http://codereview.chromium.org/9124028/diff/9001/chrome/common/url_constants....
chrome/common/url_constants.h:23: extern const char
kChromeUIAboutPageFrameURL[];
On 2012/01/09 23:05:51, Dan Beam wrote:
> alpha here and below if appropriate (seems to be)

Done.

[Dan Beam, 2012-01-09 23:53:14]

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/browser_resou...
File chrome/browser/browser_resources.grd (right):

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/browser_resou...
chrome/browser/browser_resources.grd:136: <include name="IDR_ABOUT_PAGE_HTML"
file="resources\about_page\about_page.html" flattenhtml="true" type="BINDATA" />
On 2012/01/09 23:31:16, Evan Stade wrote:
> On 2012/01/09 23:05:51, Dan Beam wrote:
> > When are going to evaluate whether we should be flattening or not?
> 
> in the future
> 
> I did notice some jiggle when the uber page loads, I think due to loading css
> and/or fonts after the initial render. So I will be looking into that soon. As
> far as size/speed tradeoff in general, I think we should optimize for size.
> flattenhtml="true" does not necessarily flatten everything, it just allows you
> to use flattening (via relative paths). Absolute paths (e.g.
> chrome://aboutPage/about.js) are still not flattened.

OK, is there somewhere to annotate that we'd like to investigate this (either
via a bug or TODO() or something)?  Either way, as long as you're aware of this
I'll leave to your discretion.

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/resources/ube...
File chrome/browser/resources/uber/uber.html (right):

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/resources/ube...
chrome/browser/resources/uber/uber.html:40: <iframe
src="chrome://about-page-frame/"></iframe></div>
On 2012/01/09 23:31:16, Evan Stade wrote:
> On 2012/01/09 23:05:51, Dan Beam wrote:
> > Why isn't </div> on following line?
> 
> I haven't actually looked at an html style guide, if one even exists, however
> I've seen this done a lot and I don't think it hurts readability. Do you?

At first I thought there was no end </div> tag as it wasn't nested as I'm used
to.

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/ui/webui/abou...
File chrome/browser/ui/webui/about_page/about_page_handler.cc (right):

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/ui/webui/abou...
chrome/browser/ui/webui/about_page/about_page_handler.cc:110: { "pageTitle",
IDS_ABOUT_TAB_TITLE },
On 2012/01/09 23:31:16, Evan Stade wrote:
> On 2012/01/09 23:05:51, Dan Beam wrote:
> > Do you think it would be worth alpha sorting these to find one easier?
> 
> no I prefer to keep the related ones together, e.g. stable near beta and os
near
> platform. Anyway it's not my call as I don't own this code.

If it's in preference order or an order I'm not aware of, that's great, just
wanted that to have a significance (which it seems it does).

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/ui/webui/abou...
chrome/browser/ui/webui/about_page/about_page_handler.cc:135:
l10n_util::GetStringUTF16(resources[i].ids));
On 2012/01/09 23:31:16, Evan Stade wrote:
> On 2012/01/09 23:05:51, Dan Beam wrote:
> > Should we both DCHECK()ing that the key doesn't exist here?  Seems like it'd
> be
> > pretty easy to write over an existing key accidentally (which may be a bug
or
> > feature, depending on how you look at it, but I think bug).
> 
> I don't think so, it seems a little overly cautious. I can't recall ever
> encountering a bug where a string was overwritten. This function is the only
> place where any strings should be written to this dictionary so it's not like
a
> code change in another file could affect this list.

But what happens when you start to merge dictionaries from various files by
passing in a partly populated DictionaryValue* via |localized_strings| (or
you're saying that Just Won't Happen™)?  Why not just return a new
DictionaryValue* to the caller, clear the DictionaryValue* at start, or DCHECK()
the keys being set (or key size at the beginning of the method).  Any of these
seem like good preventative solutions.

[Evan Stade, 2012-01-10 00:04:50]

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/ui/webui/abou...
File chrome/browser/ui/webui/about_page/about_page_handler.cc (right):

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/ui/webui/abou...
chrome/browser/ui/webui/about_page/about_page_handler.cc:135:
l10n_util::GetStringUTF16(resources[i].ids));
On 2012/01/09 23:53:14, Dan Beam wrote:
> On 2012/01/09 23:31:16, Evan Stade wrote:
> > On 2012/01/09 23:05:51, Dan Beam wrote:
> > > Should we both DCHECK()ing that the key doesn't exist here?  Seems like
it'd
> > be
> > > pretty easy to write over an existing key accidentally (which may be a bug
> or
> > > feature, depending on how you look at it, but I think bug).
> > 
> > I don't think so, it seems a little overly cautious. I can't recall ever
> > encountering a bug where a string was overwritten. This function is the only
> > place where any strings should be written to this dictionary so it's not
like
> a
> > code change in another file could affect this list.
> 
> But what happens when you start to merge dictionaries from various files by
> passing in a partly populated DictionaryValue* via |localized_strings| (or
> you're saying that Just Won't Happen™)?

that's indeed possible, if we wanted to fold about into another page. However to
actually be safe in that case we'd have to add DCHECKs for every SetString
(below as well) which would be onerous. I think if we're going to fold pages
together the person doing it should namespace the strings, i.e. put "about_" in
front of each one of them.

> Why not just return a new
> DictionaryValue* to the caller, clear the DictionaryValue* at start, or
DCHECK()
> the keys being set (or key size at the beginning of the method).  Any of these
> seem like good preventative solutions.

empty check added

[csilv, 2012-01-10 00:22:11]

http://codereview.chromium.org/9124028/diff/15001/chrome/browser/resources/ab...
File chrome/browser/resources/about_page/about_page.js (right):

http://codereview.chromium.org/9124028/diff/15001/chrome/browser/resources/ab...
chrome/browser/resources/about_page/about_page.js:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
I guess this page can be reverted (unless rietveld is lying to me)

http://codereview.chromium.org/9124028/diff/15001/chrome/browser/resources/ex...
File chrome/browser/resources/extensions/extensions.html (right):

http://codereview.chromium.org/9124028/diff/15001/chrome/browser/resources/ex...
chrome/browser/resources/extensions/extensions.html:143: </html>
The changes to the two extensions files seem unrelated to the purpose of this
CL... but i'm fine w/ leaving them in.  Is there a reason not to have all JS
files down in this section?

[Dan Beam, 2012-01-10 00:33:39]

http://codereview.chromium.org/9124028/diff/15001/chrome/browser/resources/ab...
File chrome/browser/resources/about_page/about_page.js (right):

http://codereview.chromium.org/9124028/diff/15001/chrome/browser/resources/ab...
chrome/browser/resources/about_page/about_page.js:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/01/10 00:22:11, csilv wrote:
> I guess this page can be reverted (unless rietveld is lying to me)

File was moved (check out "Download patch" link).

http://codereview.chromium.org/9124028/diff/15029/chrome/browser/ui/webui/abo...
File chrome/browser/ui/webui/about_page/about_page_handler.cc (right):

http://codereview.chromium.org/9124028/diff/15029/chrome/browser/ui/webui/abo...
chrome/browser/ui/webui/about_page/about_page_handler.cc:103:
DCHECK(localized_strings->empty());
Super, thanks!

[Evan Stade, 2012-01-10 04:09:45]

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/resources/ube...
File chrome/browser/resources/uber/uber.html (right):

http://codereview.chromium.org/9124028/diff/9001/chrome/browser/resources/ube...
chrome/browser/resources/uber/uber.html:40: <iframe
src="chrome://about-page-frame/"></iframe></div>
On 2012/01/09 23:53:14, Dan Beam wrote:
> On 2012/01/09 23:31:16, Evan Stade wrote:
> > On 2012/01/09 23:05:51, Dan Beam wrote:
> > > Why isn't </div> on following line?
> > 
> > I haven't actually looked at an html style guide, if one even exists,
however
> > I've seen this done a lot and I don't think it hurts readability. Do you?
> 
> At first I thought there was no end </div> tag as it wasn't nested as I'm used
> to.

I dislike the idea of putting every closing tag on a new line because that will
lead to a lot of new lines. In our code, there is a lot of precedent for placing
multiple end tags on the same line, and no style guide exists to contradict it,
so I prefer to leave it this way.

http://codereview.chromium.org/9124028/diff/15001/chrome/browser/resources/ex...
File chrome/browser/resources/extensions/extensions.html (right):

http://codereview.chromium.org/9124028/diff/15001/chrome/browser/resources/ex...
chrome/browser/resources/extensions/extensions.html:143: </html>
On 2012/01/10 00:22:11, csilv wrote:
> The changes to the two extensions files seem unrelated to the purpose of this
> CL... but i'm fine w/ leaving them in.  Is there a reason not to have all JS
> files down in this section?

yes, they are sort of unrelated, but it's the type of small cleanup that I hope
is ok to fold into other semi-related work. I believe we put js at the top for
readability purposes and to match C++ (includes at top), but in this case the
order is important because they execute code which needs the dom to exist.

[csilv, 2012-01-10 19:27:26]

lgtm

[Dan Beam, 2012-01-10 19:38:24]

http://codereview.chromium.org/9124028/diff/18001/chrome/browser/resources/ab...
File chrome/browser/resources/about_page/about_page.html (right):

http://codereview.chromium.org/9124028/diff/18001/chrome/browser/resources/ab...
chrome/browser/resources/about_page/about_page.html:144: <script
src="chrome://resources/js/i18n_process.js"></script>
Why are these outside the <body>?

[Evan Stade, 2012-01-10 20:18:51]

(verbal lg from dan)

[commit-bot: I haz the power, 2012-01-10 20:19:18]

No LGTM from valid reviewers yet.

[Dan Beam, 2012-01-10 23:50:34]

lgtm

[commit-bot: I haz the power, 2012-01-10 23:54:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/9124028/23001

[commit-bot: I haz the power, 2012-01-11 01:06:38]

Change committed as 117136