Bug 37939 – Fix PluginViewNone.cpp compilation for ports that also compile PluginView.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Kevin Ollivier]

Fix PluginViewNone.cpp compilation for ports that also compile PluginView.cpp

[Kevin Ollivier]

Created attachment 53990 [details]
Fix PluginViewNone.cpp for ports that compile PluginView.cpp

[Darin Adler]

Comment on attachment 53990 [details]
Fix PluginViewNone.cpp for ports that compile PluginView.cpp

> +// FIXME: PluginViewNone.cpp should be a drop-in stubs file for a port's 
> +// PluginView<Port>.cpp file and should only contain port-specific methods. 
> +// These three APIs in the PLATFORM(MAC) block are not port-specific and 
> +// are only here because the Mac port wishes not to compile PluginView.cpp, 
> +// where the implementations for these methods live. Note also that it does not
> +// need any of the other method stubs in this file. These functions probably
> +// belong in a Mac-specific file somewhere.

This wall of text is confusing!

Can you do something better so you don't have to check this in?

[Kevin Ollivier]

(In reply to comment #2)
> (From update of attachment 53990 [details])
> > +// FIXME: PluginViewNone.cpp should be a drop-in stubs file for a port's 
> > +// PluginView<Port>.cpp file and should only contain port-specific methods. 
> > +// These three APIs in the PLATFORM(MAC) block are not port-specific and 
> > +// are only here because the Mac port wishes not to compile PluginView.cpp, 
> > +// where the implementations for these methods live. Note also that it does not
> > +// need any of the other method stubs in this file. These functions probably
> > +// belong in a Mac-specific file somewhere.
> 
> This wall of text is confusing!
> 
> Can you do something better so you don't have to check this in?

I'm open to suggestions, but the fact that these methods are defined in this file is the source of the confusion, and if we're to avoid similar breakages in the future, we need to make sure people are clear about what the Mac port is using this file for and how it differs from what everyone else is using this file for. 

To avoid this approach, the three methods in this file needed by Mac need to either go somewhere else (if so, where?) or we need to search the codebase for the calls to these three methods and preface them with #if !PLATFORM(MAC). Any thoughts / preferences? I'm open to any ideas that fix this right so that people don't keep adding more stubbed versions of methods from PluginView.cpp into this file (which will break compilation for pretty much any other port using it).

[Mark Rowe (bdash)]

(In reply to comment #3)
> I'm open to suggestions, but the fact that these methods are defined in this
> file is the source of the confusion, and if we're to avoid similar breakages in
> the future, we need to make sure people are clear about what the Mac port is
> using this file for and how it differs from what everyone else is using this
> file for. 

This is clearly not specific to the Mac since there are Chromium-related conditions in the #if statements.

> To avoid this approach, the three methods in this file needed by Mac need to
> either go somewhere else (if so, where?) or we need to search the codebase for
> the calls to these three methods and preface them with #if !PLATFORM(MAC). Any
> thoughts / preferences? I'm open to any ideas that fix this right so that
> people don't keep adding more stubbed versions of methods from PluginView.cpp
> into this file (which will break compilation for pretty much any other port
> using it).

I think that the right way to fix it is to unify the manner in which platforms that do not use PluginView build.  Some ports do not compile PluginView.cpp at all.  Others compile both PluginView.cpp and PluginViewNone.cpp.  I think that having all ports that fall in to this category treat things in a consistent manner would address the problems.

[Kevin Ollivier]

(In reply to comment #4)
> (In reply to comment #3)
> > I'm open to suggestions, but the fact that these methods are defined in this
> > file is the source of the confusion, and if we're to avoid similar breakages in
> > the future, we need to make sure people are clear about what the Mac port is
> > using this file for and how it differs from what everyone else is using this
> > file for. 
> 
> This is clearly not specific to the Mac since there are Chromium-related
> conditions in the #if statements.

Actually, those were added after I submitted my patch, and that change largely does the same thing as my patch but without the note (and, it does not guard one extra method it should).

http://trac.webkit.org/changeset/58043/trunk/WebCore/plugins/PluginViewNone.cpp

> > To avoid this approach, the three methods in this file needed by Mac need to
> > either go somewhere else (if so, where?) or we need to search the codebase for
> > the calls to these three methods and preface them with #if !PLATFORM(MAC). Any
> > thoughts / preferences? I'm open to any ideas that fix this right so that
> > people don't keep adding more stubbed versions of methods from PluginView.cpp
> > into this file (which will break compilation for pretty much any other port
> > using it).
> 
> I think that the right way to fix it is to unify the manner in which platforms
> that do not use PluginView build.  Some ports do not compile PluginView.cpp at
> all.  Others compile both PluginView.cpp and PluginViewNone.cpp.  I think that
> having all ports that fall in to this category treat things in a consistent
> manner would address the problems.

So if we're going to keep this all in one file, we'd need a system along the lines of:

#if PLATFORM(MAC) || PLATFORM(CHROMIUM) || PLATFORM(EFL)
#define SUPPORTS_PLUGIN_VIEW 0
#else
#define SUPPORTS_PLUGIN_VIEW 1
#endif

#if SUPPORTS_PLUGIN_VIEW
// compile PluginView port-specific stubs
#else
// compile stubs for static PluginView methods or methods called after checking widget->IsPluginView()
#endif

Thoughts?

[Darin Adler]

Comment on attachment 53990 [details]
Fix PluginViewNone.cpp for ports that compile PluginView.cpp

> +// FIXME: PluginViewNone.cpp should be a drop-in stubs file for a port's 
> +// PluginView<Port>.cpp file and should only contain port-specific methods. 
> +// These three APIs in the PLATFORM(MAC) block are not port-specific and 
> +// are only here because the Mac port wishes not to compile PluginView.cpp, 
> +// where the implementations for these methods live. Note also that it does not
> +// need any of the other method stubs in this file. These functions probably
> +// belong in a Mac-specific file somewhere.

This comment is too big and I found it confusing. You should find a way of saying this with fewer words or just leave the comment out.

That part about "Mac port wishes not to compile" is inaccurate. WebKit contributors want the Mac port to share more code with the plug-in handling on the other ports. The Mac plug-in handling predates the plug-in handling done for other ports and the programmers who worked on plug-in machinery later on built on work done for the Windows port and never did the work to make the new code paths be used on Mac. If we really need a comment like this, a better one would make it clear that the future direction is to share more code and would not anthropomorphize the Mac port with a naughty uncooperative personality.

[Kevin Ollivier]

(In reply to comment #6)
> (From update of attachment 53990 [details])
> > +// FIXME: PluginViewNone.cpp should be a drop-in stubs file for a port's 
> > +// PluginView<Port>.cpp file and should only contain port-specific methods. 
> > +// These three APIs in the PLATFORM(MAC) block are not port-specific and 
> > +// are only here because the Mac port wishes not to compile PluginView.cpp, 
> > +// where the implementations for these methods live. Note also that it does not
> > +// need any of the other method stubs in this file. These functions probably
> > +// belong in a Mac-specific file somewhere.
> 
> This comment is too big and I found it confusing. You should find a way of saying this with fewer words or just leave the comment out.

I do think there needs to be a comment, as different ports are using this file for different purposes, and it seems neither are aware of what the other is using it for. Case in point, adding the Mac stubs broke the wx port, meaning that whoever added those methods did not realize that ports using this file might also compile PluginView.cpp. To prevent similar problems from happening in the future, I do think there should be some documentation of this issue.
 
> That part about "Mac port wishes not to compile" is inaccurate. WebKit contributors want the Mac port to share more code with the plug-in handling on the other ports. The Mac plug-in handling predates the plug-in handling done for other ports and the programmers who worked on plug-in machinery later on built on work done for the Windows port and never did the work to make the new code paths be used on Mac. If we really need a comment like this, a better one would make it clear that the future direction is to share more code and would not anthropomorphize the Mac port with a naughty uncooperative personality.

I apologize that I worded it poorly, and I'll try to use better phrasing in the next revision. In any case, the current patch won't apply cleanly with SVN head, should I resubmit or just fix it and land?

[Kevin Ollivier]

Created attachment 59521 [details]
Patch w/updated comment and rebased to latest trunk

The comment is a bit wordy still, but I think it's much clearer and easy to understand. I also moved those three methods to the bottom of the file so that there is a clear 'place' in the file for these methods to go, which will hopefully help keep things cleanly separated.

[Darin Adler]

Comment on attachment 59521 [details]
Patch w/updated comment and rebased to latest trunk

> +// The methods below are for platforms that do not use PluginView for plugins

The word "methods" is not a C++ term. The term in C++ is "functions" or "member functions".

r=me

[Kevin Ollivier]

Members changed to functions and landed in r61697, thanks!