Separate handler initialization from page initialization

chrome/browser/ui/webui/options2/options_ui2.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/options2/options_ui2.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "base/callback.h"
@@ skipping 292 matching lines @@
   // Set up the chrome://userimage/ source.
   chromeos::options2::UserImageSource* user_image_source =
       new chromeos::options2::UserImageSource();
   profile->GetChromeURLDataManager()->AddDataSource(user_image_source);
 
   pointer_device_observer_.reset(
       new chromeos::system::PointerDeviceObserver());
   pointer_device_observer_->AddObserver(browser_options_handler);
   pointer_device_observer_->AddObserver(pointer_handler);
 #endif
+
+  InitializeHandlers();

[Dan Beam, 2012/04/05 21:38:21]

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.

 }
 
 OptionsUI::~OptionsUI() {
   // Uninitialize all registered handlers. Deleted by WebUIImpl.
   for (size_t i = 0; i < handlers_.size(); ++i)
     handlers_[i]->Uninitialize();
 }
 
 void OptionsUI::RenderViewCreated(RenderViewHost* render_view_host) {
   SetCommandLineString(render_view_host);
 }
 
 void OptionsUI::RenderViewReused(RenderViewHost* render_view_host) {
   SetCommandLineString(render_view_host);
 }
 
 void OptionsUI::DidBecomeActiveForReusedRenderView() {
   // When the renderer is re-used (e.g., for back/forward navigation within
   // options), the handlers are torn down and rebuilt, so are no longer
   // initialized, but the web page's DOM may remain intact, in which case onload
   // won't fire to initilize the handlers. To make sure initialization always
   // happens, call reinitializeCore (which is a no-op unless the DOM was already
   // initialized).
   web_ui()->CallJavascriptFunction("OptionsPage.reinitializeCore");

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

I'm not sure whether we still need this.

[Dan Beam, 2012/04/05 21:38:21]

I don't think we do.

 }
 
 // static
 void OptionsUI::ProcessAutocompleteSuggestions(
       const AutocompleteResult& autocompleteResult,
       ListValue * const suggestions) {
   for (size_t i = 0; i < autocompleteResult.size(); ++i) {
     const AutocompleteMatch& match = autocompleteResult.match_at(i);
     AutocompleteMatch::Type type = match.type;
     if (type != AutocompleteMatch::HISTORY_URL &&
@@ skipping 23 matching lines @@
   // The reinitialize call from DidBecomeActiveForReusedRenderView end up being
   // delivered after a new web page DOM has been brought up in an existing
   // renderer (due to IPC delays), causing this method to be called twice. If
   // that happens, ignore the second call.
   if (!initialized_handlers_) {
     for (size_t i = 0; i < handlers_.size(); ++i)
       handlers_[i]->InitializeHandler();
     initialized_handlers_ = true;
   }
 
-  // Always initialize the page as when handlers are left over we still need to
-  // do various things like show/hide sections and send data to the Javascript.
-  for (size_t i = 0; i < handlers_.size(); ++i)
-    handlers_[i]->InitializePage();
-
 #if defined(OS_CHROMEOS)
   pointer_device_observer_->Init();
 #endif
 }
 
+void OptionsUI::InitializePages() {
+  // Always initialize the page as when handlers are left over we still need to
+  // do various things like show/hide sections and send data to the Javascript.
+  for (size_t i = 0; i < handlers_.size(); ++i)
+    handlers_[i]->InitializePage();
+}
+
 void OptionsUI::AddOptionsPageUIHandler(DictionaryValue* localized_strings,
                                         OptionsPageUIHandler* handler_raw) {
   scoped_ptr<OptionsPageUIHandler> handler(handler_raw);
   DCHECK(handler.get());
   // Add only if handler's service is enabled.
   if (handler->IsEnabled()) {
     // Add handler to the list and also pass the ownership.
     web_ui()->AddMessageHandler(handler.release());

[Dan Beam, 2012/04/05 21:38:21]

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...

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

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.

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

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.
It's fine to name |handler| and |handler_raw|, IMO.
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.

[Tyler Breisacher (Chromium), 2012/04/06 16:59:44]

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.

     handler_raw->GetLocalizedValues(localized_strings);

[Dan Beam, 2012/04/05 21:38:21]

handler->InitializeHandler();  // here instead of InitializeHandlers(), perhaps?

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

+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.

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

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.

     handlers_.push_back(handler_raw);

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

I don't know why handlers_ exists. There's already an identical handlers_ member
on WebUI, we should just expose that.

   }
 }
 
 void OptionsUI::SetCommandLineString(RenderViewHost* render_view_host) {
   std::string command_line_string;
 
 #if defined(OS_WIN)
   command_line_string =
       WideToASCII(CommandLine::ForCurrentProcess()->GetCommandLineString());
 #else
   command_line_string =
       CommandLine::ForCurrentProcess()->GetCommandLineString();
 #endif
 
   render_view_host->SetWebUIProperty("commandLineString", command_line_string);
 }
 
 }  // namespace options2