Support getting the font list in Pepper. This currently only works out of

Support getting the font list in Pepper. This currently only works out of
process.

This adds a function to the font interface to get the font list. Since we
don't have arrays or dictionaries in Pepper yet, I used a string with nulls
separating the names. A previous attempt to make a "font list resource" proved
excessively complicated and not actually much easier for clients to deal with.

This refactors the existing font list getting that used to be in the options
for the browser. I moved it to content and split it into two pieces, the
synchronous version, and then an asynchronous wrapper around that which both
the prefs code and the pepper code use. This cleaned up some of the preferences
code, and also fixes the leak of the entire font list in the code.

I used the new callback/bind system for the async font loading. I had to add
BrowserThread support for the new system.

This uses the PepperMessageFilter to listen for font load requests from the
plugin in the browser process. This is nice because we can add stuff here and
have messages serviced for both in-process and out-of-process plugins. This
proved to be complicated due to the HostResolver used in some of the existing
code, and thread restrictions for how to deal with it. This is why there are
two modes for the filter object.

I changed the delegates around for the Dispatcher. Now the PluginDispatcher
has the delegate interface since the HostDispatcher didn't actually need any
of them and we were accumulating a lot of empty functions in the
PepperPluginRegistry.

It's possible for the fonts to be loaded on Windows and Mac without IPC, since
enumerating fonts should be possible inside the sandbox. I didn't implement
this since it adds extra complexity and probably doesn't give that much
benefit.

TEST=manual
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85827

[viettrungluu, 2011-05-18 20:21:18]

So far, so good, but there's a lot more to go through....

http://codereview.chromium.org/7044012/diff/7004/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/font_settings_utils_gtk.cc (right):

http://codereview.chromium.org/7044012/diff/7004/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/font_settings_utils_gtk.cc:7: void
FontSettingsUtilities::ValidateSavedFonts(PrefService* prefs) {
nit: isn't this missing a "// static" comment?

http://codereview.chromium.org/7044012/diff/7004/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/font_settings_utils_mac.mm (right):

http://codereview.chromium.org/7044012/diff/7004/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/font_settings_utils_mac.mm:17: // The native
font settings dialog saved fonts by the font name, rather
This makes me sad. I suspect that there must be a more appropriate place to put
it which would allow us to delete all the font_settings_utils* files....

http://codereview.chromium.org/7044012/diff/7004/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/font_settings_utils_mac.mm:33: void
FontSettingsUtilities::ValidateSavedFonts(PrefService* prefs) {
Ditto about "// static"?

http://codereview.chromium.org/7044012/diff/7004/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/font_settings_utils_win.cc (right):

http://codereview.chromium.org/7044012/diff/7004/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/font_settings_utils_win.cc:7: void
FontSettingsUtilities::ValidateSavedFonts(PrefService* prefs) {
"

http://codereview.chromium.org/7044012/diff/7004/content/browser/browser_thre...
File content/browser/browser_thread.h (right):

http://codereview.chromium.org/7044012/diff/7004/content/browser/browser_thre...
content/browser/browser_thread.h:13: #include "base/callback.h"
Oooh, alphabetical order!

http://codereview.chromium.org/7044012/diff/7004/content/browser/font_list_as...
File content/browser/font_list_async.cc (right):

http://codereview.chromium.org/7044012/diff/7004/content/browser/font_list_as...
content/browser/font_list_async.cc:23: void GetFontListOnBackgroundThread(
Maybe you should be more specific and s/Background/File/ ?

http://codereview.chromium.org/7044012/diff/7004/content/browser/font_list_as...
content/browser/font_list_async.cc:43: bool well_known_thread =
BrowserThread::GetCurrentThreadIdentifier(&id);
Is it true that all threads with IDs run message loops?

http://codereview.chromium.org/7044012/diff/7004/content/browser/font_list_as...
content/browser/font_list_async.cc:45: << "Can only call GetFontList from a
well-known thread";
Does the message deserve punctuation?

http://codereview.chromium.org/7044012/diff/7004/content/browser/font_list_as...
File content/browser/font_list_async.h (right):

http://codereview.chromium.org/7044012/diff/7004/content/browser/font_list_as...
content/browser/font_list_async.h:16: // Wraps ownership of a listvalue so we
can send it across threads without
s/listvalue/ListValue

http://codereview.chromium.org/7044012/diff/7004/content/browser/font_list_as...
content/browser/font_list_async.h:18: // before the callback is executed, we
want the ListValue to be destroyed.
)))))))))))))))))))))))))))))))))))))))))))))

