Now command line flag does not beat Policy

If DefaultBrowserSettingEnabled set to false and there is the command line
flag that makes chrome default browser, the flag beats Policy. This is wrong
behavior. Thus added the condition that checks policy before set chrome as
a default browser.

BUG=361527
Committed: https://crrev.com/86af4462b98b956599232b6c1e89ada0ccbaac45
Cr-Commit-Position: refs/heads/master@{#324224}

[peletskyi, 2015-02-16 18:33:11]

peletskyi@chromium.org changed reviewers:
+ pastarmovj@google.com

[peletskyi, 2015-02-16 18:33:12]

Hi Julian,
please review my changes.
Thanks,
Oleksandr

[peletskyi, 2015-03-18 15:55:53]

On 2015/02/16 18:33:12, peletskyi wrote:
> Hi Julian,
> please review my changes.
> Thanks,
> Oleksandr

Hi Julian, what about review?

[pastarmovj, 2015-03-18 16:02:16]

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

[pastarmovj, 2015-03-18 16:02:17]

I am basically ok with this and know the context already but I have one
suggestion to make the calling code more aware of what is going on.

https://codereview.chromium.org/927163002/diff/1/chrome/browser/chrome_browse...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/927163002/diff/1/chrome/browser/chrome_browse...
chrome/browser/chrome_browser_main.cc:1155: return
static_cast<int>(chrome::RESULT_CODE_SHELL_INTEGRATION_FAILED);
Any chance you can add a new error result code here to distinguish between
failure reasons. Especially if this error is then propagated to the user in one
way or another even if only through logging.

[peletskyi, 2015-03-18 17:58:40]

On 2015/03/18 16:02:17, pastarmovj wrote:
> I am basically ok with this and know the context already but I have one
> suggestion to make the calling code more aware of what is going on.
> 
>
https://codereview.chromium.org/927163002/diff/1/chrome/browser/chrome_browse...
> File chrome/browser/chrome_browser_main.cc (right):
> 
>
https://codereview.chromium.org/927163002/diff/1/chrome/browser/chrome_browse...
> chrome/browser/chrome_browser_main.cc:1155: return
> static_cast<int>(chrome::RESULT_CODE_SHELL_INTEGRATION_FAILED);
> Any chance you can add a new error result code here to distinguish between
> failure reasons. Especially if this error is then propagated to the user in
one
> way or another even if only through logging.

PTAL

[pastarmovj, 2015-03-19 09:49:28]

LGTM with one last suggestion. Beware that I am not an owner so you will need
someone else to give you the final approval though.

https://codereview.chromium.org/927163002/diff/20001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/927163002/diff/20001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:1163: if (!is_managed ||
g_browser_process->local_state()->GetBoolean(
One last suggestion i got is to reverse the if condition here to be 
if (is_managed && !value)
  return DISALLOWED...
return ShellIntegr...

It makes the logic clearer imho.

[peletskyi, 2015-03-20 09:57:55]

The CQ bit was checked by peletskyi@chromium.org

[peletskyi, 2015-03-20 09:57:56]

The CQ bit was unchecked by peletskyi@chromium.org

[peletskyi, 2015-03-20 10:10:37]

peletskyi@chromium.org changed reviewers:
+ jochen@chromium.org
- pastarmovj@google.com

[peletskyi, 2015-03-20 10:10:38]

Jochen,
please take a look at these small changes.

https://codereview.chromium.org/927163002/diff/20001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/927163002/diff/20001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:1163: if (!is_managed ||
g_browser_process->local_state()->GetBoolean(
On 2015/03/19 09:49:28, pastarmovj wrote:
> One last suggestion i got is to reverse the if condition here to be 
> if (is_managed && !value)
>   return DISALLOWED...
> return ShellIntegr...
> 
> It makes the logic clearer imho.

Done.

[pastarmovj, 2015-03-20 10:57:04]

https://codereview.chromium.org/927163002/diff/40001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/927163002/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:1162:
prefs::kDefaultBrowserSettingEnabled);
nit: indentation.

https://codereview.chromium.org/927163002/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:1164:
prefs::kDefaultBrowserSettingEnabled)) {
nit: indentation.

[peletskyi, 2015-03-20 11:02:02]

https://codereview.chromium.org/927163002/diff/40001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/927163002/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:1162:
prefs::kDefaultBrowserSettingEnabled);
On 2015/03/20 10:57:04, pastarmovj wrote:
> nit: indentation.

