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.
Created attachment 58891 [details] Initial patch
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.
Attachment 58891 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/3272243
Created attachment 58892 [details] Updated patch Missing include.
Attachment 58891 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3266244
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 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.
(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 on attachment 58892 [details] Updated patch r=me
Landed in r61255.
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.
(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.
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. :(
(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.