Make MediaInternalsUI talk to MediaInternals.

Make MediaInternalsUI talk to MediaInternals.

BUG=none
TEST=navigate to chrome://media-internals without crashing

[Scott Franklin, 2011-06-28 22:29:57]

This was the best solution I could come up with for the threading issues. All
interaction with IO objects (IOThread, MediaInternals) happens on the IO thread,
and the same with UI objects (MediaInternalsUI, BrowserProcess) on the UI
thread.

The proxy is ref_counted so that it will continue to exist after
MediaInternalsUI destruction until it has removed itself from the MediaInternals
observer list and processed (dropped) all pending updates.

[scherkus (not reviewing), 2011-06-29 01:03:07]

overall approach looks good

could you move the proxy into a separate file?

you can forward decl the MediaInternalsUI class in the proxy .h file

http://codereview.chromium.org/7272036/diff/1/chrome/browser/ui/webui/media_i...
File chrome/browser/ui/webui/media_internals_ui.h (right):

http://codereview.chromium.org/7272036/diff/1/chrome/browser/ui/webui/media_i...
chrome/browser/ui/webui/media_internals_ui.h:10: 
a/z ordering for includes

http://codereview.chromium.org/7272036/diff/1/chrome/browser/ui/webui/media_i...
chrome/browser/ui/webui/media_internals_ui.h:14: class MediaInternalsUI;
a/z ordering

[Scott Franklin, 2011-06-29 17:35:23]

Yeah, it really does belong in its own file.

+estade@, since this is his directory.

http://codereview.chromium.org/7272036/diff/1/chrome/browser/ui/webui/media_i...
File chrome/browser/ui/webui/media_internals_ui.h (right):

http://codereview.chromium.org/7272036/diff/1/chrome/browser/ui/webui/media_i...
chrome/browser/ui/webui/media_internals_ui.h:10: 
On 2011/06/29 01:03:07, scherkus wrote:
> a/z ordering for includes

Done.

http://codereview.chromium.org/7272036/diff/1/chrome/browser/ui/webui/media_i...
chrome/browser/ui/webui/media_internals_ui.h:14: class MediaInternalsUI;
On 2011/06/29 01:03:07, scherkus wrote:
> a/z ordering

Done.

[scherkus (not reviewing), 2011-06-29 17:55:12]

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
File chrome/browser/ui/webui/media_internals_proxy.cc (right):

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.cc:14: };
remove ;

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.cc:17: ui_ = ui;
since these are public methods add DCHECK for being on UI thread

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.cc:21: };
remove ;

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.cc:24: ui_ = NULL;
ditto

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
File chrome/browser/ui/webui/media_internals_proxy.h (right):

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.h:17: : public
MediaInternalsObserver,
docs for class

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.h:21: MediaInternalsProxy();
nit: typically have a blank line between the ctor/dtor section + methods

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.h:22: void SetUI(MediaInternalsUI*
ui);
why not not pass in the ui in the ctor?

then instead of RemoveUI() you can name it Detach() or similar

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.h:24: void GetEverything();
docs for non-interface-implementation methods

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.h:40: };
DISALLOW etc

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
File chrome/browser/ui/webui/media_internals_ui.cc (right):

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_ui.cc:83: proxy_(new
MediaInternalsProxy()) {
re: pass in UI in ctor, you'll probably have to use ALLOW_THIS_IN_CONSTRUCTOR()
macro

see base/compiler_specific.h

[Scott Franklin, 2011-06-29 18:44:33]

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
File chrome/browser/ui/webui/media_internals_proxy.cc (right):

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.cc:14: };
On 2011/06/29 17:55:12, scherkus wrote:
> remove ;

Done.

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.cc:17: ui_ = ui;
On 2011/06/29 17:55:12, scherkus wrote:
> since these are public methods add DCHECK for being on UI thread

Done.

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.cc:21: };
On 2011/06/29 17:55:12, scherkus wrote:
> remove ;

Done.

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.cc:24: ui_ = NULL;
On 2011/06/29 17:55:12, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
File chrome/browser/ui/webui/media_internals_proxy.h (right):

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.h:17: : public
MediaInternalsObserver,
On 2011/06/29 17:55:12, scherkus wrote:
> docs for class

Done.

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.h:21: MediaInternalsProxy();
On 2011/06/29 17:55:12, scherkus wrote:
> nit: typically have a blank line between the ctor/dtor section + methods

Done.

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.h:22: void SetUI(MediaInternalsUI*
ui);
On 2011/06/29 17:55:12, scherkus wrote:
> why not not pass in the ui in the ctor?
> 
> then instead of RemoveUI() you can name it Detach() or similar

I can't call PostTask in the ctor (which makes sense; wouldn't want tasks
operating on a partially-constructed object).

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.h:24: void GetEverything();
On 2011/06/29 17:55:12, scherkus wrote:
> docs for non-interface-implementation methods

Done.

http://codereview.chromium.org/7272036/diff/4001/chrome/browser/ui/webui/medi...
chrome/browser/ui/webui/media_internals_proxy.h:40: };
On 2011/06/29 17:55:12, scherkus wrote:
> DISALLOW etc

Done.

[scherkus (not reviewing), 2011-06-29 18:49:32]

LGTM w/ one micro-nit

http://codereview.chromium.org/7272036/diff/10001/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_internals_proxy.h (right):

http://codereview.chromium.org/7272036/diff/10001/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_internals_proxy.h:28: void
AttachUI(MediaInternalsUI* ui);
pedantic nit: space out these methods w/ one blank line so it isn't a WALLOFTEXT

[Evan Stade, 2011-06-29 20:13:01]

