[webkit-reviews] review denied: [Bug 17051] safari should treat "url()" as a valid CSS value : [Attachment 21520] Hopefully properly formatted patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Bradley Meck <genisis329 at gmail.com>'s
request for review:
Bug 17051: safari should treat "url()" as a valid CSS value
http://bugs.webkit.org/show_bug.cgi?id=17051

Attachment 21520: Hopefully properly formatted patch
http://bugs.webkit.org/attachment.cgi?id=21520&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Change looks fine. Patch is still messed up though:

> 2008-06-05  Bradley Meck  <set EMAIL_ADDRESS environment variable>

This line should have an email address, not the error message in brackets. You
can fix it yourself. The prepare-ChangeLog script is there to just help you
write the log, there's nothing sacred about what it writes out and you should
edit by hand to improve what it makes automatically.

>	  Reviewed by NOBODY (OOPS!).
> 
>	  Test: css2.1/t17051-url-empty.html
> 
>	  * css/CSSParser.cpp:
>	  (WebCore::CSSParser::parseValue):
>	  (WebCore::CSSParser::parseFillImage):
>	  (WebCore::CSSParser::parseBorderImage):

Here, you need comments explaining the change and usually mentioning the URL of
the bug report as well. Please look at other ChangeLog entries to get an idea.
Ideally you also have comments on each function explaining what changed there,
although many people skip that step.

The part of your patch in the WebKit directory modifying
WebKit.xcodeproj/project.pbxproj shouldn't be there at all. Please remove that.
The best way is probably to use "svn revert" in that directory, or you can just
edit them out of the patch before you post it.

The ChangeLog in the LayoutTests directory is messed up, with multiple entries.
Please edit it and fix it. It should contain comments explaining what the
changes are.

We normally don't add tests to the LayoutTests/css2.1 directory, since that's a
set of tests made by others that we imported into the WebKit project. So
ideally, the test would be in LayoutTests/fast/css instead. Also, it'd be best
to give it an easier to understand name. I don't know why it's named t17051,
although perhaps that's the name of the original test you derived it from.

> <div>This tests that the invalid cursor property value does not get applied.
See Bug 11221. Updated to comply with Bug 17051.</div>

I don't think there's a need to add this "Updated" text to the
invalid-cursor-property-crash.html test. It's fine to just correct the test
without changing its descriptive text.

> if (style && style.cursor == "url("+document.location+"), auto")

We normally put spaces around operators like "+", and you should do so here.

review- mainly because the change logs are wrong and because the patch contains
changes we don't want to land. If someone does want to land this and fix up all
the minor problems that's OK. The actual substance of the change is fine.