[uber] Redoing the homepage selection UI

[uber] Redoing the homepage selection UI

BUG=111139
TEST=SessionStartupPrefTest.HomePageMigration

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

[Tyler Breisacher (Chromium), 2012-01-29 21:02:21]

Still kind of a work in progress, mainly because I'm not sure how to do the
migration from "open the homepage on startup" to "on startup, open the following
pages: <homepage>" cleanly. But I wanted to get this uploaded ASAP so I can
start getting some feedback.

[Tyler Breisacher (Chromium), 2012-01-30 21:58:24]

Okay, I have something in place now for migrating the startup preference. There
may be a "cleaner" way to do this though.

[csilv, 2012-02-01 20:37:29]

This is not a complete review, but I think i've covered the most important items
to move forward.

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/app/generated...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/app/generated...
chrome/app/generated_resources.grd:8077: Home Page:
You have the title-case and non-title case string switched.  However, this
string should always be non-titlecase, so remove it from this condition block
(along with the title case version below).

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/app/generated...
chrome/app/generated_resources.grd:8080: Home Page:
ditto

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
File chrome/browser/prefs/session_startup_pref.cc (right):

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.cc:33: const int kPrefValueHomePage =
0;
Let's save this longer explanation for where the prefs migration is done.  All
we need here is a // Deprecated command at the end of line 33.

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.cc:41: case
SessionStartupPref::HOMEPAGE: return kPrefValueHomePage;
This line is unneeded since we won't ever write this value.

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.cc:60: void SetNewUrlList(PrefService
* prefs) {
get rid of space before '*'

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.cc:61: ListValue* new_url_pref_list =
new ListValue();
This leaks.  I would recommend instead:
  ListValue new_url_pref_list;

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.cc:63:
new_url_pref_list->Append(static_cast<Value*>(homePage));
I don't think you need the static_cast here

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.cc:65:
prefs->Set(prefs::kURLsToRestoreOnStartup, *const_list);
kill line 64, change 64 to:
  prefs->Set(prefs::kURLsToRestoreOnStartup, new_url_pref_list);

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.cc:67: 
After making the above changes, the function may be trivial enough to just
incorporate into GetStartupPref()

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
File chrome/browser/prefs/session_startup_pref.h (right):

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.h:24: HOMEPAGE,
I recommend keeping the enum the same as it was.  the 'DEFAULT' value will
continue to exist, but the new meaning will be to show the new tab page.  See
comments in implementation for more details.

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/resou...
File chrome/browser/resources/options2/browser_options.html (right):

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/resou...
chrome/browser/resources/options2/browser_options.html:109: <option
value="url"></option>
I think we need to build this menu dynamically.  The url option may not exist if
it has not been set by the user.

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/resou...
File chrome/browser/resources/options2/home_page_overlay.js (right):

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/resou...
chrome/browser/resources/options2/home_page_overlay.js:56: 
My preference is to not do this... and instead let browserOptions listen for
changes to the homepage preference and adjust accordingly.

[Tyler Breisacher (Chromium), 2012-02-01 21:49:38]

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/app/generated...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/app/generated...
chrome/app/generated_resources.grd:8077: Home Page:
On 2012/02/01 20:37:29, csilv wrote:
> You have the title-case and non-title case string switched.  However, this
> string should always be non-titlecase, so remove it from this condition block
> (along with the title case version below).

Done.

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/app/generated...
chrome/app/generated_resources.grd:8080: Home Page:
On 2012/02/01 20:37:29, csilv wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
File chrome/browser/prefs/session_startup_pref.cc (right):

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.cc:33: const int kPrefValueHomePage =
0;
On 2012/02/01 20:37:29, csilv wrote:
> Let's save this longer explanation for where the prefs migration is done.  All
> we need here is a // Deprecated command at the end of line 33.

Done.

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.cc:41: case
SessionStartupPref::HOMEPAGE: return kPrefValueHomePage;
On 2012/02/01 20:37:29, csilv wrote:
> This line is unneeded since we won't ever write this value.

Done.

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.cc:60: void SetNewUrlList(PrefService
* prefs) {
On 2012/02/01 20:37:29, csilv wrote:
> get rid of space before '*'

Done.

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.cc:61: ListValue* new_url_pref_list =
new ListValue();
On 2012/02/01 20:37:29, csilv wrote:
> This leaks.  I would recommend instead:
>   ListValue new_url_pref_list;

Done.

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.cc:63:
new_url_pref_list->Append(static_cast<Value*>(homePage));
On 2012/02/01 20:37:29, csilv wrote:
> I don't think you need the static_cast here

Thanks, I think I was misreading the compiler errors. Done.

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.cc:65:
prefs->Set(prefs::kURLsToRestoreOnStartup, *const_list);
On 2012/02/01 20:37:29, csilv wrote:
> kill line 64, change 64 to:
>   prefs->Set(prefs::kURLsToRestoreOnStartup, new_url_pref_list);

Done.

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
File chrome/browser/prefs/session_startup_pref.h (right):

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/prefs...
chrome/browser/prefs/session_startup_pref.h:24: HOMEPAGE,
On 2012/02/01 20:37:29, csilv wrote:
> I recommend keeping the enum the same as it was.  the 'DEFAULT' value will
> continue to exist, but the new meaning will be to show the new tab page.  See
> comments in implementation for more details.

Done.

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/resou...
File chrome/browser/resources/options2/browser_options.html (right):

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/resou...
chrome/browser/resources/options2/browser_options.html:109: <option
value="url"></option>
On 2012/02/01 20:37:29, csilv wrote:
> I think we need to build this menu dynamically.  The url option may not exist
if
> it has not been set by the user.

Currently, it is automatically set to google.com

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/resou...
File chrome/browser/resources/options2/home_page_overlay.js (right):

https://chromiumcodereview.appspot.com/9296038/diff/8013/chrome/browser/resou...
chrome/browser/resources/options2/home_page_overlay.js:56: 
On 2012/02/01 20:37:29, csilv wrote:
> My preference is to not do this... and instead let browserOptions listen for
> changes to the homepage preference and adjust accordingly.

If the user cancels then the preference doesn't change, so that doesn't quite
work, I think. For the OK button, it does, but we need to explicitly change the
prefs as in patch set 6.

[csilv, 2012-02-02 01:36:40]

https://chromiumcodereview.appspot.com/9296038/diff/20001/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/9296038/diff/20001/chrome/app/generate...
chrome/app/generated_resources.grd:8104: Home Page:
Move this one out of the title case condition block.

https://chromiumcodereview.appspot.com/9296038/diff/20001/chrome/browser/reso...
File chrome/browser/resources/options2/home_page_overlay.js (right):

https://chromiumcodereview.appspot.com/9296038/diff/20001/chrome/browser/reso...
chrome/browser/resources/options2/home_page_overlay.js:57: 
We should probably change SettingsDialog so that there are public methods that
can be overridden.  Something like willCommit(), willCancel().

[Tyler Breisacher (Chromium), 2012-02-02 18:52:44]

https://chromiumcodereview.appspot.com/9296038/diff/20001/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/9296038/diff/20001/chrome/app/generate...
chrome/app/generated_resources.grd:8104: Home Page:
On 2012/02/02 01:36:40, csilv wrote:
> Move this one out of the title case condition block.

Done.

https://chromiumcodereview.appspot.com/9296038/diff/20001/chrome/browser/reso...
File chrome/browser/resources/options2/home_page_overlay.js (right):

https://chromiumcodereview.appspot.com/9296038/diff/20001/chrome/browser/reso...
chrome/browser/resources/options2/home_page_overlay.js:57: 
On 2012/02/02 01:36:40, csilv wrote:
> We should probably change SettingsDialog so that there are public methods that
> can be overridden.  Something like willCommit(), willCancel().

Done.

[csilv, 2012-02-02 19:32:26]

https://chromiumcodereview.appspot.com/9296038/diff/16003/chrome/browser/pref...
File chrome/browser/prefs/session_startup_pref.cc (right):

https://chromiumcodereview.appspot.com/9296038/diff/16003/chrome/browser/pref...
chrome/browser/prefs/session_startup_pref.cc:28: const int kPrefValueHomePage =
0; // Deprecated
nit: two spaces before //

https://chromiumcodereview.appspot.com/9296038/diff/16003/chrome/browser/pref...
chrome/browser/prefs/session_startup_pref.cc:56: StringValue* homePage = new
StringValue(prefs->GetString(prefs::kHomePage));
nit: homePage -> home_page

https://chromiumcodereview.appspot.com/9296038/diff/16003/chrome/browser/reso...
File chrome/browser/resources/options2/browser_options.html (right):

https://chromiumcodereview.appspot.com/9296038/diff/16003/chrome/browser/reso...
chrome/browser/resources/options2/browser_options.html:105:
i18n-content="homePage"></label>
nit: 'home-page' vs 'homepage' in ids

https://chromiumcodereview.appspot.com/9296038/diff/16003/chrome/browser/reso...
File chrome/browser/resources/options2/options.js (right):

https://chromiumcodereview.appspot.com/9296038/diff/16003/chrome/browser/reso...
chrome/browser/resources/options2/options.js:178: [$('homepageURL')]);
This should point to the element that invokes this overlay, which I believe is
$('homepage-select')

[Tyler Breisacher (Chromium), 2012-02-02 19:52:44]

https://chromiumcodereview.appspot.com/9296038/diff/16003/chrome/browser/pref...
File chrome/browser/prefs/session_startup_pref.cc (right):

https://chromiumcodereview.appspot.com/9296038/diff/16003/chrome/browser/pref...
chrome/browser/prefs/session_startup_pref.cc:28: const int kPrefValueHomePage =
0; // Deprecated
On 2012/02/02 19:32:26, csilv wrote:
> nit: two spaces before //

Done.

https://chromiumcodereview.appspot.com/9296038/diff/16003/chrome/browser/pref...
chrome/browser/prefs/session_startup_pref.cc:56: StringValue* homePage = new
StringValue(prefs->GetString(prefs::kHomePage));
On 2012/02/02 19:32:26, csilv wrote:
> nit: homePage -> home_page

Done.

https://chromiumcodereview.appspot.com/9296038/diff/16003/chrome/browser/reso...
File chrome/browser/resources/options2/browser_options.html (right):

https://chromiumcodereview.appspot.com/9296038/diff/16003/chrome/browser/reso...
chrome/browser/resources/options2/browser_options.html:105:
i18n-content="homePage"></label>
On 2012/02/02 19:32:26, csilv wrote:
> nit: 'home-page' vs 'homepage' in ids

Done.

https://chromiumcodereview.appspot.com/9296038/diff/16003/chrome/browser/reso...
File chrome/browser/resources/options2/options.js (right):

https://chromiumcodereview.appspot.com/9296038/diff/16003/chrome/browser/reso...
chrome/browser/resources/options2/options.js:178: [$('homepageURL')]);
On 2012/02/02 19:32:26, csilv wrote:
> This should point to the element that invokes this overlay, which I believe is
> $('homepage-select')

