Refactoring the master_preferences functions....

Refactoring the master_preferences functions.

* Now there's a class named MasterPreferences that owns
  the json dictionary and supports methods that previously
  were global.

* There's now a global object for the current process'
  master preference object, which avoids parsing the
  preference file multiple times.

* Refactored the tests to be more data driven

TEST=Run the installer util unit tests.  Set the filter to
  *MasterPref* if you only want to run the tests affected by
  this change.
BUG=none


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

[tommi (sloooow) - chröme, 2010-11-02 21:33:48]

hey robert - feel free to suggest additional reviewers.

[grt (UTC plus 2), 2010-11-03 13:44:22]

Drive-by.

http://codereview.chromium.org/4342001/diff/1/7
File chrome/installer/util/install_util.h (right):

http://codereview.chromium.org/4342001/diff/1/7#newcode125
chrome/installer/util/install_util.h:125: static
installer_util::MasterPreferences&
Return const ref?

http://codereview.chromium.org/4342001/diff/1/8
File chrome/installer/util/master_preferences.cc (right):

http://codereview.chromium.org/4342001/diff/1/8#newcode55
chrome/installer/util/master_preferences.cc:55: DLOG(ERROR) << "Failed to read
master prefs file. ";
Do we have any diagnosability for file IO in release builds?  I can imagine it
being helpful to have the ability to log such failures (i.e., tell a customer to
re-run the installer with some logging flags).

