Separate handler initialization from page initialization

Separate handler initialization from page initialization

BUG=121741
TEST=no crashes

[Tyler Breisacher (Chromium), 2012-04-05 17:54:04]

See bug for more details.

[Dan Beam, 2012-04-05 17:58:34]

http://codereview.chromium.org/9994005/diff/4001/chrome/browser/resources/opt...
File chrome/browser/resources/options2/options.js (right):

http://codereview.chromium.org/9994005/diff/4001/chrome/browser/resources/opt...
chrome/browser/resources/options2/options.js:245:
OptionsPage.reinitializeCore();
nit: no \n, possibly move above adding DOMContentLoaded, but it may not matter

http://codereview.chromium.org/9994005/diff/4001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options2/options_ui2.cc (right):

http://codereview.chromium.org/9994005/diff/4001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options2/options_ui2.cc:378: }
nit: \n

http://codereview.chromium.org/9994005/diff/4001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options2/search_engine_manager_handler2.cc (left):

http://codereview.chromium.org/9994005/diff/4001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options2/search_engine_manager_handler2.cc:252:
CHECK(list_controller_->table_model());
yay!

[Evan Stade, 2012-04-05 20:41:02]

http://codereview.chromium.org/9994005/diff/4002/chrome/browser/resources/opt...
File chrome/browser/resources/options2/options.js (right):

http://codereview.chromium.org/9994005/diff/4002/chrome/browser/resources/opt...
chrome/browser/resources/options2/options.js:245:
OptionsPage.reinitializeCore();
remove, call from C++ as discussed offline

[Tyler Breisacher (Chromium), 2012-04-05 20:57:40]

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/ui/we...
File chrome/browser/ui/webui/options2/options_ui2.cc (right):

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/options_ui2.cc:338:
web_ui()->CallJavascriptFunction("OptionsPage.reinitializeCore");
I'm not sure whether we still need this.