Done.

[csilv, 2012-02-02 21:13:07]

lgtm

[csilv, 2012-02-02 21:15:12]

On 2012/02/02 21:13:07, csilv wrote:
> lgtm

Sorry, one more thing... please take a look at session_startup_pref_unittest.cc
to verify that these changes don't break those tests and consider whether there
are any tests worth adding.

[Tyler Breisacher (Chromium), 2012-02-02 23:12:12]

The ones that were already there are unaffected, and I added one more to test
the migration.

There's also a spacing/CSS issue: The <select> bumps right up against the
buttons above it. There are lots of easy fixes, but I don't know what's the
cleanest (add a margin to the select, add a margin to the buttons, etc.) so let
me know what you think.

[csilv, 2012-02-03 01:04:33]

On 2012/02/02 23:12:12, Tyler Breisacher wrote:
> The ones that were already there are unaffected, and I added one more to test
> the migration.
> 
> There's also a spacing/CSS issue: The <select> bumps right up against the
> buttons above it. There are lots of easy fixes, but I don't know what's the
> cleanest (add a margin to the select, add a margin to the buttons, etc.) so
let
> me know what you think.

The added test looks great!

On the spacing issue, for now I suggest adding some margin to a specific
element, perhaps the div that contains the buttons.  The real solution involves
coming up a complete set of spacing rules for uber UI settings and then trying
to implement those without having to add CSS rules for a bunch of specific
elements. That work is part of http://crbug.com/110760.