http://codereview.chromium.org/7272036/diff/10001/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_internals_proxy.h (right):

http://codereview.chromium.org/7272036/diff/10001/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_internals_proxy.h:20: class MediaInternalsProxy
seems this class should be a WebUIMessageHandler (see *_handler.{h,cc})

http://codereview.chromium.org/7272036/diff/10001/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_internals_ui.cc (right):

http://codereview.chromium.org/7272036/diff/10001/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_internals_ui.cc:93: void
MediaInternalsUI::OnWebUISend(const GURL& source_url,
what is this? I dont see docs for OnWebUISend anywhere. If you are just trying
to handle a chrome.send call, create a handler class and use
RegisterMessageCallback

[Scott Franklin, 2011-06-29 23:47:08]

Adding a MediaInternalsMessageHandler similar to that of gpu- and net-
internals. It looks like different WebUIs differ on whether the proxy should be
subclassed into the MessageHandler or not, so I went with not.

If it were, I suppose the handler could be broken off into a separate file, but
this doesn't seem too long as is (esp. compared to the 1603 lines in
net_internals_ui.cc).

http://codereview.chromium.org/7272036/diff/10001/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_internals_proxy.h (right):

http://codereview.chromium.org/7272036/diff/10001/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_internals_proxy.h:20: class MediaInternalsProxy
On 2011/06/29 20:13:01, Evan Stade wrote:
> seems this class should be a WebUIMessageHandler (see *_handler.{h,cc})

As far as I can see, all of those still have proxies.

In fact, net_internals_ui appears to do exactly the same thing. They just name
it IOThreadImpl instead of calling it a proxy.

http://codereview.chromium.org/7272036/diff/10001/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_internals_proxy.h:28: void
AttachUI(MediaInternalsUI* ui);
On 2011/06/29 18:49:32, scherkus wrote:
> pedantic nit: space out these methods w/ one blank line so it isn't a
WALLOFTEXT

Done.

http://codereview.chromium.org/7272036/diff/10001/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_internals_ui.cc (right):

http://codereview.chromium.org/7272036/diff/10001/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_internals_ui.cc:93: void
MediaInternalsUI::OnWebUISend(const GURL& source_url,
On 2011/06/29 20:13:01, Evan Stade wrote:
> what is this? I dont see docs for OnWebUISend anywhere. If you are just trying
> to handle a chrome.send call, create a handler class and use
> RegisterMessageCallback

It's defined in content/browser/webui/webui.h. I pulled this from
sync_internals_ui, which looks like it handles a single message manually before
passing the rest off to another class.

Creating a handler does indeed appear to be the correct way of doing this.

[Evan Stade, 2011-06-30 22:24:27]

http://codereview.chromium.org/7272036/diff/10001/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_internals_proxy.h (right):

http://codereview.chromium.org/7272036/diff/10001/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_internals_proxy.h:20: class MediaInternalsProxy
On 2011/06/29 23:47:08, Scott Franklin wrote:
> On 2011/06/29 20:13:01, Evan Stade wrote:
> > seems this class should be a WebUIMessageHandler (see *_handler.{h,cc})
> 
> As far as I can see, all of those still have proxies.
> 
> In fact, net_internals_ui appears to do exactly the same thing. They just name
> it IOThreadImpl instead of calling it a proxy.

Most chrome:// pages have handlers but not dedicated proxies (most of them don't
directly deal with multi-threading, instead they use UI-thread APIs for
accessing data). If you want/need separate proxy and handler classes, that's
fine with me.

http://codereview.chromium.org/7272036/diff/12002/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_internals_ui.cc (right):

http://codereview.chromium.org/7272036/diff/12002/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_internals_ui.cc:44: class
MediaInternalsMessageHandler : public WebUIMessageHandler {
please do break this into a separate file. net_internals_ui sets a bad precedent
that should not be followed.

since you will have 6 files now instead of 2, I think it's a good idea to give
media_internals its own directory.

[Scott Franklin, 2011-07-01 01:42:32]

http://codereview.chromium.org/7272036/diff/12002/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_internals_ui.cc (right):

http://codereview.chromium.org/7272036/diff/12002/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_internals_ui.cc:44: class
MediaInternalsMessageHandler : public WebUIMessageHandler {
On 2011/06/30 22:24:27, Evan Stade wrote:
> please do break this into a separate file. net_internals_ui sets a bad
precedent
> that should not be followed.
> 
> since you will have 6 files now instead of 2, I think it's a good idea to give
> media_internals its own directory.

Alright, split and filed (directoried?). I'm assuming breaking off the
HTMLSource would be considered overkill.

It is nice when entire files fit on one screen.

[scherkus (not reviewing), 2011-07-01 17:32:59]

things LGTM to me

only nit I'd have is s/media_internals/media/ for the directory name

[Evan Stade, 2011-07-07 20:42:43]

thanks. LGTM. Sorry it took me a while to get back to you. Feel free to ping me
if I don't respond within 24 hours

http://codereview.chromium.org/7272036/diff/15001/chrome/browser/ui/webui/med...
File chrome/browser/ui/webui/media_internals/media_internals_ui.cc (right):

http://codereview.chromium.org/7272036/diff/15001/chrome/browser/ui/webui/med...
chrome/browser/ui/webui/media_internals/media_internals_ui.cc:19: class
MediaInternalsHTMLSource : public ChromeURLDataManager::DataSource {
yes this is fine to have here

[commit-bot: I haz the power, 2011-07-08 00:54:11]

Can't process patch for file
chrome/browser/ui/webui/media_internals/media_internals_ui.h.
A +

[scherkus (not reviewing), 2011-07-08 16:53:32]

Committed as r91863.