Move "default for protocol" probing logic out of shell_integration_win.cc into shell_util.cc.

Move "default for protocol" probing logic out of shell_integration_win.cc into shell_util.cc.

This will allow us to probe for default from the installer (as the installer cannot otherwise call chrome code); this is already the way it is factored to "make Chrome default" and now will be factored the same way for "is Chrome default".

BUG=154806
TEST=Chrome knows whether it's default or not.


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

[grt (UTC plus 2), 2012-10-12 19:59:35]

just passing through...

http://codereview.chromium.org/11127002/diff/4001/chrome/browser/shell_integr...
File chrome/browser/shell_integration_win.cc (right):

http://codereview.chromium.org/11127002/diff/4001/chrome/browser/shell_integr...
chrome/browser/shell_integration_win.cc:249: return
ShellIntegration::UNKNOWN_DEFAULT_WEB_CLIENT;
DCHECK for the UNKNOWN ShellUtil::DefaultState value here

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

http://codereview.chromium.org/11127002/diff/4001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:37: NOT_DEFAULT = 0,
nit: remove " = 0"

http://codereview.chromium.org/11127002/diff/4001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:39: UNKNOWN_DEFAULT = -1
does this need to be -1?  why not 2?

[gab, 2012-10-12 20:35:11]

See comments below.

http://codereview.chromium.org/11127002/diff/4001/chrome/browser/shell_integr...
File chrome/browser/shell_integration_win.cc (right):

http://codereview.chromium.org/11127002/diff/4001/chrome/browser/shell_integr...
chrome/browser/shell_integration_win.cc:249: return
ShellIntegration::UNKNOWN_DEFAULT_WEB_CLIENT;
On 2012/10/12 19:59:35, grt wrote:
> DCHECK for the UNKNOWN ShellUtil::DefaultState value here

Done.

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

http://codereview.chromium.org/11127002/diff/4001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:37: NOT_DEFAULT = 0,
On 2012/10/12 19:59:35, grt wrote:
> nit: remove " = 0"

This mimics the enum defined in shell_integration.h, I'd rather keep them as
similar as possible.

http://codereview.chromium.org/11127002/diff/4001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:39: UNKNOWN_DEFAULT = -1
On 2012/10/12 19:59:35, grt wrote:
> does this need to be -1?  why not 2?

Same justification as above.

[grt (UTC plus 2), 2012-10-15 13:44:33]

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

http://codereview.chromium.org/11127002/diff/4001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:37: NOT_DEFAULT = 0,
On 2012/10/12 20:35:12, gab wrote:
> On 2012/10/12 19:59:35, grt wrote:
> > nit: remove " = 0"
> 
> This mimics the enum defined in shell_integration.h, I'd rather keep them as
> similar as possible.

I don't think this is necessary since they're independent types (see
GetDefaultWebClientStateFromShellUtilDefaultState).  If you intend for them to
be identical, I suggest you document this with a comment and enforce it with
COMPILE_ASSERT.

http://codereview.chromium.org/11127002/diff/4001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:39: UNKNOWN_DEFAULT = -1
On 2012/10/12 20:35:12, gab wrote:
> On 2012/10/12 19:59:35, grt wrote:
> > does this need to be -1?  why not 2?
> 
> Same justification as above.

Would you at least move this above NOT_DEFAULT and get rid of the " = 0" there? 
It's nonsensical to me to have the values out of order like this.

[robertshield, 2012-10-15 13:58:46]

LGTM, please address Greg's comments first.

[gab, 2012-10-15 14:20:39]

sgtm, done.

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

http://codereview.chromium.org/11127002/diff/4001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:37: NOT_DEFAULT = 0,
On 2012/10/15 13:44:33, grt wrote:
> On 2012/10/12 20:35:12, gab wrote:
> > On 2012/10/12 19:59:35, grt wrote:
> > > nit: remove " = 0"
> > 
> > This mimics the enum defined in shell_integration.h, I'd rather keep them as
> > similar as possible.
> 
> I don't think this is necessary since they're independent types (see
> GetDefaultWebClientStateFromShellUtilDefaultState).  If you intend for them to
> be identical, I suggest you document this with a comment and enforce it with
> COMPILE_ASSERT.

