37939 2010-04-21 11:54:45 -0700 Fix PluginViewNone.cpp compilation for ports that also compile PluginView.cpp 2010-06-23 10:54:46 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 kevino webkit-unassigned oldest_to_newest 215435 0 kevino 2010-04-21 11:54:45 -0700 Fix PluginViewNone.cpp compilation for ports that also compile PluginView.cpp 215495 1 53990 kevino 2010-04-21 14:11:22 -0700 Created attachment 53990 Fix PluginViewNone.cpp for ports that compile PluginView.cpp 215597 2 53990 darin 2010-04-21 18:08:56 -0700 Comment on attachment 53990 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? 215623 3 kevino 2010-04-21 18:25:46 -0700 (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). 215744 4 mrowe 2010-04-22 00:06:21 -0700 (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. 215947 5 kevino 2010-04-22 10:30:36 -0700 (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? 237410 6 53990 darin 2010-06-12 18:59:53 -0700 Comment on attachment 53990 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. 237700 7 kevino 2010-06-13 21:39:30 -0700 (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? 241887 8 59521 kevino 2010-06-23 09:34:36 -0700 Created attachment 59521 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. 241888 9 59521 darin 2010-06-23 09:35:59 -0700 Comment on attachment 59521 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 241937 10 kevino 2010-06-23 10:54:46 -0700 Members changed to functions and landed in r61697, thanks! 53990 2010-04-21 14:11:22 -0700 2010-06-23 09:34:36 -0700 Fix PluginViewNone.cpp for ports that compile PluginView.cpp pluginviewnone.patch text/plain 1623 kevino SW5kZXg6IFdlYkNvcmUvcGx1Z2lucy9QbHVnaW5WaWV3Tm9uZS5jcHAKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g V2ViQ29yZS9wbHVnaW5zL1BsdWdpblZpZXdOb25lLmNwcAkocmV2aXNpb24gNTc5OTkpCisrKyBX ZWJDb3JlL3BsdWdpbnMvUGx1Z2luVmlld05vbmUuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMjAs NiArMTIwLDE0IEBACiB7CiB9CiAKKy8vIEZJWE1FOiBQbHVnaW5WaWV3Tm9uZS5jcHAgc2hvdWxk IGJlIGEgZHJvcC1pbiBzdHVicyBmaWxlIGZvciBhIHBvcnQncyAKKy8vIFBsdWdpblZpZXc8UG9y dD4uY3BwIGZpbGUgYW5kIHNob3VsZCBvbmx5IGNvbnRhaW4gcG9ydC1zcGVjaWZpYyBtZXRob2Rz LiAKKy8vIFRoZXNlIHRocmVlIEFQSXMgaW4gdGhlIFBMQVRGT1JNKE1BQykgYmxvY2sgYXJlIG5v dCBwb3J0LXNwZWNpZmljIGFuZCAKKy8vIGFyZSBvbmx5IGhlcmUgYmVjYXVzZSB0aGUgTWFjIHBv cnQgd2lzaGVzIG5vdCB0byBjb21waWxlIFBsdWdpblZpZXcuY3BwLCAKKy8vIHdoZXJlIHRoZSBp bXBsZW1lbnRhdGlvbnMgZm9yIHRoZXNlIG1ldGhvZHMgbGl2ZS4gTm90ZSBhbHNvIHRoYXQgaXQg ZG9lcyBub3QKKy8vIG5lZWQgYW55IG9mIHRoZSBvdGhlciBtZXRob2Qgc3R1YnMgaW4gdGhpcyBm aWxlLiBUaGVzZSBmdW5jdGlvbnMgcHJvYmFibHkKKy8vIGJlbG9uZyBpbiBhIE1hYy1zcGVjaWZp YyBmaWxlIHNvbWV3aGVyZS4KKyNpZiBQTEFURk9STShNQUMpCiAjaWYgRU5BQkxFKE5FVFNDQVBF X1BMVUdJTl9BUEkpCiB2b2lkIFBsdWdpblZpZXc6OmtlZXBBbGl2ZShOUFApCiB7CkBAIC0xMzMs NiArMTQxLDcgQEAKIHZvaWQgUGx1Z2luVmlldzo6c2V0SmF2YVNjcmlwdFBhdXNlZChib29sKQog ewogfQorI2VuZGlmCiAKICNpZiBkZWZpbmVkKFhQX1VOSVgpICYmIEVOQUJMRShORVRTQ0FQRV9Q TFVHSU5fQVBJKQogdm9pZCBQbHVnaW5WaWV3OjpoYW5kbGVGb2N1c0luRXZlbnQoKQpAQCAtMTQz LDUgKzE1Miw0IEBACiB7CiB9CiAjZW5kaWYKLQogfSAvLyBuYW1lc3BhY2UgV2ViQ29yZQpJbmRl eDogV2ViQ29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9DaGFuZ2VMb2cJKHJl dmlzaW9uIDU3OTk5KQorKysgV2ViQ29yZS9DaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAgLTEs MyArMSwxMiBAQAorMjAxMC0wNC0yMSAgS2V2aW4gT2xsaXZpZXIgIDxrZXZpbm9AdGhlb2xsaXZp ZXJzLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBGaXggUGx1Z2luVmlld05vbmUuY3BwIGNvbXBpbGF0aW9uIGZvciBwb3J0cyB0aGF0IGFsc28g Y29tcGlsZSBQbHVnaW5WaWV3LmNwcAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9Mzc5MzkKKworICAgICAgICAqIHBsdWdpbnMvUGx1Z2luVmlld05vbmUu Y3BwOgorCiAyMDEwLTA0LTIxICBBbGV4ZXkgUHJvc2t1cnlha292ICA8YXBAYXBwbGUuY29tPgog CiAgICAgICAgIFRpZ2VyIGJ1aWxkIGZpeC4K 59521 2010-06-23 09:34:36 -0700 2010-06-23 09:35:59 -0700 Patch w/updated comment and rebased to latest trunk pluginview.patch text/plain 2221 kevino SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2MTY5MikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTcgQEAKKzIwMTAtMDYtMjMgIEtldmluIE9sbGl2aWVyICA8a2V2aW5vQHRoZW9s bGl2aWVycy5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgRml4IFBsdWdpblZpZXdOb25lLmNwcCBjb21waWxhdGlvbiBmb3IgcG9ydHMgdGhhdCBh bHNvIGNvbXBpbGUgUGx1Z2luVmlldy5jcHAKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTM3OTM5CisKKyAgICAgICAgKiBwbHVnaW5zL1BsdWdpblZpZXdO b25lLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlBsdWdpblZpZXc6OmhhbmRsZUZvY3VzSW5FdmVu dCk6CisgICAgICAgIChXZWJDb3JlOjpQbHVnaW5WaWV3OjpoYW5kbGVGb2N1c091dEV2ZW50KToK KyAgICAgICAgKFdlYkNvcmU6OlBsdWdpblZpZXc6OmtlZXBBbGl2ZSk6CisgICAgICAgIChXZWJD b3JlOjpQbHVnaW5WaWV3Ojpwcml2YXRlQnJvd3NpbmdTdGF0ZUNoYW5nZWQpOgorICAgICAgICAo V2ViQ29yZTo6UGx1Z2luVmlldzo6c2V0SmF2YVNjcmlwdFBhdXNlZCk6CisKIDIwMTAtMDYtMjMg IFNpbW9uIEZyYXNlciAgPHNpbW9uLmZyYXNlckBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3 ZWQgYnkgRGFuIEJlcm5zdGVpbi4KSW5kZXg6IFdlYkNvcmUvcGx1Z2lucy9QbHVnaW5WaWV3Tm9u ZS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9wbHVnaW5zL1BsdWdpblZpZXdOb25lLmNwcAko cmV2aXNpb24gNjE2OTIpCisrKyBXZWJDb3JlL3BsdWdpbnMvUGx1Z2luVmlld05vbmUuY3BwCSh3 b3JraW5nIGNvcHkpCkBAIC0xMjAsMjggKzEyMCwzMyBAQAogewogfQogCi0jaWYgRU5BQkxFKE5F VFNDQVBFX1BMVUdJTl9BUEkpCi12b2lkIFBsdWdpblZpZXc6OmtlZXBBbGl2ZShOUFApCisjaWYg ZGVmaW5lZChYUF9VTklYKSAmJiBFTkFCTEUoTkVUU0NBUEVfUExVR0lOX0FQSSkKK3ZvaWQgUGx1 Z2luVmlldzo6aGFuZGxlRm9jdXNJbkV2ZW50KCkKIHsKIH0KLSNlbmRpZgogCi0jaWYgUExBVEZP Uk0oTUFDKSB8fCBQTEFURk9STShDSFJPTUlVTSkgfHwgUExBVEZPUk0oRUZMKQotdm9pZCBQbHVn aW5WaWV3Ojpwcml2YXRlQnJvd3NpbmdTdGF0ZUNoYW5nZWQoYm9vbCkKK3ZvaWQgUGx1Z2luVmll dzo6aGFuZGxlRm9jdXNPdXRFdmVudCgpCiB7CiB9CisjZW5kaWYKIAotdm9pZCBQbHVnaW5WaWV3 OjpzZXRKYXZhU2NyaXB0UGF1c2VkKGJvb2wpCisvLyBUaGUgbWV0aG9kcyBiZWxvdyBhcmUgZm9y IHBsYXRmb3JtcyB0aGF0IGRvIG5vdCB1c2UgUGx1Z2luVmlldyBmb3IgcGx1Z2lucworLy8gZHVl IHRvIGFyY2hpdGVjdHVyYWwgZGlmZmVyZW5jZXMuIFRoZSBwbGFuIGlzIHRvIGV2ZW50dWFsbHkg aGF2ZSBhbGwKKy8vIHBvcnRzIHVzaW5nIFBsdWdpblZpZXcsIGJ1dCB1bnRpbCB0aGVuLCBpZiBu ZXcgbWV0aG9kcyBsaWtlIHRoaXMgYXJlIAorLy8gYWRkZWQsIHBsZWFzZSBtYWtlIHN1cmUgdGhl eSBoYXZlIHRoZSBwcm9wZXIgcGxhdGZvcm0gI2lmcyBzbyB0aGF0IGNoYW5nZXMKKy8vIGRvIG5v dCBicmVhayBwb3J0cyB3aG8gY29tcGlsZSBib3RoIHRoaXMgZmlsZSBhbmQgUGx1Z2luVmlldy5j cHAuICAgCisjaWYgUExBVEZPUk0oTUFDKSB8fCBQTEFURk9STShDSFJPTUlVTSkgfHwgUExBVEZP Uk0oRUZMKQorI2lmIEVOQUJMRShORVRTQ0FQRV9QTFVHSU5fQVBJKQordm9pZCBQbHVnaW5WaWV3 OjprZWVwQWxpdmUoTlBQKQogewogfQogI2VuZGlmCiAKLSNpZiBkZWZpbmVkKFhQX1VOSVgpICYm IEVOQUJMRShORVRTQ0FQRV9QTFVHSU5fQVBJKQotdm9pZCBQbHVnaW5WaWV3OjpoYW5kbGVGb2N1 c0luRXZlbnQoKQordm9pZCBQbHVnaW5WaWV3Ojpwcml2YXRlQnJvd3NpbmdTdGF0ZUNoYW5nZWQo Ym9vbCkKIHsKIH0KIAotdm9pZCBQbHVnaW5WaWV3OjpoYW5kbGVGb2N1c091dEV2ZW50KCkKK3Zv aWQgUGx1Z2luVmlldzo6c2V0SmF2YVNjcmlwdFBhdXNlZChib29sKQogewogfQogI2VuZGlmCg==