[Webkit-unassigned] [Bug 11108] Replace usage of split by proper parsers

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=11108





------- Comment #11 from mitz at webkit.org  2006-10-20 07:30 PDT -------
(From update of attachment 10944)
I didn't read the entire patch, but here are some observations:

Conflict markers:
+>>>>>>> .r16800

Typo:
+        - use functions from SVGParserUtils instead of spli

Some cleanup opportunities:
     else if (attr->name() == SVGNames::valuesAttr)
     {

This:
     String parse = m_rgbColor.stripWhiteSpace();
followed by:
+        const UChar* ptr = reinterpret_cast<const
UChar*>(parse.charactersWithNullTermination()) + 4;
always copies the buffer once (I think in many cases it copies it twice). I
think you can avoid one copy using a combination of skipOptionalSpaces and
String::find() (or a single condition that checks the next 4 characters)
starting after the run of spaces. I can't say if it's worth it.

Is it necessary to cast the result of charactersWithNullTermination()?

Shouldn't you set ec before returning in these cases?
+        if (!tryParseCoord(ptr, r))
+            return;

The use of skipOptionalSpaces in the non-percentage case is redundant, as
parseCoord always ends with skipOptionalSpacesOrDelimiter which ends with
skipOptionalSpaces:
+        if (*ptr == '%') {
+            percentages++;
+            ptr++;
+            skipOptionalSpacesOrDelimiter(ptr);
+        } else {
+            normal++;
+            skipOptionalSpaces(ptr);

It also seems that this will successfully parse things like "rgb(25 , % , 50%,
75%)".

tryParseNumberList is only used in this pattern:
+        int nr = tryParseNumberList(value, x, y);
+        if (nr > 0) {
+            setKernelUnitLengthXBaseValue(x);
+            if (nr == 1)
+                setKernelUnitLengthYBaseValue(x);
+            else
+                setKernelUnitLengthYBaseValue(y);
+        }

I think you can factor out the common logic so that you'll have a function
bool tryParseNumberOptionalNumber(const String&, double& h, double& v)
that sets h and v to the same value if there is only one value, and returns
false if there is no value. I don't know how it should behave for more than 2
values (or any junk after the second value).

SVGNumberList::parse() allows things like "  , 1, 2, , 3 ," (since
tryParseCoord allows one comma, and then skipOptionalSpacesOrDelimiter allows
another one; it is also what allows the leading comma).

This is a funny variable name:
+    const UChar* eoString = currSegment + points.length();

This can be changed to use tryParseCoord:
         currSegment = parseCoord(currSegment, xPos); 
         if (currSegment == prevSegment)

Some redundant parameter names:
+    const UChar* parseCoord(const UChar* ptr, double& number);
+        void parsePoints(const String& points) const;

Extra spaces inside the parentheses; "d" is unnecessary:
+        void parseSVG( const String &d, bool process = false );

skipOptionalSpaces(" ") tests the first character twice. Does the compiler
optimize it?

skipOptionalSpacesOrDelimiter(" ") tests the first character three times. Same
question.

This should be FIXME:
+    // TODO : more error checking/reporting


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.