[webkit-reviews] review canceled: [Bug 88380] Revert verticalPositionForBox to int : [Attachment 145902] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Emil A Eklund
<eae at chromium.org>'s request for review:
Bug 88380: Revert verticalPositionForBox to int
https://bugs.webkit.org/show_bug.cgi?id=88380

Attachment 145902: Patch
https://bugs.webkit.org/attachment.cgi?id=145902&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=145902&action=review


The change looks good, I am concerned about the image.

> Source/WebCore/rendering/RootInlineBox.cpp:873
> +	       verticalPosition += -static_cast<int>(fontMetrics.xHeight() / 2)
- 
> +		   floorToInt(renderer->lineHeight(firstLine, lineDirection)) /
2 +
> +		   floorToInt(renderer->baselinePosition(baselineType(),
firstLine, lineDirection));

Though there is no explicit preferred breaking rule for arithmetic operators, I
would try to be consistent with the rule for logical expressions and put the
sign at the beginning of the line.

> LayoutTests/fast/sub-pixel/consistent-alignment-across-lines-expected.html:12

> +	   font-family: webkit-ahem;

Is this needed for the test to pass?

* If not, I would rather have this line removed as Ahem makes the output
unreadable.
* If yes, just use font-family: Ahem; and remove the @font-face above as DRT
should know where to fetch Ahem already.

> LayoutTests/fast/sub-pixel/consistent-alignment-across-lines-expected.html:18

> +	  background-image:
url(data:image/gif;base64,R0lGODlhEgANAOMKAAAAABUVFRoaGisrKzk5OUxMTGRkZLS0tM/Pz
9/f3////////////////////////yH5BAEKAA8ALAAAAAASAA0AAART8Ml5Arg3nMkluQIhXMRUYNiw
SceAnYAwAkOCGISBJC4mSKMDwpJBHFC/h+xhQAEMSuSo9EFRnSCmEzrDComAgBGbsuF0PHJq9WipnYJ
B9/UmFyIAOw==);
> +	  background-repeat: no-repeat;

My understanding of copyright is that including this image creates a derivative
work which means you either have the right to submit the image under license
WebKit accepts or we cannot land such a patch. I am not a lawyer though so
don't think this is a definite answer.

Please replace this image by a proper background-color if you need to color
.class (or an existing image) as this simplifies the above legal implication.

> LayoutTests/fast/sub-pixel/consistent-alignment-across-lines-expected.html:30

> +    <div>

Could we have a short description of what you test? I would also recommend
adding a link to this bug.