Mac: First steps for default plugin

Reland Mac: First steps for default plugin

BUG=17392
TEST=Go to http://www.adobe.com/shockwave/welcome/. The task manager should show that the default plugin is running, and the "Missing Plug-in" text should look slightly different than it did before. (this test depends on a small webkit patch)

Requires commenting out

    if (objectContentType(url, mimeType) != ObjectContentNetscapePlugin)
        return 0;

in WebKit/WebKit/chromium/src/FrameLoaderClientImpl.cpp's createPlugin() as well, else the default plugin won't load.

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

[Nico, 2010-05-18 21:42:51]

Mark: gyp file
Stuart: rest

[stuartmorgan, 2010-05-18 22:19:46]

http://codereview.chromium.org/2075006/diff/23001/24003
File webkit/default_plugin/default_plugin.gyp (right):

http://codereview.chromium.org/2075006/diff/23001/24003#newcode37
webkit/default_plugin/default_plugin.gyp:37: 'plugin_impl_mac.mm',
I would think we would want the plugin_impl_* stuff in a non-conditionalized
'sources' section.

http://codereview.chromium.org/2075006/diff/23001/24003#newcode43
webkit/default_plugin/default_plugin.gyp:43: # Figure out why once they're
needed.
Looks like webkit_support depends on 'glue' in webkit_glue, which in turn
depends on default_plugin.

http://codereview.chromium.org/2075006/diff/23001/24004
File webkit/default_plugin/plugin_impl_mac.h (right):

http://codereview.chromium.org/2075006/diff/23001/24004#newcode29
webkit/default_plugin/plugin_impl_mac.h:29: };
Can we consolidate some of this cross-platform stuff? In fact, looking at the
diffs it looks like this should never have been forked in the first place; just
combine the headers and #ifdef where there aren't easy abstractions (like we do
with webplugin_delegate_impl.h)

http://codereview.chromium.org/2075006/diff/23001/24005
File webkit/default_plugin/plugin_impl_mac.mm (right):

http://codereview.chromium.org/2075006/diff/23001/24005#newcode26
webkit/default_plugin/plugin_impl_mac.mm:26: //      install_dialog_(NULL),
Seriously? How instead of checking in acres of commented out code, you just
refer to the Windows file as necessary while fleshing out the implementation.
Maybe toss in a bunch of NOTIMPLEMENTED()s if you need placeholders (or just
//TODOs if you prefer).

http://codereview.chromium.org/2075006/diff/23001/24005#newcode379
webkit/default_plugin/plugin_impl_mac.mm:379: NSSize text_size = [text
sizeWithAttributes:attributes];
I think we are supposed to use camelCase in obj-C code.

http://codereview.chromium.org/2075006/diff/23001/24005#newcode384
webkit/default_plugin/plugin_impl_mac.mm:384: text_rect.origin.y =
(bounds.height - NSHeight(text_rect))/2;
Use NSMakeRect?

[Nico, 2010-05-18 22:44:38]

That was quick. Thanks.

http://codereview.chromium.org/2075006/diff/23001/24003
File webkit/default_plugin/default_plugin.gyp (right):

http://codereview.chromium.org/2075006/diff/23001/24003#newcode37
webkit/default_plugin/default_plugin.gyp:37: 'plugin_impl_mac.mm',
On 2010/05/18 22:19:47, stuartmorgan wrote:
> I would think we would want the plugin_impl_* stuff in a non-conditionalized
> 'sources' section.

Done, I think. Does every gyp file need to define that file exclusion/inclusion
logic based on _mac etc?

http://codereview.chromium.org/2075006/diff/23001/24003#newcode43
webkit/default_plugin/default_plugin.gyp:43: # Figure out why once they're
needed.
On 2010/05/18 22:19:47, stuartmorgan wrote:
> Looks like webkit_support depends on 'glue' in webkit_glue, which in turn
> depends on default_plugin.

Thanks. I meant to say "figure out why and how to resolve once…" i guess :-)

http://codereview.chromium.org/2075006/diff/23001/24004
File webkit/default_plugin/plugin_impl_mac.h (right):

http://codereview.chromium.org/2075006/diff/23001/24004#newcode29
webkit/default_plugin/plugin_impl_mac.h:29: };
On 2010/05/18 22:19:47, stuartmorgan wrote:
> Can we consolidate some of this cross-platform stuff? In fact, looking at the
> diffs it looks like this should never have been forked in the first place;
just
> combine the headers and #ifdef where there aren't easy abstractions (like we
do
> with webplugin_delegate_impl.h)

I wanted to split this class into platform-specific view parts and xplatform
model parts when I implement the actual default plugin logic.

http://codereview.chromium.org/2075006/diff/23001/24005
File webkit/default_plugin/plugin_impl_mac.mm (right):

