[uber page] Split up initialization of handlers from initialization of webui pages

[uber page] Split up initialization of handlers from initialization of webui
pages to make re-initialization work as expected. Additionally, ignore some
events that might get us into a wasteful scenario.

BUG=117671
TEST=Session restore or re-clicking same nav item loads settings correctly,
no regressions.
R=csilv,estade,mirandac,kevers

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

[Dan Beam, 2012-03-13 05:58:02]

estade: chrome/browser/resources/*
csilv: chrome/browser/ui/webui/options2/*
tbreisacher & jhawkins: FYI (comments welcome)

[Dan Beam, 2012-03-13 16:44:23]

On 2012/03/13 05:58:02, Dan Beam wrote:
> estade: chrome/browser/resources/*
> csilv: chrome/browser/ui/webui/options2/*
> tbreisacher & jhawkins: FYI (comments welcome)

Cs

[Dan Beam, 2012-03-13 16:46:26]

On 2012/03/13 16:44:23, Dan Beam wrote:
> On 2012/03/13 05:58:02, Dan Beam wrote:
> > estade: chrome/browser/resources/*
> > csilv: chrome/browser/ui/webui/options2/*
> > tbreisacher & jhawkins: FYI (comments welcome)
> 
> Cs

Sorry for spam, on my phone.

csilv: I meant chrome/browser/ui/webui/*

[Evan Stade, 2012-03-13 20:44:49]

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/resource...
File chrome/browser/resources/uber/uber_frame.css (right):

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/resource...
chrome/browser/resources/uber/uber_frame.css:45: pointer-events: none;
is this necessary? I feel that sometimes people like to click the same thing to
force a reload. For example, clicking a website's logo usually leads to the
homepage. If you're already on the homepage, it is still clickable, and still
navigates to the homepage (effectively reloading).

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/options_ui.h (right):

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/options_ui.h:38: // Actually sends down values
after initialization to the webui page.
expand comment to highlight differences between this and initialize()

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/options_ui.h:39: virtual void SendPageValues()
{}
I am not too keen on this name. How about:

RefreshPageState()

UpdatePageState()

etc.

Or, you could change the two functions to be

InitializeHandler/InitializePage instead of Initialize/SendPageValues

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right):

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/options2/browser_options_handler2.cc:812: void
BrowserOptionsHandler::UpdateSearchEngines() {
this function name could probably be improved

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options2/handler_options_handler2.h (right):

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/options2/handler_options_handler2.h:3: // found in the
LICENSE file.
can you add a file comment here? Offhand I don't have any clue what a
HandlerOptionsHandler is

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options2/import_data_handler2.h (right):

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/options2/import_data_handler2.h:58: bool
profiles_loaded_;
rather than adding this, expose source_profiles_loaded_ in ImporterList

[James Hawkins, 2012-03-13 20:46:47]

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/resource...
File chrome/browser/resources/uber/uber_frame.css (right):

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/resource...
chrome/browser/resources/uber/uber_frame.css:30: -webkit-user-select: none;
Why did you make this change?

[csilv, 2012-03-13 20:59:48]

http://codereview.chromium.org/9693032/diff/1/chrome/browser/resources/uber/u...
File chrome/browser/resources/uber/uber_frame.css (right):

http://codereview.chromium.org/9693032/diff/1/chrome/browser/resources/uber/u...
chrome/browser/resources/uber/uber_frame.css:30: -webkit-user-select: none;
On 2012/03/13 20:46:47, James Hawkins wrote:
> Why did you make this change?

FWIW, I like this change. This prevents accidental text selection from clicking
on nav items.  I find this a minor annoyance.

http://codereview.chromium.org/9693032/diff/1/chrome/browser/resources/uber/u...
chrome/browser/resources/uber/uber_frame.css:45: pointer-events: none;
I believe the purpose is to prevent accidental or reduncent page loads. I don't
believe the user would expect this to occur for this use case and there's never
a benefit (unless there's some bug requiting a reload.)

http://codereview.chromium.org/9693032/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/options_ui.h (right):

http://codereview.chromium.org/9693032/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/options_ui.h:38: // Actually sends down values
after initialization to the webui page.
nit: remove the word "Actually".

http://codereview.chromium.org/9693032/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/options_ui.h:39: virtual void SendPageValues()
{}
I had the same reaction to the method name.

I really like Evan's suggestion to use InitializeHandler/InitializePage

http://codereview.chromium.org/9693032/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options2/options_ui2.cc (right):

http://codereview.chromium.org/9693032/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options2/options_ui2.cc:319: initialized_handlers_ =
true;
Move this line into the 'if' block.

[Dan Beam, 2012-03-13 23:01:45]

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/resource...
File chrome/browser/resources/uber/uber_frame.css (right):

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/resource...
chrome/browser/resources/uber/uber_frame.css:30: -webkit-user-select: none;
On 2012/03/13 20:59:49, csilv wrote:
> On 2012/03/13 20:46:47, James Hawkins wrote:
> > Why did you make this change?
> 
> FWIW, I like this change. This prevents accidental text selection from
clicking
> on nav items.  I find this a minor annoyance.

That ^

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/resource...
chrome/browser/resources/uber/uber_frame.css:45: pointer-events: none;
On 2012/03/13 20:59:49, csilv wrote:
> I believe the purpose is to prevent accidental or reduncent page loads. I
don't
> believe the user would expect this to occur for this use case and there's
never
> a benefit (unless there's some bug requiting a reload.)

That ^ and if we remove indicators that this is clickable (like the hand
cursor), they don't have the same inclination to click it.

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/options_ui.h (right):

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/options_ui.h:38: // Actually sends down values
after initialization to the webui page.
On 2012/03/13 20:59:49, csilv wrote:
> nit: remove the word "Actually".

Done.

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/options_ui.h:38: // Actually sends down values
after initialization to the webui page.
On 2012/03/13 20:44:50, Evan Stade wrote:
> expand comment to highlight differences between this and initialize()

Done.

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/options_ui.h:39: virtual void SendPageValues()
{}
On 2012/03/13 20:59:49, csilv wrote:
> I had the same reaction to the method name.
> 
> I really like Evan's suggestion to use InitializeHandler/InitializePage

Done.

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right):

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/options2/browser_options_handler2.cc:812: void
BrowserOptionsHandler::UpdateSearchEngines() {
On 2012/03/13 20:44:50, Evan Stade wrote:
> this function name could probably be improved

Done.

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options2/handler_options_handler2.h (right):

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/options2/handler_options_handler2.h:3: // found in the
LICENSE file.
On 2012/03/13 20:44:50, Evan Stade wrote:
> can you add a file comment here? Offhand I don't have any clue what a
> HandlerOptionsHandler is

Done.

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options2/import_data_handler2.h (right):

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/options2/import_data_handler2.h:58: bool
profiles_loaded_;
On 2012/03/13 20:44:50, Evan Stade wrote:
> rather than adding this, expose source_profiles_loaded_ in ImporterList

Done.

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options2/options_ui2.cc (right):

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/options2/options_ui2.cc:319: initialized_handlers_ =
true;
On 2012/03/13 20:59:49, csilv wrote:
> Move this line into the 'if' block.

Done.

https://chromiumcodereview.appspot.com/9693032/diff/2003/chrome/browser/ui/we...
File chrome/browser/ui/webui/options/chromeos/stats_options_handler.cc (right):

https://chromiumcodereview.appspot.com/9693032/diff/2003/chrome/browser/ui/we...
chrome/browser/ui/webui/options/chromeos/stats_options_handler.cc:26: void
StatsOptionsHandler::InitializeHandler() {
I'll just remove this.

https://chromiumcodereview.appspot.com/9693032/diff/2003/chrome/browser/ui/we...
File
chrome/browser/ui/webui/options/chromeos/virtual_keyboard_manager_handler.cc
(right):

https://chromiumcodereview.appspot.com/9693032/diff/2003/chrome/browser/ui/we...
chrome/browser/ui/webui/options/chromeos/virtual_keyboard_manager_handler.cc:62:
void VirtualKeyboardManagerHandler::InitializeHandler() {
and this

https://chromiumcodereview.appspot.com/9693032/diff/2003/chrome/browser/ui/we...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

https://chromiumcodereview.appspot.com/9693032/diff/2003/chrome/browser/ui/we...
chrome/browser/ui/webui/options/extension_settings_handler.cc:590: void
ExtensionSettingsHandler::InitializeHandler() {
and this

https://chromiumcodereview.appspot.com/9693032/diff/2003/chrome/browser/ui/we...
File chrome/browser/ui/webui/options/pack_extension_handler.cc (right):

https://chromiumcodereview.appspot.com/9693032/diff/2003/chrome/browser/ui/we...
chrome/browser/ui/webui/options/pack_extension_handler.cc:22: void
PackExtensionHandler::InitializeHandler() {
and this

https://chromiumcodereview.appspot.com/9693032/diff/2003/chrome/browser/ui/we...
File chrome/browser/ui/webui/options2/chromeos/stats_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9693032/diff/2003/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/chromeos/stats_options_handler2.cc:27: void
StatsOptionsHandler::InitializeHandler() {
and this

https://chromiumcodereview.appspot.com/9693032/diff/2003/chrome/browser/ui/we...
File
chrome/browser/ui/webui/options2/chromeos/virtual_keyboard_manager_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9693032/diff/2003/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/chromeos/virtual_keyboard_manager_handler2.cc:63:
void VirtualKeyboardManagerHandler::InitializeHandler() {
and here

[Evan Stade, 2012-03-13 23:08:18]

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/resource...
File chrome/browser/resources/uber/uber_frame.css (right):

https://chromiumcodereview.appspot.com/9693032/diff/1/chrome/browser/resource...
chrome/browser/resources/uber/uber_frame.css:45: pointer-events: none;
On 2012/03/13 23:01:45, Dan Beam wrote:
> On 2012/03/13 20:59:49, csilv wrote:
> > I believe the purpose is to prevent accidental or reduncent page loads. I
> don't
> > believe the user would expect this to occur for this use case and there's
> never
> > a benefit (unless there's some bug requiting a reload.)
> 
> That ^ and if we remove indicators that this is clickable (like the hand
> cursor), they don't have the same inclination to click it.

well, not really --- the decision to click generally comes before the mouse
over. It's a good hint that it's not clickable, but it isn't visible until well
after the user action is actually initiated.

[Dan Beam, 2012-03-13 23:15:42]

+mirandac for chrome/browser/importer/*

[Evan Stade, 2012-03-13 23:17:14]

https://chromiumcodereview.appspot.com/9693032/diff/11002/chrome/browser/impo...
File chrome/browser/importer/importer_list.h (right):

https://chromiumcodereview.appspot.com/9693032/diff/11002/chrome/browser/impo...
chrome/browser/importer/importer_list.h:55: bool AreProfilesLoaded() const {
return source_profiles_loaded_; }
name of this fn should be source_profiles_loaded()

[Dan Beam, 2012-03-13 23:32:32]

https://chromiumcodereview.appspot.com/9693032/diff/11002/chrome/browser/impo...
File chrome/browser/importer/importer_list.h (right):

https://chromiumcodereview.appspot.com/9693032/diff/11002/chrome/browser/impo...
chrome/browser/importer/importer_list.h:55: bool AreProfilesLoaded() const {
return source_profiles_loaded_; }
On 2012/03/13 23:17:14, Evan Stade wrote:
> name of this fn should be source_profiles_loaded()

Done.

[Dan Beam, 2012-03-13 23:49:28]

ptal at earliest convenience

[James Hawkins, 2012-03-13 23:51:05]

-me

[csilv, 2012-03-14 00:09:11]

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/bluetooth_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/bluetooth_options_handler2.cc:139:
AdapterPresentChanged(adapter_.get(), adapter_->IsPresent());
seems like this should be moved to InitializePage().  InitializePage() will also
need the command line check

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/internet_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler2.cc:445:
cros_->RequestNetworkScan();
perhaps this should be InitializePage() instead?

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/pointer_handler2.cc (right):

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/pointer_handler2.cc:62:
DeviceHierarchyChanged();
this line should ve moved to InitializePage()

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/search_engine_manager_handler2.h (right):

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/search_engine_manager_handler2.h:24: virtual
void InitializeHandler() OVERRIDE;
nit: move this into the group below

[Dan Beam, 2012-03-14 01:13:14]

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/bluetooth_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/bluetooth_options_handler2.cc:139:
AdapterPresentChanged(adapter_.get(), adapter_->IsPresent());
On 2012/03/14 00:09:11, csilv wrote:
> seems like this should be moved to InitializePage().  InitializePage() will
also
> need the command line check

Done.

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/internet_options_handler2.cc
(right):

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/internet_options_handler2.cc:445:
cros_->RequestNetworkScan();
On 2012/03/14 00:09:11, csilv wrote:
> perhaps this should be InitializePage() instead?

Done.

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/chromeos/pointer_handler2.cc (right):

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/chromeos/pointer_handler2.cc:62:
DeviceHierarchyChanged();
On 2012/03/14 00:09:11, csilv wrote:
> this line should ve moved to InitializePage()

Done.

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/search_engine_manager_handler2.h (right):

https://chromiumcodereview.appspot.com/9693032/diff/15088/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/search_engine_manager_handler2.h:24: virtual
void InitializeHandler() OVERRIDE;
On 2012/03/14 00:09:11, csilv wrote:
> nit: move this into the group below

Done.

[csilv, 2012-03-14 06:55:51]

lgtm

[Miranda Callahan, 2012-03-14 14:35:39]

importer/* LGTM with comment nit -- there are so many different meanings for
"profile" in the code base at this point, it's important to be ridiculously
specific.

http://codereview.chromium.org/9693032/diff/8011/chrome/browser/importer/impo...
File chrome/browser/importer/importer_list.h (right):

http://codereview.chromium.org/9693032/diff/8011/chrome/browser/importer/impo...
chrome/browser/importer/importer_list.h:54: // Tells interested callers if class
is done loading profiles.
nit: please change this comment to say "...if class is done loading source
profiles."

[csilv, 2012-03-14 18:34:20]

+kevers for review

(chromeos specific files).

[Dan Beam, 2012-03-14 20:23:59]

https://chromiumcodereview.appspot.com/9693032/diff/8011/chrome/browser/impor...
File chrome/browser/importer/importer_list.h (right):

https://chromiumcodereview.appspot.com/9693032/diff/8011/chrome/browser/impor...
chrome/browser/importer/importer_list.h:54: // Tells interested callers if class
is done loading profiles.
On 2012/03/14 14:35:39, Miranda Callahan wrote:
> nit: please change this comment to say "...if class is done loading source
> profiles." 

Done.

[Evan Stade, 2012-03-14 21:12:15]

lgtm

[Evan Stade, 2012-03-14 21:12:24]

https://chromiumcodereview.appspot.com/9693032/diff/8013/chrome/browser/resou...
File chrome/browser/resources/uber/uber_frame.js (right):

https://chromiumcodereview.appspot.com/9693032/diff/8013/chrome/browser/resou...
chrome/browser/resources/uber/uber_frame.js:37:
assert(!e.target.classList.contains('selected'));
seems like it will still be clickable when we implement better keyboard support

[kevers, 2012-03-14 22:11:53]

lgtm

[Dan Beam, 2012-03-14 22:59:43]

https://chromiumcodereview.appspot.com/9693032/diff/8013/chrome/browser/resou...
File chrome/browser/resources/uber/uber_frame.js (right):

https://chromiumcodereview.appspot.com/9693032/diff/8013/chrome/browser/resou...
chrome/browser/resources/uber/uber_frame.js:37:
assert(!e.target.classList.contains('selected'));
On 2012/03/14 21:12:25, Evan Stade wrote:
> seems like it will still be clickable when we implement better keyboard
support

Done.