Move WebUIFactory to chrome/browser.

Move WebUIFactory to chrome/browser.

This reduces dependencies from content/ to chrome/.

WebUIFactory is the interface in content/ to ChromeWebUIFactory in chrome/

BUG=77092
TEST=none

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

[Evan Stade, 2011-03-24 01:45:30]

looks like a big review, but most files only have 1-2 lines changed.

[jam, 2011-03-24 03:07:46]

Thanks for doing this.  I haven't looked at the WebUI code before, so I feel
unqualified to review this though.


On Wed, Mar 23, 2011 at 6:45 PM, <estade@chromium.org> wrote:

> Reviewers: Avi, John Abd-El-Malek,
>
> Message:
> looks like a big review, but most files only have 1-2 lines changed.
>
> Description:
> Move WebUIFactory to chrome/browser.
>
> This reduces dependencies from content/ to chrome/.
>
> WebUISource - interface for browser/ to implement. It provides callbacks to
> be
> implemented that can then be used from within content/
>
> WebUIRegister - object that tracks the currently active WebUISource.
> Singleton.
>
> WebUIFactory - changed from a collection of static function to a singleton
> with
> member functions (so that it can implement WebUISource). Moved to chrome/
>
> WebUI - This is the prototype for individual WebUI pages. Not
> chrome-specific.
> moved to content/.
>
> BUG=77092
> TEST=none
>
>
> Please review this at http://codereview.chromium.org/6713082/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/browser/browser_url_handler.cc
>  M chrome/browser/extensions/extension_host.cc
>  M chrome/browser/favicon_service.cc
>  M chrome/browser/notifications/balloon_host.h
>  M chrome/browser/notifications/balloon_host.cc
>  M chrome/browser/prerender/prerender_contents.h
>  M chrome/browser/tab_contents/background_contents.h
>  M chrome/browser/tab_contents/background_contents.cc
>  M chrome/browser/tab_contents/render_view_host_delegate_helper.h
>  M chrome/browser/tab_contents/render_view_host_delegate_helper.cc
>  M chrome/browser/ui/browser.cc
>  M chrome/browser/ui/browser_navigator.cc
>  A chrome/browser/ui/webui/web_ui_factory.h
>  A + chrome/browser/ui/webui/web_ui_factory.cc
>  M chrome/chrome_browser.gypi
>  M content/browser/browsing_instance.cc
>  M content/browser/site_instance.cc
>  M content/browser/tab_contents/render_view_host_manager.cc
>  M content/browser/tab_contents/tab_contents.h
>  M content/browser/tab_contents/tab_contents.cc
>  M content/browser/tab_contents/tab_contents_delegate.h
>  M content/browser/tab_contents/tab_contents_view.cc
>  A + content/browser/webui/generic_handler.h
>  A + content/browser/webui/generic_handler.cc
>  M content/browser/webui/web_ui.h
>  M content/browser/webui/web_ui.cc
>  D content/browser/webui/web_ui_factory.h
>  A content/browser/webui/web_ui_register.h
>  A content/browser/webui/web_ui_register.cc
>  A content/browser/webui/web_ui_source.h
>  M content/content_browser.gypi
>
>
>

[Avi (use Gerrit), 2011-03-24 14:29:19]

On 2011/03/24 03:07:46, John Abd-El-Malek wrote:
> I haven't looked at the WebUI code before, so I feel
> unqualified to review this though.

Ditto; the best I could do is review for plausibility.

[Evan Stade, 2011-03-24 18:38:33]

this doesn't change the WebUI code at all. If one or both of you want to review
to make sure the interface between content/ and chrome/ makes sense I can ask
arv to take a look as well.

+arv for review

[Evan Stade, 2011-03-24 20:21:55]

arv OOO, adding Stuart instead

[stuartmorgan, 2011-03-24 21:09:41]

jam/avi: FWIW, there's nothing that's specific to WebUI in the essence of this
CL which is: chrome implements a FooFactory, but content needs to be able to
create Foo objects.

The approach here, unless I'm misunderstanding, is to have an interface for the
factory in content, an implementation of the factory in chrome, and a singleton
factory-container in content that chrome can give the factory implementation to.

Is that pretty much the design we're using for this kind of thing in carnitas?

http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
File content/browser/webui/web_ui_register.h (right):

