Start splitting out WebUI into an implementation class and an interface that each page implements...

Start splitting out WebUI into an implementation class and an interface that each page implements. This first patch moves the virtual functions in WebUI that were overridden by pages into a separate interface, content::WebUIController.

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

[Evan Stade, 2012-01-12 23:55:36]

just a question

http://codereview.chromium.org/9188056/diff/103/content/public/browser/web_ui...
File content/public/browser/web_ui_controller.h (right):

http://codereview.chromium.org/9188056/diff/103/content/public/browser/web_ui...
content/public/browser/web_ui_controller.h:33: bool* overridden) {}
why is overridden an outparam instead of return value? I'm more used to using
return values for this type of thing, and it makes it harder to mess up
(compiler complains if you don't explicitly return something, but doesn't do
anything if you forget to set overridden)

[mmenke, 2012-01-12 23:59:23]

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

http://codereview.chromium.org/9188056/diff/103/content/browser/webui/web_ui....
content/browser/webui/web_ui.h:41: WebUI(content::WebContents* contentsm,
content::WebUIController* controller);
random driveby nit:  -m

[jam, 2012-01-13 00:06:03]

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

http://codereview.chromium.org/9188056/diff/103/content/browser/webui/web_ui....
content/browser/webui/web_ui.h:41: WebUI(content::WebContents* contentsm,
content::WebUIController* controller);
On 2012/01/12 23:59:23, Matt Menke wrote:
> random driveby nit:  -m

Done.

http://codereview.chromium.org/9188056/diff/103/content/public/browser/web_ui...
File content/public/browser/web_ui_controller.h (right):

http://codereview.chromium.org/9188056/diff/103/content/public/browser/web_ui...
content/public/browser/web_ui_controller.h:33: bool* overridden) {}
On 2012/01/12 23:55:37, Evan Stade wrote:
> why is overridden an outparam instead of return value? I'm more used to using
> return values for this type of thing, and it makes it harder to mess up
> (compiler complains if you don't explicitly return something, but doesn't do
> anything if you forget to set overridden)

this is slightly nicer because we can then avoid a cc file (if a virtual
function has a return value, we can't put the return statement in the header or
else clank complains). in general we try to avoid a cc file if we can in the
content\public directory.

[Evan Stade, 2012-01-13 00:16:46]

On 2012/01/13 00:06:03, John Abd-El-Malek wrote:
> http://codereview.chromium.org/9188056/diff/103/content/browser/webui/web_ui.h
> File content/browser/webui/web_ui.h (right):
> 
>
http://codereview.chromium.org/9188056/diff/103/content/browser/webui/web_ui....
> content/browser/webui/web_ui.h:41: WebUI(content::WebContents* contentsm,
> content::WebUIController* controller);
> On 2012/01/12 23:59:23, Matt Menke wrote:
> > random driveby nit:  -m
> 
> Done.
> 
>
http://codereview.chromium.org/9188056/diff/103/content/public/browser/web_ui...
> File content/public/browser/web_ui_controller.h (right):
> 
>
http://codereview.chromium.org/9188056/diff/103/content/public/browser/web_ui...
> content/public/browser/web_ui_controller.h:33: bool* overridden) {}
> On 2012/01/12 23:55:37, Evan Stade wrote:
> > why is overridden an outparam instead of return value? I'm more used to
using
> > return values for this type of thing, and it makes it harder to mess up
> > (compiler complains if you don't explicitly return something, but doesn't do
> > anything if you forget to set overridden)
> 
> this is slightly nicer because we can then avoid a cc file (if a virtual
> function has a return value, we can't put the return statement in the header
or
> else clank complains). in general we try to avoid a cc file if we can in the
> content\public directory.

I would rather have the return value for the reasons stated (compile-time
checking and consistency with other event dispatch models). For example it's
somewhat subtle that there's no *overridden = false in
UberUI::HandleWebUIMessage for the not-overridden case.

[eroman, 2012-01-13 00:35:38]

http://codereview.chromium.org/9188056/diff/103/chrome/browser/ui/webui/net_i...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

