Bug 40676 - Add new files, needed in SVG Text rewrite
Summary: Add new files, needed in SVG Text rewrite
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 40663
  Show dependency treegraph
 
Reported: 2010-06-16 07:03 PDT by Nikolas Zimmermann
Modified: 2010-06-17 00:12 PDT (History)
4 users (show)

See Also:


Attachments
Initial patch (50.58 KB, patch)
2010-06-16 07:06 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (51.12 KB, patch)
2010-06-16 07:17 PDT, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2010-06-16 07:03:07 PDT
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.
Comment 1 Nikolas Zimmermann 2010-06-16 07:06:01 PDT
Created attachment 58891 [details]
Initial patch
Comment 2 WebKit Review Bot 2010-06-16 07:08:11 PDT
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.
Comment 3 Eric Seidel (no email) 2010-06-16 07:14:38 PDT
Attachment 58891 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3272243
Comment 4 Nikolas Zimmermann 2010-06-16 07:17:19 PDT
Created attachment 58892 [details]
Updated patch

Missing include.
Comment 5 Early Warning System Bot 2010-06-16 07:17:42 PDT
Attachment 58891 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3266244
Comment 6 WebKit Review Bot 2010-06-16 07:19:25 PDT
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.
Comment 7 Dirk Schulze 2010-06-16 07:59:52 PDT
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.
Comment 8 Nikolas Zimmermann 2010-06-16 08:05:16 PDT
(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.
Comment 9 Dirk Schulze 2010-06-16 08:08:41 PDT
Comment on attachment 58892 [details]
Updated patch

r=me
Comment 10 Nikolas Zimmermann 2010-06-16 08:10:54 PDT
Landed in r61255.
Comment 11 Eric Seidel (no email) 2010-06-16 11:23:25 PDT
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.
Comment 12 Dirk Schulze 2010-06-16 12:55:04 PDT
(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.
Comment 13 Eric Seidel (no email) 2010-06-16 13:52:54 PDT
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. :(
Comment 14 Nikolas Zimmermann 2010-06-17 00:12:43 PDT
(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.