Fix RegisterChromeBrowser definition and usage in shell_util

Fix RegisterChromeBrowser definition and usage in shell_util

BUG=122756
TEST=None


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

[gab, 2012-06-08 15:06:41]

Robert, can you take a quick look to alleviate Greg's review burden :).

This is one a function change :) to clean up after discussion at:
https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/u...

[grt (UTC plus 2), 2012-06-08 15:17:05]

I had a quick look at this, and although the code change is good, the
changes to the comment aren't correct given the way register protocol
handler works. I can provide detail when I'm not typing on my phone.
On Jun 8, 2012 11:06 AM, <gab@chromium.org> wrote:

> Robert, can you take a quick look to alleviate Greg's review burden :).
>
> This is one a function change :) to clean up after discussion at:
> https://chromiumcodereview.**appspot.com/10453041/diff/**
>
10003/chrome/installer/util/**shell_util.cc#newcode1052<https://chromiumcodereview.appspot.com/10453041/diff/10003/chrome/installer/util/shell_util.cc#newcode1052>
>
>
http://codereview.chromium.**org/10512026/<http://codereview.chromium.org/105...
>

[grt (UTC plus 2), 2012-06-08 15:42:01]

https://chromiumcodereview.appspot.com/10512026/diff/1/chrome/installer/util/...
File chrome/installer/util/shell_util.h (right):

https://chromiumcodereview.appspot.com/10512026/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.h:250: // all the protocols and file types it
can handle. This method requires write
"all the protocols" is incorrect since registerProtocolHandler will lead to
heretofore unknown protocols being handled.

[gab, 2012-06-08 16:03:10]

Done, see new comment.

Added the referred bug to this CL to point whoever starts this bug to this fact.

Cheers,
Gab

https://chromiumcodereview.appspot.com/10512026/diff/1/chrome/installer/util/...
File chrome/installer/util/shell_util.h (right):

https://chromiumcodereview.appspot.com/10512026/diff/1/chrome/installer/util/...
chrome/installer/util/shell_util.h:250: // all the protocols and file types it
can handle. This method requires write
On 2012/06/08 15:42:01, grt wrote:
> "all the protocols" is incorrect since registerProtocolHandler will lead to
> heretofore unknown protocols being handled.

Adapted the comment to reflect this.

[grt (UTC plus 2), 2012-06-08 18:01:21]

http://codereview.chromium.org/10512026/diff/3002/chrome/installer/util/shell...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/10512026/diff/3002/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:247: // This method first checks if Chrome is
fully registered (as outlined below)
Please begin the comment with what the function does (e.g., "Registers Chrome as
a potential default browser and handler for filetypes and protocols.") and then
follow with the details.  This gives the reader the most important info
up-front.

http://codereview.chromium.org/10512026/diff/3002/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:252: // TODO(gab): This should be
ShellUtil::kBrowserProtocolAssociations (i.e. not
Avoid implementation details like this in doc comments; keep it to what the
caller really needs to know.  References like this to the implementation details
means that this documentation comment will either require future maintenance or
become stale.  The reader will know where to find the source if they're curious
(the corresponding .cc file), so I don't see an upside to putting this here. 
See the "Function Comments" section of the Google C++ style guide for more.

[gab, 2012-06-11 16:19:55]

Done.

Cheers,
Gab

http://codereview.chromium.org/10512026/diff/3002/chrome/installer/util/shell...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/10512026/diff/3002/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:247: // This method first checks if Chrome is
fully registered (as outlined below)
On 2012/06/08 18:01:21, grt wrote:
> Please begin the comment with what the function does (e.g., "Registers Chrome
as
> a potential default browser and handler for filetypes and protocols.") and
then
> follow with the details.  This gives the reader the most important info
> up-front.

Done.

http://codereview.chromium.org/10512026/diff/3002/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:252: // TODO(gab): This should be
ShellUtil::kBrowserProtocolAssociations (i.e. not
On 2012/06/08 18:01:21, grt wrote:
> Avoid implementation details like this in doc comments; keep it to what the
> caller really needs to know.  References like this to the implementation
details
> means that this documentation comment will either require future maintenance
or
> become stale.  The reader will know where to find the source if they're
curious
> (the corresponding .cc file), so I don't see an upside to putting this here. 
> See the "Function Comments" section of the Google C++ style guide for more.

Done.

[grt (UTC plus 2), 2012-06-11 17:57:38]

lgtm w/ nits

http://codereview.chromium.org/10512026/diff/8001/chrome/installer/util/shell...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/10512026/diff/8001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:267: //   registered to run when  the user
clicks on an http link or an html file.
nit: "when   the" -> "when the"

http://codereview.chromium.org/10512026/diff/8001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:269: // |elevate_if_not_admin| should only be
true when following a user action
merge this in with the documentation for |elevate_if_not_admin| on line 277

[gab, 2012-06-11 19:38:11]

Thanks, done.

Cheers,
Gab

http://codereview.chromium.org/10512026/diff/8001/chrome/installer/util/shell...
File chrome/installer/util/shell_util.h (right):

http://codereview.chromium.org/10512026/diff/8001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:267: //   registered to run when  the user
clicks on an http link or an html file.
On 2012/06/11 17:57:38, grt wrote:
> nit: "when   the" -> "when the"

Done.

http://codereview.chromium.org/10512026/diff/8001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:269: // |elevate_if_not_admin| should only be
true when following a user action
On 2012/06/11 17:57:38, grt wrote:
> merge this in with the documentation for |elevate_if_not_admin| on line 277

Ah good catch! Done.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/10512026/6004

[commit-bot: I haz the power, 2012-06-11 20:47:27]

Change committed as 141488