http://codereview.chromium.org/9188056/diff/103/chrome/browser/ui/webui/net_i...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1640:
NetInternalsUI::NetInternalsUI(WebContents* contents) : WebUI(contents, this) {
I imagine this setup is just transitional while refactoring?

In particular I am a bit weary of passing the |this| pointer down to the base
class. This can be a risky pattern since destruction happens in reverse order,
and if the base class's destructor were to call virtual methods on this pointer
the results can be unexpected.

Still LGTM, but please be aware of this limitation.

[jam, 2012-01-13 00:39:03]

On 2012/01/13 00:16:46, Evan Stade wrote:
> On 2012/01/13 00:06:03, John Abd-El-Malek wrote:
> >
http://codereview.chromium.org/9188056/diff/103/content/browser/webui/web_ui.h
> > File content/browser/webui/web_ui.h (right):
> > 
> >
>
http://codereview.chromium.org/9188056/diff/103/content/browser/webui/web_ui....
> > content/browser/webui/web_ui.h:41: WebUI(content::WebContents* contentsm,
> > content::WebUIController* controller);
> > On 2012/01/12 23:59:23, Matt Menke wrote:
> > > random driveby nit:  -m
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/9188056/diff/103/content/public/browser/web_ui...
> > File content/public/browser/web_ui_controller.h (right):
> > 
> >
>
http://codereview.chromium.org/9188056/diff/103/content/public/browser/web_ui...
> > content/public/browser/web_ui_controller.h:33: bool* overridden) {}
> > On 2012/01/12 23:55:37, Evan Stade wrote:
> > > why is overridden an outparam instead of return value? I'm more used to
> using
> > > return values for this type of thing, and it makes it harder to mess up
> > > (compiler complains if you don't explicitly return something, but doesn't
do
> > > anything if you forget to set overridden)
> > 
> > this is slightly nicer because we can then avoid a cc file (if a virtual
> > function has a return value, we can't put the return statement in the header
> or
> > else clank complains). in general we try to avoid a cc file if we can in the
> > content\public directory.
> 
> I would rather have the return value for the reasons stated (compile-time
> checking and consistency with other event dispatch models). For example it's
> somewhat subtle that there's no *overridden = false in
> UberUI::HandleWebUIMessage for the not-overridden case.

ok, updated patch up

[jam, 2012-01-13 00:39:50]

http://codereview.chromium.org/9188056/diff/103/chrome/browser/ui/webui/net_i...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

http://codereview.chromium.org/9188056/diff/103/chrome/browser/ui/webui/net_i...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1640:
NetInternalsUI::NetInternalsUI(WebContents* contents) : WebUI(contents, this) {
On 2012/01/13 00:35:38, eroman wrote:
> I imagine this setup is just transitional while refactoring?
> 
> In particular I am a bit weary of passing the |this| pointer down to the base
> class. This can be a risky pattern since destruction happens in reverse order,
> and if the base class's destructor were to call virtual methods on this
pointer
> the results can be unexpected.
> 
> Still LGTM, but please be aware of this limitation.

yep this is definitely transitional, so i don't send a huge patch out for review
:)

[Evan Stade, 2012-01-13 03:00:16]

lgtm, thanks!

[James Hawkins, 2012-01-13 19:03:18]

This commit broke a few pieces of chrome://chrome, notably the sync sign in
section and the search engine drop-down.  We have a tight deadline for a UI
review next Tuesday, so I'm going to revert this.  Please check chrome://chrome
works before relanding.

[jam, 2012-01-13 19:35:13]

apologies for the inconvenience, I have a fix on the way in
http://codereview.chromium.org/9174010.

I'll test with chrome://chrome in the future, but in general note it's hard
to expect people to test every single webui for ex when changing the core
code. Is it possible to write tests for this?

On Fri, Jan 13, 2012 at 11:03 AM, <jhawkins@chromium.org> wrote:

> This commit broke a few pieces of chrome://chrome, notably the sync sign in
> section and the search engine drop-down.  We have a tight deadline for a UI
> review next Tuesday, so I'm going to revert this.  Please check
> chrome://chrome
> works before relanding.
>
>
http://codereview.chromium.**org/9188056/<http://codereview.chromium.org/9188...
>