Refactor the about:version code out of about_ui.

Refactor the about:version code out of about_ui.

BUG=None
TEST=No user visible changes in about:version.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=158699

[SteveT, 2012-09-07 21:16:04]

Hey Evan - Please take an initial look.

Note that this is not quite done yet - I still need to figure out how to get
ChromeOSVersionHandler's async behaviour to work with the VersionUI class.

Can I make a delayed call to ChromeURLDataManager::AddDataSource? Have the
callback in ChromeOSVersionHandler call AddDataSource once it reads the version?

The page is also having issues loading chrome://theme/IDR_PRODUCT_LOGO...
Looking into it, but let me know if there is something specific that needs to be
done to have image resources load.

[Evan Stade, 2012-09-10 09:46:46]

On 2012/09/07 21:16:04, SteveT wrote:
> Hey Evan - Please take an initial look.
> 
> Note that this is not quite done yet - I still need to figure out how to get
> ChromeOSVersionHandler's async behaviour to work with the VersionUI class.
> 
> Can I make a delayed call to ChromeURLDataManager::AddDataSource? Have the
> callback in ChromeOSVersionHandler call AddDataSource once it reads the
version?

I don't think that would work because you'd try to load strings.js and fail
before the version is read. Just send the version info over as soon as you have
it on chromeos (CallJavaScriptFunction).

> 
> The page is also having issues loading chrome://theme/IDR_PRODUCT_LOGO...
> Looking into it, but let me know if there is something specific that needs to
be
> done to have image resources load.

you have to add the theme data source.

[Evan Stade, 2012-09-10 09:46:59]

http://codereview.chromium.org/10916182/diff/1/chrome/browser/resources/about...
File chrome/browser/resources/about_version.html (right):

http://codereview.chromium.org/10916182/diff/1/chrome/browser/resources/about...
chrome/browser/resources/about_version.html:39: <td class="version"
id="version"><span i18n-content="version"></span> (<span
i18n-content="official"></span> <span i18n-content="cl"></span>) <span
i18n-content="version_modifier"></span></td>
fix up line wraps please

http://codereview.chromium.org/10916182/diff/1/chrome/browser/resources/about...
chrome/browser/resources/about_version.html:79: <script
src="chrome://resources/js/jstemplate_compiled.js"></script>
this still necessary?

http://codereview.chromium.org/10916182/diff/1/chrome/browser/ui/webui/versio...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/1/chrome/browser/ui/webui/versio...
chrome/browser/ui/webui/version_ui.cc:66: std::vector<std::string> variations;
declare where it's used.

[SteveT, 2012-09-10 15:14:57]

Patchset up with (most of your) comments addressed. PTAL.

Patchset coming shortly with a CrOS fix.

http://codereview.chromium.org/10916182/diff/1/chrome/browser/resources/about...
File chrome/browser/resources/about_version.html (right):

http://codereview.chromium.org/10916182/diff/1/chrome/browser/resources/about...
chrome/browser/resources/about_version.html:39: <td class="version"
id="version"><span i18n-content="version"></span> (<span
i18n-content="official"></span> <span i18n-content="cl"></span>) <span
i18n-content="version_modifier"></span></td>
So this is tricky because it looks like the original author wrote this line like
this to get the spacing right. The resulting text with this current formatting
looks like this:
"23.0.1257.0 (Developer Build 154954)"

I can break the <td></td> into separate lines and that's fine... but if I break
each <span> onto their own lines, the spacing starts getting messed up. It looks
like this:
"23.0.1257.0 ( Developer Build 154954)"

Any thoughts on how to handle the spacing around the parentheses properly?

BTW I thought that the html style guide doesn't impose line-length caps?

http://codereview.chromium.org/10916182/diff/1/chrome/browser/resources/about...
chrome/browser/resources/about_version.html:79: <script
src="chrome://resources/js/jstemplate_compiled.js"></script>
Nope, removed it.

http://codereview.chromium.org/10916182/diff/1/chrome/browser/ui/webui/versio...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/1/chrome/browser/ui/webui/versio...
chrome/browser/ui/webui/version_ui.cc:66: std::vector<std::string> variations;
Added a comment and moved it down closer to the use-site.

