Bug 34539 - Implement NPN_GetValueForURL and NPN_SetValueForURL and provide a stub for NPN_GetAuthenticationInfo
Summary: Implement NPN_GetValueForURL and NPN_SetValueForURL and provide a stub for NP...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 48213 (view as bug list)
Depends on: 47022
Blocks: 33044
  Show dependency treegraph
 
Reported: 2010-02-03 12:34 PST by Gustavo Noronha (kov)
Modified: 2011-06-08 08:25 PDT (History)
13 users (show)

See Also:


Attachments
Set the record straight regarding NPAPI compatibility, hopefully (1.32 KB, patch)
2010-02-03 12:34 PST, Gustavo Noronha (kov)
eric: review-
Details | Formatted Diff | Diff
Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo (6.20 KB, patch)
2010-03-21 20:23 PDT, Garth Dahlstrom
no flags Details | Formatted Diff | Diff
Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo -- Fixes proxy string logic problem (7.72 KB, patch)
2010-03-25 21:43 PDT, Garth Dahlstrom
no flags Details | Formatted Diff | Diff
Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo -- Fixes junk being added to end of NPN_GetValueForURL strings (6.40 KB, patch)
2010-03-31 16:32 PDT, Garth Dahlstrom
no flags Details | Formatted Diff | Diff
Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo [Updated] (6.46 KB, patch)
2010-05-10 14:26 PDT, Dawit A.
no flags Details | Formatted Diff | Diff
Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo [Update II] (6.52 KB, patch)
2010-05-10 16:09 PDT, Dawit A.
no flags Details | Formatted Diff | Diff
Implements NPN_GetValueForURL and NPN_SetValueForURL, stub for NPN_GetAuthenticationInfo [Update III] (6.94 KB, patch)
2010-05-11 06:18 PDT, Dawit A.
andersca: review-
Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and provide a stub for NPN_GetAuthenticationInfo [Update IV]... (6.52 KB, patch)
2010-05-11 11:50 PDT, Dawit A.
abarth: review-
Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update V]... (17.00 KB, patch)
2010-08-20 22:44 PDT, Dawit A.
akling: review-
Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update VI]... (16.83 KB, patch)
2010-08-21 09:30 PDT, Dawit A.
akling: review-
Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update VII]... (17.33 KB, patch)
2010-08-21 21:19 PDT, Dawit A.
no flags Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final Update]... (17.12 KB, patch)
2010-08-22 01:09 PDT, Dawit A.
hausmann: review-
Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update IX]... (20.43 KB, patch)
2010-09-02 08:21 PDT, Dawit A.
no flags Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update X]... (20.46 KB, patch)
2010-09-02 08:45 PDT, Dawit A.
akling: review-
Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #2]... (20.63 KB, patch)
2010-09-22 01:55 PDT, Dawit A.
no flags Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #3]... (20.69 KB, patch)
2010-09-22 12:04 PDT, Dawit A.
andersca: review-
Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #4]... (12.95 KB, patch)
2010-10-02 14:26 PDT, Dawit A.
no flags Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #5]... (12.99 KB, patch)
2010-10-07 19:48 PDT, Dawit A.
no flags Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #6]... (14.27 KB, patch)
2010-10-08 23:12 PDT, Dawit A.
no flags Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #7]... (15.27 KB, patch)
2010-10-09 01:46 PDT, Dawit A.
akling: review-
Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #8]... (15.30 KB, patch)
2010-10-09 13:42 PDT, Dawit A.
akling: review-
Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #9]... (15.03 KB, patch)
2010-10-09 13:54 PDT, Dawit A.
no flags Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #10]... (15.26 KB, patch)
2010-10-14 11:18 PDT, Dawit A.
no flags Details | Formatted Diff | Diff
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #11]... (15.33 KB, patch)
2010-10-14 12:28 PDT, Dawit A.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2010-02-03 12:34:04 PST
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.
Comment 1 Gustavo Noronha (kov) 2010-02-03 12:34:59 PST
Created attachment 48060 [details]
Set the record straight regarding NPAPI compatibility, hopefully
Comment 2 Eric Seidel 2010-02-03 16:11:50 PST
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 3 Eric Seidel 2010-02-17 15:49:36 PST
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.
Comment 4 Garth Dahlstrom 2010-03-19 20:47:38 PDT
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.
Comment 5 Garth Dahlstrom 2010-03-21 20:23:52 PDT
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.
Comment 6 Kenneth Rohde Christiansen 2010-03-22 05:47:02 PDT
Should this be up for review? I think e don't want to commit this with debugging using qDebug, nor with out-commented code.
Comment 7 Garth Dahlstrom 2010-03-25 21:43:27 PDT
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)?
Comment 8 Garth Dahlstrom 2010-03-31 16:32:08 PDT
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.
Comment 9 Dawit A. 2010-05-10 14:26:27 PDT
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.
Comment 10 WebKit Review Bot 2010-05-10 14:31:52 PDT
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.
Comment 11 Early Warning System Bot 2010-05-10 14:34:36 PDT
Attachment 55604 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2203113
Comment 12 Dawit A. 2010-05-10 16:09:31 PDT
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...
Comment 13 WebKit Review Bot 2010-05-10 16:13:50 PDT
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.
Comment 14 Early Warning System Bot 2010-05-10 16:17:43 PDT
Attachment 55617 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2172117
Comment 15 WebKit Review Bot 2010-05-10 17:08:08 PDT
Attachment 55617 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2227119
Comment 16 Dawit A. 2010-05-10 18:20:44 PDT
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...
Comment 17 WebKit Review Bot 2010-05-11 02:45:37 PDT
Attachment 55617 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2230156
Comment 18 Dawit A. 2010-05-11 06:18:42 PDT
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.
Comment 19 WebKit Review Bot 2010-05-11 06:20:46 PDT
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.
Comment 20 Early Warning System Bot 2010-05-11 06:26:12 PDT
Attachment 55694 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2178120
Comment 21 Anders Carlsson 2010-05-11 09:53:40 PDT
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.
Comment 22 Dawit A. 2010-05-11 10:54:46 PDT
(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...
Comment 23 Anders Carlsson 2010-05-11 11:09:03 PDT
(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.
Comment 24 Dawit A. 2010-05-11 11:34:43 PDT
(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...
Comment 25 Dawit A. 2010-05-11 11:50:22 PDT
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
Comment 26 WebKit Review Bot 2010-05-11 11:51:58 PDT
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.
Comment 27 WebKit Review Bot 2010-05-20 05:08:02 PDT
Attachment 55729 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2285327
Comment 28 Dawit A. 2010-05-24 09:12:56 PDT
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 29 Adam Barth 2010-06-21 19:20:48 PDT
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.
Comment 30 Dawit A. 2010-08-20 22:44:45 PDT
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 31 Andreas Kling 2010-08-21 04:32:56 PDT
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"
Comment 32 Dawit A. 2010-08-21 09:30:11 PDT
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...
Comment 33 Dawit A. 2010-08-21 09:49:14 PDT
BTW, here is the link to the java applet test page:

http://www.java.com/en/download/help/testvm.xml
Comment 34 Andreas Kling 2010-08-21 16:07:51 PDT
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 "".
Comment 35 Dawit A. 2010-08-21 21:19:22 PDT
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 36 Andreas Kling 2010-08-21 21:35:30 PDT
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. :-)
Comment 37 Dawit A. 2010-08-22 00:59:14 PDT
(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...
Comment 38 Dawit A. 2010-08-22 01:09:00 PDT
Created attachment 65051 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final Update]...

Final update based on review in comment #36.
Comment 39 Simon Hausmann 2010-08-30 00:22:01 PDT
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.
Comment 40 Dawit A. 2010-08-30 01:14:05 PDT
(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 ?
Comment 41 Dawit A. 2010-09-02 08:21:02 PDT
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...
Comment 42 Early Warning System Bot 2010-09-02 08:35:27 PDT
Attachment 66372 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3909055
Comment 43 Dawit A. 2010-09-02 08:45:32 PDT
Created attachment 66374 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Update X]...

*no comment*
Comment 44 Simon Hausmann 2010-09-03 03:51:26 PDT
(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 45 Andreas Kling 2010-09-10 04:34:05 PDT
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.
Comment 46 Dawit A. 2010-09-10 09:47:42 PDT
(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...
Comment 47 Andreas Kling 2010-09-10 09:51:12 PDT
(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. ;-)
Comment 48 Dawit A. 2010-09-22 01:55:38 PDT
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...
Comment 49 Early Warning System Bot 2010-09-22 02:04:55 PDT
Attachment 68348 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4058102
Comment 50 Dawit A. 2010-09-22 12:04:13 PDT
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 51 Anders Carlsson 2010-10-01 16:31:49 PDT
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?
Comment 52 Dawit A. 2010-10-02 08:46:23 PDT
(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...
Comment 53 Dawit A. 2010-10-02 14:26:13 PDT
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.
Comment 54 Dawit A. 2010-10-07 19:48:10 PDT
Created attachment 70193 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #5]...

Minor changes...
Comment 55 Anders Carlsson 2010-10-08 10:17:00 PDT
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?
Comment 56 Dawit A. 2010-10-08 12:09:15 PDT
(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...
Comment 57 Dawit A. 2010-10-08 23:12:50 PDT
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 58 Dawit A. 2010-10-08 23:17:33 PDT
Comment on attachment 70193 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #5]...

Removed r+
Comment 59 Early Warning System Bot 2010-10-08 23:22:08 PDT
Attachment 70343 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4320006
Comment 60 Dawit A. 2010-10-09 01:46:33 PDT
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 61 Andreas Kling 2010-10-09 13:10:51 PDT
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/
Comment 62 Dawit A. 2010-10-09 13:42:07 PDT
Created attachment 70372 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #8]...

Addressed the issues from review in comment #61...
Comment 63 Andreas Kling 2010-10-09 13:52:57 PDT
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"
Comment 64 Dawit A. 2010-10-09 13:54:11 PDT
Created attachment 70373 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #9]...

Corrected the WebCore/ChangeLog patch...
Comment 65 Andreas Kling 2010-10-09 13:55:35 PDT
Comment on attachment 70373 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #9]...

r=me, let's land this puppy.
Comment 66 Dawit A. 2010-10-14 11:18:15 PDT
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 67 Dawit A. 2010-10-14 11:22:38 PDT
Comment on attachment 70373 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #9]...

removed r+
Comment 68 Early Warning System Bot 2010-10-14 11:42:22 PDT
Attachment 70755 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4426034
Comment 69 Dawit A. 2010-10-14 12:28:08 PDT
Created attachment 70765 [details]
Implement NPN_GetValueForURL and NPN_SetValueForURL and NPN_GetAuthenticationInfo [Final #11]...

Fixed build problem with previous commit...
Comment 70 Andreas Kling 2010-10-14 15:11:00 PDT
Blocking QtWebKit 2.1 on this. See https://bugs.webkit.org/show_bug.cgi?id=33044#c30 for more information.
Comment 71 WebKit Commit Bot 2010-10-14 15:17:00 PDT
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>
Comment 72 WebKit Commit Bot 2010-10-14 15:17:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 73 Ademar Reis 2010-10-22 13:04:26 PDT
(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?
Comment 74 Ademar Reis 2010-11-10 09:50:50 PST
(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.
Comment 75 Ademar Reis 2010-11-18 11:08:41 PST
Revision r69808 cherry-picked into qtwebkit-2.1 with commit 3c8023c <http://gitorious.org/webkit/qtwebkit/commit/3c8023c>
Comment 76 Luiz Agostini 2011-06-08 08:25:34 PDT
*** Bug 48213 has been marked as a duplicate of this bug. ***