Bug 116887

Created attachment 203093 [details] proposed fix

Attachment 203093 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http/tests/uri/curly-braces-escaping-expected.txt', u'LayoutTests/http/tests/uri/curly-braces-escaping.html', u'LayoutTests/http/tests/uri/resources/echo-uri.php', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.vcproj/WebCore.vcproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/KURL.cpp', u'Source/WebCore/platform/KURL.h', u'Source/WebCore/platform/cf/CFURLExtras.cpp', u'Source/WebCore/platform/cf/CFURLExtras.h', u'Source/WebCore/platform/cf/KURLCFNet.cpp', u'Source/WebCore/platform/mac/KURLMac.mm', u'Source/WebCore/platform/network/cf/ResourceErrorCF.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/API/c/cf/WKURLCF.cpp', u'Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp']" exit_code: 1 Source/WebCore/platform/KURL.h:200: The parameter name "buffer" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/cf/KURLCFNet.cpp:30: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 203093 [details] proposed fix Attachment 203093 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/710321

Comment on attachment 203093 [details] proposed fix Attachment 203093 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/700103

Comment on attachment 203093 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=203093&action=review r=me, but the patch as-is breaks builds on all non-CF platforms so please do fix that > Source/WebCore/platform/KURL.h:29 > +#include "CFURLExtras.h" This is a platform-specific header file so it should not be included unconditionally. I think it’s hard to know where to put the URLCharBuffer typedef. Maybe we can repeat it in both CFURLExtras.h and in KURL.h. >> Source/WebCore/platform/KURL.h:200 >> + void copyToBuffer(URLCharBuffer& buffer) const; > > The parameter name "buffer" adds no information, so it should be removed. [readability/parameter_name] [5] I agree with the style bot on this. > Source/WebCore/platform/cf/CFURLExtras.h:36 > +typedef Vector<char, 512> URLCharBuffer; This is not CF-specific and needs to be compiled on platform that can’t compile CFURLRef. > Source/WebCore/platform/cf/KURLCFNet.cpp:44 > + // FIXME: Why is it OK to ignore base URL here? The normal way to deal with this would be to call CFURLCopyAbsoluteURL. I think the comment should say something more like: // FIXME: Should we call CFURLCopyAbsoluteURL here? > Source/WebCore/platform/mac/KURLMac.mm:62 > + // We use the toll-free bridge between NSURL and CFURL to > + // create a CFURLRef supporting both empty and null values. What does “a CFURLRef supporting both empty and null values” mean? > Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp:532 > + // FIXME: This loses base URL, which seems incorrect. I would say “discards” rather than “loses”.

Comment on attachment 203093 [details] proposed fix Attachment 203093 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/691132

Created attachment 203097 [details] with build fixes

Comment on attachment 203097 [details] with build fixes Attachment 203097 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/741078

(In reply to comment #5) > > Source/WebCore/platform/cf/KURLCFNet.cpp:44 > > + // FIXME: Why is it OK to ignore base URL here? > > The normal way to deal with this would be to call CFURLCopyAbsoluteURL. I think the comment should say something more like: I don't think that we can do this. As I found when working on this patch, the main thing to know about CFURL is to not use any accessors other than CFURLGetBytes, as everything else works on a normalized string. I'll change the FIXME to suggest that we may want to recursively apply base URL. > > Source/WebCore/platform/mac/KURLMac.mm:62 > > + // We use the toll-free bridge between NSURL and CFURL to > > + // create a CFURLRef supporting both empty and null values. > > What does “a CFURLRef supporting both empty and null values” mean? I don't understand it in enough detail to change (it's a pre-existing one that someone else added). It certainly has something to do with a comment we have in WebKit2 code, "CFURL can't hold an empty URL, unlike NSURL", but that is also dubious.

Created attachment 203100 [details] with more build fixes

Comment on attachment 203100 [details] with more build fixes Attachment 203100 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/659058

Comment on attachment 203100 [details] with more build fixes Attachment 203100 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/724194

Committed <http://trac.webkit.org/r150853>.