[SteveT, 2012-09-10 15:40:00]

A couple more questions for you... See inline.

http://codereview.chromium.org/10916182/diff/8001/chrome/browser/ui/webui/abo...
File chrome/browser/ui/webui/about_ui.cc (right):

http://codereview.chromium.org/10916182/diff/8001/chrome/browser/ui/webui/abo...
chrome/browser/ui/webui/about_ui.cc:1361: base::Bind(&HandleAboutVersionStrings,
I just realized that this bit here that handles about:version's strings.js needs
to wait for Plugins to load before it can call the rest of the code that builds
the strings.js contents. Currently in my refactor, we don't handle this... The
ChromeWebUIDataSource approach seems to need to run synchronously when
strings.js is requested (I can't dispatch a call and get called back later when
the data is ready).

Should I follow the same suggestion you had before for CrOS version? Don't
populate the Flash version until it's ready.. and when it is, populate it with
CallJavascriptFunction?

http://codereview.chromium.org/10916182/diff/8001/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/8001/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:200: //
ChromeWebUIDataSource::SetFontAndTextDirection(localized_strings);
Any thoughts here on this TODO?

[Evan Stade, 2012-09-10 17:07:22]

http://codereview.chromium.org/10916182/diff/1/chrome/browser/resources/about...
File chrome/browser/resources/about_version.html (right):

http://codereview.chromium.org/10916182/diff/1/chrome/browser/resources/about...
chrome/browser/resources/about_version.html:39: <td class="version"
id="version"><span i18n-content="version"></span> (<span
i18n-content="official"></span> <span i18n-content="cl"></span>) <span
i18n-content="version_modifier"></span></td>
On 2012/09/10 15:14:57, SteveT wrote:
> So this is tricky because it looks like the original author wrote this line
like
> this to get the spacing right. The resulting text with this current formatting
> looks like this:
> "23.0.1257.0 (Developer Build 154954)"
> 
> I can break the <td></td> into separate lines and that's fine... but if I
break
> each <span> onto their own lines, the spacing starts getting messed up. It
looks
> like this:
> "23.0.1257.0 ( Developer Build 154954)"
> 
> Any thoughts on how to handle the spacing around the parentheses properly?

just don't line break in between the paren and the span it contains.

> 
> BTW I thought that the html style guide doesn't impose line-length caps?

if all your consoles are set up to handle 80-character columns (as rietveld
does) then excess line length makes it harder to read regardless of the
language.

http://codereview.chromium.org/10916182/diff/8001/chrome/browser/ui/webui/abo...
File chrome/browser/ui/webui/about_ui.cc (right):

http://codereview.chromium.org/10916182/diff/8001/chrome/browser/ui/webui/abo...
chrome/browser/ui/webui/about_ui.cc:1361: base::Bind(&HandleAboutVersionStrings,
On 2012/09/10 15:40:00, SteveT wrote:
> I just realized that this bit here that handles about:version's strings.js
needs
> to wait for Plugins to load before it can call the rest of the code that
builds
> the strings.js contents. Currently in my refactor, we don't handle this... The
> ChromeWebUIDataSource approach seems to need to run synchronously when
> strings.js is requested (I can't dispatch a call and get called back later
when
> the data is ready).

that's not true. Look at favicon_source; that loads favicons asynchronously.

> 
> Should I follow the same suggestion you had before for CrOS version? Don't
> populate the Flash version until it's ready.. and when it is, populate it with
> CallJavascriptFunction?

yes, I think that's the easiest way.

http://codereview.chromium.org/10916182/diff/8001/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/8001/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:200: //
ChromeWebUIDataSource::SetFontAndTextDirection(localized_strings);
I think you should use this line, yes... it will make the font platform-native
which is always a good thing.

[SteveT, 2012-09-10 20:14:51]

Addressed a couple more small things.

Will ping this thread again once the Async+CrOS stuff is refactored...

http://codereview.chromium.org/10916182/diff/1/chrome/browser/resources/about...
File chrome/browser/resources/about_version.html (right):

http://codereview.chromium.org/10916182/diff/1/chrome/browser/resources/about...
chrome/browser/resources/about_version.html:39: <td class="version"
id="version"><span i18n-content="version"></span> (<span
i18n-content="official"></span> <span i18n-content="cl"></span>) <span
i18n-content="version_modifier"></span></td>
On 2012/09/10 17:07:23, Evan Stade wrote:
> On 2012/09/10 15:14:57, SteveT wrote:
> > So this is tricky because it looks like the original author wrote this line
> like
> > this to get the spacing right. The resulting text with this current
formatting
> > looks like this:
> > "23.0.1257.0 (Developer Build 154954)"
> > 
> > I can break the <td></td> into separate lines and that's fine... but if I
> break
> > each <span> onto their own lines, the spacing starts getting messed up. It
> looks
> > like this:
> > "23.0.1257.0 ( Developer Build 154954)"
> > 
> > Any thoughts on how to handle the spacing around the parentheses properly?
> 
> just don't line break in between the paren and the span it contains.

Okay, that works. Interestingly... if I leave the <span>s all on one line, the
product logo loads properly. But if I break them down, Chrome fails to fetch the
resource. Not sure why that is. Anyways, adding the Theme data source like you
suggested resolves the Logo issue consistently.

> 
> > 
> > BTW I thought that the html style guide doesn't impose line-length caps?
> 
> if all your consoles are set up to handle 80-character columns (as rietveld
> does) then excess line length makes it harder to read regardless of the
> language.

Okay. I've tried to break up some of the lines where-ever I could.

http://codereview.chromium.org/10916182/diff/8001/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/8001/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:200: //
ChromeWebUIDataSource::SetFontAndTextDirection(localized_strings);
Oh, I think that |html_source| calls SetFontAndTextDirection for us in it's
StartDataRequest method for strings.js requests. So we don't need to do anything
extra here.

[SteveT, 2012-09-12 21:16:30]

Okay, async and CrOS stuff now completely pulled out of AboutUI and into
VersionUI. PTAL.

[Evan Stade, 2012-09-13 08:05:50]

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:49: chrome::VersionInfo version_info;
switch lines 49 and 50

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:52: // Bug 79458: Need to evaluate the use
of getting the version string on
nit: Prefer URL format: http://crbug.com/79458

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:97: // below we assumes it's UTF-8.
you've changed this code and now this comment is no longer true. You assume it's
ASCII. This will crash on non-ascii command lines, which are probably quite
common.

You should add a version of AddString that takes std::string.

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:102: // since the following operation will
complete quickly
1) do not execute file operations on the UI thread. I see this code already did
that, but it's bad. This is a great opportunity to excise it.

2) comments should end in punctuation.

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:186: class VersionDOMHandler : public
content::WebUIMessageHandler {
Should just be VersionHandler, and should be in its own file.

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_ui.h (right):

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.h:10: // The WebUI for chrome://version
nit: WebUI is the name of an object, and this is not of that type.

[SteveT, 2012-09-13 14:58:01]

Thanks for your comments - they're addressed now.

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:49: chrome::VersionInfo version_info;
On 2012/09/13 08:05:50, Evan Stade wrote:
> switch lines 49 and 50

Done.

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:52: // Bug 79458: Need to evaluate the use
of getting the version string on
On 2012/09/13 08:05:50, Evan Stade wrote:
> nit: Prefer URL format: http://crbug.com/79458

Done.

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:97: // below we assumes it's UTF-8.
Added AddString as you suggested and used that instead. I believe this comment
still applies?

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:102: // since the following operation will
complete quickly
Resolved this whole thing by having DomHandler dispatch a call to the FILE
thread for this operation, followed by returning it to the front end the same
way we do the flash version.

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:186: class VersionDOMHandler : public
content::WebUIMessageHandler {
On 2012/09/13 08:05:50, Evan Stade wrote:
> Should just be VersionHandler, and should be in its own file.

Done.

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_ui.h (right):

http://codereview.chromium.org/10916182/diff/6003/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.h:10: // The WebUI for chrome://version
"The WebUI handler for chrome://version."

[Evan Stade, 2012-09-13 15:23:27]

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_ui.cc:56: html_source->AddString("js_engine",
ASCIIToUTF16("V8"));
please remove all unnecessary string16 conversions

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_ui.cc:72: : IDS_ABOUT_VERSION_UNOFFICIAL);
operands go at end of line

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_ui.cc:105: if
(file_util::AbsolutePath(&profile_path))
curlies

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_ui.cc:105: if
(file_util::AbsolutePath(&profile_path))
file_util::AbsolutePath does file access, does it not?

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_ui.cc:108: else
curlies

[SteveT, 2012-09-13 15:41:24]

Back to you - PTAL.

FYI I am wrangling with the bots currently, so you might see some other changes
in this patch.

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_ui.cc:56: html_source->AddString("js_engine",
ASCIIToUTF16("V8"));
On 2012/09/13 15:23:27, Evan Stade wrote:
> please remove all unnecessary string16 conversions

Done.

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_ui.cc:72: : IDS_ABOUT_VERSION_UNOFFICIAL);
On 2012/09/13 15:23:27, Evan Stade wrote:
> operands go at end of line

