Bug 51456

Created attachment 77220 [details] Patch

Attachment 77220 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7231100

Attachment 77220 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7333056

Attachment 77220 [details] did not build on qt: Build output: http://queues.webkit.org/results/7284072

(In reply to comment #0) > In order to fix several SVG bugs, regarding very small font sizes, I'd ideally have access to floating-point ascent/descent information. SimpleFontData stores lineGap/lineHeight/ascent/descent as integers at the moment. For CG the ascent/descent is actually a floating-point number that (integerAscent scaled according to emPerUnits, multiplied with pointSize == floatAscent) gets rounded using lroundf and then stored as integer. > > Just changing the floats to integers, and storing accurate ascent/descent/.. information is not easily doable, as the int ascent() accessors would need to round the float m_ascent, every time on access. I am not sure I understand why that would not be easily doable. Can you explain? > That means for HTML layout (where we only need integers atm) we'd round much more often than necessary. How much more often? > Storing both int and float variants of the metrics in Font, seems also redundant and costly. Okay… > > I'm proposing a new FontMetrics class, that holds floating-point values for lineGap/lineHeight/ascent/descent, plus another IntegerFontMetrics class, that holds rounded integers. > The IntegerFontMetrics class is dynamically created and stored as OwnPtr in FontMetrics. Whenever someone accesses the int versions of ascent/descent/etc.. that datastructure is created > and stored in FontMetrics. Follow-up calls to ascent/descent, just return the cached integer, no need to round again. So, if I understand correctly, with the rare exception of fonts used only in SVG, all fonts will store both the floating point and integer values. That seems to be at least as “redundant and costly” as just adding the floating point data members alongside the integer ones. > Of course that will double the amount of memory to hold font metrics, in the worst-case, but gives us the ability to expose accurate ascent/descent/lineHeight/lineGap values for users that need it -> SVG. > I've worked on this the last weeks, and could fix all bugs related to small-font usages & SVG with this approach. > > Stripped down my current work, to a patch which just introduces FontMetrics, w/o any functional changes. > I'm open for all ideas, on how to make it better... uploading the patch to discuss soon. I am not convinced that this is the best approach to take.

(In reply to comment #5) > (In reply to comment #0) > > In order to fix several SVG bugs, regarding very small font sizes, I'd ideally have access to floating-point ascent/descent information. SimpleFontData stores lineGap/lineHeight/ascent/descent as integers at the moment. For CG the ascent/descent is actually a floating-point number that (integerAscent scaled according to emPerUnits, multiplied with pointSize == floatAscent) gets rounded using lroundf and then stored as integer. > > > > Just changing the floats to integers, and storing accurate ascent/descent/.. information is not easily doable, as the int ascent() accessors would need to round the float m_ascent, every time on access. > > I am not sure I understand why that would not be easily doable. Can you explain? It's easy doable, sorry, but I think we'd round a lot more than now.. > > > That means for HTML layout (where we only need integers atm) we'd round much more often than necessary. > > How much more often? On every access of any font metrics like ascent/descent. During each layout call.. > > > Storing both int and float variants of the metrics in Font, seems also redundant and costly. > > Okay… > > > > > I'm proposing a new FontMetrics class, that holds floating-point values for lineGap/lineHeight/ascent/descent, plus another IntegerFontMetrics class, that holds rounded integers. > > The IntegerFontMetrics class is dynamically created and stored as OwnPtr in FontMetrics. Whenever someone accesses the int versions of ascent/descent/etc.. that datastructure is created > > and stored in FontMetrics. Follow-up calls to ascent/descent, just return the cached integer, no need to round again. > > So, if I understand correctly, with the rare exception of fonts used only in SVG, all fonts will store both the floating point and integer values. That seems to be at least as “redundant and costly” as just adding the floating point data members alongside the integer ones. Yeah it's not a good approach, it only avoids the cost of storing both integer & float for SVG... for HTML it doubles the cost, so it's not good. > I am not convinced that this is the best approach to take. I agree, I mostly uploaded it to discuss the issue. Another option would be to use a union { float f; int i; } and to decide upon creation of the FontMetrics object, wheter floating point or integer accurancy is desired. That would only require rounding the incoming float values once, if integers are wanted, but it makes it impossible to reconstruct floating-point ascent/descent values, for the Font object, once constructed with IntegerMetrics. The most easy approach would be to use int ascent() const { return lroundf(m_ascent); } of course, but I was worried that it would be a performance decrease, so I thought that maybe it's a better trade-off to use more memory for the Font object, rather than decreasing performance. Note, I did not benchmark this at all, it's just a feeling. If I upload another, simpler version, which just does the rounding on every invocation, would you be able to run it through Apples PLT?

Created attachment 77235 [details] Patch v2 The easier way: always round, don't cache an IntegerFontMetrics object.

Comment on attachment 77235 [details] Patch v2 There should be no need to add a new class and touch dozens of files for this. Why not just start by rounding in the accessors, and measuring performance?

Attachment 77235 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7252110

Attachment 77235 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7306109

(In reply to comment #8) > (From update of attachment 77235 [details]) > There should be no need to add a new class and touch dozens of files for this. Why not just start by rounding in the accessors, and measuring performance? Well, I still think introducing a FontMetrics class is cleaner, than duplicating each accessor in Font and SimpleFontData. Especially if I want to introduce floatAscent/floatDescent/floatLineGap/floatLineSpacing methods, I'd need to add them to both Font and SimpleFontData, which is a bad design. The metrics should go into their own class. Of course, to just measure performance it's not needed, but I have already written it, so why not?

Attachment 77235 [details] did not build on win: Build output: http://queues.webkit.org/results/7272105

Created attachment 77315 [details] Patch v3 Patch v3: Attempt to make it build on more platform. Is it really unacceptable to just apply this patch and try a PLT run with it?? It at least works fine on Mac, at the moment.

Attachment 77315 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7214141

Created attachment 77317 [details] Patch v4 Uploading Patch v4, hopefully chromium build works now.

Attachment 77317 [details] did not build on win: Build output: http://queues.webkit.org/results/7211127

I performance tested this for a while (without PLT, of course) and didn't notice any measurable slowdown. I created an artifical testcase, compiled w/o optimizations: #include <math.h> class FooInt { public: FooInt(float bar) : m_bar(lroundf(bar)) { } int bar() const { return m_bar; } private: int m_bar; }; class FooFloat { public: FooFloat(float bar) : m_bar(bar) { } int bar() const { return lroundf(m_bar); } private: float m_bar; }; int main() { int result = 0; FooInt foo(1.7f); for (long i = 0; i < 1e8; ++i) { result = foo.bar(); } } Results: using FooFloat(1.7f) and 1e8 calls: 1.531s (1e8 lroundf calls) using FooInt(1.7f) and 1e8 calls: 0.6754s (1 lroundf call) This is artifical of course, but it gives a rough estimation of the slowdown, but of course 1e8 calls is _a lot_. The usual count is several orders of magnitudes lower. I suspect that's why I didn't notice any slowdown when runing DRT. I tested 10 DRT runs w/o the patch and 10 with the patch, and there was no difference, except the usual deviations betweens runs. I'll upload a new patch soon, fixing the win build, hopefully the approach is sane enough now to get it reviewed?

Created attachment 77452 [details] Patch v5

Attachment 77452 [details] did not build on win: Build output: http://queues.webkit.org/results/7292189

Created attachment 77453 [details] Patch v6 Forgot to alter files in WebKit/win, uploading patch v6, let's see if win build is fixed.

Attachment 77453 [details] did not build on win: Build output: http://queues.webkit.org/results/7274174

Created attachment 77491 [details] Patch v7 Fixed another win problem. void FontMetrics(..) clashes with FontMetrics class, use WebCore:: prefix, hopefully it builds now.

Attachment 77491 [details] did not build on win: Build output: http://queues.webkit.org/results/7295199

Created attachment 77501 [details] Patch v8 Fix WebKit2/win build.

Comment on attachment 77501 [details] Patch v8 Going to rebase to trunk first, the patch doesn't seem to apply on cr-mac-ews otherhwise, obsoleting the patch in the meanwhile.

Created attachment 77502 [details] Patch v9 Rebased against trunk, hopefully patch v9 now builds everywhere.

I don't understand why mac-ews and cr-mac-ews reject this patch. It tells me: patching file WebCore/WebCore.vcproj/WebCore.vcproj Hunk #1 FAILED at 26381. 1 out of 1 hunk FAILED -- saving rejects to file WebCore/WebCore.vcproj/WebCore.vcproj.rej I'm on trunk, and the patch just works fine on the other ews bots: nikolaszimmermann ~/Coding/WebKit > svn diff WebCore/WebCore.vcproj Index: WebCore/WebCore.vcproj/WebCore.vcproj =================================================================== --- WebCore/WebCore.vcproj/WebCore.vcproj (revision 74683) +++ WebCore/WebCore.vcproj/WebCore.vcproj (working copy) @@ -26381,6 +26381,10 @@ > </File> <File + RelativePath="..\platform\graphics\FontMetrics.h" + > + </File> + <File RelativePath="..\platform\graphics\FontRenderingMode.h" > </File> nikolaszimmermann ~/Coding/WebKit > svn up WebCore/WebCore.vcproj At revision 74683. Any idea?

(In reply to comment #27) > I don't understand why mac-ews and cr-mac-ews reject this patch. It tells me: > > patching file WebCore/WebCore.vcproj/WebCore.vcproj > Hunk #1 FAILED at 26381. > 1 out of 1 hunk FAILED -- saving rejects to file WebCore/WebCore.vcproj/WebCore.vcproj.rej > > I'm on trunk, and the patch just works fine on the other ews bots: > > nikolaszimmermann ~/Coding/WebKit > svn diff WebCore/WebCore.vcproj > Index: WebCore/WebCore.vcproj/WebCore.vcproj > =================================================================== > --- WebCore/WebCore.vcproj/WebCore.vcproj (revision 74683) > +++ WebCore/WebCore.vcproj/WebCore.vcproj (working copy) > @@ -26381,6 +26381,10 @@ > > > </File> > <File > + RelativePath="..\platform\graphics\FontMetrics.h" > + > > + </File> > + <File > RelativePath="..\platform\graphics\FontRenderingMode.h" > > > </File> > nikolaszimmermann ~/Coding/WebKit > svn up WebCore/WebCore.vcproj > At revision 74683. > > Any idea? Not sure if you hit the same problem like me, but I had this with some editors (and not always, just sometimes). It helped to use another editor. I think I took vi.

(In reply to comment #28) > > > > Any idea? > > Not sure if you hit the same problem like me, but I had this with some editors (and not always, just sometimes). It helped to use another editor. I think I took vi. It bothers me, that only the mac bots can't process it. Anyway, I'm going to revert the file and reedit, then resubmit. Let's see..

Created attachment 77512 [details] Patch v10

(In reply to comment #30) > Created an attachment (id=77512) [details] > Patch v10 You're right, just the two bots can't apply the patch. Are they not up to trunk?

Is anyone available for a real review? Simon? David? Dan?

Comment on attachment 77512 [details] Patch v10 I don't think FontMetrics warrants its own class and header. I think it should just be a struct in Font.h.

(In reply to comment #33) > (From update of attachment 77512 [details]) > I don't think FontMetrics warrants its own class and header. I think it should just be a struct in Font.h. I'm a huge fan of our one-class-one-file policy. So even though a struct in Font.h might sound convienent, it's violating our style guidelines. nikolaszimmermann ~/Coding/WebKit/WebCore/platform/graphics > ls Font*.h Font.h FontCache.h FontDescription.h FontFamily.h FontOrientation.h FontSelector.h FontTraitsMask.h FontBaseline.h FontData.h FontFallbackList.h FontRenderingMode.h FontSmoothingMode.h Why should adding FontMetrics.h be any problem?

Simon, did I convince you?

Comment on attachment 77512 [details] Patch v10 View in context: https://bugs.webkit.org/attachment.cgi?id=77512&action=review I'm not sure why a struct is violating the style guidelines. Sometime a struct is the better choice. But in this case I agree to Niko, because this code makes use of various functions (with more than one line) and this should really be in a header. I'd like to see another run r-, but not because of the header itself. > WebCore/platform/graphics/FontMetrics.h:19 > +/* > + Copyright (C) Research In Motion Limited 2010. All rights reserved. > + > + This library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Library General Public > + License as published by the Free Software Foundation; either > + version 2 of the License, or (at your option) any later version. > + > + This library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Library General Public License for more details. > + > + You should have received a copy of the GNU Library General Public License > + aint with this library; see the file COPYING.LIB. If not, write to > + the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, > + Boston, MA 02110-1301, USA. > +*/ > + Can you start every line with a space and a star? Also why do you copy&paste wrong licenses? aint? What is aint? Do you want to save memory? ;-) > WebCore/platform/graphics/FontMetrics.h:112 > +private: Do you think this 'private:' is helpful? I don't. > WebCore/platform/graphics/SimpleFontData.cpp:85 > + int xHeight = static_cast<int>(svgFontFaceElement->xHeight() * scale); > + int ascent = static_cast<int>(svgFontFaceElement->ascent() * scale); > + int descent = static_cast<int>(svgFontFaceElement->descent() * scale); why not floorf? > WebCore/platform/graphics/SimpleFontData.cpp:86 > + int lineGap = 0.1f * size; This doesn't cause a compiler warning?

(In reply to comment #36) > (From update of attachment 77512 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77512&action=review > > I'm not sure why a struct is violating the style guidelines. Sometime a struct is the better choice. But in this case I agree to Niko, because this code makes use of various functions (with more than one line) and this should really be in a header. Okay. > I'd like to see another run r-, but not because of the header itself. > > Can you start every line with a space and a star? Also why do you copy&paste wrong licenses? aint? What is aint? Do you want to save memory? ;-) Fixed. > > > WebCore/platform/graphics/FontMetrics.h:112 > > +private: > > Do you think this 'private:' is helpful? I don't. Removed. > > WebCore/platform/graphics/SimpleFontData.cpp:85 > > + int xHeight = static_cast<int>(svgFontFaceElement->xHeight() * scale); > > + int ascent = static_cast<int>(svgFontFaceElement->ascent() * scale); > > + int descent = static_cast<int>(svgFontFaceElement->descent() * scale); > why not floorf? There's a FIXME just before the lines to use floating-point FontMetrics, in a follow-up patch Note the static_cast<int>(..) is existing code - changing this causes multiple SVGFonts DRT differences (progressions, but I want to land them seperated). > > > WebCore/platform/graphics/SimpleFontData.cpp:86 > > + int lineGap = 0.1f * size; > > This doesn't cause a compiler warning? Wrapped in the same static_cast<int>, with the same warning, that this is gonne change soon to use floating-point metrics, fixing bugs with small font sizes.

Created attachment 79730 [details] Patch v11 Updated patch v11 to trunk. Save memory in FontMetrics by not storing but computing the lineSpacing() from the lineGap / ascent / descent, remaining compatibility with existing DRT expectations. Current assumption across most ports (mac/win/chromium/gtk...) is "lineSpacing = lroundf(ascent) + lroundf(descent) + lroundf(lineGap)". In my previous patch I stored m_lineSpacing as "lineSpacing = lroundf(floatAscent + floatDescent + floatLineGap)" which lead to several changes in layout tests, which might be correct, but I don't want to cause any layout test changes with this patch.

Comment on attachment 79730 [details] Patch v11 r=me

Committed r76442: <http://trac.webkit.org/changeset/76442>

Reopening bug. Landed cr-win build fix in r76443. I broke several tests on the bot, due the last-minute changes regarding lineSpacing rounding. I'm going to rechek and post a follow-up patch, if that doesn't work out I'm going to roll it out.

http://trac.webkit.org/changeset/76442 might have broken Qt Linux Release The following tests are not passing: animations/missing-values-first-keyframe.html animations/missing-values-last-keyframe.html animations/state-at-end-event.html css1/basic/class_as_selector.html css1/basic/comments.html css1/basic/containment.html css1/basic/contextual_selectors.html css1/basic/grouping.html css1/basic/id_as_selector.html css1/basic/inheritance.html css1/box_properties/border.html css1/box_properties/border_bottom.html css1/box_properties/border_bottom_inline.html css1/box_properties/border_bottom_width.html css1/box_properties/border_bottom_width_inline.html css1/box_properties/border_color.html css1/box_properties/border_color_inline.html css1/box_properties/border_inline.html css1/box_properties/border_left.html css1/box_properties/border_left_inline.html css1/box_properties/border_left_width.html css1/box_properties/border_left_width_inline.html css1/box_properties/border_right.html css1/box_properties/border_right_inline.html css1/box_properties/border_right_width.html css1/box_properties/border_right_width_inline.html css1/box_properties/border_style.html css1/box_properties/border_style_inline.html css1/box_properties/border_top.html css1/box_properties/border_top_inline.html css1/box_properties/border_top_width.html css1/box_properties/border_top_width_inline.html css1/box_properties/border_width.html css1/box_properties/border_width_inline.html css1/box_properties/clear.html css1/box_properties/clear_float.html css1/box_properties/float.html css1/box_properties/float_elements_in_series.html css1/box_properties/float_margin.html css1/box_properties/float_on_text_elements.html css1/box_properties/height.html css1/box_properties/margin.html css1/box_properties/margin_bottom.html css1/box_properties/margin_bottom_inline.html css1/box_properties/margin_inline.html css1/box_properties/margin_left.html css1/box_properties/margin_left_inline.html css1/box_properties/margin_right.html css1/box_properties/margin_right_inline.html css1/box_properties/margin_top.html css1/box_properties/margin_top_inline.html css1/box_properties/padding.html css1/box_properties/padding_bottom.html css1/box_properties/padding_bottom_inline.html css1/box_properties/padding_inline.html css1/box_properties/padding_left.html css1/box_properties/padding_left_inline.html css1/box_properties/padding_right.html css1/box_properties/padding_right_inline.html css1/box_properties/padding_top.html css1/box_properties/padding_top_inline.html css1/box_properties/width.html css1/cascade/cascade_order.html css1/cascade/important.html css1/classification/display.html css1/classification/list_style.html css1/classification/list_style_image.html css1/classification/list_style_position.html css1/classification/list_style_type.html css1/classification/white_space.html css1/color_and_background/background.html css1/color_and_background/background_attachment.html css1/color_and_background/background_color.html css1/color_and_background/background_image.html css1/color_and_background/background_position.html css1/color_and_background/background_repeat.html css1/color_and_background/color.html css1/conformance/forward_compatible_parsing.html css1/font_properties/font.html css1/font_properties/font_family.html css1/font_properties/font_size.html css1/font_properties/font_style.html css1/font_properties/font_variant.html css1/font_properties/font_weight.html css1/formatting_model/canvas.html css1/formatting_model/floating_elements.html css1/formatting_model/height_of_lines.html css1/formatting_model/horizontal_formatting.html css1/formatting_model/inline_elements.html css1/formatting_model/replaced_elements.html css1/formatting_model/vertical_formatting.html css1/pseudo/anchor.html css1/pseudo/firstletter.html css1/pseudo/firstline.html css1/pseudo/multiple_pseudo_elements.html css1/pseudo/pseudo_elements_in_selectors.html css1/text_properties/letter_spacing.html css1/text_properties/line_height.html css1/text_properties/text_align.html css1/text_properties/text_decoration.html css1/text_properties/text_indent.html css1/text_properties/text_transform.html css1/text_properties/vertical_align.html css1/text_properties/word_spacing.html css1/units/color_units.html css1/units/length_units.html css1/units/percentage_units.html css1/units/urls.html css2.1/t010403-shand-border-00-c.html css2.1/t010403-shand-font-00-b.html css2.1/t010403-shand-font-01-b.html css2.1/t040102-keywords-00-b.html css2.1/t040102-keywords-01-b.html css2.1/t040103-case-00-b.html css2.1/t040103-case-01-c.html css2.1/t040103-escapes-00-b.html css2.1/t040103-escapes-01-b.html css2.1/t040103-escapes-02-d.html css2.1/t040103-escapes-03-b.html css2.1/t040103-escapes-04-b.html css2.1/t040103-escapes-05-c.html css2.1/t040103-escapes-06-b.html css2.1/t040103-escapes-07-b.html css2.1/t040103-escapes-08-b.html css2.1/t040103-ident-00-c.html css2.1/t040103-ident-01-c.html css2.1/t040103-ident-02-c.html css2.1/t040103-ident-03-c.html css2.1/t040103-ident-04-c.html css2.1/t040103-ident-05-c.html css2.1/t040103-ident-06-c.html css2.1/t040103-ident-07-c.html css2.1/t040103-ident-08-c.html css2.1/t040103-ident-09-c.html css2.1/t040103-ident-10-c.html css2.1/t040103-ident-11-c.html css2.1/t040103-ident-12-c.html css2.1/t040103-ident-13-c.html css2.1/t040105-atkeyw-00-b.html css2.1/t040105-atkeyw-01-b.html css2.1/t040105-atkeyw-02-b.html css2.1/t040105-atrule-00-b.html css2.1/t040105-atrule-01-b.html css2.1/t040105-atrule-02-b.html css2.1/t040105-atrule-03-b.html css2.1/t040105-atrule-04-b.html css2.1/t040105-import-00-b.html css2.1/t040105-import-01-b.html css2.1/t040105-import-10-b.html css2.1/t040109-c17-comments-00-b.html css2.1/t040109-c17-comments-01-b.html css2.1/t0402-c71-fwd-parsing-00-f.html css2.1/t0402-c71-fwd-parsing-01-f.html css2.1/t0402-c71-fwd-parsing-02-f.html css2.1/t0402-c71-fwd-parsing-03-f.html css2.1/t0402-c71-fwd-parsing-04-f.html css2.1/t0402-syntax-01-f.html css2.1/t0402-syntax-02-f.html css2.1/t0402-syntax-03-f.html css2.1/t0402-syntax-04-f.html css2.1/t0402-syntax-05-f.html css2.1/t0402-syntax-06-f.html css2.1/t040302-c61-ex-len-00-b-a.html css2.1/t040302-c61-phys-len-00-b.html css2.1/t040302-c61-rel-len-00-b-ag.html css2.1/t040303-c62-percent-00-b-ag.html css2.1/t040304-c64-uri-00-a-g.html css2.1/t040306-c63-color-00-b-ag.html css2.1/t040306-syntax-01-f.html css2.1/t040307-syntax-01-b.html css2.1/t050201-c12-grouping-00-b.html css2.1/t0505-c16-descendant-00-e.html css2.1/t0505-c16-descendant-01-e.html css2.1/t0505-c16-descendant-02-e.html css2.1/t050803-c14-classes-00-e.html css2.1/t0509-c15-ids-00-a.html css2.1/t0509-c15-ids-01-e.html css2.1/t0509-id-sel-syntax-01-f.html css2.1/t0509-id-sel-syntax-02-b.html css2.1/t0510-c25-pseudo-elmnt-00-c.html css2.1/t0511-c21-pseud-anch-00-e-i.html css2.1/t0511-c21-pseud-link-00-e.html css2.1/t0511-c21-pseud-link-01-e.html css2.1/t051103-c21-activ-ln-00-e-i.html css2.1/t051103-c21-focus-ln-00-e-i.html css2.1/t051103-c21-hover-ln-00-e-i.html css2.1/t051103-dom-hover-01-c-io.html css2.1/t051103-dom-hover-02-c-io.html css2.1/t0602-c13-inheritance-00-e.html css2.1/t0602-inherit-bdr-pad-b-00.html css2.1/t0603-c11-import-00-b.html css2.1/t060401-c32-cascading-00-b.html css2.1/t060402-c31-important-00-b.html css2.1/t060403-c21-pseu-cls-00-e-i.html css2.1/t060403-c21-pseu-id-00-e-i.html css2.1/t0801-c412-hz-box-00-b-a.html css2.1/t0803-c5501-imrgn-t-00-b-ag.html css2.1/t0803-c5501-mrgn-t-00-b-a.html css2.1/t0803-c5502-imrgn-r-00-b-ag.html css2.1/t0803-c5502-imrgn-r-01-b-ag.html css2.1/t0803-c5502-imrgn-r-02-b-a.html css2.1/t0803-c5502-imrgn-r-03-b-a.html css2.1/t0803-c5502-imrgn-r-04-b-ag.html css2.1/t0803-c5502-imrgn-r-05-b-ag.html css2.1/t0803-c5502-imrgn-r-06-b-ag.html css2.1/t0803-c5502-mrgn-r-00-c-ag.html css2.1/t0803-c5502-mrgn-r-01-c-a.html css2.1/t0803-c5502-mrgn-r-02-c.html css2.1/t0803-c5502-mrgn-r-03-c.html css2.1/t0803-c5503-imrgn-b-00-b-a.html css2.1/t0803-c5503-mrgn-b-00-b-a.html css2.1/t0803-c5504-imrgn-l-00-b-ag.html css2.1/t0803-c5504-imrgn-l-01-b-ag.html css2.1/t0803-c5504-imrgn-l-02-b-ag.html css2.1/t0803-c5504-imrgn-l-03-b-a.html css2.1/t0803-c5504-imrgn-l-04-b-ag.html css2.1/t0803-c5504-imrgn-l-05-b-ag.html css2.1/t0803-c5504-imrgn-l-06-b-ag.html css2.1/t0803-c5504-mrgn-l-00-c-ag.html css2.1/t0803-c5504-mrgn-l-01-c-a.html css2.1/t0803-c5504-mrgn-l-02-c.html css2.1/t0803-c5504-mrgn-l-03-c.html css2.1/t0803-c5505-imrgn-00-a-ag.html css2.1/t0803-c5505-mrgn-00-b-ag.html css2.1/t0803-c5505-mrgn-01-e-a.html css2.1/t0803-c5505-mrgn-02-c.html css2.1/t0803-c5505-mrgn-03-c-ag.html css2.1/t080301-c411-vt-mrgn-00-b.html css2.1/t0804-c5506-ipadn-t-00-b-a.html css2.1/t0804-c5506-ipadn-t-01-b-a.html css2.1/t0804-c5506-ipadn-t-02-b-a.html css2.1/t0804-c5506-padn-t-00-b-a.html css2.1/t0804-c5507-ipadn-r-00-b-ag.html css2.1/t0804-c5507-ipadn-r-01-b-ag.html css2.1/t0804-c5507-ipadn-r-03-b-a.html css2.1/t0804-c5507-ipadn-r-04-b-ag.html css2.1/t0804-c5507-padn-r-00-c-ag.html css2.1/t0804-c5507-padn-r-01-c-a.html css2.1/t0804-c5507-padn-r-02-f.html css2.1/t0804-c5507-padn-r-03-f.html css2.1/t0804-c5508-ipadn-b-00-b-a.html css2.1/t0804-c5508-ipadn-b-01-f-a.html css2.1/t0804-c5508-ipadn-b-02-b-a.html css2.1/t0804-c5508-ipadn-b-03-b-a.html css2.1/t0804-c5509-ipadn-l-00-b-ag.html css2.1/t0804-c5509-ipadn-l-01-b-ag.html css2.1/t0804-c5509-ipadn-l-02-b-ag.html css2.1/t0804-c5509-ipadn-l-03-b-a.html css2.1/t0804-c5509-ipadn-l-04-f-ag.html css2.1/t0804-c5509-padn-l-00-b-ag.html css2.1/t0804-c5509-padn-l-01-b-a.html css2.1/t0804-c5509-padn-l-02-f.html css2.1/t0804-c5509-padn-l-03-f-g.html css2.1/t0804-c5510-ipadn-00-b-ag.html css2.1/t0804-c5510-padn-00-b-ag.html css2.1/t0804-c5510-padn-01-e-a.html css2.1/t0804-c5510-padn-02-f.html css2.1/t0805-c5511-brdr-tw-00-b.html css2.1/t0805-c5511-brdr-tw-01-b-g.html css2.1/t0805-c5511-brdr-tw-02-b.html css2.1/t0805-c5511-brdr-tw-03-b.html css2.1/t0805-c5511-ibrdr-tw-00-a.html css2.1/t0805-c5512-brdr-rw-00-b.html css2.1/t0805-c5512-brdr-rw-01-b-g.html css2.1/t0805-c5512-brdr-rw-02-b.html css2.1/t0805-c5512-brdr-rw-03-b.html css2.1/t0805-c5512-ibrdr-rw-00-a.html css2.1/t0805-c5513-brdr-bw-00-b.html css2.1/t0805-c5513-brdr-bw-01-b-g.html css2.1/t0805-c5513-brdr-bw-02-b.html css2.1/t0805-c5513-brdr-bw-03-b.html css2.1/t0805-c5513-ibrdr-bw-00-a.html css2.1/t0805-c5514-brdr-lw-00-b.html css2.1/t0805-c5514-brdr-lw-01-b-g.html css2.1/t0805-c5514-brdr-lw-02-b.html css2.1/t0805-c5514-brdr-lw-03-b.html css2.1/t0805-c5514-ibrdr-lw-00-a.html css2.1/t0805-c5515-brdr-w-00-a.html css2.1/t0805-c5515-brdr-w-01-b-g.html css2.1/t0805-c5515-brdr-w-02-b.html css2.1/t0805-c5515-ibrdr-00-b.html css2.1/t0805-c5516-brdr-c-00-a.html css2.1/t0805-c5516-ibrdr-c-00-a.html css2.1/t0805-c5517-brdr-s-00-c.html css2.1/t0805-c5517-ibrdr-s-00-a.html css2.1/t0805-c5518-brdr-t-00-a.html css2.1/t0805-c5518-brdr-t-01-e.html css2.1/t0805-c5518-ibrdr-t-00-a.html css2.1/t0805-c5519-brdr-r-00-a.html css2.1/t0805-c5519-brdr-r-01-e.html css2.1/t0805-c5519-brdr-r-02-e.html css2.1/t0805-c5519-ibrdr-r-00-a.html css2.1/t0805-c5520-brdr-b-00-a.html css2.1/t0805-c5520-brdr-b-01-e.html css2.1/t0805-c5520-ibrdr-b-00-a.html css2.1/t0805-c5521-brdr-l-00-a.html css2.1/t0805-c5521-brdr-l-01-e.html css2.1/t0805-c5521-brdr-l-02-e.html css2.1/t0805-c5521-ibrdr-l-00-a.html css2.1/t0805-c5522-brdr-00-b.html css2.1/t0805-c5522-brdr-01-b-g.html css2.1/t0805-c5522-ibrdr-00-a.html css2.1/t090204-display-change-01-b-ao.html css2.1/t090402-c42-ibx-pad-00-d-ag.html css2.1/t0905-c414-flt-00-d.html css2.1/t0905-c414-flt-01-d-g.html css2.1/t0905-c414-flt-02-c.html css2.1/t0905-c414-flt-03-c.html css2.1/t0905-c414-flt-04-c.html css2.1/t0905-c414-flt-fit-00-d.html css2.1/t0905-c414-flt-wrap-00-e.html css2.1/t0905-c414-flt-wrap-01-d-g.html css2.1/t0905-c5525-fltblck-00-d-ag.html css2.1/t0905-c5525-fltblck-01-d.html css2.1/t0905-c5525-fltclr-00-c-ag.html css2.1/t0905-c5525-fltcont-00-d-g.html css2.1/t0905-c5525-fltinln-00-c-ag.html css2.1/t0905-c5525-fltmrgn-00-c-ag.html css2.1/t0905-c5525-fltmult-00-d-g.html css2.1/t0905-c5525-fltwidth-00-c-g.html css2.1/t0905-c5525-fltwidth-01-c-g.html css2.1/t0905-c5525-fltwidth-02-c-g.html css2.1/t0905-c5525-fltwidth-03-c-g.html css2.1/t0905-c5525-fltwrap-00-b.html css2.1/t0905-c5526-fltclr-00-c-ag.html css2.1/t090501-c414-flt-00-d.html css2.1/t090501-c414-flt-01-b.html css2.1/t090501-c414-flt-02-d-g.html css2.1/t090501-c414-flt-03-b-g.html css2.1/t090501-c414-flt-ln-00-d.html css2.1/t090501-c414-flt-ln-01-d-g.html css2.1/t090501-c414-flt-ln-02-d.html css2.1/t090501-c414-flt-ln-03-d.html css2.1/t090501-c5525-flt-l-00-b-g.html css2.1/t090501-c5525-flt-r-00-b-g.html css2.1/t1002-c5523-width-00-b-g.html css2.1/t1002-c5523-width-01-b-g.html css2.1/t1002-c5523-width-02-b-g.html css2.1/t100303-c412-blockw-00-d-ag.html css2.1/t100304-c43-rpl-bbx-00-d-g.html css2.1/t100304-c43-rpl-bbx-01-d-g.html css2.1/t1004-c43-rpl-bbx-00-d-ag.html css2.1/t1004-c5524-width-00-b-g.html css2.1/t1005-c5524-width-00-b-g.html css2.1/t1005-c5524-width-01-b-g.html css2.1/t1008-c44-ln-box-03-d-ag.html css2.1/t100801-c544-valgn-00-a-ag.html css2.1/t100801-c544-valgn-02-d-agi.html css2.1/t100801-c544-valgn-03-d-agi.html css2.1/t100801-c544-valgn-04-d-agi.html css2.1/t100801-c548-leadin-00-d-a.html css2.1/t100801-c548-ln-ht-00-c-a.html css2.1/t100801-c548-ln-ht-01-b-ag.html css2.1/t100801-c548-ln-ht-03-d-ag.html css2.1/t100801-c548-ln-ht-04-d-ag.html css2.1/t1202-counter-00-b.html css2.1/t1202-counter-01-b.html css2.1/t1202-counter-02-b.html css2.1/t1202-counter-03-b.html css2.1/t1202-counter-04-b.html css2.1/t1202-counter-05-b.html css2.1/t1202-counter-06-b.html css2.1/t1202-counter-07-b.html css2.1/t1202-counter-08-b.html css2.1/t1202-counter-11-b.html css2.1/t1202-counter-12-b.html css2.1/t1202-counter-13-b.html css2.1/t1202-counter-14-b.html css2.1/t1202-counter-16-f.html css2.1/t1202-counters-00-b.html css2.1/t1202-counters-01-b.html css2.1/t1202-counters-02-b.html css2.1/t1202-counters-03-b.html css2.1/t1202-counters-05-b.html css2.1/t1202-counters-06-b.html css2.1/t1202-counters-07-b.html css2.1/t1202-counters-08-b.html css2.1/t1202-counters-11-b.html css2.1/t1202-counters-12-b.html css2.1/t1202-counters-13-b.html css2.1/t1202-counters-14-b.html css2.1/t1202-counters-16-c.html css2.1/t1202-counters-17-d.html css2.1/t1202-counters-18-f.html css2.1/t1204-implied-00-b.html css2.1/t1204-implied-01-c.html css2.1/t1204-implied-02-d.html css2.1/t1204-multiple-00-c.html css2.1/t1204-multiple-01-c.html css2.1/t1204-order-00-c.html css2.1/t1204-order-01-d.html css2.1/t1204-root-e.html css2.1/t120401-scope-00-b.html css2.1/t120401-scope-01-c.html css2.1/t120401-scope-02-c.html css2.1/t120401-scope-03-c.html css2.1/t120401-scope-04-d.html css2.1/t120403-content-none-00-c.html css2.1/t120403-display-none-00-c.html css2.1/t120403-visibility-00-c.html css2.1/t1205-c561-list-displ-00-b.html css2.1/t1205-c563-list-type-00-b.html css2.1/t1205-c563-list-type-01-b.html css2.1/t1205-c564-list-img-00-b-g.html css2.1/t1205-c565-list-pos-00-b.html css2.1/t1205-c566-list-stl-00-e-ag.html css2.1/t1401-c531-color-00-a.html css2.1/t1402-c45-bg-canvas-00-b.html css2.1/t140201-c532-bgcolor-00-a.html css2.1/t140201-c532-bgcolor-01-b.html css2.1/t140201-c533-bgimage-00-a.html css2.1/t140201-c533-bgimage-01-b-g.html css2.1/t140201-c534-bgre-00-b-ag.html css2.1/t140201-c534-bgre-01-b-ag.html css2.1/t140201-c535-bg-fixd-00-b-g.html css2.1/t140201-c536-bgpos-00-b-ag.html css2.1/t140201-c536-bgpos-01-b-ag.html css2.1/t1503-c522-font-family-00-b.html css2.1/t1504-c523-font-style-00-b.html css2.1/t1504-c543-txt-decor-00-d-g.html css2.1/t1505-c524-font-var-00-b.html css2.1/t1506-c525-font-wt-00-b.html css2.1/t1507-c526-font-sz-00-b.html css2.1/t1507-c526-font-sz-01-b-a.html css2.1/t1507-c526-font-sz-02-b-a.html css2.1/t1507-c526-font-sz-03-f-a.html css2.1/t1508-c527-font-00-b.html css2.1/t1601-c547-indent-01-d.html css2.1/t1602-c546-txt-align-00-b.html css2.1/t1606-c562-white-sp-00-b-ag.html css2.1/t170602-bdr-conflct-w-00-d.html css2.1/t170602-bdr-conflct-w-01-d.html css2.1/t170602-bdr-conflct-w-02-d.html css2.1/t170602-bdr-conflct-w-03-d.html css2.1/t170602-bdr-conflct-w-04-d.html css2.1/t170602-bdr-conflct-w-05-d.html css2.1/t170602-bdr-conflct-w-06-d.html css2.1/t170602-bdr-conflct-w-07-d.html css2.1/t170602-bdr-conflct-w-08-d.html css2.1/t170602-bdr-conflct-w-09-d.html css2.1/t170602-bdr-conflct-w-10-d.html css2.1/t170602-bdr-conflct-w-11-d.html css2.1/t170602-bdr-conflct-w-12-d.html css2.1/t170602-bdr-conflct-w-13-d.html css2.1/t170602-bdr-conflct-w-14-d.html css2.1/t170602-bdr-conflct-w-15-d.html css2.1/t170602-bdr-conflct-w-16-d.html css2.1/t170602-bdr-conflct-w-17-d.html css2.1/t170602-bdr-conflct-w-18-d.html css2.1/t170602-bdr-conflct-w-19-d.html css2.1/t170602-bdr-conflct-w-20-d.html css2.1/t170602-bdr-conflct-w-21-d.html css2.1/t170602-bdr-conflct-w-22-d.html css2.1/t170602-bdr-conflct-w-23-d.html css2.1/t170602-bdr-conflct-w-24-d.html css2.1/t170602-bdr-conflct-w-25-d.html css2.1/t170602-bdr-conflct-w-26-d.html css2.1/t170602-bdr-conflct-w-27-d.html css2.1/t170602-bdr-conflct-w-28-d.html css2.1/t170602-bdr-conflct-w-29-d.html css2.1/t170602-bdr-conflct-w-30-d.html css2.1/t170602-bdr-conflct-w-31-d.html css2.1/t170602-bdr-conflct-w-32-d.html css2.1/t170602-bdr-conflct-w-33-d.html css2.1/t170602-bdr-conflct-w-34-d.html css2.1/t170602-bdr-conflct-w-35-d.html css2.1/t170602-bdr-conflct-w-36-d.html css2.1/t170602-bdr-conflct-w-37-d.html css2.1/t170602-bdr-conflct-w-38-d.html css2.1/t170602-bdr-conflct-w-39-d.html css2.1/t170602-bdr-conflct-w-40-d.html css2.1/t170602-bdr-conflct-w-41-d.html css2.1/t170602-bdr-conflct-w-42-d.html css2.1/t170602-bdr-conflct-w-43-d.html css2.1/t170602-bdr-conflct-w-44-d.html css2.1/t170602-bdr-conflct-w-45-d.html css2.1/t170602-bdr-conflct-w-46-d.html css2.1/t170602-bdr-conflct-w-47-d.html css2.1/t170602-bdr-conflct-w-48-d.html css2.1/t170602-bdr-conflct-w-49-d.html css2.1/t170602-bdr-conflct-w-50-d.html css2.1/t170602-bdr-conflct-w-51-d.html css2.1/t170602-bdr-conflct-w-52-d.html css2.1/t170602-bdr-conflct-w-53-d.html css2.1/t170602-bdr-conflct-w-54-d.html css2.1/t170602-bdr-conflct-w-55-d.html css2.1/t170602-bdr-conflct-w-56-d.html css2.1/t170602-bdr-conflct-w-57-d.html css2.1/t170602-bdr-conflct-w-58-d.html css2.1/t170602-bdr-conflct-w-59-d.html css2.1/t170602-bdr-conflct-w-60-d.html css2.1/t170602-bdr-conflct-w-61-d.html css2.1/t170602-bdr-conflct-w-62-d.html css2.1/t170602-bdr-conflct-w-63-d.html css2.1/t170602-bdr-conflct-w-64-d.html css2.1/t170602-bdr-conflct-w-65-d.html css2.1/t170602-bdr-conflct-w-66-d.html css2.1/t170602-bdr-conflct-w-67-d.html css2.1/t170602-bdr-conflct-w-68-d.html css2.1/t170602-bdr-conflct-w-69-d.html

Bug 52960 covers fixing the regression. Closing this bug, continuing the work there.

This patch seems to have introduced some rounding error between 64-bit and 32-bit platforms. You can see that both 64-bit GTK+ bots now fail this set of tests, even after your rebaselines: svg/W3C-SVG-1.1-SE/filters-image-03-f.svg svg/W3C-SVG-1.1-SE/pservers-pattern-03-f.svg svg/text/text-hkern-on-vertical-text.svg svg/text/text-hkern.svg expecte svg/text/text-vkern-on-horizontal-text.svg svg/text/text-vkern.svg

(In reply to comment #44) > This patch seems to have introduced some rounding error between 64-bit and 32-bit platforms. You can see that both 64-bit GTK+ bots now fail this set of tests, even after your rebaselines: > > svg/W3C-SVG-1.1-SE/filters-image-03-f.svg > svg/W3C-SVG-1.1-SE/pservers-pattern-03-f.svg > svg/text/text-hkern-on-vertical-text.svg > svg/text/text-hkern.svg expecte > svg/text/text-vkern-on-horizontal-text.svg > svg/text/text-vkern.svg Similar problem on Qt: https://bugs.webkit.org/show_bug.cgi?id=52812

(In reply to comment #45) > (In reply to comment #44) > > This patch seems to have introduced some rounding error between 64-bit and 32-bit platforms. You can see that both 64-bit GTK+ bots now fail this set of tests, even after your rebaselines: > > > > svg/W3C-SVG-1.1-SE/filters-image-03-f.svg > > svg/W3C-SVG-1.1-SE/pservers-pattern-03-f.svg > > svg/text/text-hkern-on-vertical-text.svg > > svg/text/text-hkern.svg expecte > > svg/text/text-vkern-on-horizontal-text.svg > > svg/text/text-vkern.svg > > Similar problem on Qt: https://bugs.webkit.org/show_bug.cgi?id=52812 Ok, I'm not sure whether I can manage to investigate today. Does anyone have an idea? Maybe the lroundf usage causes differences across 32/64 bit boundaries? If we can't find a solution in the next few hours, it should probably be rolled out. Sorry for the trouble, but that was really unexpected to me...

(In reply to comment #46) > (In reply to comment #45) > > (In reply to comment #44) > > > This patch seems to have introduced some rounding error between 64-bit and 32-bit platforms. You can see that both 64-bit GTK+ bots now fail this set of tests, even after your rebaselines: > > > > > > svg/W3C-SVG-1.1-SE/filters-image-03-f.svg > > > svg/W3C-SVG-1.1-SE/pservers-pattern-03-f.svg > > > svg/text/text-hkern-on-vertical-text.svg > > > svg/text/text-hkern.svg expecte > > > svg/text/text-vkern-on-horizontal-text.svg > > > svg/text/text-vkern.svg > > > > Similar problem on Qt: https://bugs.webkit.org/show_bug.cgi?id=52812 > > Ok, I'm not sure whether I can manage to investigate today. > Does anyone have an idea? Maybe the lroundf usage causes differences across 32/64 bit boundaries? > > If we can't find a solution in the next few hours, it should probably be rolled out. > Sorry for the trouble, but that was really unexpected to me... This patch is causing ~25 tests to fail on the Windows 7 Release Tests bots: http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/8525 http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/8526 http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r76442%20(8526)/results.html

I'm aware of the breakage, and working on a fix...

Created attachment 80041 [details] Fixup patch (after patch v11 landed) Potential fix, let's see whether it builds.

Comment on attachment 80041 [details] Fixup patch (after patch v11 landed) r=me. It may had been better to fix this all at once :-/

Created attachment 80043 [details] Fixup patch v2 (new approach, only SVGFonts changes) Another attempt, which does not involve changing the lroundf() rounding for lineSpacing, I'm worried about the number of rebaselines, that would need discussion first, as it changes the visual appearance of lots of tests. I'll try again with just fixing SVGFonts alone, let ossy try on Qt/64 and see if it helps :-)

Created attachment 80044 [details] Fixup patch v3 Fix ChangeLog, included mac layout test changes, for a full review.

Comment on attachment 80044 [details] Fixup patch v3 LGTM. r=me.

http://trac.webkit.org/changeset/76586 might have broken Qt Linux Release The following tests are not passing: svg/W3C-SVG-1.1-SE/filters-image-03-f.svg svg/W3C-SVG-1.1-SE/pservers-grad-17-b.svg svg/W3C-SVG-1.1-SE/pservers-grad-20-b.svg svg/W3C-SVG-1.1-SE/pservers-pattern-03-f.svg svg/W3C-SVG-1.1/fonts-glyph-02-t.svg svg/custom/svg-fonts-with-no-element-reference.html svg/custom/svg-fonts-without-missing-glyph.xhtml svg/text/text-hkern-on-vertical-text.svg svg/text/text-hkern.svg svg/text/text-vkern-on-horizontal-text.svg svg/text/text-vkern.svg svg/transforms/text-with-mask-with-svg-transform.svg

(In reply to comment #54) > http://trac.webkit.org/changeset/76586 might have broken Qt Linux Release > The following tests are not passing: > svg/W3C-SVG-1.1-SE/filters-image-03-f.svg > svg/W3C-SVG-1.1-SE/pservers-grad-17-b.svg > svg/W3C-SVG-1.1-SE/pservers-grad-20-b.svg > svg/W3C-SVG-1.1-SE/pservers-pattern-03-f.svg > svg/W3C-SVG-1.1/fonts-glyph-02-t.svg > svg/custom/svg-fonts-with-no-element-reference.html > svg/custom/svg-fonts-without-missing-glyph.xhtml > svg/text/text-hkern-on-vertical-text.svg > svg/text/text-hkern.svg > svg/text/text-vkern-on-horizontal-text.svg > svg/text/text-vkern.svg > svg/transforms/text-with-mask-with-svg-transform.svg Fixed by http://trac.webkit.org/changeset/76588. Thx.

Everything is fine again after r76588, no diffs between 32/64 bit bots anymore, closing bug.