I'm not really sure for what ports the version is correct. This was pointed out to me by Christian Perch, while discussing why the java plugin doesn't work for us: <chpe> PluginPackage.cpp: m_browserFuncs.version = NP_VERSION_MINOR; this line looks wrong, btw. that define is the latest version, not the one that you actually implement, which is 19 I think (at most!) if any plugin actually depends on something later, it might crash :) <kov> I sense platform differences weirdness chpe, I believe that is defined inside webkit chpe, http://trac.webkit.org/browser/trunk/WebCore/bridge/npapi.h <chpe> yes, in npapi.h, value "24" <chpe> but the actually exported functions are only up to pluginthreadasynccall, aka v19 <kov> right I'm attaching a patch that conditionalizes the version definition, in the hopes that we can figure out what should be done here.
Created attachment 48060 [details] Set the record straight regarding NPAPI compatibility, hopefully
Comment on attachment 48060 [details] Set the record straight regarding NPAPI compatibility, hopefully We need a test case, or at least a COMPILE_ASSERT in DumpRenderTree or something.
Comment on attachment 48060 [details] Set the record straight regarding NPAPI compatibility, hopefully This was just changed in another way by another patch approved today. At least I think it was. NPVersion(). I remember approving such a patch.
The incorrect reporting of NPAPI compatibility basically has been blocking #33044 (applet support patch for Qt port) moving forward from Qt 4.5.3's WebKit (branched from master at sha1: ed4a2c954ba6dd6f0ec4cc0c80f2ac14c8938ba7) for the better part of a couple of months of part-time hacking. The Java plugin on Windows invokes the unimplemented methods and mysteriously seg faults. I reverted the changes one by one till I hit changing NP_VERSION_MINOR from 23 to 20 in npapi.h, and then things stopped seg faulting.
Created attachment 51264 [details] Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo I'm attaching a work in progress patch to help implement the functions for NPAPI to bring it up to v1.9 support... It is a patch against the QtWebKit Week11 source (https://trac.webkit.org/wiki/QtWebKitJournal#WeeklyBuildWeek11) It builds on Windows and OSX, runs on Windows using the Java Applet patch without crashing. I can see NPN_GetValueForURL being exercised for both Proxy and Cookies when hitting http://www.java.com/en/download/help/testvm.xml on Windows. It could use some work in implementing a Qt agnostic means of retrieving Proxy values from the Network/Application layer. Please feel free to take a stab at improve it.
Should this be up for review? I think e don't want to commit this with debugging using qDebug, nor with out-commented code.
Created attachment 51714 [details] Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo -- Fixes proxy string logic problem I've updated the patch to fix the proxy support which had a logic error. It looks like some corruption is happening in the get value method for cookies. This is not seen in the debug statements of either implementation from the Qt app (this patch provides to functionally identical implementations in Qt and pure WebKit) but is observed as occasional extra junk being appended to the end of the Java cookies in the debug console, sometimes it indicates a cookie value is returned when there is no value supposed to be returned. Hosting test browser app: [2960] npapi.cpp - NPN_GetValueForURL:: Proxy url: http://www.javatester.org:80/ [2960] npapi.cpp - NPN_GetValueForURL::Proxy fetching Proxy from Qt [2960] npapi.cpp - NPN_GetValueForURL::Proxy DIRECT [2960] npapi.cpp - NPN_GetValueForURL:: Cookie url: http://www.javatester.org/JavaVersionDisplayApplet.class [2960] npapi.cpp - NPN_GetValueForURL::Cookies [ ] Length: 0 [2960] len is set before: 0x28d20c [2960] len is set after: 0x28d20c [2960] len is: 0 Java Console: Java Plug-in 1.6.0_17 Using JRE version 1.6.0_17-b04 Java HotSpot(TM) Client VM ... security: property package.definition new value com.sun.javaws,com.sun.deploy,com.sun.jnlp,org.mozilla.jss basic: Added progress listener: sun.plugin.util.GrayBoxPainter$GrayBoxProgressListener@b166b5 network: Cache entry found [url: http://www.javatester.org/JavaVersionDisplayApplet.class, version: null] network: Connecting http://www.javatester.org/JavaVersionDisplayApplet.class with proxy=DIRECT network: Connecting http://www.javatester.org:80/ with proxy=DIRECT network: Connecting http://www.javatester.org/JavaVersionDisplayApplet.class with cookie "°REC" <--- This is junk since there is no value returned from the host Qt app as far as I can tell. ... I suspect the problem is something to do with the statements in the if block at line 36 of the attached patch, however I'm unable to figure out what's happening (I've tried to implement it 2 ways, results are the same)... I'm not very good at low level C/C++, which is hence I use Qt for native compiled apps... Maybe I'm doing something obviously wrong (i.e. does the length pointer thing look okay)?
Created attachment 52222 [details] Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo -- Fixes junk being added to end of NPN_GetValueForURL strings This fix basically replaces memcpy w/ strcpy. memcpy was causing junk to be appended to the end of the strings. Next step is to clean this up to meet WebKit code submission guidelines. Someone could implement some kind of callback implementation for the NPN_GetAuthenticationInfo stub, but at the moment this function is not of particular interest to me.
Created attachment 55604 [details] Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo [Updated] Since there is no movement on this patch and Java applet support in QtWebKit depends on it, here is a cleaned up version of the original patch that removes all the swaps all the improper use of qDebug for LOG and makes minor code adjustment.
Attachment 55604 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/plugins/npapi.cpp:31: Alphabetical sorting problem. [build/include_order] [4] WebCore/plugins/npapi.cpp:190: NPN_GetValueForURL is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/plugins/npapi.cpp:228: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] WebCore/plugins/npapi.cpp:243: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/plugins/npapi.cpp:268: NPN_SetValueForURL is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/plugins/npapi.cpp:272: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] WebCore/plugins/npapi.cpp:297: NPN_GetAuthenticationInfo is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/plugins/npapi.cpp:302: Could not find a newline character at the end of the file. [whitespace/ending_newline] [5] Total errors found: 8 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 55604 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/2203113
Created attachment 55617 [details] Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo [Update II] Though it compiles fine here for me, corrected #include statements and order to attempt and fix problems reported by the Qt compile bot...
Attachment 55617 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/plugins/npapi.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/plugins/npapi.cpp:188: NPN_GetValueForURL is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/plugins/npapi.cpp:226: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] WebCore/plugins/npapi.cpp:241: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/plugins/npapi.cpp:266: NPN_SetValueForURL is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/plugins/npapi.cpp:270: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] WebCore/plugins/npapi.cpp:295: NPN_GetAuthenticationInfo is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/plugins/npapi.cpp:300: Could not find a newline character at the end of the file. [whitespace/ending_newline] [5] Total errors found: 8 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 55617 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/2172117
Attachment 55617 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/2227119
No clue why the qt or gtk build bots fail! Those things the error claims are undefined are defined in npapi.h. At least in my checked out version ot qtwebkit. Is the version on the build machine different ? old ? Perhaps I am missing something...
Attachment 55617 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/2230156
Created attachment 55694 [details] Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo [Update III] Maybe third time is the charm.... Copied the conditional defines that typedef unit32 and int32 from npapi.h.
Attachment 55694 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/plugins/npapi.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/plugins/npapi.cpp:202: NPN_GetValueForURL is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/plugins/npapi.cpp:240: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] WebCore/plugins/npapi.cpp:255: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/plugins/npapi.cpp:280: NPN_SetValueForURL is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/plugins/npapi.cpp:284: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] WebCore/plugins/npapi.cpp:309: NPN_GetAuthenticationInfo is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 7 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 55694 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/2178120
Comment on attachment 55694 [details] Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo [Update III] > + > +// FIXME: This is added to work around build failure by the qt/gtk build boats. > +// See comments # 11,14 & 15 of https://bugs.webkit.org/show_bug.cgi?id=34539. > +#ifndef _INT32 > +#define _INT32 > +#ifdef __LP64__ > +typedef int int32; > +typedef unsigned int uint32; > +#else /* __LP64__ */ > +typedef long int32; > +typedef unsigned long uint32; > +#endif /* __LP64__ */ > +#endif > + > + This is wrong. You should use the new C99 types instead, int32_t and uint32_t. Marking r- for this reason.
(In reply to comment #21) > (From update of attachment 55694 [details]) > > + > > +// FIXME: This is added to work around build failure by the qt/gtk build boats. > > +// See comments # 11,14 & 15 of https://bugs.webkit.org/show_bug.cgi?id=34539. > > +#ifndef _INT32 > > +#define _INT32 > > +#ifdef __LP64__ > > +typedef int int32; > > +typedef unsigned int uint32; > > +#else /* __LP64__ */ > > +typedef long int32; > > +typedef unsigned long uint32; > > +#endif /* __LP64__ */ > > +#endif > > + > > + > > This is wrong. You should use the new C99 types instead, int32_t and uint32_t. > > Marking r- for this reason. It is not by choice... See the function declaration for NPN{Get/Set}ValueForURL in WebCore/bridge/npapi.h. Actually the redefinition here is not necessary if the build bots (both gtk and qt) did not fail. However, if this definition is wrong here, then it is also wrong the header file I mentioned above...
(In reply to comment #22) > (In reply to comment #21) > > (From update of attachment 55694 [details] [details]) > > > + > > > +// FIXME: This is added to work around build failure by the qt/gtk build boats. > > > +// See comments # 11,14 & 15 of https://bugs.webkit.org/show_bug.cgi?id=34539. > > > +#ifndef _INT32 > > > +#define _INT32 > > > +#ifdef __LP64__ > > > +typedef int int32; > > > +typedef unsigned int uint32; > > > +#else /* __LP64__ */ > > > +typedef long int32; > > > +typedef unsigned long uint32; > > > +#endif /* __LP64__ */ > > > +#endif > > > + > > > + > > > > This is wrong. You should use the new C99 types instead, int32_t and uint32_t. > > > > Marking r- for this reason. > > It is not by choice... See the function declaration for NPN{Get/Set}ValueForURL in WebCore/bridge/npapi.h. Actually the redefinition here is not necessary if the build bots (both gtk and qt) did not fail. However, if this definition is wrong here, then it is also wrong the header file I mentioned above... The function declaration here? http://trac.webkit.org/browser/trunk/WebCore/bridge/npapi.h#L768 This uses uint32_t.
(In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > (From update of attachment 55694 [details] [details] [details]) > > > > + > > > > +// FIXME: This is added to work around build failure by the qt/gtk build boats. > > > > +// See comments # 11,14 & 15 of https://bugs.webkit.org/show_bug.cgi?id=34539. > > > > +#ifndef _INT32 > > > > +#define _INT32 > > > > +#ifdef __LP64__ > > > > +typedef int int32; > > > > +typedef unsigned int uint32; > > > > +#else /* __LP64__ */ > > > > +typedef long int32; > > > > +typedef unsigned long uint32; > > > > +#endif /* __LP64__ */ > > > > +#endif > > > > + > > > > + > > > > > > This is wrong. You should use the new C99 types instead, int32_t and uint32_t. > > > > > > Marking r- for this reason. > > > > It is not by choice... See the function declaration for NPN{Get/Set}ValueForURL in WebCore/bridge/npapi.h. Actually the redefinition here is not necessary if the build bots (both gtk and qt) did not fail. However, if this definition is wrong here, then it is also wrong the header file I mentioned above... > > The function declaration here? Yup... > http://trac.webkit.org/browser/trunk/WebCore/bridge/npapi.h#L768 > > This uses uint32_t. Hmmm... I see. The QtWebKit 2.0 branch contains an old npapi.h file then because the declarations I see there are completely different. Anyways, I will update the patch accordingly. Thanks...
Created attachment 55729 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and provide a stub for NPN_GetAuthenticationInfo [Update IV]... Fixed patch per feedback comment #21
Attachment 55729 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/plugins/npapi.cpp:28: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/plugins/npapi.cpp:188: NPN_GetValueForURL is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/plugins/npapi.cpp:226: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] WebCore/plugins/npapi.cpp:241: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/plugins/npapi.cpp:266: NPN_SetValueForURL is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/plugins/npapi.cpp:270: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] WebCore/plugins/npapi.cpp:295: NPN_GetAuthenticationInfo is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 7 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 55729 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/2285327
Can someone with karma please change the title of this bug report to reflect what the currently attached patches do ? Perhaps "Implement NPN_GetValueForURL and NPN_SetValueForURL and provide a stub for NPN_GetAuthenticationInfo" The original approach for addressing the issue is completely different from the latest patches...
Comment on attachment 55729 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and provide a stub for NPN_GetAuthenticationInfo [Update IV]... This patch needs a lot of love. WebCore/ChangeLog:6 + NPN_GetAuthenticationInfo, This ChangeLog doesn't tell me any of the "why" behind this patch. I can see the "what" by looking at the code, but I don't understand what problem we're solving with this patch. WebCore/plugins/npapi.cpp:196 + KURL URL = KURL(ParsedURLString, url); // NSURL *URL = [self URLWithCString:url]; Why would |url| be a ParsedURLString? That doesn't seem right. WebCore/plugins/npapi.cpp:205 + strcpy(*value, cookieStringUTF8.data()); please use strncpy ! WebCore/plugins/npapi.cpp:220 + #if defined(BUILDING_QT__) Surely this is the wrong preprocessor check. Also, why do we have Qt-specific code in a non-Qt-specific file? I think we need some better kind of abstraction here. WebCore/plugins/npapi.cpp:252 + memcpy(*value, proxyUTF8.data(), proxyUTF8.length()); Why do you use memcpy here but strcpy above? One of the two must be wrong. WebCore/plugins/npapi.cpp:289 + } You should have an explicit default case in to handle garbage from our caller. WebCore/plugins/npapi.cpp:259 + } You should have an explicit default case in to handle garbage from our caller. WebCore/plugins/npapi.cpp:271 + KURL URL = KURL(ParsedURLString, url); // NSURL *URL = [self URLWithCString:url]; same ParsedURLString comment here. WebCore/plugins/npapi.cpp:273 + String cookieString = String::fromUTF8(value, len); Are you sure this is supposed to be UTF8? WebCore/plugins/npapi.cpp:276 + Frame* frame = pluginViewForInstance(instance)->parentFrame(); parentFrame? Could be, but looks scary. WebCore/plugins/npapi.cpp:278 + setCookies(frame->document(), URL, cookieString); URL should be in lower case.
Created attachment 65021 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update V]... A cleaner implementation of the aforementioned NPN functions necessary for properly handling Java applets...
Comment on attachment 65021 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update V]... >WebCore/plugins/PluginView.cpp:1387 > + if (platformGetValueForURL(variable, url, value, len, &result)) > + return result; platformGetValueForURL() doesn't actually modify 'result' so you're returning uninitialized data here. >WebCore/plugins/PluginView.cpp:1393 > + if (u.isValid()) { If !isValid(), result should be set to an error code. >WebCore/plugins/PluginView.cpp:1398 > + strncpy(*value, cookieStr.utf8().data(), cookieStrLen); cookieStrLen should come from cookieStr.utf8().length() here. >WebCore/plugins/PluginView.cpp:1431 > + if (u.isValid()) { If !isValid(), result should be set to an error code. >WebCore/plugins/qt/PluginViewQt.cpp:71 > + #include <QString> Unnecessary include. >WebCore/plugins/qt/PluginViewQt.cpp:725 > + qDebug() << "PluginView::platformGetValueForURL:" << variable << "," << url; Please put this inside a preprocessor conditional of some sort. >WebCore/plugins/qt/PluginViewQt.cpp:758 > + strncpy(*value, proxyStr.toUtf8().constData(), proxyStrLen); Though safe in this particular case, proxyStrLen should come from proxyStr.toUtf8().length() >WebCore/plugins/PluginView.cpp:1455 > + LOG(Plugins, "PluginView::getAuthenticationInfo: protocol=%s, host=%d, port=%d", protocol, host, port); "host" is const char*, so it should be %s, not %d. Furthermore there are some coding style issues: - Tabs should always be 4 spaces - "type *foo" should be "type* foo"
Created attachment 65028 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update VI]... Corrected all of the issues pointed out in the review execpt for the "type *foo" should be "type* foo" because I cannot find where I did that and the code passes the check-webkit-style script as well. It is entirely possibe I might have missed it, but please point out where that is so it can get fixed...
BTW, here is the link to the java applet test page: http://www.java.com/en/download/help/testvm.xml
Comment on attachment 65028 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update VI]... >WebCore/plugins/PluginView.cpp:1386 > + NPError result; Should probably be initialized to NPERR_GENERIC_ERROR like in setValueForURL() below. >WebCore/plugins/PluginView.cpp:1393 > + if (u.isValid()) { Missing an "else" that sets result to NPERR_INVALID_URL. >WebCore/plugins/PluginView.cpp:1397 > + strncpy(*value, cookieStr.utf8().data(), cookieStr.length() + 1); cookieStr.utf8() may be of a different length than cookieStr.length() + 1. You should use the length() of cookieStr.utf8() instead of cookieStr itself. >WebCore/plugins/PluginView.cpp:1432 > + if (u.isValid()) { Missing an "else" that sets result to NPERR_INVALID_URL. >WebCore/plugins/PluginView.cpp:1452 > + NPError PluginView::getAuthenticationInfo(const char* protocol, const char* host, int32_t port, const char* scheme, const char *realm, char** username, uint32_t* ulen, char** password, uint32_t* plen) Coding style, "const char *realm" should be "const char* realm". This error is repeated a number of times in the patch. >WebCore/plugins/qt/PluginViewQt.cpp:754 > + *value = static_cast<char*>(NPN_MemAlloc(proxyStr.length() + 1)); If NPN_MemAlloc() fails, we should return NPN_OUT_OF_MEMORY_ERROR. >WebCore/plugins/qt/PluginViewQt.cpp:755 > + strncpy(*value, proxyStr.toUtf8().constData(), proxyStr.length() + 1); > + if (len) > + *len = strlen(*value); Again mixing the UTF8 data with the Unicode length. This should be something like: QByteArray utf8 = proxyStr.toUtf8(); memcpy(*value, utf8.constData(), utf8.length()); if (len) *len = utf8.length(); Furthermore, are we really supposed to return an error code when setting an empty cookie string? Same question for when getting the cookie string returns "".
Created attachment 65046 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update VII]... Addressed all the issues mentioned in comment #34. Let us see if the 3rd time is the charm...
Comment on attachment 65046 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update VII]... >WebCore/plugins/PluginDebug.cpp:171 > + default: return "Unkown variable"; Spelling mistake, should be "Unknown variable". >WebCore/plugins/PluginView.cpp:1387 > + if (platformGetValueForURL(variable, url, value, len, &result) || (result != NPERR_NO_ERROR)) The " || (result != NPERR_NO_ERROR)" is not needed since platformGetValueForURL() returns true after setting either result code. >WebCore/plugins/PluginView.cpp:1401 > + } else { Coding style, should be "} else" >WebCore/plugins/PluginView.cpp:1405 > + } else { Ditto. >WebCore/plugins/PluginView.cpp:1416 > + LOG(Plugins, "PluginView::getValueForURL(%s): Unknown variable type.", prettyNameForNPNURLVariable(variable).data()); This will log "PluginView::getValueForURL(Unknown variable): Unknown variable type." which isn't very helpful. >WebCore/plugins/PluginView.cpp:1429 > + if (platformSetValueForURL(variable, url, value, len, &result) || (result != NPERR_GENERIC_ERROR)) Same as above. platformSetValueForURL() should return true if it sets any result code IMO. >WebCore/plugins/PluginView.cpp:1440 > + } else { Coding style, should be "} else" >WebCore/plugins/PluginView.cpp:1461 > + if (platformGetAuthenticationInfo(protocol, host, port, scheme, realm, username, ulen, password, plen, &result) || (result != NPERR_GENERIC_ERROR)) Same comment as earlier about the superfluous " || (result != NPERR_GENERIC_ERROR)" >WebCore/plugins/qt/PluginViewQt.cpp:762 > + } else { Coding style, should be "} else" Overall though, it's starting to look pretty good. I'll let an authorized reviewer take it from here. :-)
(In reply to comment #36) > (From update of attachment 65046 [details]) > >WebCore/plugins/PluginDebug.cpp:171 > > + default: return "Unkown variable"; > Spelling mistake, should be "Unknown variable". > > >WebCore/plugins/PluginView.cpp:1387 > > + if (platformGetValueForURL(variable, url, value, len, &result) || (result != NPERR_NO_ERROR)) > The " || (result != NPERR_NO_ERROR)" is not needed since platformGetValueForURL() returns true after setting either result code. For the record I did that because it did not feel right to return true while setting an error from platformGetValueForURL, but I realized afterwards that the return value from these "platform" functions is to indicate whether or not the request got handled ; not to indicate an error condition occurred. Anyhow, fixed... > >WebCore/plugins/PluginView.cpp:1401 > > + } else { > Coding style, should be "} else" No clue why that would be seen as good coding practise in the first place, but whatever. One can easily shoot oneself in the foot in a deep nested if/else statement while following that practise. Still fixed. > >WebCore/plugins/PluginView.cpp:1416 > > + LOG(Plugins, "PluginView::getValueForURL(%s): Unknown variable type.", prettyNameForNPNURLVariable(variable).data()); > This will log "PluginView::getValueForURL(Unknown variable): Unknown variable type." which isn't very helpful. Yes, that is indeed useless. Fixed. I will post the final patch... Java applet support should work fine in QtWebKit once this patch gets accepted...
Created attachment 65051 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final Update]... Final update based on review in comment #36.
Comment on attachment 65051 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final Update]... > WebCore/ChangeLog:3 > + Reviewed by Andreas Kling Until the patch is landed this line should continue to say NOBODY (OOPS!). > :722 > +bool PluginView::platformGetValueForURL(NPNURLVariable variable, const char* url, char** value, uint32_t* len, NPError* result) Shouldn't this function use the QNetworkAccessManager's proxy settings instead of the application settings? > WebCore/plugins/symbian/PluginViewSymbian.cpp:341 > +bool PluginView::platformGetValueForURL(NPNURLVariable variable, const char* url, char** value, uint32_t* len, NPError* result) The current code duplication between the views sucks a bit, but in this "Symbian" implementation we should use the Qt bits and pieces. Qt is the system toolkit on Symbian. I think overall this patch looks okay now, I agree with Andreas. However there is one thing missing: Test coverage for the new code. At least the cookie bits should be relatively easy to cover with the test netscape plugin. I'm going to say r- primarily because of missing test coverage.
(In reply to comment #39) > (From update of attachment 65051 [details]) > > WebCore/ChangeLog:3 > > + Reviewed by Andreas Kling > Until the patch is landed this line should continue to say NOBODY (OOPS!). > > > :722 > > +bool PluginView::platformGetValueForURL(NPNURLVariable variable, const char* url, char** value, uint32_t* len, NPError* result) > Shouldn't this function use the QNetworkAccessManager's proxy settings instead of the application settings? > > > WebCore/plugins/symbian/PluginViewSymbian.cpp:341 > > +bool PluginView::platformGetValueForURL(NPNURLVariable variable, const char* url, char** value, uint32_t* len, NPError* result) > The current code duplication between the views sucks a bit, but in this "Symbian" implementation we should use the Qt bits and pieces. Qt is the system toolkit on Symbian. > > I think overall this patch looks okay now, I agree with Andreas. However there is one thing missing: Test coverage for the new code. At least the cookie bits should be relatively easy to cover with the test netscape plugin. > > I'm going to say r- primarily because of missing test coverage. (In reply to comment #39) > (From update of attachment 65051 [details]) > > WebCore/ChangeLog:3 > > + Reviewed by Andreas Kling > Until the patch is landed this line should continue to say NOBODY (OOPS!). > > > :722 > > +bool PluginView::platformGetValueForURL(NPNURLVariable variable, const char* url, char** value, uint32_t* len, NPError* result) > Shouldn't this function use the QNetworkAccessManager's proxy settings instead of the application settings? Probably, but I am not entirely sure how I can access the QNetworkAccessManager's proxy from this implementation... > > WebCore/plugins/symbian/PluginViewSymbian.cpp:341 > > +bool PluginView::platformGetValueForURL(NPNURLVariable variable, const char* url, char** value, uint32_t* len, NPError* result) > The current code duplication between the views sucks a bit, but in this > "Symbian" implementation we should use the Qt bits and pieces. Qt is the system > toolkit on Symbian. Well no generic way to access proxy information means that this is the only viable approach at the moment. > I think overall this patch looks okay now, I agree with Andreas. However there is > one thing missing: Test coverage for the new code. At least the cookie bits > should be relatively easy to cover with the test netscape plugin. > > I'm going to say r- primarily because of missing test coverage. Where does the test coverage go ? LayoutTests/plugin ?
Created attachment 66372 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update IX]... * Corrected the Reviewed By entry in the ChangeLog file. * Use QNetworkAccessManager's proxy instead the application's. * Copied the platformGetValueForURL implementation from qt port to symbian port. I cannot figure out where and how the requested test coverage for the new code is supposed to be implemented though...
Attachment 66372 [details] did not build on qt: Build output: http://queues.webkit.org/results/3909055
Created attachment 66374 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update X]... *no comment*
(In reply to comment #40) > > I'm going to say r- primarily because of missing test coverage. > > Where does the test coverage go ? LayoutTests/plugin ? Yep. See also WebKitTools/DumpRenderTree/TestNetscapePlugin for the test netscape plugin :)
Comment on attachment 66374 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update X]... This page is rather interesting: https://developer.mozilla.org/en/NPN_GetValueForURL About the "value" argument: "Out parameter. The browser passes back the requested information. If the function succeeds, the result buffer will be allocated with NPN_MemAlloc; the plugin is responsible for freeing the buffer. Note: the value may have internal NULL bytes and may not be NULL-terminated." I think we should be nice and null-terminate the returned value, it won't hurt and will shield against broken plugins. > + *value = static_cast<char*>(NPN_MemAlloc(cookieStr.length())); Allocate cookieStr.length() + 1 bytes. > + strncpy(*value, cookieStr.data(), cookieStr.length()); Use memcpy() instead of strncpy(). (And cookieStr.length() + 1.) The above two comments apply to all cases where you're NPN_MemAlloc()ing a char buffer and copying something to it. > + LOG(Plugins, "PluginView::getValueForURL(%s): Need to implement PluginView::platformGetValueForURL!", prettyNameForNPNURLVariable(variable).data()); > ... > + LOG(Plugins, "PluginView::getValueForURL: %s", prettyNameForNPNURLVariable(variable).data()); Nit: inconsistent formatting, second case should be something like: LOG(Plugins, "PluginView::getValueForURL(%s): Unhandled variable", prettyNameForNPNURLVariable(variable).data()); r- because of lack of test, that's the important missing piece.
(In reply to comment #45) > (From update of attachment 66374 [details]) > This page is rather interesting: https://developer.mozilla.org/en/NPN_GetValueForURL > > About the "value" argument: > "Out parameter. The browser passes back the requested information. If the function succeeds, the result buffer will be allocated with NPN_MemAlloc; the plugin is responsible for freeing the buffer. Note: the value may have internal NULL bytes and may not be NULL-terminated." > > I think we should be nice and null-terminate the returned value, it won't hurt and will shield against broken plugins. > > > + *value = static_cast<char*>(NPN_MemAlloc(cookieStr.length())); > > Allocate cookieStr.length() + 1 bytes. > > > + strncpy(*value, cookieStr.data(), cookieStr.length()); > > Use memcpy() instead of strncpy(). (And cookieStr.length() + 1.) > > The above two comments apply to all cases where you're NPN_MemAlloc()ing a char buffer and copying something to it. I have already changed this locally and is waiting to be committed once I figure out on how to add a test coverage for it. > > + LOG(Plugins, "PluginView::getValueForURL(%s): Need to implement PluginView::platformGetValueForURL!", prettyNameForNPNURLVariable(variable).data()); > > ... > > + LOG(Plugins, "PluginView::getValueForURL: %s", prettyNameForNPNURLVariable(variable).data()); > > Nit: inconsistent formatting, second case should be something like: > LOG(Plugins, "PluginView::getValueForURL(%s): Unhandled variable", prettyNameForNPNURLVariable(variable).data()); This was done because of your own feedback in comment #36. If it's changed to what you are suggesting here, it would read: "PluginView::getValueForURL(Unknown variable): Unhandled variable" which is not much different than it was before comment #36. Instead it should correctly, as pointed out by yourself in comment #36, read: "PluginView::getValueForURL: Unknown variable" > r- because of lack of test, that's the important missing piece. Still trying to figure that out... No clue when that will happen ; so this patch will have to sit and wait until I figure that out or someone else with better understanding of how the test coverages work provides one for it...
(In reply to comment #46) > This was done because of your own feedback in comment #36. If it's changed to what you are suggesting here, it would read: Oops! Sorry about the schizophrenia. I again agree with my past self. ;-)
Created attachment 68348 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #2]... With the exception of the requested test coverage, this is the final impementation of the missing NPN functions that are needed to ensure Java applets work in QtWebKit. I cannot figure out how to create the test coverage for this change ; so I have given up on doing so now... Perhaps someone more familiar will be able to add the test coverage...
Attachment 68348 [details] did not build on qt: Build output: http://queues.webkit.org/results/4058102
Created attachment 68411 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #3]... Fixed compilation issue caused by layering violation in the previous patch...
Comment on attachment 68411 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #3]... I don't like the platformGetValueForURL/platformSetValueForURL/platformGetAuthenticationInfo approach. In http://trac.webkit.org/changeset/68951 I landed ProxyServer.h which has a proxyServersForURL function. If Qt provided an implementation of that it could be used in PluginView.cpp (I plan to do the same thing for getAuthenticationInfo). Does that make sense?
(In reply to comment #51) > (From update of attachment 68411 [details]) > I don't like the platformGetValueForURL/platformSetValueForURL/platformGetAuthenticationInfo approach. > > In http://trac.webkit.org/changeset/68951 I landed ProxyServer.h which has a proxyServersForURL function. If Qt provided an implementation of that it could be used in PluginView.cpp (I plan to do the same thing for getAuthenticationInfo). > > Does that make sense? Yes. If there is a platform independent way of accessing proxy and authentication information, then there is no need for keeping the platform specific methods...
Created attachment 69582 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #4]... Per review in comment #51, changed code to use WebCore::proxyServersForURL from ProxyServer.h for obtaining proxy information and removed the platform* functions.
Created attachment 70193 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #5]... Minor changes...
Comment on attachment 70193 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #5]... View in context: https://bugs.webkit.org/attachment.cgi?id=70193&action=review Looks good otherwise! > WebCore/platform/network/ProxyServer.h:71 > +Vector<ProxyServer> proxyServersForURL(const KURL&, const NetworkingContext* context = 0); I think it's a bit weird to make this default to null. Couldn't we instead always require the argument to be passed?
(In reply to comment #55) > (From update of attachment 70193 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=70193&action=review > > Looks good otherwise! > > > WebCore/platform/network/ProxyServer.h:71 > > +Vector<ProxyServer> proxyServersForURL(const KURL&, const NetworkingContext* context = 0); > > I think it's a bit weird to make this default to null. Couldn't we instead always require the argument to be passed? Yes, there is no reason why that cannot be the case. I did that to simply avoid breaking any of the other implementations...
Created attachment 70343 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #6]... Removed the default value from proxyServersForURL's second parameter...
Comment on attachment 70193 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #5]... Removed r+
Attachment 70343 [details] did not build on qt: Build output: http://queues.webkit.org/results/4320006
Created attachment 70346 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #7]... Attempt to fix WebKit2 compile issues for qt port...
Comment on attachment 70346 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #7]... View in context: https://bugs.webkit.org/attachment.cgi?id=70346&action=review Looks good, but please fix the following things. > WebCore/platform/network/ProxyServer.h:71 > +Vector<ProxyServer> proxyServersForURL(const KURL&, const NetworkingContext* context); There's no need for the NetworkingContext* to have a variable name here. > WebCore/platform/network/qt/ProxyServerQt.cpp:44 > + if (context) { This function nests unnecessarily, you should use early return style, i.e: if (!context) return servers; And the same for !accessManager and !proxyFactory > WebKit2/WebProcess/Plugins/PluginView.cpp:847 > + const NetworkingContext* netContext = frameLoader ? frameLoader->networkingContext() : 0; > + Vector<ProxyServer> proxyServers = proxyServersForURL(KURL(KURL(), urlString), netContext); s/netContext/context/
Created attachment 70372 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #8]... Addressed the issues from review in comment #61...
Comment on attachment 70372 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #8]... View in context: https://bugs.webkit.org/attachment.cgi?id=70372&action=review r- for the ChangeLog issue, the rest is cosmetics. > WebCore/ChangeLog:6 > + Fix compile error when bulding webkit's Qt port using the "--v8" option. > + https://bugs.webkit.org/show_bug.cgi?id=47455 Oops, looks like you have some extra ChangeLog lying around. > WebCore/platform/network/qt/ProxyServerQt.cpp:47 > + if (proxyFactory) { if (!proxyFactory) return servers; > WebKit2/WebProcess/Plugins/PluginView.cpp:847 > + const NetworkingContext* netContext = frameLoader ? frameLoader->networkingContext() : 0; > + Vector<ProxyServer> proxyServers = proxyServersForURL(KURL(KURL(), urlString), netContext); Nit: "netContext" -> "context"
Created attachment 70373 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #9]... Corrected the WebCore/ChangeLog patch...
Comment on attachment 70373 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #9]... r=me, let's land this puppy.
Created attachment 70755 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #10]... * Factored out the code that obtains the frame to use when calling the cookie and proxy related functions. * Added more checks to make sure invalid pointers are not used.
Comment on attachment 70373 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #9]... removed r+
Attachment 70755 [details] did not build on qt: Build output: http://queues.webkit.org/results/4426034
Created attachment 70765 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #11]... Fixed build problem with previous commit...
Blocking QtWebKit 2.1 on this. See https://bugs.webkit.org/show_bug.cgi?id=33044#c30 for more information.
Comment on attachment 70765 [details] Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #11]... Clearing flags on attachment: 70765 Committed r69808: <http://trac.webkit.org/changeset/69808>
All reviewed patches have been landed. Closing bug.
(In reply to comment #70) > Blocking QtWebKit 2.1 on this. See https://bugs.webkit.org/show_bug.cgi?id=33044#c30 for more information. Following this logic (bug 33044#c30), shouldn't bug #38911 be cherry-picked as well?
(In reply to comment #70) > Blocking QtWebKit 2.1 on this. See https://bugs.webkit.org/show_bug.cgi?id=33044#c30 for more information. The complexity to backport/cherry-pick this change to qtwebkit-2.1 is way too big and risky at this point. We need tons of other changes, including bug 47022 and major refactorings such as bug 42292. I'm giving up on the cherry-pick. If really needed (I'll discuss it with the stakeholders), we should create a new bug/task for the backport.
Revision r69808 cherry-picked into qtwebkit-2.1 with commit 3c8023c <http://gitorious.org/webkit/qtwebkit/commit/3c8023c>
*** Bug 48213 has been marked as a duplicate of this bug. ***