Summary: | Implement NPAPI redirect handling | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vicki Pfau <jeffrey+webkit> | ||||||||||||||||||||
Component: | Plug-ins | Assignee: | Alexey Proskuryakov <ap> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, andersca, bfulgham, buildbot, cgarcia, commit-queue, ddkilzer, puhley, rniwa | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | 138987 | ||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||
Attachments: |
|
Description
Vicki Pfau
2014-11-12 15:24:09 PST
Created attachment 241447 [details]
Patch
Created attachment 241454 [details]
Patch
Attachment 241454 [details] did not pass style-queue:
ERROR: Source/WebCore/plugins/npapi.h:871: Extra space between NPBool and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:872: The parameter name "direction" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/plugins/npapi.h:872: Extra space between NPBool and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:873: Extra space between void and NP_LOADDS [whitespace/declaration] [3]
Total errors found: 4 in 44 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 241456 [details]
Patch
Attachment 241456 [details] did not pass style-queue:
ERROR: Source/WebCore/plugins/npapi.h:871: Extra space between NPBool and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:872: The parameter name "direction" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/plugins/npapi.h:872: Extra space between NPBool and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:873: Extra space between void and NP_LOADDS [whitespace/declaration] [3]
Total errors found: 4 in 45 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 241460 [details]
Patch
Attachment 241460 [details] did not pass style-queue:
ERROR: Source/WebCore/plugins/npapi.h:871: Extra space between NPBool and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:872: The parameter name "direction" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/plugins/npapi.h:872: Extra space between NPBool and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:873: Extra space between void and NP_LOADDS [whitespace/declaration] [3]
Total errors found: 4 in 45 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 241460 [details]
Patch
Couple of things:
* I think you should split up the patch and land the loader parts before the plug-in support.
* Why are no changes to ResourceHandle needed? It seems like asynchronous willSendRequest would require that.
* I think you should just change willSendRequest and have it take an std::function instead of adding "maybeSendRequest".
* Having ResourceRequest and bool seems strange, maybe you can use a WTF::Optional<ResourceRequest> instead.
Comment on attachment 241460 [details] Patch Attachment 241460 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6530079978946560 New failing tests: http/tests/plugins/get-url-redirect.html Created attachment 241543 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 241911 [details]
Patch
Comment on attachment 241543 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
This should fix all of the build and test issues, but I should probably still split this into two patches.
Attachment 241911 [details] did not pass style-queue:
ERROR: Source/WebCore/plugins/npapi.h:871: Extra space between NPBool and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:872: The parameter name "direction" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/plugins/npapi.h:872: Extra space between NPBool and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:873: Extra space between void and NP_LOADDS [whitespace/declaration] [3]
Total errors found: 4 in 46 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 242469 [details]
Patch
Attachment 242469 [details] did not pass style-queue:
ERROR: Source/WebCore/plugins/npapi.h:871: Extra space between NPBool and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:872: The parameter name "direction" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/plugins/npapi.h:872: Extra space between NPBool and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:873: Extra space between void and NP_LOADDS [whitespace/declaration] [3]
Total errors found: 4 in 43 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 242469 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242469&action=review > Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:70 > +void NetscapePlugInStreamLoader::willSendRequest(ResourceRequest&& request, const ResourceResponse& redirectResponse, std::function<void(ResourceRequest&)> callback) function parameter should be ResourceRequest&&. > Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:76 > + if (request.isNull() || !protect->m_client) { > + request = ResourceRequest(); This is weird. Just pass an empty ResourceRequest to callback() instead. > Source/WebCore/loader/NetscapePlugInStreamLoader.h:41 > + virtual void willSendRequest(NetscapePlugInStreamLoader*, ResourceRequest&&, const ResourceResponse& redirectResponse, std::function<void(ResourceRequest&)> callback) = 0; I think "completionHandler" is a better name than callback. > Source/WebKit/mac/Plugins/Hosted/HostedNetscapePluginStream.mm:201 > + // FIXME: Call NPP_URLRedirectNotify Is there a bug number for this FIXME? > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapeBrowserFuncs.cpp:1052 > + netscapeFuncs.urlredirectresponse = NPN_URLRedirectResponse; This should only be set if we're on an OS where this is supported. > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:407 > +void NetscapePlugin::registerRedirect(NetscapePluginStream* stream, const URL& requestURL, int redirectResponseStatus, void* notificationData) "registerRedirect" is a weird name. > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:420 > + auto pair = m_redirects.take(notifyData); This needs to handle invalid hash keys. Also, pair is a bad variable name, especially since you're using auto here. > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePluginStream.cpp:352 > +void NetscapePluginStream::redirect(const String& newURLString) > +{ > + m_requestURLString = newURLString; > +} This is just a setter. It should have a setter name. > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:141 > + std::function<void(ResourceRequest&)> m_callback; This should be named better. Created attachment 242550 [details]
Patch
Attachment 242550 [details] did not pass style-queue:
ERROR: Source/WebCore/plugins/PluginStream.cpp:406: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/plugins/npapi.h:871: Extra space between NPBool and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:872: The parameter name "direction" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/plugins/npapi.h:872: Extra space between NPBool and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:873: Extra space between void and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebKit/mac/Plugins/Hosted/HostedNetscapePluginStream.h:64: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit/mac/Plugins/Hosted/HostedNetscapePluginStream.mm:199: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PluginView.cpp:132: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PluginView.cpp:141: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PluginView.cpp:227: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit/mac/Plugins/WebNetscapePluginStream.h:82: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit/mac/Plugins/WebNetscapePluginStream.mm:300: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/plugins/PluginStream.h:79: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/loader/ResourceLoader.cpp:289: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/loader/NetscapePlugInStreamLoader.cpp:70: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/loader/ResourceLoader.h:96: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/loader/NetscapePlugInStreamLoader.h:41: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/loader/NetscapePlugInStreamLoader.h:60: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 18 in 46 files
If any of these errors are false positives, please file a bug against check-webkit-style.
*** Bug 49480 has been marked as a duplicate of this bug. *** Created attachment 258133 [details] proposed patch Worked through how this patch works by re-writing many parts, and ended up with the same result. A few differences: - used rvalue references more; - removed a "cancel loaded" clause in WebResourceLoader that was added in r176626 without explanation, and shouldn't be needed; - performed a small refactoring in ResourceHandle to clean up "willSendRequest" variants. Attachment 258133 [details] did not pass style-queue:
ERROR: Source/WebKit/win/Plugins/PluginStream.cpp:406: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/plugins/npapi.h:871: Extra space between NPBool and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:872: The parameter name "direction" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/plugins/npapi.h:872: Extra space between NPBool and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebCore/plugins/npapi.h:873: Extra space between void and NP_LOADDS [whitespace/declaration] [3]
ERROR: Source/WebKit/mac/Plugins/Hosted/HostedNetscapePluginStream.h:89: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit/mac/Plugins/Hosted/HostedNetscapePluginStream.mm:132: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit/win/Plugins/PluginStream.h:85: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PluginView.cpp:132: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PluginView.cpp:141: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PluginView.cpp:224: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit/mac/Plugins/WebNetscapePluginStream.h:100: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit/mac/Plugins/WebNetscapePluginStream.mm:300: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebCore/loader/NetscapePlugInStreamLoader.h:41: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 14 in 48 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 258133 [details] proposed patch Clearing flags on attachment: 258133 Committed r187886: <http://trac.webkit.org/changeset/187886> All reviewed patches have been landed. Closing bug. This patch broke the Windows build: ..\..\win\Plugins\PluginView.cpp(800): error C2248: 'WebCore::PluginStream::didReceiveResponse' : cannot access private member declared in class 'WebCore::PluginStream' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj] c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(86) : see declaration of 'WebCore::PluginStream::didReceiveResponse' c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(57) : see declaration of 'WebCore::PluginStream' ..\..\win\Plugins\PluginView.cpp(811): error C2248: 'WebCore::PluginStream::didReceiveData' : cannot access private member declared in class 'WebCore::PluginStream' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj] c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(87) : see declaration of 'WebCore::PluginStream::didReceiveData' c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(57) : see declaration of 'WebCore::PluginStream' ..\..\win\Plugins\PluginView.cpp(822): error C2248: 'WebCore::PluginStream::didFinishLoading' : cannot access private member declared in class 'WebCore::PluginStream' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj] c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(89) : see declaration of 'WebCore::PluginStream::didFinishLoading' c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(57) : see declaration of 'WebCore::PluginStream' ..\..\win\Plugins\PluginView.cpp(833): error C2248: 'WebCore::PluginStream::didFail' : cannot access private member declared in class 'WebCore::PluginStream' [C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj] c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(88) : see declaration of 'WebCore::PluginStream::didFail' c:\cygwin\home\buildbot\slave\win-release\build\source\webkit\win\plugins\PluginStream.h(57) : see declaration of 'WebCore::PluginStream' |