RESOLVED FIXED 117617
CSSParser::parseImageSet() doesn't need a parameter.
https://bugs.webkit.org/show_bug.cgi?id=117617
Summary CSSParser::parseImageSet() doesn't need a parameter.
Jaehun Lim
Reported 2013-06-13 16:46:50 PDT
parseImageSet() can get m_valueList directly. And minor fixes.
Attachments
Patch (5.10 KB, patch)
2013-06-13 16:49 PDT, Jaehun Lim
no flags
Patch (5.23 KB, patch)
2013-06-13 19:31 PDT, Jaehun Lim
no flags
Jaehun Lim
Comment 1 2013-06-13 16:49:31 PDT
Darin Adler
Comment 2 2013-06-13 17:56:34 PDT
Comment on attachment 204653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204653&action=review > Source/WebCore/css/CSSParser.cpp:8028 > + ASSERT(value->unit == CSSParserValue::Function && value->function); We never use && in an ASSERT. Instead use two separate assertions so you can see which failed. ASSERT(value->unit == CSSParserValue::Function); ASSERT(value->function); But also, I don’t think that ASSERT(value->function) is all that useful. We’ll see the null dereference right after this, it won’t be hard to debug. And there’s no reason to expect a null here. > Source/WebCore/css/CSSParser.cpp:8042 > RefPtr<CSSImageValue> image = CSSImageValue::create(completeURL(arg->string)); > - imageSet->append(image); > + imageSet->append(image.release()); Why use a local variable at all? I think this is better in a single line.
Jaehun Lim
Comment 3 2013-06-13 19:26:28 PDT
Comment on attachment 204653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204653&action=review Thanks for your review. >> Source/WebCore/css/CSSParser.cpp:8028 >> + ASSERT(value->unit == CSSParserValue::Function && value->function); > > We never use && in an ASSERT. Instead use two separate assertions so you can see which failed. > > ASSERT(value->unit == CSSParserValue::Function); > ASSERT(value->function); > > But also, I don’t think that ASSERT(value->function) is all that useful. We’ll see the null dereference right after this, it won’t be hard to debug. And there’s no reason to expect a null here. I'll remove 'value->function' >> Source/WebCore/css/CSSParser.cpp:8042 >> + imageSet->append(image.release()); > > Why use a local variable at all? I think this is better in a single line. I'll make it one line.
Jaehun Lim
Comment 4 2013-06-13 19:31:18 PDT
WebKit Commit Bot
Comment 5 2013-06-14 09:37:00 PDT
Comment on attachment 204664 [details] Patch Clearing flags on attachment: 204664 Committed r151597: <http://trac.webkit.org/changeset/151597>
WebKit Commit Bot
Comment 6 2013-06-14 09:37:02 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.