Bug 13029

Created attachment 13569 [details] Proposed solution

Please review existing patch, but note I'm working on adding a test as well, using the test plugin. Will include this in next version of patch, as well as any suggested changes. Thanks.

Comment on attachment 13569 [details] Proposed solution Looks great. I'm a little concerned that the headers string is in random (dictionary key) order. I'd prefer some sort of sorted order. That could be done using keysSortedByValueUsingSelector: and then objectEnumerator instead of just keyEnumerator. It seems a shame that we have to compose this string every time even though it's only used for certain headers. I think it would be better if the headers were in the form of NSData rather than NSString. I think we want NSUTF8StringEncoding rather than NSASCIIStringEncoding. Is it really safe to claim to have features like XPConnect scripting (which we almost certainly won't ever implement), form values, and popups enabled state? How can the plug-in detect the fact that we don't implement those?

(In reply to comment #3) Thanks Darin. I'm revising the patch to address these comments and add a test case, but I'll respond here now in advance of having the next version ready. > I'm a little concerned that the headers string is in random (dictionary key) > order. I'd prefer some sort of sorted order. That could be done using > keysSortedByValueUsingSelector: and then objectEnumerator instead of just > keyEnumerator. Fair enough, the order isn't supposed to matter, but stable ordering is a good thing. If we had access to the raw HTTP stream, we could send things through verbatim, but since we must choose an ordering, a stable one seems like a good idea. I'm going for sorting on header names rather than header values, using allKeys and sortedArrayUsingSelector:@selector(caseInsensitiveCompare:). > It seems a shame that we have to compose this string every time even though > it's only used for certain headers. Agreed, but I don't know how to solve this problem, given that we don't have access to the raw HTTP stream. We could define a more complex interface via the NPAPI (get status line, enumerate all headers, get specific header), but simple seems good for cross-vendor standards, and plugin-futures has already approved this API, and Opera has already implemented it. Since the string only needs to be built once per URL transfer, it doesn't seem likely to have a major performance impact, but if someone can point to a performance regression, I'll certainly see what I can do. > I think it would be better if the headers were in the form of NSData rather > than NSString. I'm still learning here, so I'm happy to make this change if someone can give me a more detailed reason why. As far as I can tell, though, this choice of NSData vs. NSString is only used in one place (the interface between startStreamWithResponse: and startStreamResponseURL:etc), the difference in container classes is minor, the headers are in this case in fact ASCII text, and the string classes permit handy manipulation through things like appendFormat:. NSString seemed natural to me in these circumstances. Again, tell me why I'm being thick, and I'll change it... > I think we want NSUTF8StringEncoding rather than NSASCIIStringEncoding. You're probably right. I've made this change, and added the following comment: // HACK: pass the headers through as UTF-8. // This is not the intended behavior; we're supposed to pass original bytes verbatim. // But we don't have the original bytes, we have NSStrings built by the URL loading system. // It hopefully shouldn't matter, since RFC2616/RFC822 require ASCII-only headers, // but surely someone out there is using non-ASCII characters, and hopefully UTF-8 is adequate here. // It seems better than NSASCIIStringEncoding, which will lose information if non-ASCII is used. > Is it really safe to claim to have features like XPConnect scripting (which we > almost certainly won't ever implement), form values, and popups enabled state? > How can the plug-in detect the fact that we don't implement those? A tricky topic, since the NPAPI versioning system wasn't designed with optional features in mind. But I've reassured myself that all the version numbers we're hopping over are handled safely: 12 (actual) or 13 (documented): XPConnect. WebKit was already 14 before I came along, to promise NPRuntime, and the implicit promise of XPConnect doesn't seem to have done any harm. I think the reason is that XPConnect starts out with either the plugin or the browser querying the other for a root XPConnect object, using NPN_GetValue or NPP_GetValue, and if NULL is returned, the attempt is aborted. NULL is the value that comes back for the "I don't understand that variable" situation. Anyone not checking for NULL when calling NPN_GetValue or NPP_GetValue is asking for trouble. 15: Plugin form values. This actually doesn't seem to promise anything on the browser's part, only on the plugin's part. The plugin supports an additional variable in NPP_GetValue that indicates the plugin's (text-only) value for a form submission. And, as above, a plugin is free to return NULL here. 16: Popups enabled state. This just promises the existence of NPN_PushPopupsEnabledState and NPN_PopPopupsEnabledState. My patch includes stub implementations of these that can safely be called, but don't do anything. If anyone wants to implement these for real, enter a separate bug and ask on plugin-futures for a detailed spec of what they do. The basic idea is that they allow a plugin to inform the browser "I'm handling a user input event, so don't disable popups right now", which is useful because the browser doesn't know about input events to plugins on some systems, e.g. Windows. On the Mac, however, I think all events go through the browser, so it's probably a non-issue except in WebKit ports. 17: That's this feature, which would end up implemented as promised. New patch soon, hopefully tomorrow. Thanks again to anyone who's reading.

(In reply to comment #4) > If we had access to the raw HTTP stream, we could send things through verbatim, I'd like to address this eventually by requesting a new feature in the NSURLResponse API. Would you be willing to file a bug about that at <http://bugreport.apple.com>? In the short term I assume this won't happen but in the longer term it would be good. > I'm going for sorting on header names rather than header values, using > allKeys and sortedArrayUsingSelector:@selector(caseInsensitiveCompare:). Right. Even though that's not what I said, it *is* what I meant. > > I think it would be better if the headers were in the form of NSData rather > > than NSString. > > I'm still learning here, so I'm happy to make this change if someone can give > me a more detailed reason why. As far as I can tell, though, this choice of > NSData vs. NSString is only used in one place (the interface between > startStreamWithResponse: and startStreamResponseURL:etc), the difference in > container classes is minor, the headers are in this case in fact ASCII text, > and the string classes permit handy manipulation through things like > appendFormat:. NSString seemed natural to me in these circumstances. Again, > tell me why I'm being thick, and I'll change it... The only convincing argument in favor of NSString is the appeal appendFormat:. I think it's wrong, but not terribly so, to convert to and from a UTF-16 string here. Although NSString looks abstract, really it's effectively UTF-16. I think it's relatively easy to append to an NSMutableData with appendBytes:length: and I slightly prefer that approach. > You're probably right. I've made this change, and added the following comment: Good comment. > 15: Plugin form values. This actually doesn't seem to promise anything on the > browser's part, only on the plugin's part. The plugin supports an additional > variable in NPP_GetValue that indicates the plugin's (text-only) value for a > form submission. And, as above, a plugin is free to return NULL here. Wow, we should really implement that. It would be *so* easy. Would you be willing to file another bug in bugs.webkit.org about this feature?

One other thing I forgot! We should make the test plug-in in DumpRenderTree use this so we can make a regression test for it.

(In reply to comment #2) > Please review existing patch, but note I'm working on adding a test as well, > using the test plugin. Oh, oops. I missed this. Great news!

I think the comments "not implemented in WebKit" in the npapi.h header are a bit misleading. For one thing, these features are "not implemented *yet* in WebKit". For another, this really should be a shared copy of a common header, not a WebKit-specific one. I can't decide what I want here, but I figured I'd at least share my thoughts.

(In reply to comment #5) > I'd like to address this eventually by requesting a new feature in the > NSURLResponse API. Would you be willing to file a bug about that at > <http://bugreport.apple.com>? Sure, if you think it would be effective - but I'm surprised to hear you suggest this, since it seems like the request might carry more weight coming from inside Apple. Maybe it's just the opposite? Let me know. > I think it's wrong, but not terribly so, to convert to and from a UTF-16 string > here. Although NSString looks abstract, really it's effectively UTF-16. But I don't see how there'd be any conversion taking place. If I look at the objects that come out of the NSDictionary returned by allHeaders, they are all NSCFString, both keys and values. It seems that the URL Loading System has already converted everything to UTF-16. Still, I will go ahead and change to NSData, because it reinforces the way we actually would prefer it to work in the future: just send a sequence of bytes. > > 15: Plugin form values. > Wow, we should really implement that. It would be *so* easy. Would you be > willing to file another bug in bugs.webkit.org about this feature? Done: bug 13061.

(In reply to comment #8) > I think the comments "not implemented in WebKit" in the npapi.h header are a > bit misleading. For one thing, these features are "not implemented *yet* in > WebKit". For another, this really should be a shared copy of a common header, > not a WebKit-specific one. > > I can't decide what I want here, but I figured I'd at least share my thoughts. Funny, I actually started by writing "not yet implemented in WebKit". But then I changed my mind and decided to just imitate what was already written elsewhere in many places in npapi.h. After all, who am I to decide what will never be implemented and what is expected to be implemented eventually? This needs feedback from many other people than me, and I'd prefer to land this with the current generic comments and then let others tweak them to suit their more informed ideas of what's to come. When you say a shared copy of a common header, I'm not sure I understand precisely what that means, but I did find the location of npapi.h in JavaScriptCore to be somewhat odd. However, that's definitely where it seems to be, and there are no other apparent copies in the WebKit code. If you feel it should be moved, you'll need to either accomplish that independently of these changes, or give me some very specific advice on where to move the file to and why.

(In reply to comment #9) > > I'd like to address this eventually by requesting a new feature in the > > NSURLResponse API. Would you be willing to file a bug about that at > > <http://bugreport.apple.com>? > > Sure, if you think it would be effective - but I'm surprised to hear you > suggest this, since it seems like the request might carry more weight coming > from inside Apple. Maybe it's just the opposite? Let me know. It will carry a lot of weight if you report the bug and then I push it inside the company. Can't really say which way will carry the most weight, but the context of a request from a developer at Adobe won't hurt.

(In reply to comment #11) > It will carry a lot of weight if you report the bug and then I push it inside > the company. Okay, it's a deal, I'll enter a Radar bug. But, if it's OK with you, I think I'll wait until this patch is landed. I think that would have two advantages: (1) once the patch is landed, I can easily refer to the new code as an example of where the missing API would have been handy; (2) by waiting to file the bug, I avoid any confusion at Apple about priorities, increasing my odds of getting this patch in without holding it up for the potentially more difficult job of adding a new Cocoa API.

Through plugin-futures, I have been informed that version 17, which I had been using, has already been taken, for a feature called NPRuntime object enumeration. I have entered bug 13064 to track that as an enhancement request. When I submit my next patch (ah, how these things can take longer than one expects), I will be declaring version 18. This is actually a little hairier than the other version skips I discuss in comment #4. Version 17 added both NPN_Enumerate and NPClass::enumerate. I really don't want the HTTP header work to get hung up waiting for NPRuntime enumeration, and I don't have the time or expertise to code NPRuntime enumeration, so I think I've found a simple workaround: declare version 18 in NPN_Version and NPNetscapeFuncs::version, but fill in NPNetscapeFuncs::enumerate as NULL, and declare version 1 (rather than 2) for NPClass::structVersion. This will mean that plugins can only run into trouble if they blindly switch on NPN_Version, without checking that NPNetscapeFuncs::enumerate is non-NULL, or without checking that NPClass::structVersion is >= 2. In addition, I'm not sure there are any plugins out there that could be using this yet; AFAIK it has not yet landed in any shipping browsers, so any plugins attempting to use this will hopefully see WebKit having this stub implementation before they release anything depending on a real implementation. This is definitely less than ideal, but similar things have happened in many browsers due to the NPAPI's lousy flat versioning scheme. On the bright side, bug 13064 is probably worth fixing anyway, and if it is, then that's one less lie that our NPN_Version will be telling. (It is already telling several.)

Created attachment 13640 [details] Proposed solution, take 2 Okay! New patch, with Darin's suggested changes, plus a test case (plugin C code and JavaScript to confirm results). I have stuck with NPAPI version 17, after a discussion on plugin-futures. It seems no browsers have ever shipped declaring version 17, or including NPRuntime enumeration, so people seem to be okay with the idea of assigning version 17 to response headers, and changing enumeration to be version 18. I also have a patch ready that deals with things the other way around, where enumeration is 17 and headers are 18, but things are simpler if we don't have to use that. I'll assume that landing this patch will have to wait until a final decision is made on versions, but since the differences are tiny, I think this patch is ready for review and stands a chance of being the final version. Many thanks to Darin and everyone else who's helped out.

Created attachment 13641 [details] Proposed solution, take 3 Sorry, I'm an idiot. Global variables in the test plugin code meant that it didn't work when instantiated multiple times. Another patch version to fix this.

Comment on attachment 13641 [details] Proposed solution, take 3 Some of your new code in PluginObject.c uses if statements with the body on the same line as the if. This goes against our coding style guidelines (<http://webkit.org/coding/coding-style.html>). There are also a few instances of single-line if statements having braces around their bodies. We omit them in this situation. Rather than using cStringUsingEncoding: followed by appendBytes:length: it would be cleaner to use dataUsingEncoding:. This would also acommodate nul bytes in the headers, though I suppose it's unlikely that this will happen. You also seem to nul-terminate theHeaders twice -- once when creating the NSMutableData, and again when converting it to a byte array. Your test case looks good. Some of the JavaScript doesn't match the style guidelines (braces on same line as function declaration, one-line if statements with braces, etc.) It'd be good to have another pass over this patch before it goes in.

Created attachment 13710 [details] Proposed solution, take 4 New patch addressing bdash's comments - thanks for the guidance. Updated everything to what I think is correct brace style. Made suggested change from cStringUsingEncoding: to dataUsingEncoding:. The double null termination was defensive coding, but I removed the redundant call and added a comment that the bottleneck lower down would do the job.

Comment on attachment 13710 [details] Proposed solution, take 4 + char statusStr[10]; + sprintf(statusStr, "%d", [httpResponse statusCode]); To avoid getting the security "cops" on our backs, lets use snprintf here instead of plain old sprintf. + if (obj && (browser->version >= NPVERS_HAS_RESPONSE_HEADERS)) { + notifyStream(obj, stream->url, stream->headers); + } No braces around a single-line if statement. I'm going to say r=me, but I'd like to see a version with snprintf.

Created attachment 13728 [details] Proposed solution, take 5 Thanks, both comments addressed. New patch.

Comment on attachment 13728 [details] Proposed solution, take 5 r=me

Being version 17 requires that we implement NPN_enumerate too. I have a patch which does that so I'll land this after I've landed the first patch

(In reply to comment #21) > Being version 17 requires that we implement NPN_enumerate too. Actually no! After discussion on plugin-futures, we decided that NPN_enumerate would become version 18 and response headers would take over 17. See comment #14. It would still be great to implement NPN_enumerate, and see bug 13064 for that, but that's version 18. Check out http://lxr.mozilla.org/seamonkey/source/modules/plugin/base/public/npapi.h - this is the latest in Mozilla, with: #define NPVERS_HAS_RESPONSE_HEADERS 17 #define NPVERS_HAS_NPOBJECT_ENUM 18

I have tried to land this patch, but it didn't apply cleanly, and I couldn't resolve the conflicts manually.

Sorry, I was out last week. Man, the code velocity here is higher than I realized. I'll make a new patch soon, hopefully tomorrow.

Created attachment 14013 [details] Proposed solution, take 6 New patch that should be compatible with latest code from svn trunk. Changes from last patch are: - No longer need to add NPObject enumeration stubs, since this is now implemented - Dovetail with code related to NPObject enumeration in test plugin - Deal with WebBaseNetscapePluginStream.m being renamed WebBaseNetscapePluginStream.mm - Fix a bug in my usage of snprintf in PluginObject.c Otherwise it's the same code. Builds, loads pages, passes LayoutTests. Hopefully review is short & sweet this time around, so this can get landed before it rots again :) Thanks.

Comment on attachment 14013 [details] Proposed solution, take 6 r=me

Created attachment 14014 [details] Proposed solution, take 6, really Uh... with the right attachment this time

Created attachment 14015 [details] Proposed solution, take 6, i swear Dude, I suck, this is really the right attachment this time (ack, many copies of the codebase sitting around)

Committed revision 20867.

Looks good in a build of the latest code. Thanks everybody!