Done.

http://codereview.chromium.org/11127002/diff/4001/chrome/installer/util/shell...
chrome/installer/util/shell_util.h:39: UNKNOWN_DEFAULT = -1
On 2012/10/15 13:44:33, grt wrote:
> On 2012/10/12 20:35:12, gab wrote:
> > On 2012/10/12 19:59:35, grt wrote:
> > > does this need to be -1?  why not 2?
> > 
> > Same justification as above.
> 
> Would you at least move this above NOT_DEFAULT and get rid of the " = 0"
there? 
> It's nonsensical to me to have the values out of order like this.

Done.

[grt (UTC plus 2), 2012-10-15 14:25:24]

lgtm

[commit-bot: I haz the power, 2012-10-15 14:29:59]

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

[commit-bot: I haz the power, 2012-10-15 14:30:02]

Presubmit check for 11127002-16001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

[gab, 2012-10-15 14:33:04]

+jhawkins for chrome/browser/shell_integration_win.cc OWNER approval.

Thanks!
Gab

[gab, 2012-10-15 16:54:36]

+Nico as jhawkins doesn't appear to be in this morning and this is the first CL
of a set of 3, 2 of which need to be merged in M23 asap.

Thanks,
Gab

[Nico, 2012-10-15 17:09:24]

lgtm

I didn't check that the moved code didn't change.

http://codereview.chromium.org/11127002/diff/16001/chrome/browser/shell_integ...
File chrome/browser/shell_integration_win.cc (right):

http://codereview.chromium.org/11127002/diff/16001/chrome/browser/shell_integ...
chrome/browser/shell_integration_win.cc:250: return
ShellIntegration::UNKNOWN_DEFAULT_WEB_CLIENT;
nit: I believe it's preferable to list all cases explicitly and not have a
default branch. msvc should be smart enough to realize that every enum value is
covered and not warn. That way, warning 14062 will catch missing branches when
more values are added to an enum at compile time, instead of at run time.

[gab, 2012-10-15 17:45:33]

Thanks, see comment below.

http://codereview.chromium.org/11127002/diff/16001/chrome/browser/shell_integ...
File chrome/browser/shell_integration_win.cc (right):

http://codereview.chromium.org/11127002/diff/16001/chrome/browser/shell_integ...
chrome/browser/shell_integration_win.cc:250: return
ShellIntegration::UNKNOWN_DEFAULT_WEB_CLIENT;
On 2012/10/15 17:09:24, Nico wrote:
> nit: I believe it's preferable to list all cases explicitly and not have a
> default branch. msvc should be smart enough to realize that every enum value
is
> covered and not warn. That way, warning 14062 will catch missing branches when
> more values are added to an enum at compile time, instead of at run time.

I would have liked that, but this doesn't compile on Windows:

g:\src\component\src\chrome\browser\shell_integration_win.cc(251) : warning
C4715: '`anonymous
namespace'::GetDefaultWebClientStateFromShellUtilDefaultState' : not all control
paths return a value

[commit-bot: I haz the power, 2012-10-15 17:46:27]

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

[Nico, 2012-10-15 17:47:43]

On 2012/10/15 17:45:33, gab wrote:
> Thanks, see comment below.
> 
>
http://codereview.chromium.org/11127002/diff/16001/chrome/browser/shell_integ...
> File chrome/browser/shell_integration_win.cc (right):
> 
>
http://codereview.chromium.org/11127002/diff/16001/chrome/browser/shell_integ...
> chrome/browser/shell_integration_win.cc:250: return
> ShellIntegration::UNKNOWN_DEFAULT_WEB_CLIENT;
> On 2012/10/15 17:09:24, Nico wrote:
> > nit: I believe it's preferable to list all cases explicitly and not have a
> > default branch. msvc should be smart enough to realize that every enum value
> is
> > covered and not warn. That way, warning 14062 will catch missing branches
when
> > more values are added to an enum at compile time, instead of at run time.
> 
> I would have liked that, but this doesn't compile on Windows:
> 
> g:\src\component\src\chrome\browser\shell_integration_win.cc(251) : warning
> C4715: '`anonymous
> namespace'::GetDefaultWebClientStateFromShellUtilDefaultState' : not all
control
> paths return a value

