Fix the first_run regression issue related to skip_first_run_ui setting processing.

Fix the first_run regression issue related to skip_first_run_ui setting processing.


BUG=132353
TEST=First run bubble should appear if skip_first_run_ui is set to false.

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

[msw, 2012-07-10 01:51:21]

[+cc relevant people]

Please separate the early-return logic (based on the presence of the
skip-first-run-ui flag) from the actual determination and use of the flag's
value. I believe we should continue parsing the master prefs regardless of
skip-first-run-ui's value (so long as a value is specified).

http://codereview.chromium.org/10756023/diff/1/chrome/browser/first_run/first...
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/10756023/diff/1/chrome/browser/first_run/first...
chrome/browser/first_run/first_run.cc:269: bool
NotSkipFirstRunUI(installer::MasterPreferences* install_prefs) {
I dislike this name, please leave it SkipFirstRunUI.

http://codereview.chromium.org/10756023/diff/1/chrome/browser/first_run/first...
chrome/browser/first_run/first_run.cc:272: // Note we are skipping all other
master preferences if skip-first-run-ui
This note/comment belong with the code that actually returns early if the value
isn't specified. Or, (if you prefer) you can make a utility function here like
"bool HasMasterPrefs()" that just returns whether or not this bool is specified
(presuming that we still intend to enforce the behavior described here), and put
the comment there.

http://codereview.chromium.org/10756023/diff/1/chrome/browser/first_run/first...
chrome/browser/first_run/first_run.cc:275: return (!install_prefs->GetBool(
In keeping the original name, this function can just do:
bool value = false;             (returns false by default)
install_prefs->GetBool(...);    (ignore the read success/fail)
return value;

http://codereview.chromium.org/10756023/diff/1/chrome/browser/first_run/first...
File chrome/browser/first_run/first_run_internal.h (right):

http://codereview.chromium.org/10756023/diff/1/chrome/browser/first_run/first...
chrome/browser/first_run/first_run_internal.h:69: bool
NotSkipFirstRunUI(installer::MasterPreferences* install_prefs);
Restore the original name and comment.

[Ben Goodger (Google), 2012-07-10 15:07:09]

agreed the old name was better

[jennyz, 2012-07-10 16:07:16]

http://codereview.chromium.org/10756023/diff/1/chrome/browser/first_run/first...
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/10756023/diff/1/chrome/browser/first_run/first...
chrome/browser/first_run/first_run.cc:269: bool
NotSkipFirstRunUI(installer::MasterPreferences* install_prefs) {
On 2012/07/10 01:51:21, msw wrote:
> I dislike this name, please leave it SkipFirstRunUI.

Done.

http://codereview.chromium.org/10756023/diff/1/chrome/browser/first_run/first...
chrome/browser/first_run/first_run.cc:272: // Note we are skipping all other
master preferences if skip-first-run-ui
On 2012/07/10 01:51:21, msw wrote:
> This note/comment belong with the code that actually returns early if the
value
> isn't specified. Or, (if you prefer) you can make a utility function here like
> "bool HasMasterPrefs()" that just returns whether or not this bool is
specified
> (presuming that we still intend to enforce the behavior described here), and
put
> the comment there.

Done.

http://codereview.chromium.org/10756023/diff/1/chrome/browser/first_run/first...
chrome/browser/first_run/first_run.cc:275: return (!install_prefs->GetBool(
On 2012/07/10 01:51:21, msw wrote:
> In keeping the original name, this function can just do:
> bool value = false;             (returns false by default)
> install_prefs->GetBool(...);    (ignore the read success/fail)
> return value;

Done.

http://codereview.chromium.org/10756023/diff/1/chrome/browser/first_run/first...
File chrome/browser/first_run/first_run_internal.h (right):

http://codereview.chromium.org/10756023/diff/1/chrome/browser/first_run/first...
chrome/browser/first_run/first_run_internal.h:69: bool
NotSkipFirstRunUI(installer::MasterPreferences* install_prefs);
On 2012/07/10 01:51:21, msw wrote:
> Restore the original name and comment.

Done.

[msw, 2012-07-10 17:31:46]

http://codereview.chromium.org/10756023/diff/7001/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run_posix.cc (right):

http://codereview.chromium.org/10756023/diff/7001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run_posix.cc:154: if
(!internal::SkipFirstRunUI(install_prefs.get()))
We probably shouldn't ignore the rest of the master prefs if skip_first_run_ui
is actually specified as false. The code here (and in first_run_win.cc) should
be something like the following (feel free to remove |internal::SkipFirstRunUI|
or add a separate |internal::HasValidMasterPrefs| that returns true iff
skip_first_run_ui is specified):

bool skip_first_run_ui = false;
// Ignore the remaining master prefs if skip-first-run-ui is not specified.
if
(!install_prefs->GetBool(installer::master_preferences::kDistroSkipFirstRunPref,
&skip_first_run_ui))
  return true;

// From here...
// bubble anyway...
SetShowFirstRunBubblePref(skip_first_run_ui);

[jennyz, 2012-07-10 18:42:52]

http://codereview.chromium.org/10756023/diff/7001/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run_posix.cc (right):

http://codereview.chromium.org/10756023/diff/7001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run_posix.cc:154: if
(!internal::SkipFirstRunUI(install_prefs.get()))
On 2012/07/10 17:31:46, msw wrote:
> We probably shouldn't ignore the rest of the master prefs if skip_first_run_ui
> is actually specified as false. The code here (and in first_run_win.cc) should
> be something like the following (feel free to remove
|internal::SkipFirstRunUI|
> or add a separate |internal::HasValidMasterPrefs| that returns true iff
> skip_first_run_ui is specified):
> 
> bool skip_first_run_ui = false;
> // Ignore the remaining master prefs if skip-first-run-ui is not specified.
> if
>
(!install_prefs->GetBool(installer::master_preferences::kDistroSkipFirstRunPref,
> &skip_first_run_ui))
>   return true;
> 
> // From here...
> // bubble anyway...
> SetShowFirstRunBubblePref(skip_first_run_ui);
For the above change you proposed, I am a little concerned
ProcessMasterPreferences will return false in the case skip_first_run_ui is set
to false, which should return true originally. This will affect the
fist_run_ui_bypass_ value in the calling code of chrome_browser_main.cc, and
have an impact the later code for whether to call first_run::AutoImport. Does
this matter for the first_run flow?

[msw, 2012-07-10 19:33:19]

This is a good fix for the original regression, LGTM.
My earlier comment would have altered the original behavior.
With verification, this looks safe to merge to M21.

That said, this code desperately needs an owner, proper test coverage, and
clarification (through comments and/or refactoring) to be more maintainable
going forward.

http://codereview.chromium.org/10756023/diff/7001/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run_posix.cc (right):

http://codereview.chromium.org/10756023/diff/7001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run_posix.cc:154: if
(!internal::SkipFirstRunUI(install_prefs.get()))
Ah, I see what you mean. This code is horribly convoluted and not maintainable,
but this Patch Set does restore the original logic before the regression in
crrev.com/122023 (returning true here if skip_first_run_ui is not found or if
its value is false).

[commit-bot: I haz the power, 2012-07-10 20:21:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/10756023/7001

[commit-bot: I haz the power, 2012-07-10 22:26:26]

Try job failure for 10756023-7001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...