[Tyler Breisacher (Chromium), 2012-02-03 01:57:41]

https://chromiumcodereview.appspot.com/9296038/diff/13016/chrome/browser/reso...
File chrome/browser/resources/options2/browser_options.html (right):

https://chromiumcodereview.appspot.com/9296038/diff/13016/chrome/browser/reso...
chrome/browser/resources/options2/browser_options.html:55:
metric="Options_Startup_Homepage">
Sorry, noticed one more thing. This metric no longer makes sense. Can we just
switch it to say "Options_Startup_Newtab" or does that string have to be added
to a list or something?

[csilv, 2012-02-06 19:06:20]

On 2012/02/03 01:57:41, Tyler Breisacher wrote:
>
https://chromiumcodereview.appspot.com/9296038/diff/13016/chrome/browser/reso...
> File chrome/browser/resources/options2/browser_options.html (right):
> 
>
https://chromiumcodereview.appspot.com/9296038/diff/13016/chrome/browser/reso...
> chrome/browser/resources/options2/browser_options.html:55:
> metric="Options_Startup_Homepage">
> Sorry, noticed one more thing. This metric no longer makes sense. Can we just
> switch it to say "Options_Startup_Newtab" or does that string have to be added
> to a list or something?

No need to list it, change Options_Startup_Homepage sounds perfect.