Not sure what you mean here. Did you mean:

version_inf.IsOfficlaBuild() ? IDS_ABOUT_VERSION_OFFICIAL
                             : IDS_ABOUT_VERSION_UNOFFCIAL);


?

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_ui.cc:105: if
(file_util::AbsolutePath(&profile_path))
On 2012/09/13 15:23:27, Evan Stade wrote:
> curlies

Done.

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_ui.cc:105: if
(file_util::AbsolutePath(&profile_path))
On 2012/09/13 15:23:27, Evan Stade wrote:
> file_util::AbsolutePath does file access, does it not?

According to this, no:
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/file_util.h&exact_pac...

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_ui.cc:108: else
On 2012/09/13 15:23:27, Evan Stade wrote:
> curlies

Done.

[Evan Stade, 2012-09-13 16:05:47]

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_ui.cc:72: : IDS_ABOUT_VERSION_UNOFFICIAL);
On 2012/09/13 15:41:27, SteveT wrote:
> On 2012/09/13 15:23:27, Evan Stade wrote:
> > operands go at end of line
> 
> Not sure what you mean here. Did you mean:
> 
> version_inf.IsOfficlaBuild() ? IDS_ABOUT_VERSION_OFFICIAL
>                              : IDS_ABOUT_VERSION_UNOFFCIAL);
> 
> 
> ?

