Help: Implement the initial version of the cross-platform help/about page.

Help: Implement the initial version of the cross-platform help/about page.

BUG=110506
TEST=none
R=csilv

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

[csilv, 2012-02-03 19:43:52]

http://codereview.chromium.org/9320056/diff/2003/chrome/app/chromium_strings.grd
File chrome/app/chromium_strings.grd (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/app/chromium_strings....
chrome/app/chromium_strings.grd:577: Updating Chromium in the background...
(commentary) "in the background" is techie talk.  this is not meaningful to
average users.

http://codereview.chromium.org/9320056/diff/2003/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6362: Checking for updates in the
background...
(commentary) "in the background" is techie talk.  this is not meaningful to
average users.

http://codereview.chromium.org/9320056/diff/2003/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6375: Report a problem
add title-case version

http://codereview.chromium.org/9320056/diff/2003/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:8038: Relaunch
You may want to add a title-case version of this since translations might be
more than one word.

http://codereview.chromium.org/9320056/diff/2003/chrome/app/google_chrome_str...
File chrome/app/google_chrome_strings.grd (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/app/google_chrome_str...
chrome/app/google_chrome_strings.grd:197: Get help with using Chrome
add title-case version for buttons

http://codereview.chromium.org/9320056/diff/2003/chrome/app/google_chrome_str...
chrome/app/google_chrome_strings.grd:556: Updating Google Chrome in the
background...
yadda yadda "in the background" (see previous comments)

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:536: }
Did you intend to include this change as part of this CL?  Looks fine, just
curious.

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/hel...
File chrome/browser/resources/help/help.css (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/hel...
chrome/browser/resources/help/help.css:77: background-image:
url('../../../app/theme/update_available.png');
chrome://theme/IDR_UPDATE_AVAILABLE

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/hel...
chrome/browser/resources/help/help.css:81: background-image:
url('../../../app/theme/update_uptodate.png');
chrome://theme/IDR_UPDATE_UPTODATE

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/hel...
File chrome/browser/resources/help/help.html (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/hel...
chrome/browser/resources/help/help.html:34: <div id="update-percentage"
hidden>15%</div>
remove the 15% placeholder

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/hel...
File chrome/browser/resources/help/help.js (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/hel...
chrome/browser/resources/help/help.js:49: console.log(status);
remove this

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

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/ube...
chrome/browser/resources/uber/uber.html:29:
data-favicon="chrome://theme/IDR_PRODUCT_LOGO_16">
this may be a better favicon:
  chrome://theme/IDR_HELP_MENU

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:1231:
browser->GetSelectedWebContents()->GetView()->SetInitialFocus();
just checking... intentional for this CL, or should be separated?

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/ui/webui/help...
File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_chromeos.cc:9: #include
"chrome/browser/chromeos/login/wizard_controller.h"
alphabetical order

[James Hawkins, 2012-02-03 22:48:39]

http://codereview.chromium.org/9320056/diff/2003/chrome/app/chromium_strings.grd
File chrome/app/chromium_strings.grd (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/app/chromium_strings....
chrome/app/chromium_strings.grd:577: Updating Chromium in the background...
On 2012/02/03 19:43:52, csilv wrote:
> (commentary) "in the background" is techie talk.  this is not meaningful to
> average users.

Sure, it's what's in the mocks though.

http://codereview.chromium.org/9320056/diff/2003/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6375: Report a problem
On 2012/02/03 19:43:52, csilv wrote:
> add title-case version

Done.

http://codereview.chromium.org/9320056/diff/2003/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:8038: Relaunch
On 2012/02/03 19:43:52, csilv wrote:
> You may want to add a title-case version of this since translations might be
> more than one word.

Wha? How do we decide which cases we do that for?

http://codereview.chromium.org/9320056/diff/2003/chrome/app/google_chrome_str...
File chrome/app/google_chrome_strings.grd (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/app/google_chrome_str...
chrome/app/google_chrome_strings.grd:197: Get help with using Chrome
On 2012/02/03 19:43:52, csilv wrote:
> add title-case version for buttons

Done.

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:536: }
On 2012/02/03 19:43:52, csilv wrote:
> Did you intend to include this change as part of this CL?  Looks fine, just
> curious.

Sorta, not really.  I need to file a bug for it.  I'll take it out of this CL.

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/hel...
File chrome/browser/resources/help/help.css (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/hel...
chrome/browser/resources/help/help.css:77: background-image:
url('../../../app/theme/update_available.png');
On 2012/02/03 19:43:52, csilv wrote:
> chrome://theme/IDR_UPDATE_AVAILABLE

Done.

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/hel...
chrome/browser/resources/help/help.css:81: background-image:
url('../../../app/theme/update_uptodate.png');
On 2012/02/03 19:43:52, csilv wrote:
> chrome://theme/IDR_UPDATE_UPTODATE

Done.

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/hel...
File chrome/browser/resources/help/help.html (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/hel...
chrome/browser/resources/help/help.html:34: <div id="update-percentage"
hidden>15%</div>
On 2012/02/03 19:43:52, csilv wrote:
> remove the 15% placeholder

Done.

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/hel...
File chrome/browser/resources/help/help.js (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/hel...
chrome/browser/resources/help/help.js:49: console.log(status);
On 2012/02/03 19:43:52, csilv wrote:
> remove this

Done.

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

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/resources/ube...
chrome/browser/resources/uber/uber.html:29:
data-favicon="chrome://theme/IDR_PRODUCT_LOGO_16">
On 2012/02/03 19:43:52, csilv wrote:
> this may be a better favicon:
>   chrome://theme/IDR_HELP_MENU

This is the 'About' part of the help page.  It's supposed to be the icon of the
product.

http://folder/mlchan/no_crawl/UX/AboutHelpCombined/v3/

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:1231:
browser->GetSelectedWebContents()->GetView()->SetInitialFocus();
On 2012/02/03 19:43:52, csilv wrote:
> just checking... intentional for this CL, or should be separated?

Will remove and do a separate CL.

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/ui/webui/help...
File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_chromeos.cc:9: #include
"chrome/browser/chromeos/login/wizard_controller.h"
On 2012/02/03 19:43:52, csilv wrote:
> alphabetical order

Done.

[csilv, 2012-02-03 23:50:03]

http://codereview.chromium.org/9320056/diff/2003/chrome/app/chromium_strings.grd
File chrome/app/chromium_strings.grd (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/app/chromium_strings....
chrome/app/chromium_strings.grd:577: Updating Chromium in the background...
On 2012/02/03 22:48:40, James Hawkins wrote:
> On 2012/02/03 19:43:52, csilv wrote:
> > (commentary) "in the background" is techie talk.  this is not meaningful to
> > average users.
> 
> Sure, it's what's in the mocks though.

Gotcha, no change needed.  I'll ping glen about it and we can change it later if
he accepts my reasoning.

http://codereview.chromium.org/9320056/diff/2003/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:8038: Relaunch
On 2012/02/03 22:48:40, James Hawkins wrote:
> On 2012/02/03 19:43:52, csilv wrote:
> > You may want to add a title-case version of this since translations might be
> > more than one word.
> 
> Wha? How do we decide which cases we do that for?

Anytime we have a title for a menu or a button, we should provide title-case and
non-title case versions, even if it is only a single word.  If we don't do that,
then the localizer can only provide a single version of the string and that will
be problematic if the translation is more tha a single word.

[James Hawkins, 2012-02-04 00:00:15]

http://codereview.chromium.org/9320056/diff/2003/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/9320056/diff/2003/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:8038: Relaunch
On 2012/02/03 23:50:03, csilv wrote:
> On 2012/02/03 22:48:40, James Hawkins wrote:
> > On 2012/02/03 19:43:52, csilv wrote:
> > > You may want to add a title-case version of this since translations might
be
> > > more than one word.
> > 
> > Wha? How do we decide which cases we do that for?
> 
> Anytime we have a title for a menu or a button, we should provide title-case
and
> non-title case versions, even if it is only a single word.  If we don't do
that,
> then the localizer can only provide a single version of the string and that
will
> be problematic if the translation is more tha a single word.

Done.

[csilv, 2012-02-04 00:16:21]

lgtm

[Nico, 2012-02-05 04:50:28]

This broke the chromeos clang build. I'm trying to fix at
https://chromiumcodereview.appspot.com/9307111/ . Please take a look, I don't
really know what I'm doing.

https://chromiumcodereview.appspot.com/9320056/diff/3038/chrome/browser/ui/we...
File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right):

https://chromiumcodereview.appspot.com/9320056/diff/3038/chrome/browser/ui/we...
chrome/browser/ui/webui/help/version_updater_chromeos.cc:51: const
UpdateEngineClient::Status& status) OVERRIDE {
OVERRIDE in cc files is a compile error

https://chromiumcodereview.appspot.com/9320056/diff/3038/chrome/browser/ui/we...
File chrome/browser/ui/webui/help/version_updater_chromeos.h (right):

https://chromiumcodereview.appspot.com/9320056/diff/3038/chrome/browser/ui/we...
chrome/browser/ui/webui/help/version_updater_chromeos.h:17: virtual bool
CanBeUpdated() const OVERRIDE;
Doesn't actually override anything.

https://chromiumcodereview.appspot.com/9320056/diff/3038/chrome/browser/ui/we...
File chrome/browser/ui/webui/help/version_updater_unimplemented.cc (right):

https://chromiumcodereview.appspot.com/9320056/diff/3038/chrome/browser/ui/we...
chrome/browser/ui/webui/help/version_updater_unimplemented.cc:8: return
static_cast<VersionUpdater*>(new VersionUpdaterUnimplemented);
Is this cast needed? Can't you just leave it out?