Done.

https://codereview.chromium.org/927163002/diff/40001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:1164:
prefs::kDefaultBrowserSettingEnabled)) {
On 2015/03/20 10:57:04, pastarmovj wrote:
> nit: indentation.

Done.

[jochen (gone - plz use gerrit), 2015-03-20 14:49:14]

i don't get how the code relates to the CL description. It looks like policy
wins?

can we have a test for this?

https://codereview.chromium.org/927163002/diff/60001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/927163002/diff/60001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:1169:
static_cast<int>(content::RESULT_CODE_NORMAL_EXIT) :
unrelated change?

[peletskyi, 2015-03-24 13:50:52]

On 2015/03/20 14:49:14, jochen wrote:
> i don't get how the code relates to the CL description. It looks like policy
> wins?
> 
> can we have a test for this?
> 
>
https://codereview.chromium.org/927163002/diff/60001/chrome/browser/chrome_br...
> File chrome/browser/chrome_browser_main.cc (right):
> 
>
https://codereview.chromium.org/927163002/diff/60001/chrome/browser/chrome_br...
> chrome/browser/chrome_browser_main.cc:1169:
> static_cast<int>(content::RESULT_CODE_NORMAL_EXIT) :
> unrelated change?

I've changed the description. Actually the behavior is correct.
Added a browser test.

[peletskyi, 2015-03-24 14:00:26]

peletskyi@chromium.org changed reviewers:
+ pneubeck@chromium.org

[peletskyi, 2015-03-24 14:00:27]

Please review my patch :)

https://codereview.chromium.org/927163002/diff/60001/chrome/browser/chrome_br...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/927163002/diff/60001/chrome/browser/chrome_br...
chrome/browser/chrome_browser_main.cc:1169:
static_cast<int>(content::RESULT_CODE_NORMAL_EXIT) :
On 2015/03/20 14:49:14, jochen wrote:
> unrelated change?

Done.

[jochen (gone - plz use gerrit), 2015-03-25 12:12:35]

can you please update the CL description to have a <80c summary in the first
line?

code changes lgtm

[peletskyi, 2015-04-01 09:07:31]

peletskyi@chromium.org changed reviewers:
+ atwilson@chromium.org

[peletskyi, 2015-04-01 09:07:32]

Drew, can you take a look?

[Andrew T Wilson (Slow), 2015-04-01 09:10:49]

Change the issue description to remove the text "Fix for the bug 361527:" since
that's already covered by the BUG= line.

LGTM

[peletskyi, 2015-04-01 09:20:40]

The CQ bit was checked by peletskyi@chromium.org

[peletskyi, 2015-04-01 09:20:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/927163002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-04-01 09:21:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/927163002/100001

[commit-bot: I haz the power, 2015-04-01 11:02:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-01 11:02:40]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[peletskyi, 2015-04-08 13:42:24]

The CQ bit was checked by peletskyi@chromium.org

[peletskyi, 2015-04-08 13:42:25]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
pastarmovj@chromium.org, atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/927163002/#ps120001
(title: "Fixed test for chromeos and rebase")

[commit-bot: I haz the power, 2015-04-08 13:43:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/927163002/120001

[commit-bot: I haz the power, 2015-04-08 14:13:08]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-04-08 14:15:23]

Patchset 7 (id:??) landed as
https://crrev.com/86af4462b98b956599232b6c1e89ada0ccbaac45
Cr-Commit-Position: refs/heads/master@{#324224}