sorry, meant operators

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_ui.cc:105: if
(file_util::AbsolutePath(&profile_path))
On 2012/09/13 15:41:27, SteveT wrote:
> On 2012/09/13 15:23:27, Evan Stade wrote:
> > file_util::AbsolutePath does file access, does it not?
> 
> According to this, no:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/file_util.h&exact_pac...

yet:
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/file_util_posix.cc&ty...

[SteveT, 2012-09-13 17:00:26]

Okay, resolved those issues. Back to you. Thanks!

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_ui.cc:72: : IDS_ABOUT_VERSION_UNOFFICIAL);
On 2012/09/13 16:05:47, Evan Stade wrote:
> On 2012/09/13 15:41:27, SteveT wrote:
> > On 2012/09/13 15:23:27, Evan Stade wrote:
> > > operands go at end of line
> > 
> > Not sure what you mean here. Did you mean:
> > 
> > version_inf.IsOfficlaBuild() ? IDS_ABOUT_VERSION_OFFICIAL
> >                              : IDS_ABOUT_VERSION_UNOFFCIAL);
> > 
> > 
> > ?
> 
> sorry, meant operators

Done.

http://codereview.chromium.org/10916182/diff/14015/chrome/browser/ui/webui/ve...
chrome/browser/ui/webui/version_ui.cc:105: if
(file_util::AbsolutePath(&profile_path))
D'oh. Fixed by making this bit async as well.

[SteveT, 2012-09-13 19:32:02]

Since Evan seems to be out for the day, I'm going to kick off OWNERS reviews.

jhawkins: Please review for chrome.
steveblock: Please review for android_webview (though I am still seeing a
problem with generated_resources.grd.orig in the whitelist file, which is
hopefully being addressed by troopers now).