[Dan Beam, 2012-04-05 21:38:20]

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/resou...
File chrome/browser/resources/options2/options_page.js (right):

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/resou...
chrome/browser/resources/options2/options_page.js:638:
OptionsPage.reinitializeCore = function() {
And if we remove that other stuff, we can probably remove this as well. We could
probably already remove this.initialized_ unless we're using it anywhere else as
the C++ already keeps this info.

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/ui/we...
File chrome/browser/ui/webui/options2/options_ui2.cc (right):

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/options_ui2.cc:314: InitializeHandlers();
We could also simply call handler->InitializeHandler() from
AddOptionsPageUIHandler(), eh? Don't know if that's cleaner or not. I'm
understanding more and more what estade was mentioning of just doing this in
each ctor instead. Doesn't seem like there's a good reason this is necessary any
more now that we've de-coupled handler vs page initialization.

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/options_ui2.cc:338:
web_ui()->CallJavascriptFunction("OptionsPage.reinitializeCore");
On 2012/04/05 20:57:40, Tyler Breisacher wrote:
> I'm not sure whether we still need this.

I don't think we do.

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/options_ui2.cc:401:
web_ui()->AddMessageHandler(handler.release());
seeing .release() at the beginning here is weird... .get() here and .release()
later seems more typical (or changing around the order that these methods are
called), then we can continue to use |handler| normally - otherwise it just
looks like you're using a pointer you don't own...

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/options_ui2.cc:402:
handler_raw->GetLocalizedValues(localized_strings);
handler->InitializeHandler();  // here instead of InitializeHandlers(), perhaps?

[Tyler Breisacher (Chromium), 2012-04-05 22:56:16]

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/ui/we...
File chrome/browser/ui/webui/options2/options_ui2.cc (right):

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/options_ui2.cc:401:
web_ui()->AddMessageHandler(handler.release());
On 2012/04/05 21:38:21, Dan Beam wrote:
> seeing .release() at the beginning here is weird... .get() here and .release()
> later seems more typical (or changing around the order that these methods are
> called), then we can continue to use |handler| normally - otherwise it just
> looks like you're using a pointer you don't own...

If IsEnabled is true, we release it because we want to pass ownership to the web
ui. If it's not, we let `handler` go out of scope, which actually deletes the
handler. Right? I wonder if it would be clearer to just do an explicit `delete`
then?

if (handler_raw->IsEnabled() {
  web_ui()->AddMessageHandler(handler_raw);
  // etc.
} else {
  delete handler_raw;
}

and of course then we could rename it to just 'handler'. But that's kind of ugly
too.

web_ui()->AddMessageHandler has to happen first, because InitializeHandler
doesn't work if the handler isn't attached to a WebUI yet.

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/options_ui2.cc:402:
handler_raw->GetLocalizedValues(localized_strings);
On 2012/04/05 21:38:21, Dan Beam wrote:
> handler->InitializeHandler();  // here instead of InitializeHandlers(),
perhaps?

+1

If we do it before it gets added to handlers_ then it's guaranteed that every
handler in handlers_ is initialized. I think that helps a lot to make things
understandable.

Done.

[Dan Beam, 2012-04-06 03:38:16]

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/ui/we...
File chrome/browser/ui/webui/options2/options_ui2.cc (right):

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/options_ui2.cc:401:
web_ui()->AddMessageHandler(handler.release());
On 2012/04/05 22:56:16, Tyler Breisacher wrote:
> On 2012/04/05 21:38:21, Dan Beam wrote:
> > seeing .release() at the beginning here is weird... .get() here and
.release()
> > later seems more typical (or changing around the order that these methods
are
> > called), then we can continue to use |handler| normally - otherwise it just
> > looks like you're using a pointer you don't own...
> 
> If IsEnabled is true, we release it because we want to pass ownership to the
web
> ui. If it's not, we let `handler` go out of scope, which actually deletes the
> handler. Right? I wonder if it would be clearer to just do an explicit
`delete`
> then?
> 
> if (handler_raw->IsEnabled() {
>   web_ui()->AddMessageHandler(handler_raw);
>   // etc.
> } else {
>   delete handler_raw;
> }

Use classes to do resource management, don't delete explicitly (see: effective
C++). It's a common paradigm already to .release() a pointer when you transfer
ownership, otherwise delete it when it goes out of scope. It's just not common
to do anything after .release()ing it, AFAIK.

> and of course then we could rename it to just 'handler'. But that's kind of
ugly
> too.

It's fine to name |handler| and |handler_raw|, IMO.

> web_ui()->AddMessageHandler has to happen first, because InitializeHandler
> doesn't work if the handler isn't attached to a WebUI yet.

That's fine, but we could just as easily simply say

  web_ui()->AddMessageHandler(handler);  // maybe .get() if C++ is confused
  handler->GetLocalizedValues(localized_strings);
  handler->InitializeHandler();
  handlers_.push_back(handler.release());  // .release() last

It's not the end of the world, it just jumped out at me because it's a little
irregular.

https://chromiumcodereview.appspot.com/9994005/diff/7012/chrome/browser/ui/we...
File chrome/browser/ui/webui/options2/options_ui2.cc (right):

https://chromiumcodereview.appspot.com/9994005/diff/7012/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/options_ui2.cc:295:
DCHECK(!profile->IsOffTheRecord() || Profile::IsGuestSession());
what's this from?

[Tyler Breisacher (Chromium), 2012-04-06 16:59:43]

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/ui/we...
File chrome/browser/ui/webui/options2/options_ui2.cc (right):

https://chromiumcodereview.appspot.com/9994005/diff/4003/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/options_ui2.cc:401:
web_ui()->AddMessageHandler(handler.release());
It *should* jump out at us. We *are* using a pointer that we've given up
ownership of. So if the two rules are:

1. release() the pointer when you give up ownership.
2. Don't keep using the pointer once you've release()d it.

then we have to break one rule or the other, AFAICT. I think your way is
probably the best approach though.

https://chromiumcodereview.appspot.com/9994005/diff/7012/chrome/browser/ui/we...
File chrome/browser/ui/webui/options2/options_ui2.cc (right):

https://chromiumcodereview.appspot.com/9994005/diff/7012/chrome/browser/ui/we...
chrome/browser/ui/webui/options2/options_ui2.cc:295:
DCHECK(!profile->IsOffTheRecord() || Profile::IsGuestSession());
On 2012/04/06 03:38:16, Dan Beam wrote:
> what's this from?

It was in the old InitializeHandlers method. I'm not sure how important it is,
so I didn't want to just throw it out.

[Evan Stade, 2012-04-06 22:14:30]

http://codereview.chromium.org/9994005/diff/4003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options2/options_ui2.cc (right):

http://codereview.chromium.org/9994005/diff/4003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options2/options_ui2.cc:402:
handler_raw->GetLocalizedValues(localized_strings);
yes, it would be fine to move this above AddMessageHandler so that you can do
handler.get() then handler.release() which looks a little more normal.

http://codereview.chromium.org/9994005/diff/4003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options2/options_ui2.cc:403:
handlers_.push_back(handler_raw);
I don't know why handlers_ exists. There's already an identical handlers_ member
on WebUI, we should just expose that.

[Evan Stade, 2012-04-06 22:16:25]

On 2012/04/06 22:14:30, Evan Stade wrote:
>
http://codereview.chromium.org/9994005/diff/4003/chrome/browser/ui/webui/opti...
> File chrome/browser/ui/webui/options2/options_ui2.cc (right):
> 
>
http://codereview.chromium.org/9994005/diff/4003/chrome/browser/ui/webui/opti...
> chrome/browser/ui/webui/options2/options_ui2.cc:402:
> handler_raw->GetLocalizedValues(localized_strings);
> yes, it would be fine to move this above AddMessageHandler so that you can do
> handler.get() then handler.release() which looks a little more normal.
> 
>
http://codereview.chromium.org/9994005/diff/4003/chrome/browser/ui/webui/opti...
> chrome/browser/ui/webui/options2/options_ui2.cc:403:
> handlers_.push_back(handler_raw);
> I don't know why handlers_ exists. There's already an identical handlers_
member
> on WebUI, we should just expose that.

ah I guess it's not identical as it also contains a GenericHandler which is not
an OptionsPageUIHandler.

[Evan Stade, 2012-04-06 22:16:34]

lgtm

[Dan Beam, 2012-04-06 22:47:06]

http://codereview.chromium.org/9994005/diff/9007/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options2/options_ui2.cc (right):

http://codereview.chromium.org/9994005/diff/9007/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options2/options_ui2.cc:295:
DCHECK(!profile->IsOffTheRecord() || Profile::IsGuestSession());
did you try this on CrOs in guest mode or incognito?

[Tyler Breisacher (Chromium), 2012-04-09 17:42:09]

On 2012/04/06 22:47:06, Dan Beam wrote:
>
http://codereview.chromium.org/9994005/diff/9007/chrome/browser/ui/webui/opti...
> File chrome/browser/ui/webui/options2/options_ui2.cc (right):
> 
>
http://codereview.chromium.org/9994005/diff/9007/chrome/browser/ui/webui/opti...
> chrome/browser/ui/webui/options2/options_ui2.cc:295:
> DCHECK(!profile->IsOffTheRecord() || Profile::IsGuestSession());
> did you try this on CrOs in guest mode or incognito?

You can't open the settings page in incognito: If you try, it will open in a
regular window instead. I'm not sure how to get a guest session in ChromeOS, but
I did run it in ChromeOS and didn't hit that DCHECK.

I did need to make a few changes for ChromeOS though, so +achuith for chromeos
review.

[Tyler Breisacher (Chromium), 2012-04-09 18:19:28]

http://codereview.chromium.org/9994005/diff/14001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc (right):

http://codereview.chromium.org/9994005/diff/14001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc:110:
proxy_handler_->InitializePage();
If I'm not mistaken, both of these InitializePage() methods are no-op, so it
shouldn't matter that this happens after the code at line 93-98, instead of
before it.

[Dan Beam, 2012-04-09 20:02:58]

> I'm not sure how to get a guest session in ChromeOS, but
> I did run it in ChromeOS and didn't hit that DCHECK.

export GYP_DEFINES="$GYP_DEFINES chromeos=1"
gclient sync
make chrome
./out/Debug/chrome # don't add --login-* flags and it'll be in guest mode

[Dan Beam, 2012-04-09 20:04:22]

http://codereview.chromium.org/9994005/diff/14001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc (right):

http://codereview.chromium.org/9994005/diff/14001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc:110:
proxy_handler_->InitializePage();
On 2012/04/09 18:19:28, Tyler Breisacher wrote:
> If I'm not mistaken, both of these InitializePage() methods are no-op, so it
> shouldn't matter that this happens after the code at line 93-98, instead of
> before it.

They might not be no-op forever.

[Dan Beam, 2012-04-09 20:13:24]

http://codereview.chromium.org/9994005/diff/14001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc (right):

http://codereview.chromium.org/9994005/diff/14001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc:94:
proxy_tracker->UIMakeActiveNetworkCurrent();
If this is doing anything involving the page it should be in an InitializePage()
method, btw...

http://codereview.chromium.org/9994005/diff/14001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc:98:
proxy_handler_->SetNetworkName(network_name);
I don't think this currently does anything, btw...

http://codereview.chromium.org/9994005/diff/14001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc:110:
proxy_handler_->InitializePage();
On 2012/04/09 20:04:23, Dan Beam wrote:
> On 2012/04/09 18:19:28, Tyler Breisacher wrote:
> > If I'm not mistaken, both of these InitializePage() methods are no-op, so it
> > shouldn't matter that this happens after the code at line 93-98, instead of
> > before it.
> 
> They might not be no-op forever.

But additionally, there's nothing wrong this as far as I can tell,
InitializeHandler() is called on |core_handler_| and |proxy_handler_| before
InitializePages() as it's in the ctor.

[Tyler Breisacher (Chromium), 2012-04-09 20:20:56]

+kevers for chromeos and achuith to CC

On 2012/04/09 20:02:58, Dan Beam wrote:
> > I'm not sure how to get a guest session in ChromeOS, but
> > I did run it in ChromeOS and didn't hit that DCHECK.
> 
> export GYP_DEFINES="$GYP_DEFINES chromeos=1"
> gclient sync
> make chrome
> ./out/Debug/chrome # don't add --login-* flags and it'll be in guest mode

In that case I've been using guest sessions all along. I'll try a non-guest
session to make sure nothing bad happens.

On 2012/04/09 20:04:22, Dan Beam wrote:
> proxy_handler_->InitializePage();
> On 2012/04/09 18:19:28, Tyler Breisacher wrote:
> > If I'm not mistaken, both of these InitializePage() methods are no-op, so it
> > shouldn't matter that this happens after the code at line 93-98, instead of
> > before it.
> 
> They might not be no-op forever.

Definitely. I just want to make sure we're not breaking something by landing
this CL. If anything, I think this should make it easier to add code to
InitializePage, because you can be sure that the initialization at line 93 has
already happened.

But I don't understand this Chrome OS code as well as I do the non-Chrome OS
code, so kevers, please take a look.

[Dan Beam, 2012-04-10 04:38:43]

also, explain why you think this fixes the try issues

https://chromiumcodereview.appspot.com/9994005/diff/25002/chrome/browser/ui/w...
File chrome/browser/ui/webui/options2/core_options_handler2.h (right):

https://chromiumcodereview.appspot.com/9994005/diff/25002/chrome/browser/ui/w...
chrome/browser/ui/webui/options2/core_options_handler2.h:101: // TODO document
do it

[Tyler Breisacher (Chromium), 2012-04-10 17:43:28]

It looks like InitializeHandlers actually does cause the handlers to call some
JavaScript functions. Usually it's not done directly, but rather the handler
will add itself as an observer/listener of some service, and when the service
notifies it, it passes that information to the page via a JavaScript call. With
the newest patch, we don't let that happen until after BrowserOptions,
OptionsPage, etc. are defined in the JavaScript, so I think this should
eliminate the ReferenceErrors we were getting.

http://codereview.chromium.org/9994005/diff/25002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/core_options_handler2.h (right):

http://codereview.chromium.org/9994005/diff/25002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/core_options_handler2.h:101: // TODO document
On 2012/04/10 04:38:43, Dan Beam wrote:
> do it

Did it.

[Dan Beam, 2012-04-10 18:00:40]

On 2012/04/10 17:43:28, Tyler Breisacher wrote:
> It looks like InitializeHandlers actually does cause the handlers to call some
> JavaScript functions. Usually it's not done directly, but rather the handler
> will add itself as an observer/listener of some service, and when the service
> notifies it, it passes that information to the page via a JavaScript call.
With
> the newest patch, we don't let that happen until after BrowserOptions,
> OptionsPage, etc. are defined in the JavaScript, so I think this should
> eliminate the ReferenceErrors we were getting.

I'd rather change InitializeHandlers() that actually touch JS to
InitializePage(). That's the whole point of the division. Otherwise it's just a
bandaid.

[kevers, 2012-04-10 18:06:30]

http://codereview.chromium.org/9994005/diff/18007/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc (right):

http://codereview.chromium.org/9994005/diff/18007/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc:98:
proxy_handler_->SetNetworkName(network_name);
SetNetworkName is a no-op that is in the process of being removed.  Just an FYI.
 No additional action required.

[Tyler Breisacher (Chromium), 2012-04-10 22:01:47]

For now, I've just moved everything that indirectly calls JavaScript from
InitializeHandler to InitializePage. As we discussed offline, this might not be
the ideal solution, but I think it's the simplest, and it's more foolproof than
checking whether the page is initialized before every JS call. The only huge
concern I have is that InitializePage may get called multiple times, but maybe
that's actually not a concern anymore:

http://codereview.chromium.org/9994005/diff/30001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/options_ui2.cc (left):

http://codereview.chromium.org/9994005/diff/30001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/options_ui2.cc:334: void
OptionsUI::DidBecomeActiveForReusedRenderView() {
Since we took this out, does that mean it's no longer true that InitializePage
might get called multiple times?