[commit-bot: I haz the power, 2012-02-06 19:40:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbreisacher@chromium.org/9296038/13016

[commit-bot: I haz the power, 2012-02-06 19:40:20]

Presubmit check for 9296038-13016 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/ui/browser_init.cc

Presubmit checks took 1.9s to calculate.

[Tyler Breisacher (Chromium), 2012-02-06 20:03:42]

Adding estade for OWNERS review on webui and ben for OWNERS review on
browser_init.cc

[Ben Goodger (Google), 2012-02-06 20:14:11]

browser_init LGTM

[Evan Stade, 2012-02-06 20:21:05]

lgtm

[commit-bot: I haz the power, 2012-02-06 21:20:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbreisacher@chromium.org/9296038/28003

[commit-bot: I haz the power, 2012-02-06 21:20:32]

Can't process patch for file
chrome/browser/resources/options2/browser_options.html.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2012-02-06 21:23:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbreisacher@chromium.org/9296038/28003

[commit-bot: I haz the power, 2012-02-06 21:23:23]

Can't process patch for file
chrome/browser/resources/options2/browser_options.html.
File's status is None, patchset upload is incomplete.

[michaeln, 2012-02-07 20:28:15]

Tyler, i reverted this due to a handful of ui_test failures that look
attributable to it.

http://build.chromium.org/p/chromium.memory/builders/ASAN%20Tests%20%281%29/b...

revert revision is 120817

If this turns out not to be the culprit, i'll revert the revert.

[Ben Goodger (Google), 2012-02-15 22:12:48]

lgtm