Thanks!

[Evan Stade, 2012-09-14 09:18:02]

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/resources/ab...
File chrome/browser/resources/about_version.html (right):

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/resources/ab...
chrome/browser/resources/about_version.html:41: (<span
i18n-content="official"></span> <span i18n-content="cl"></span>)
you can put a line return in between </span> and <span>

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:357: if (url.host()
== chrome::kChromeUIVersionHost)
this belongs in the main block (L235)

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_handler.cc (right):

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:32: class ChromeOSAboutVersionHandler
{
I don't think this should be its own class; it should be a specialization of
VersionHandler and should get its own file.

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:76: std::string* exec_path_out,
instead of passing in member variables as outparams, make this a class method

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:83: *exec_path_out =
WideToUTF8(executable_path.value());
it would be easier to just do LossyDisplayName() (indeed, these paths are meant
for display)

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:120: new
ChromeOSAboutVersionHandler(web_ui());
what is going on here?

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:150: #if !defined(NDEBUG)
it seems like this should be disabled for official builds, not release builds
(might want to ping author)

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:186:
web_ui()->CallJavascriptFunction("returnProfilePath", *profile_path);
any particular reason these are two separate js calls?

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:98: IDS_ABOUT_VERSION_LOADING_MESSAGE);
hmmm... I don't think these loading messages are actually very useful. It's
probably less noticeable/obtrusive to just leave the area blank until it's
filled with text, rather than flashing a loading message in there (since load
time will be very short). Can you file a bug and tag it with Review-UI

[Evan Stade, 2012-09-14 09:19:27]

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_handler.cc (right):

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:120: new
ChromeOSAboutVersionHandler(web_ui());
ignore this

[SteveT, 2012-09-14 14:48:15]

Everything addressed. Back to you.

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/resources/ab...
File chrome/browser/resources/about_version.html (right):

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/resources/ab...
chrome/browser/resources/about_version.html:41: (<span
i18n-content="official"></span> <span i18n-content="cl"></span>)
On 2012/09/14 09:18:02, Evan Stade wrote:
> you can put a line return in between </span> and <span>

Done.

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:357: if (url.host()
== chrome::kChromeUIVersionHost)
On 2012/09/14 09:18:02, Evan Stade wrote:
> this belongs in the main block (L235)

Done.

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_handler.cc (right):

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:32: class ChromeOSAboutVersionHandler
{
On 2012/09/14 09:18:02, Evan Stade wrote:
> I don't think this should be its own class; it should be a specialization of
> VersionHandler and should get its own file.

Done.

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:76: std::string* exec_path_out,
Kept this as a standalone helper method and ensured that we safely request the
file paths on FILE.

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:83: *exec_path_out =
WideToUTF8(executable_path.value());
On 2012/09/14 09:18:02, Evan Stade wrote:
> it would be easier to just do LossyDisplayName() (indeed, these paths are
meant
> for display)

Done.

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:150: #if !defined(NDEBUG)
I was the author :)

We decided that we want to show this in DEBUG builds, and an alternate UI for
RELEASE builds (which I would like to work on immediately after this is
committed).

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:186:
web_ui()->CallJavascriptFunction("returnProfilePath", *profile_path);
In the case where the profile was not yet available (L131), we want to
immediately return the profile path as IDS_ABOUT_VERSION_PATH_NOTFOUND so we
need a separate JS call that can handle the profile path alone.

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/4010/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:98: IDS_ABOUT_VERSION_LOADING_MESSAGE);
Sure. I've set a reminder to log the bug as soon as this is committed.

FYI I've recently chatted with JeffC and Glen about UI reviews after changing
about:versions and they said we are pretty much free to muck around with that as
long as they're minor changes.

[Evan Stade, 2012-09-14 15:11:29]