http://codereview.chromium.org/7044012/diff/7004/content/browser/plugin_servi...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7044012/diff/7004/content/browser/plugin_servi...
content/browser/plugin_service.h:209: #endif  //
CONTENT_BROWSER_PLUGIN_SERVICE_H_
Trung wuz here.

[viettrungluu, 2011-05-18 20:40:43]

http://codereview.chromium.org/7044012/diff/7004/content/browser/ppapi_plugin...
File content/browser/ppapi_plugin_process_host.cc (right):

http://codereview.chromium.org/7044012/diff/7004/content/browser/ppapi_plugin...
content/browser/ppapi_plugin_process_host.cc:113: /*
?

http://codereview.chromium.org/7044012/diff/7004/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/7044012/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:315: this, reply_msg));
Is there any chance that |this| is destroyed before the callback is called? What
happens in that case?

http://codereview.chromium.org/7044012/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:347: if
(resource_context_)
I think you should just use ?: here. <shrug>

http://codereview.chromium.org/7044012/diff/7004/content/common/font_list.h
File content/common/font_list.h (right):

http://codereview.chromium.org/7044012/diff/7004/content/common/font_list.h#n...
content/common/font_list.h:20: // Most callers will want to use the
GetFontListAsync function in the browser
maybe you should mention the name of the header file?

http://codereview.chromium.org/7044012/diff/7004/content/common/font_list_mac.mm
File content/common/font_list_mac.mm (right):

http://codereview.chromium.org/7044012/diff/7004/content/common/font_list_mac...
content/common/font_list_mac.mm:12: #include
"chrome/browser/prefs/pref_service.h"
I believe the two prefs includes aren't needed here.

http://codereview.chromium.org/7044012/diff/7004/content/common/font_list_win.cc
File content/common/font_list_win.cc (right):

http://codereview.chromium.org/7044012/diff/7004/content/common/font_list_win...
content/common/font_list_win.cc:8: #include <string>
I suppose there's no longer a need to include <string>.

http://codereview.chromium.org/7044012/diff/7004/content/common/font_list_win...
content/common/font_list_win.cc:9: #include <windows.h>
Is this include order really kosher?

http://codereview.chromium.org/7044012/diff/7004/content/ppapi_plugin/ppapi_t...
File content/ppapi_plugin/ppapi_thread.cc (right):

http://codereview.chromium.org/7044012/diff/7004/content/ppapi_plugin/ppapi_t...
content/ppapi_plugin/ppapi_thread.cc:224: }
Trung is now here.

[viettrungluu, 2011-05-18 21:07:46]

LGTM otherwise, I think. I'm sure there's something I'll regret.

http://codereview.chromium.org/7044012/diff/7004/ppapi/cpp/dev/font_dev.h
File ppapi/cpp/dev/font_dev.h (right):

http://codereview.chromium.org/7044012/diff/7004/ppapi/cpp/dev/font_dev.h#new...
ppapi/cpp/dev/font_dev.h:114: static Var GetFontFamilies(Instance* instance);
I don't suppose you could write a version which just returns a vector of
strings, could you? (Such a version wouldn't have to change when/if we allow for
array Vars.)

http://codereview.chromium.org/7044012/diff/7004/ppapi/proxy/plugin_dispatcher.h
File ppapi/proxy/plugin_dispatcher.h (right):

http://codereview.chromium.org/7044012/diff/7004/ppapi/proxy/plugin_dispatche...
ppapi/proxy/plugin_dispatcher.h:41: // plugin side, for the host side, the
return value may be NULL.
s/,/;/ (only the first comma). (Admittedly, you're just moving this comment
around....)

http://codereview.chromium.org/7044012/diff/7004/ppapi/proxy/ppb_font_proxy.cc
File ppapi/proxy/ppb_font_proxy.cc (right):

http://codereview.chromium.org/7044012/diff/7004/ppapi/proxy/ppb_font_proxy.c...
ppapi/proxy/ppb_font_proxy.cc:82: PP_Var result;
Why don't we have a helper for these three lines of code? It seems like a common
use case. (Or why doesn't MakeString() just return a PP_Var?)

http://codereview.chromium.org/7044012/diff/7004/webkit/plugins/ppapi/ppb_fon...
File webkit/plugins/ppapi/ppb_font_impl.h (right):

http://codereview.chromium.org/7044012/diff/7004/webkit/plugins/ppapi/ppb_fon...
webkit/plugins/ppapi/ppb_font_impl.h:68: };
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/7044012/diff/7004/webkit/plugins/ppapi/resourc...
File webkit/plugins/ppapi/resource_tracker.cc (right):

http://codereview.chromium.org/7044012/diff/7004/webkit/plugins/ppapi/resourc...
webkit/plugins/ppapi/resource_tracker.cc:18: #include
"webkit/plugins/ppapi/ppb_font_impl.h"
alphabetical order

[brettw, 2011-05-18 21:14:38]

New snap up.

http://codereview.chromium.org/7044012/diff/7004/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/font_settings_utils_mac.mm (right):

http://codereview.chromium.org/7044012/diff/7004/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/font_settings_utils_mac.mm:17: // The native
font settings dialog saved fonts by the font name, rather
I didn't want to get into even more crap with this patch. If you have an idea
that's easy I'm happy to do it though. Note also that I don't have a Chrome Mac
setup to test easily.

http://codereview.chromium.org/7044012/diff/7004/content/browser/font_list_as...
File content/browser/font_list_async.cc (right):

http://codereview.chromium.org/7044012/diff/7004/content/browser/font_list_as...
content/browser/font_list_async.cc:43: bool well_known_thread =
BrowserThread::GetCurrentThreadIdentifier(&id);
Should be.

http://codereview.chromium.org/7044012/diff/7004/content/browser/font_list_as...
File content/browser/font_list_async.h (right):

http://codereview.chromium.org/7044012/diff/7004/content/browser/font_list_as...
content/browser/font_list_async.h:18: // before the callback is executed, we
want the ListValue to be destroyed.
(((((((((((((((((((((((((((((((((((((((((((((

http://codereview.chromium.org/7044012/diff/7004/content/browser/plugin_servi...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7044012/diff/7004/content/browser/plugin_servi...
content/browser/plugin_service.h:209: #endif  //
CONTENT_BROWSER_PLUGIN_SERVICE_H_
So wuz Brett.

http://codereview.chromium.org/7044012/diff/7004/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/7044012/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:315: this, reply_msg));
No, because this class is refcounted (otherwise Bind wouldn't have compiled).

http://codereview.chromium.org/7044012/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:347: if
(resource_context_)
I'm usually anti ?: unless it saves some other duplication.

http://codereview.chromium.org/7044012/diff/7004/ppapi/cpp/dev/font_dev.h
File ppapi/cpp/dev/font_dev.h (right):

http://codereview.chromium.org/7044012/diff/7004/ppapi/cpp/dev/font_dev.h#new...
ppapi/cpp/dev/font_dev.h:114: static Var GetFontFamilies(Instance* instance);
I thought about that. I really wanted to rewrite the existing code to return a
vector of something (strings? string pairs?) and have the webui code convert it.
But that's more code overall, and it seemed stupid for the webui code to have
this big dynamically allocated structure created, and then have to be re-created
in another format. For us, we only want the non-localized name, which means we
have to do some conversion anyway, so I figured it's best to keep the current
code as Values and have us converting.

http://codereview.chromium.org/7044012/diff/7004/ppapi/proxy/plugin_dispatcher.h
File ppapi/proxy/plugin_dispatcher.h (right):

http://codereview.chromium.org/7044012/diff/7004/ppapi/proxy/plugin_dispatche...
ppapi/proxy/plugin_dispatcher.h:41: // plugin side, for the host side, the
return value may be NULL.
Actually, I should have deleted this since there's no host side any more.

http://codereview.chromium.org/7044012/diff/7004/ppapi/proxy/ppb_font_proxy.cc
File ppapi/proxy/ppb_font_proxy.cc (right):

http://codereview.chromium.org/7044012/diff/7004/ppapi/proxy/ppb_font_proxy.c...
ppapi/proxy/ppb_font_proxy.cc:82: PP_Var result;
Yeah, I've been thinking about improving MakeString but this huge patch seemed
like the wrong time to do it. The var-making helpers are in pp_var.h, but since
plugins can't make strings and objects, we don't have helpers to avoid
confusion.

[viettrungluu, 2011-05-18 21:17:39]

Assuming you did all the things you said you did, LGTM.