http://codereview.chromium.org/4342001/diff/1/8#newcode93
chrome/installer/util/master_preferences.cc:93: struct
CmdLineSwitchToDistributionSwitch {
Consider making this array static const; this might allow it to end up in the
.rdata section.

http://codereview.chromium.org/4342001/diff/1/8#newcode121
chrome/installer/util/master_preferences.cc:121: std::string name(kDistroDict);
Move this std::string instance outside of the loop and use
name.assign(kDistroDict) here.  This will avoid hitting the ctor/dtor on each
iteration, and quite likely reduce the number of allocations as well.  If you
want to take it a step further, initialize the string with kDistroDict outside
of the loop, and just name.resize(arraysize(kDistroDict) - 1) here.

http://codereview.chromium.org/4342001/diff/1/8#newcode172
chrome/installer/util/master_preferences.cc:172: return
GetNamedList("first_run_tabs", master_dictionary_.get());
Why not kFirstRunTabs (which you probably have to define)?

http://codereview.chromium.org/4342001/diff/1/9
File chrome/installer/util/master_preferences.h (right):

http://codereview.chromium.org/4342001/diff/1/9#newcode145
chrome/installer/util/master_preferences.h:145: bool
GetExtensionsBlock(DictionaryValue** extensions);
Constify.

[tommi (sloooow) - chröme, 2010-11-03 15:57:32]

Thanks for the review grt.  All suggestions incorporated.

http://codereview.chromium.org/4342001/diff/1/7
File chrome/installer/util/install_util.h (right):

http://codereview.chromium.org/4342001/diff/1/7#newcode125
chrome/installer/util/install_util.h:125: static
installer_util::MasterPreferences&
On 2010/11/03 13:44:22, grt wrote:
> Return const ref?

Done.

http://codereview.chromium.org/4342001/diff/1/8
File chrome/installer/util/master_preferences.cc (right):

http://codereview.chromium.org/4342001/diff/1/8#newcode55
chrome/installer/util/master_preferences.cc:55: DLOG(ERROR) << "Failed to read
master prefs file. ";
On 2010/11/03 13:44:22, grt wrote:
> Do we have any diagnosability for file IO in release builds?  I can imagine it
> being helpful to have the ability to log such failures (i.e., tell a customer
to
> re-run the installer with some logging flags).

Changed all three DLOGs to be LOGs and changed them back to being warnings and
not errors since this is an expected case in some tests.

http://codereview.chromium.org/4342001/diff/1/8#newcode93
chrome/installer/util/master_preferences.cc:93: struct
CmdLineSwitchToDistributionSwitch {
On 2010/11/03 13:44:22, grt wrote:
> Consider making this array static const; this might allow it to end up in the
> .rdata section.

Done.

http://codereview.chromium.org/4342001/diff/1/8#newcode121
chrome/installer/util/master_preferences.cc:121: std::string name(kDistroDict);
On 2010/11/03 13:44:22, grt wrote:
> Move this std::string instance outside of the loop and use
> name.assign(kDistroDict) here.  This will avoid hitting the ctor/dtor on each
> iteration, and quite likely reduce the number of allocations as well.  If you
> want to take it a step further, initialize the string with kDistroDict outside
> of the loop, and just name.resize(arraysize(kDistroDict) - 1) here.

/Like!
done!

http://codereview.chromium.org/4342001/diff/1/8#newcode172
chrome/installer/util/master_preferences.cc:172: return
GetNamedList("first_run_tabs", master_dictionary_.get());
On 2010/11/03 13:44:22, grt wrote:
> Why not kFirstRunTabs (which you probably have to define)?

Done.  This is simply how it was before.  Having a constant is better.

http://codereview.chromium.org/4342001/diff/1/9
File chrome/installer/util/master_preferences.h (right):

http://codereview.chromium.org/4342001/diff/1/9#newcode145
chrome/installer/util/master_preferences.h:145: bool
GetExtensionsBlock(DictionaryValue** extensions);
On 2010/11/03 13:44:22, grt wrote:
> Constify.

Done.

[grt (UTC plus 2), 2010-11-03 16:09:39]

LGTM

[robertshield, 2010-11-03 17:07:49]

Mostly nits.

http://codereview.chromium.org/4342001/diff/25001/12002
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/4342001/diff/25001/12002#newcode100
chrome/browser/first_run/first_run.cc:100: if
(!prefs.preferences_read_from_file())
preferences_read_from_file -> read_from_file?

http://codereview.chromium.org/4342001/diff/25001/12006
File chrome/installer/util/install_util.cc (right):

http://codereview.chromium.org/4342001/diff/25001/12006#newcode36
chrome/installer/util/install_util.cc:36: static MasterPreferences
prefs(*CommandLine::ForCurrentProcess());
Can we DCHECK that CommandLine::Init() has been called?

http://codereview.chromium.org/4342001/diff/25001/12009
File chrome/installer/util/master_preferences.h (right):

http://codereview.chromium.org/4342001/diff/25001/12009#newcode104
chrome/installer/util/master_preferences.h:104: // Note that the entries are
usually urls but they don't have to.
to. -> to be.

http://codereview.chromium.org/4342001/diff/25001/12009#newcode107
chrome/installer/util/master_preferences.h:107: // preferences file does not
contain such list the vector is empty.
such list -> such a list

http://codereview.chromium.org/4342001/diff/25001/12009#newcode148
chrome/installer/util/master_preferences.h:148: bool
preferences_read_from_file() const {
mentioned elsewhere, can we rename this to read_from_file()?

[tommi (sloooow) - chröme, 2010-11-03 17:20:34]

all addressed I believe

http://codereview.chromium.org/4342001/diff/25001/12002
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/4342001/diff/25001/12002#newcode100
chrome/browser/first_run/first_run.cc:100: if
(!prefs.preferences_read_from_file())
On 2010/11/03 17:07:50, robertshield wrote:
> preferences_read_from_file -> read_from_file?

Done.

http://codereview.chromium.org/4342001/diff/25001/12006
File chrome/installer/util/install_util.cc (right):

http://codereview.chromium.org/4342001/diff/25001/12006#newcode36
chrome/installer/util/install_util.cc:36: static MasterPreferences
prefs(*CommandLine::ForCurrentProcess());
On 2010/11/03 17:07:50, robertshield wrote:
> Can we DCHECK that CommandLine::Init() has been called

Already done inside CommandLine::ForCurrentProcess()

http://codereview.chromium.org/4342001/diff/25001/12009
File chrome/installer/util/master_preferences.h (right):

http://codereview.chromium.org/4342001/diff/25001/12009#newcode104
chrome/installer/util/master_preferences.h:104: // Note that the entries are
usually urls but they don't have to.
On 2010/11/03 17:07:50, robertshield wrote:
> to. -> to be.

Done.

http://codereview.chromium.org/4342001/diff/25001/12009#newcode107
chrome/installer/util/master_preferences.h:107: // preferences file does not
contain such list the vector is empty.
On 2010/11/03 17:07:50, robertshield wrote:
> such list -> such a list

Done.

http://codereview.chromium.org/4342001/diff/25001/12009#newcode148
chrome/installer/util/master_preferences.h:148: bool
preferences_read_from_file() const {
On 2010/11/03 17:07:50, robertshield wrote:
> mentioned elsewhere, can we rename this to read_from_file()?

Done.

[robertshield, 2010-11-03 17:44:02]

LGTM

On 2010/11/03 17:07:49, robertshield wrote:
> Mostly nits.
> 
> http://codereview.chromium.org/4342001/diff/25001/12002
> File chrome/browser/first_run/first_run.cc (right):
> 
> http://codereview.chromium.org/4342001/diff/25001/12002#newcode100
> chrome/browser/first_run/first_run.cc:100: if
> (!prefs.preferences_read_from_file())
> preferences_read_from_file -> read_from_file?
> 
> http://codereview.chromium.org/4342001/diff/25001/12006
> File chrome/installer/util/install_util.cc (right):
> 
> http://codereview.chromium.org/4342001/diff/25001/12006#newcode36
> chrome/installer/util/install_util.cc:36: static MasterPreferences
> prefs(*CommandLine::ForCurrentProcess());
> Can we DCHECK that CommandLine::Init() has been called?
> 
> http://codereview.chromium.org/4342001/diff/25001/12009
> File chrome/installer/util/master_preferences.h (right):
> 
> http://codereview.chromium.org/4342001/diff/25001/12009#newcode104
> chrome/installer/util/master_preferences.h:104: // Note that the entries are
> usually urls but they don't have to.
> to. -> to be.
> 
> http://codereview.chromium.org/4342001/diff/25001/12009#newcode107
> chrome/installer/util/master_preferences.h:107: // preferences file does not
> contain such list the vector is empty.
> such list -> such a list
> 
> http://codereview.chromium.org/4342001/diff/25001/12009#newcode148
> chrome/installer/util/master_preferences.h:148: bool
> preferences_read_from_file() const {
> mentioned elsewhere, can we rename this to read_from_file()?