Bug 40676 – Add new files, needed in SVG Text rewrite

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

Add new files SVGTextChunkLayoutInfo.cpp / SVGTextQuery.(cpp|h) and integrate them in the build systems.
Added "if 0" on top of all files, until the SVG Text rewrite patch lands. This is supposed to reduce the review amount in the upcoming laaaarge SVG text patch.

[Nikolas Zimmermann]

Created attachment 58891 [details]
Initial patch

[WebKit Review Bot]

Attachment 58891 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/SVGTextChunkLayoutInfo.cpp:223:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Attachment 58891 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3272243

[Nikolas Zimmermann]

Created attachment 58892 [details]
Updated patch

Missing include.

[Early Warning System Bot]

Attachment 58891 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3266244

[WebKit Review Bot]

Attachment 58892 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/SVGTextChunkLayoutInfo.cpp:223:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/rendering/SVGTextChunkLayoutInfo.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Schulze]

Comment on attachment 58892 [details]
Updated patch

WebCore/rendering/SVGTextQuery.cpp:77
 +      if (transform.isIdentity())
It looks like you just look at the scaling part of the affine. Checking a and d for 1 can be faster :-)

WebCore/rendering/SVGTextChunkLayoutInfo.cpp:199
 +      return (chunk.textLength - computedLength) / float(chunk.end - chunk.start);
c-style

And please fix the alphabetic order problem.

[Nikolas Zimmermann]

(In reply to comment #7)
> (From update of attachment 58892 [details])
> WebCore/rendering/SVGTextQuery.cpp:77
>  +      if (transform.isIdentity())
> It looks like you just look at the scaling part of the affine. Checking a and d for 1 can be faster :-)
That would involve two if's as isVerticalText has to be respected, sth like
if (isVerticalText && transform.d() == 1)
return;
if (!isVerticalText && transform.a() == 1)
return;

isIdentity seems more elegant to me, and shouldn't be a speed issue.

> 
> WebCore/rendering/SVGTextChunkLayoutInfo.cpp:199
>  +      return (chunk.textLength - computedLength) / float(chunk.end - chunk.start);
> c-style
No, I'm calling the float() constructor ;-)

> 
> And please fix the alphabetic order problem.
Fixed.

[Dirk Schulze]

Comment on attachment 58892 [details]
Updated patch

r=me

[Nikolas Zimmermann]

Landed in r61255.

[Eric Seidel (no email)]

Why was this landed.   We don't land #if 0 or commented out code.  I think this is bad because it doesn't get a real review.

[Dirk Schulze]

(In reply to comment #11)
> Why was this landed.   We don't land #if 0 or commented out code.  I think this is bad because it doesn't get a real review.

It did get a review. I'm realy sorry for the temporary code style violence. The complete text patch was more than 200k which is realy unreviewable. I asked Niko to split the patch into smaler parts. He tried his best to avoid the #if 0 but this was simply not doable without breaking the bots. Niko will upload the other parts as soon as posible, so that #if 0 can be removed soon. Please be a bit more patient.

[Eric Seidel (no email)]

I just question your (or anyone's) ability to review commented out code.  WildFox should ideally be splitting up his 200k patch into tiny pieces.  Including landing the (failing) layout tests first and then landing incremental patches to fix them.  It's difficult, but it pays off in spades when your reviewers can actually understand and make intellegent commentary on your changes.

I find Git super useful for this kind of change. I commonly will write a huge patch, and then re-write it in smaller pieces, committing each one to git, making sure they build after each change.   Landing massive hunks of code blindly (as we did with this #if 0, or will do when he removes the && 0, is not good. :(

[Nikolas Zimmermann]

(In reply to comment #13)
> I just question your (or anyone's) ability to review commented out code.  WildFox should ideally be splitting up his 200k patch into tiny pieces.  Including landing the (failing) layout tests first and then landing incremental patches to fix them.  It's difficult, but it pays off in spades when your reviewers can actually understand and make intellegent commentary on your changes.

To clarify: the patch as is would be unreviewable, as it's lacking all the context (who calls the code, when is it used, etc..) But Dirk reviewed the _full_ SVG Text patch before I even created the master bug.
So no worries, this hs already gone through our normal review process.

> 
> I find Git super useful for this kind of change. I commonly will write a huge patch, and then re-write it in smaller pieces, committing each one to git, making sure they build after each change.   Landing massive hunks of code blindly (as we did with this #if 0, or will do when he removes the && 0, is not good. :(
You have a point here, EWS is not used. I did not think about this in advance. Next time I'll take it into account.