Huh! This pattern is used fairly actively in the llvm code, which builds fine
with MSVC. I wonder what they're doing differently.

[scottmg, 2012-10-15 18:04:23]

http://codereview.chromium.org/11127002/diff/16001/chrome/browser/shell_integ...
File chrome/browser/shell_integration_win.cc (right):

http://codereview.chromium.org/11127002/diff/16001/chrome/browser/shell_integ...
chrome/browser/shell_integration_win.cc:250: return
ShellIntegration::UNKNOWN_DEFAULT_WEB_CLIENT;
On 2012/10/15 17:45:33, gab wrote:
> On 2012/10/15 17:09:24, Nico wrote:
> > nit: I believe it's preferable to list all cases explicitly and not have a
> > default branch. msvc should be smart enough to realize that every enum value
> is
> > covered and not warn. That way, warning 14062 will catch missing branches
when
> > more values are added to an enum at compile time, instead of at run time.
> 
> I would have liked that, but this doesn't compile on Windows:
> 
> g:\src\component\src\chrome\browser\shell_integration_win.cc(251) : warning
> C4715: '`anonymous
> namespace'::GetDefaultWebClientStateFromShellUtilDefaultState' : not all
control
> paths return a value

4062 is off by default even at /W4 (we don't use /Wall because it's very
chatty).

I think 4715 is overly conservative and doesn't assume the enum is in range.
Kind of annoying. I'd guess that clang is probably including an NOTREACHED-style
thing that includes __declspec(noreturn) after the switch? Might be worth
investigating.

[commit-bot: I haz the power, 2012-10-15 20:38:09]

List of reviewers changed. scottmg@chromium.org did a drive-by without LGTM'ing!

[scottmg, 2012-10-15 20:39:16]

On 2012/10/15 20:38:09, I haz the power (commit-bot) wrote:
> List of reviewers changed. mailto:scottmg@chromium.org did a drive-by without
LGTM'ing!

Bah! Sorry. lgtm.

[commit-bot: I haz the power, 2012-10-15 20:39:33]

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

[commit-bot: I haz the power, 2012-10-15 20:39:50]

Change committed as 161948

[M-A Ruel, 2015-10-14 20:08:03]

maruel@chromium.org changed reviewers:
+ maruel@chromium.org

[M-A Ruel, 2015-10-14 20:08:04]

https://chromiumcodereview.appspot.com/11127002/diff/16001/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/11127002/diff/16001/chrome/installer/u...
chrome/installer/util/shell_util.cc:1117: return ShellUtil::UNKNOWN_DEFAULT;
This code is incorrect; it's possible that a path has no short name version when
the path was created with \\?\ prefix. This is now the case with isolated
testing. In this case, it should fallback to the long form.

[gab, 2015-10-14 20:32:07]

https://chromiumcodereview.appspot.com/11127002/diff/16001/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/11127002/diff/16001/chrome/installer/u...
chrome/installer/util/shell_util.cc:1117: return ShellUtil::UNKNOWN_DEFAULT;
On 2015/10/14 20:08:04, M-A Ruel wrote:
> This code is incorrect; it's possible that a path has no short name version
when
> the path was created with \\?\ prefix. This is now the case with isolated
> testing. In this case, it should fallback to the long form.

This CL moved this code from shell_integration_win.cc; so I guess it's been
incorrect forever. Happy to review a fix.

[Mark P, 2015-10-14 22:07:19]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2015-10-14 22:07:19]

https://chromiumcodereview.appspot.com/11127002/diff/16001/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/11127002/diff/16001/chrome/installer/u...
chrome/installer/util/shell_util.cc:1117: return ShellUtil::UNKNOWN_DEFAULT;
On 2015/10/14 20:32:07, gab wrote:
> On 2015/10/14 20:08:04, M-A Ruel wrote:
> > This code is incorrect; it's possible that a path has no short name version
> when
> > the path was created with \\?\ prefix. This is now the case with isolated
> > testing. In this case, it should fallback to the long form.
> 
> This CL moved this code from shell_integration_win.cc; so I guess it's been
> incorrect forever. Happy to review a fix.

Filed bug
https://code.google.com/p/chromium/issues/detail?id=543306
for follow-up.