http://codereview.chromium.org/2075006/diff/23001/24005#newcode26
webkit/default_plugin/plugin_impl_mac.mm:26: //      install_dialog_(NULL),
On 2010/05/18 22:19:47, stuartmorgan wrote:
> Seriously? How instead of checking in acres of commented out code, you just
> refer to the Windows file as necessary while fleshing out the implementation.
> Maybe toss in a bunch of NOTIMPLEMENTED()s if you need placeholders (or just
> //TODOs if you prefer).

Done.

http://codereview.chromium.org/2075006/diff/23001/24005#newcode379
webkit/default_plugin/plugin_impl_mac.mm:379: NSSize text_size = [text
sizeWithAttributes:attributes];
On 2010/05/18 22:19:47, stuartmorgan wrote:
> I think we are supposed to use camelCase in obj-C code.

I was told that the type of the containing method determines style rule. This is
a c++ method.

http://codereview.chromium.org/2075006/diff/23001/24005#newcode384
webkit/default_plugin/plugin_impl_mac.mm:384: text_rect.origin.y =
(bounds.height - NSHeight(text_rect))/2;
On 2010/05/18 22:19:47, stuartmorgan wrote:
> Use NSMakeRect?

text_rect.origin depends on text_rect.size, so that's not possible

[stuartmorgan, 2010-05-18 23:12:03]

LGTM with one maybe-change

http://codereview.chromium.org/2075006/diff/27002/28003
File webkit/default_plugin/default_plugin.gyp (right):

http://codereview.chromium.org/2075006/diff/27002/28003#newcode22
webkit/default_plugin/default_plugin.gyp:22: },
Oh, we don't get this for free from a central file? I'll defer to mento on this
then, sorry.

http://codereview.chromium.org/2075006/diff/27002/28008
File webkit/default_plugin/plugin_main.cc (right):

http://codereview.chromium.org/2075006/diff/27002/28008#newcode136
webkit/default_plugin/plugin_main.cc:136: #endif
Maybe move this to a new NegotiateModels method to keep this method cleaner?

[Nico, 2010-05-19 00:24:39]

Thanks. I'll see what Mark says about the gyp file.

http://codereview.chromium.org/2075006/diff/27002/28008
File webkit/default_plugin/plugin_main.cc (right):

http://codereview.chromium.org/2075006/diff/27002/28008#newcode136
webkit/default_plugin/plugin_main.cc:136: #endif
On 2010/05/18 23:12:03, stuartmorgan wrote:
> Maybe move this to a new NegotiateModels method to keep this method cleaner?

Done.

[Nico, 2010-05-19 00:37:23]

http://codereview.chromium.org/2075006/diff/34001/12005
File webkit/default_plugin/default_plugin.gyp (right):

http://codereview.chromium.org/2075006/diff/34001/12005#newcode21
webkit/default_plugin/default_plugin.gyp:21: ],
Also, it looks like the project is not compiled with -Wall. Is there something I
can do to get all the standard compile flags for this lib? I tried adding

  'includes': [
    '../../build/common.gypi',
  ],

at the top, but that didn't help.

[Mark Mentovai, 2010-05-19 16:06:18]

LG from my perspective. I expect you’ll get a more complete review of the NPAPI
parts from Stuart.

http://codereview.chromium.org/2075006/diff/34001/12005
File webkit/default_plugin/default_plugin.gyp (right):

http://codereview.chromium.org/2075006/diff/34001/12005#newcode7
webkit/default_plugin/default_plugin.gyp:7: 'sources/': [
You should get all of this for free, and -Wall too, if you make this .gyp file
'chromium_code': 1 in its root 'variables' dict. This plugin is actually
Chromium code (as opposed to third-party), so that should be fine.

http://codereview.chromium.org/2075006/diff/34001/12007
File webkit/default_plugin/plugin_impl_mac.mm (right):

http://codereview.chromium.org/2075006/diff/34001/12007#newcode19
webkit/default_plugin/plugin_impl_mac.mm:19: // TODO(thakis): Most methods in
this class are stubbed out an need to be
an -> and

http://codereview.chromium.org/2075006/diff/34001/12007#newcode31
webkit/default_plugin/plugin_impl_mac.mm:31: DLOG(INFO) << __FUNCTION__ << "
Mime Type : " << mime_type;
" MIME Type: "

[Nico, 2010-05-19 20:08:09]

http://codereview.chromium.org/2075006/diff/34001/12005
File webkit/default_plugin/default_plugin.gyp (right):

http://codereview.chromium.org/2075006/diff/34001/12005#newcode7
webkit/default_plugin/default_plugin.gyp:7: 'sources/': [
On 2010/05/19 16:06:18, Mark Mentovai wrote:
> You should get all of this for free, and -Wall too, if you make this .gyp file
> 'chromium_code': 1 in its root 'variables' dict. This plugin is actually
> Chromium code (as opposed to third-party), so that should be fine.

Nice. Done.

http://codereview.chromium.org/2075006/diff/34001/12007
File webkit/default_plugin/plugin_impl_mac.mm (right):

http://codereview.chromium.org/2075006/diff/34001/12007#newcode19
webkit/default_plugin/plugin_impl_mac.mm:19: // TODO(thakis): Most methods in
this class are stubbed out an need to be
On 2010/05/19 16:06:18, Mark Mentovai wrote:
> an -> and

Done.

http://codereview.chromium.org/2075006/diff/34001/12007#newcode31
webkit/default_plugin/plugin_impl_mac.mm:31: DLOG(INFO) << __FUNCTION__ << "
Mime Type : " << mime_type;
On 2010/05/19 16:06:18, Mark Mentovai wrote:
> " MIME Type: "

Done.

[Nico, 2010-05-20 19:47:22]

Reverted, 'cause it broke nacl_ui_tests. If I remove default_plugin.cc from the
gyp file, it seems to work. default_plugin.cc only declares the NPAPI extern "C"
functions, that probably conflicts with something in nacl. Since the default
plugin is an internal plugin, it's probably ok to just delete default_plugin.cc.

[Nico, 2010-05-20 21:15:39]

Filed http://code.google.com/p/chromium/issues/detail?id=44681 about why NaCl
was surprised with this change. I moved default_plugin.cc to windows-only for
this CL and will look into deleting it wholesale in a followup CL.