http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
content/browser/webui/web_ui_register.h:11: class WebUIRegister {
Register is usually a verb. Registry? Registrar?

Given the comment below though, this may not be the best name, as it sort of
implies a list. FactoryContainer?

http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
content/browser/webui/web_ui_register.h:14: static void
RegisterSource(WebUISource* source);
Maybe the behavior would be more obvious if the name were SetSource.

http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
File content/browser/webui/web_ui_source.h (right):

http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
content/browser/webui/web_ui_source.h:17: class WebUISource {
Source vs Factory seems a bit arbitrary; I'd have to look at the inheritance or
the comments to know which is which. Should we use something like Factory and
FactoryImpl instead?

[Evan Stade, 2011-03-24 21:28:40]

http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
File content/browser/webui/web_ui_register.h (right):

http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
content/browser/webui/web_ui_register.h:11: class WebUIRegister {
register is also a noun. As a noun it's basically a synonym of registry,
although I hoped that due to its use in processor land it might conjure the idea
of a place for a single value to be kept.

registrar is something that registers things for you (NotificationRegistrar
registers the observer in the NotificationService)

registry to me implies a list, as you say.

FactoryContainer sounds a little odd to me but I am open to changing the name to
whatever can be agreed upon.

http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
content/browser/webui/web_ui_register.h:14: static void
RegisterSource(WebUISource* source);
On 2011/03/24 21:09:41, stuartmorgan wrote:
> Maybe the behavior would be more obvious if the name were SetSource.

ok

http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
File content/browser/webui/web_ui_source.h (right):

http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
content/browser/webui/web_ui_source.h:17: class WebUISource {
On 2011/03/24 21:09:41, stuartmorgan wrote:
> Source vs Factory seems a bit arbitrary; I'd have to look at the inheritance
or
> the comments to know which is which. Should we use something like Factory and
> FactoryImpl instead?

sounds fine to me

[jam, 2011-03-24 21:38:43]

http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
File content/browser/webui/web_ui_register.h (right):

http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
content/browser/webui/web_ui_register.h:10: // registers a signle WebUISource at
a time.
nit: single

http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
File content/browser/webui/web_ui_source.h (right):

http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
content/browser/webui/web_ui_source.h:27: virtual WebUI::WebUITypeID
GetWebUIType(Profile* profile,
Profile is a Chrome concept that Elliot is moving out of content.  it would be
good to take it out in this change as well if possible.  it seems it's only used
to figure out if there's an extension webui.  can we move that chunk from
web_ui_factory out along this as well (extensions are a chrome concept)

[jam, 2011-03-24 21:44:36]

On Thu, Mar 24, 2011 at 2:09 PM, <stuartmorgan@chromium.org> wrote:

> jam/avi: FWIW, there's nothing that's specific to WebUI in the essence of
> this
> CL which is: chrome implements a FooFactory, but content needs to be able
> to
> create Foo objects.
>
> The approach here, unless I'm misunderstanding, is to have an interface for
> the
> factory in content, an implementation of the factory in chrome, and a
> singleton
> factory-container in content that chrome can give the factory
> implementation to.
>
> Is that pretty much the design we're using for this kind of thing in
> carnitas?
>

We have an embedder API for the browser process, ContentBrowserClient.
 That's what we've been using in the content code to allow the embedder to
have its say.  Can we use that instead of having Browser do
WebUIRegister::RegisterSource(WebUIFactory::GetInstance());
?

We also need to remove the profile and extension dependencies.


>
>
>
http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
> File content/browser/webui/web_ui_register.h (right):
>
>
>
http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
> content/browser/webui/web_ui_register.h:11: class WebUIRegister {
> Register is usually a verb. Registry? Registrar?
>
> Given the comment below though, this may not be the best name, as it
> sort of implies a list. FactoryContainer?
>
>
>
http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
> content/browser/webui/web_ui_register.h:14: static void
> RegisterSource(WebUISource* source);
> Maybe the behavior would be more obvious if the name were SetSource.
>
>
>
http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
> File content/browser/webui/web_ui_source.h (right):
>
>
>
http://codereview.chromium.org/6713082/diff/8001/content/browser/webui/web_ui...
> content/browser/webui/web_ui_source.h:17: class WebUISource {
> Source vs Factory seems a bit arbitrary; I'd have to look at the
> inheritance or the comments to know which is which. Should we use
> something like Factory and FactoryImpl instead?
>
>
> http://codereview.chromium.org/6713082/
>

[Evan Stade, 2011-03-24 21:52:40]

re: ContentBrowserClient - to make sure I understand you correctly, you just
want me to add a callback called GetWebUISource (or something like that)?

re: Extension and Profile dependencies - I plan to do that in a later patch. I
didn't want to tackle the entire WebUI task in one go because that would be
unwieldy.

[jam, 2011-03-24 22:02:56]

On Thu, Mar 24, 2011 at 2:52 PM, <estade@chromium.org> wrote:

> re: ContentBrowserClient - to make sure I understand you correctly, you
> just
> want me to add a callback called GetWebUISource (or something like that)?
>

yep.  I think it's good if all the calls to the embedder go through this
interface (including getting new interfaces) so that it's easier for us to
track dependencies.


> re: Extension and Profile dependencies - I plan to do that in a later
> patch. I
> didn't want to tackle the entire WebUI task in one go because that would be
> unwieldy.


ah ok, sure doing it in another patch sounds reasonable

>
>
> http://codereview.chromium.org/6713082/
>

[Evan Stade, 2011-03-24 23:02:20]

I added a new client, ContentWebUIClient instead of adding onto
ContentBrowserClient, because it seems like ContentBrowserClient will become a
big grab bag of unrelated functionality if we put all browser-related client
code in there. (also this way we can use WebUIFactory for the client directly
instead of writing a bunch of pass-through code)

[stuartmorgan, 2011-03-24 23:14:31]

LGTM assuming jam is happy with the new top-level client.

http://codereview.chromium.org/6713082/diff/2007/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/6713082/diff/2007/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.h:708: // Query the WebUISource for
the WebUITypeID for the current URL.
Stale reference to WebUISource

[Evan Stade, 2011-03-25 00:23:22]

updated

[jam, 2011-03-25 02:10:01]

lgtm

http://codereview.chromium.org/6713082/diff/9001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6713082/diff/9001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1460: // Override the default
ContentBrowserClient and to let Chrome participate in
nit: it was written like that on purpose.  putting "and" means that 1 & 2 are
both needed, where here 1 is done for 2

http://codereview.chromium.org/6713082/diff/9001/content/browser/browsing_ins...
File content/browser/browsing_instance.cc (right):

http://codereview.chromium.org/6713082/diff/9001/content/browser/browsing_ins...
content/browser/browsing_instance.cc:44: if
(content::GetContentClient()->browser()->GetWebUIFactory()->
content::GetContentClient()->browser()->GetWebUIFactory()

is a little long to repeat in a bunch of places.  can we have WebUI::Get() or
something similar that does this?

http://codereview.chromium.org/6713082/diff/9001/content/browser/content_brow...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/6713082/diff/9001/content/browser/content_brow...
content/browser/content_browser_client.h:28: virtual WebUIFactory*
GetWebUIFactory() = 0;
the ContentClient APIs all have default implementations, so please add one that
returns NULL

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

http://codereview.chromium.org/6713082/diff/9001/content/browser/webui/web_ui...
content/browser/webui/web_ui.h:148: typedef void* WebUITypeID;
nit: now that this is inside WebUI, can it just be TypeID so we don't end up
with the redundant WebUI::WebUITypeID

[Evan Stade, 2011-03-29 00:14:16]

http://codereview.chromium.org/6713082/diff/9001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6713082/diff/9001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1460: // Override the default
ContentBrowserClient and to let Chrome participate in
On 2011/03/25 02:10:02, John Abd-El-Malek wrote:
> nit: it was written like that on purpose.  putting "and" means that 1 & 2 are
> both needed, where here 1 is done for 2

(besides the fact that it's ungrammatical now)

I think this change was accidentally left over from when I was going to put
"Override the default ContentBrowserClient and ContentWebUIClient to..."

http://codereview.chromium.org/6713082/diff/9001/content/browser/browsing_ins...
File content/browser/browsing_instance.cc (right):

http://codereview.chromium.org/6713082/diff/9001/content/browser/browsing_ins...
content/browser/browsing_instance.cc:44: if
(content::GetContentClient()->browser()->GetWebUIFactory()->
On 2011/03/25 02:10:02, John Abd-El-Malek wrote:
> content::GetContentClient()->browser()->GetWebUIFactory()
> 
> is a little long to repeat in a bunch of places.  can we have WebUI::Get() or
> something similar that does this?

Done.

http://codereview.chromium.org/6713082/diff/9001/content/browser/content_brow...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/6713082/diff/9001/content/browser/content_brow...
content/browser/content_browser_client.h:28: virtual WebUIFactory*
GetWebUIFactory() = 0;
On 2011/03/25 02:10:02, John Abd-El-Malek wrote:
> the ContentClient APIs all have default implementations, so please add one
that
> returns NULL

well, I specifically don't want to allow NULL because I don't want to have to
check for it, but I've added a stub implementation for it to return.

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

http://codereview.chromium.org/6713082/diff/9001/content/browser/webui/web_ui...
content/browser/webui/web_ui.h:148: typedef void* WebUITypeID;
On 2011/03/25 02:10:02, John Abd-El-Malek wrote:
> nit: now that this is inside WebUI, can it just be TypeID so we don't end up
> with the redundant WebUI::WebUITypeID

Done.

[Evan Stade, 2011-03-29 04:26:53]

still lgty John?

[jam, 2011-03-29 04:59:30]

On Mon, Mar 28, 2011 at 5:14 PM, <estade@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6713082/diff/9001/chrome/browser/browser_main.cc
> File chrome/browser/browser_main.cc (right):
>
>
>
http://codereview.chromium.org/6713082/diff/9001/chrome/browser/browser_main....
> chrome/browser/browser_main.cc:1460: // Override the default
> ContentBrowserClient and to let Chrome participate in
> On 2011/03/25 02:10:02, John Abd-El-Malek wrote:
>
>> nit: it was written like that on purpose.  putting "and" means that 1
>>
> & 2 are
>
>> both needed, where here 1 is done for 2
>>
>
> (besides the fact that it's ungrammatical now)
>
> I think this change was accidentally left over from when I was going to
> put "Override the default ContentBrowserClient and ContentWebUIClient
> to..."
>
>
>
>
http://codereview.chromium.org/6713082/diff/9001/content/browser/browsing_ins...
> File content/browser/browsing_instance.cc (right):
>
>
>
http://codereview.chromium.org/6713082/diff/9001/content/browser/browsing_ins...
> content/browser/browsing_instance.cc:44: if
> (content::GetContentClient()->browser()->GetWebUIFactory()->
> On 2011/03/25 02:10:02, John Abd-El-Malek wrote:
>
>> content::GetContentClient()->browser()->GetWebUIFactory()
>>
>
>  is a little long to repeat in a bunch of places.  can we have
>>
> WebUI::Get() or
>
>> something similar that does this?
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/6713082/diff/9001/content/browser/content_brow...
> File content/browser/content_browser_client.h (right):
>
>
>
http://codereview.chromium.org/6713082/diff/9001/content/browser/content_brow...
> content/browser/content_browser_client.h:28: virtual WebUIFactory*
> GetWebUIFactory() = 0;
> On 2011/03/25 02:10:02, John Abd-El-Malek wrote:
>
>> the ContentClient APIs all have default implementations, so please add
>>
> one that
>
>> returns NULL
>>
>
> well, I specifically don't want to allow NULL because I don't want to
> have to check for it, but I've added a stub implementation for it to
> return.


can the stub implementation be moved to another file?  (i.e.
content/browser/webui/empty_web_ui_factory.h)  with that lgtm

>
>
>
>
http://codereview.chromium.org/6713082/diff/9001/content/browser/webui/web_ui.h
> File content/browser/webui/web_ui.h (right):
>
>
>
http://codereview.chromium.org/6713082/diff/9001/content/browser/webui/web_ui...
> content/browser/webui/web_ui.h:148: typedef void* WebUITypeID;
> On 2011/03/25 02:10:02, John Abd-El-Malek wrote:
>
>> nit: now that this is inside WebUI, can it just be TypeID so we don't
>>
> end up
>
>> with the redundant WebUI::WebUITypeID
>>
>
> Done.
>
>
> http://codereview.chromium.org/6713082/
>