looking good

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chrome_web_ui_data_source.h (right):

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_web_ui_data_source.h:24: // Adds a string and its
equivalent to our dictionary.
this comment doesn't make sense to me, does it make sense to you?

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_web_ui_data_source.h:30: // Adds a name and its
equivaled localized string to our dictionary.
this one almost makes sense except for "equivaled"

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_handler.cc (right):

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:24: void GetFilePaths(FilePath
profile_path,
const ref

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:31: *exec_path_out =
UTF16ToASCII(executable_path.LossyDisplayName());
you can't just convert to ascii. You'd have to convert to UTF8 if you don't want
things to fall over. But really there's no need for either conversion. Do
everything in string16/UTF16

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:70: if (profile) {
I don't think it's possible for this to fail

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:73: scoped_ptr<StringValue>
profile_path(
shadowing is confusing

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:129: scoped_ptr<StringValue>
exec_path(
just base::StringValue(*executable_path_data) is sufficient

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:53: html_source->AddString("os_version",
string16());
for blank strings (where it doesn't matter either way) I'd go with std::string
to save a conversion.

[SteveT, 2012-09-14 15:28:36]

Thanks again - back to you.

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chrome_web_ui_data_source.h (right):

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_web_ui_data_source.h:24: // Adds a string and its
equivalent to our dictionary.
Let me know if this correction seems better.

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chrome_web_ui_data_source.h:30: // Adds a name and its
equivaled localized string to our dictionary.
Ditto

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_handler.cc (right):

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:24: void GetFilePaths(FilePath
profile_path,
Unfortunately AbsolutePath below takes a FilePath* which doesn't like it when
profile_path is a const ref.

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:31: *exec_path_out =
UTF16ToASCII(executable_path.LossyDisplayName());
Everything in string16 as discussed. Thanks for catching that.

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:70: if (profile) {
The original code checked for this, but it seems like it was being handled in a
WebUIController (AboutUI). Was it safe there as well?

I've removed the check.

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:73: scoped_ptr<StringValue>
profile_path(
Shadowing? You mean use of scoped_ptr here?

I've seen this pattern elsewhere in combination with CreateStringValue and
passing to CallJSFunction. Do you prefer something else?

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:129: scoped_ptr<StringValue>
exec_path(
I believe I did this earlier but had complaints on some platforms (mac_rel?)
about using the StringValue copy ctor (which didn't make sense to me).

Also, see my comment above about using scoped_ptr this way.

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:53: html_source->AddString("os_version",
string16());
On 2012/09/14 15:11:30, Evan Stade wrote:
> for blank strings (where it doesn't matter either way) I'd go with std::string
> to save a conversion.

Done.

[Evan Stade, 2012-09-14 16:31:03]

gotta head out. I'll finish up the review monday morning.

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_handler.cc (right):

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:24: void GetFilePaths(FilePath
profile_path,
On 2012/09/14 15:28:37, SteveT wrote:
> Unfortunately AbsolutePath below takes a FilePath* which doesn't like it when
> profile_path is a const ref.

ok. This should still be a const ref. Explicitly create a copy to pass into
AbsolutePath.

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:73: scoped_ptr<StringValue>
profile_path(
On 2012/09/14 15:28:37, SteveT wrote:
> Shadowing? You mean use of scoped_ptr here?
> 
> I've seen this pattern elsewhere in combination with CreateStringValue and
> passing to CallJSFunction. Do you prefer something else?

I meant that this local profile_path shadowed the profile_path defined on L68

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:129: scoped_ptr<StringValue>
exec_path(
On 2012/09/14 15:28:37, SteveT wrote:
> I believe I did this earlier but had complaints on some platforms (mac_rel?)
> about using the StringValue copy ctor (which didn't make sense to me).
> 
> Also, see my comment above about using scoped_ptr this way.

well it seems to work in some places:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/webui/op...

[SteveT, 2012-09-14 17:03:15]

Addressed everything below.

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_handler.cc (right):

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:24: void GetFilePaths(FilePath
profile_path,
Okay sounds good. Copy made.

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:73: scoped_ptr<StringValue>
profile_path(
Oh damn, I didn't notice that. It's been resolved now since we no longer need
this call due to the profile change above.

http://codereview.chromium.org/10916182/diff/8014/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:129: scoped_ptr<StringValue>
exec_path(
You're right that it's a simpler approach. Done everywhere.

[SteveT, 2012-09-14 18:04:50]

FYI OWNERS reviewers: This is lower priority now, as it likely won't make it in
M23.

[Evan Stade, 2012-09-17 09:24:45]

lgtm with nits

http://codereview.chromium.org/10916182/diff/2032/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_handler.cc (right):

http://codereview.chromium.org/10916182/diff/2032/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:69: FilePath profile_path =
Profile::FromWebUI(web_ui())->GetPath();
probably not worth storing in a local var

http://codereview.chromium.org/10916182/diff/2032/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:122:
web_ui()->CallJavascriptFunction("returnExecutablePath", exec_path);
and now these two js calls can be combined.

[SteveT, 2012-09-17 15:45:28]

Nits addressed and rebase-merged with a related patch.

OWNERS Approval Reviewers: PTAL when you have a chance. This is still low-pri
but can probably make it before the cut if it looks good to you guys.


Steve

http://codereview.chromium.org/10916182/diff/2032/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_handler.cc (right):

http://codereview.chromium.org/10916182/diff/2032/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:69: FilePath profile_path =
Profile::FromWebUI(web_ui())->GetPath();
On 2012/09/17 09:24:45, Evan Stade wrote:
> probably not worth storing in a local var

Done.

http://codereview.chromium.org/10916182/diff/2032/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_handler.cc:122:
web_ui()->CallJavascriptFunction("returnExecutablePath", exec_path);
On 2012/09/17 09:24:45, Evan Stade wrote:
> and now these two js calls can be combined.

Ah yes. Done.

[SteveT, 2012-09-21 12:51:58]

Hey James and Steve, could you guys take a look at this now that we've cleared
the M23 cut? I'd like to get this in soon given others are making changes to the
original unrefactored about_ui code
(https://chromiumcodereview.appspot.com/10957027/).


Steve

[SteveT, 2012-09-24 13:40:21]

Pinging James and Steve on this again.

[SteveT, 2012-09-25 15:04:22]

Seems like James and Steve are too busy for a review right now, so swapping new
OWNERS reviewers in.

Lei: Could you tackle chrome/ ?
Jonathan: Could you tackle android_webview/ ?

Thanks!

[SteveT, 2012-09-25 15:04:44]

Forgot to add joth@.

[joth, 2012-09-25 18:21:34]

lgtm for android_webview

[Lei Zhang, 2012-09-25 18:45:19]

lgtm

http://codereview.chromium.org/10916182/diff/7038/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/7038/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:47:
base::ThreadRestrictions::ScopedAllowIO allow_io;
this scope can be a bit more narrow maybe?

http://codereview.chromium.org/10916182/diff/7038/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:80: std::string command_line = "";
curious, why can't we use GetCommandLineString() on posix?

[SteveT, 2012-09-25 21:09:06]

Addressed one of your comments. Will commit shortly unless there is opposition.

http://codereview.chromium.org/10916182/diff/7038/chrome/browser/ui/webui/ver...
File chrome/browser/ui/webui/version_ui.cc (right):

http://codereview.chromium.org/10916182/diff/7038/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:47:
base::ThreadRestrictions::ScopedAllowIO allow_io;
Seems like GetVersionStringModifier is the only thing that needs the
ScopedAllowIO now. I'll scope it around that one call.

Also, I'll update crbug.com/79458 and ping the owner to see if it's any easier
for them to get rid of this after this refactoring...

http://codereview.chromium.org/10916182/diff/7038/chrome/browser/ui/webui/ver...
chrome/browser/ui/webui/version_ui.cc:80: std::string command_line = "";
I'm actually not sure - I haven't touched this code too much from where it
originally existed.

I can get in touch with viettrungluu@ about his TODO and see if he had the
answer to that...

[commit-bot: I haz the power, 2012-09-25 21:09:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/10916182/29001

[commit-bot: I haz the power, 2012-09-25 21:09:43]

Presubmit check for 10916182-29001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Banned functions were used.
    chrome/browser/ui/webui/version_ui.cc:48:
      New code should not use ScopedAllowIO. Post a task to the blocking
      pool or the FILE thread instead.

Presubmit checks took 2.1s to calculate.