Make WebUI objects not derive from WebUI. WebUI objects own the controller. This is the ownership...

Make WebUI objects not derive from WebUI. WebUI objects own the controller. This is the ownership model since otherwise we'd have to make the interface class delete it, which is better avoided since it's supposed to be an interface. The controller has a pointer to the WebUI object.

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

[jam, 2012-01-17 17:08:12]

Joi: only web_ui.cc/h have real changes, the rest are trivial updates
accordingly

Estade: FYI: this is the next step of the change that we discussed

[Jói, 2012-01-17 17:34:05]

LGTM once ownership model comment below is fixed or answered.

Cheers,
Jói

http://codereview.chromium.org/9224002/diff/32001/content/browser/webui/web_u...
File content/browser/webui/web_ui.cc (right):

http://codereview.chromium.org/9224002/diff/32001/content/browser/webui/web_u...
content/browser/webui/web_ui.cc:179: controller_.reset(controller);
There might be a bug here w.r.t. the ownership model:  You're destroying the
controller in the destructor, implying that you take over ownership of
controller_, so here you should probably delete the existing controller_ if it
is non-NULL before resetting it.

[jam, 2012-01-17 17:38:34]

thanks for the quick review!

http://codereview.chromium.org/9224002/diff/32001/content/browser/webui/web_u...
File content/browser/webui/web_ui.cc (right):

http://codereview.chromium.org/9224002/diff/32001/content/browser/webui/web_u...
content/browser/webui/web_ui.cc:179: controller_.reset(controller);
On 2012/01/17 17:34:05, Jói wrote:
> There might be a bug here w.r.t. the ownership model:  You're destroying the
> controller in the destructor, implying that you take over ownership of
> controller_, so here you should probably delete the existing controller_ if it
> is non-NULL before resetting it.

scoped_ptr::reset() does that :)

[Evan Stade, 2012-01-17 19:14:55]

lgtm

http://codereview.chromium.org/9224002/diff/32001/content/browser/tab_content...
File content/browser/tab_contents/render_view_host_manager_unittest.cc (right):

http://codereview.chromium.org/9224002/diff/32001/content/browser/tab_content...
content/browser/tab_contents/render_view_host_manager_unittest.cc:54: virtual
WebUIController* CreateWebUIForURL(WebUI* web_ui,
ditto

http://codereview.chromium.org/9224002/diff/32001/content/browser/webui/web_ui.h
File content/browser/webui/web_ui.h (right):

http://codereview.chromium.org/9224002/diff/32001/content/browser/webui/web_u...
content/browser/webui/web_ui.h:83: content::PageTransition
GetLinkTransitionType() const;
c-style getters and setters are not used in the content api?

http://codereview.chromium.org/9224002/diff/32001/content/public/browser/web_...
File content/public/browser/web_ui_factory.h (right):

http://codereview.chromium.org/9224002/diff/32001/content/public/browser/web_...
content/public/browser/web_ui_factory.h:24: // Returns a WebUI instance for the
given URL, or NULL if the URL doesn't
this comment is out of date (WebUIController instead of WebUI) and perhaps the
function name should be updated as well

[jam, 2012-01-17 19:20:45]

thanks for looking this through

http://codereview.chromium.org/9224002/diff/32001/content/browser/webui/web_ui.h
File content/browser/webui/web_ui.h (right):

http://codereview.chromium.org/9224002/diff/32001/content/browser/webui/web_u...
content/browser/webui/web_ui.h:83: content::PageTransition
GetLinkTransitionType() const;
On 2012/01/17 19:14:56, Evan Stade wrote:
> c-style getters and setters are not used in the content api?

right, because the API will be interfaces, so c-style won't be appropriate for
virtual methods. that's coming in the next changelist. i renamed c-style
getters/setters to CamelCase in this change to separate that from the change to
switch to an interface, just to keep things easier to review.

http://codereview.chromium.org/9224002/diff/32001/content/public/browser/web_...
File content/public/browser/web_ui_factory.h (right):

http://codereview.chromium.org/9224002/diff/32001/content/public/browser/web_...
content/public/browser/web_ui_factory.h:24: // Returns a WebUI instance for the
given URL, or NULL if the URL doesn't
On 2012/01/17 19:14:56, Evan Stade wrote:
> this comment is out of date (WebUIController instead of WebUI) and perhaps the
> function name should be updated as well

yep I'm planning on updating the interface and method names. i didn't do this in
this change because it was already touching a large number of files and i wanted
it to be mostly trivial so that the files are easy to scan through. i have this
on my task list to update though