[webkit-reviews] review granted: [Bug 82798] Implement new TextMetrics, returned by canvas measureText() : [Attachment 316272] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Fujii Hironori
<Hironori.Fujii at sony.com>'s request for review:
Bug 82798: Implement new TextMetrics, returned by canvas measureText()
https://bugs.webkit.org/show_bug.cgi?id=82798

Attachment 316272: Patch

https://bugs.webkit.org/attachment.cgi?id=316272&action=review




--- Comment #33 from Dean Jackson <dino at apple.com> ---
Comment on attachment 316272
  --> https://bugs.webkit.org/attachment.cgi?id=316272
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316272&action=review

Looks good. Just tiny changes (that I'm sorry I didn't notice last time) - I'll
commit when a new patch is uploaded.

> LayoutTests/fast/canvas/canvas-measureText-2.html:31
> +			       if ((metrics.actualBoundingBoxLeft +
metrics.actualBoundingBoxRight) < (metrics.width - 1) ||
> +				   (metrics.actualBoundingBoxLeft +
metrics.actualBoundingBoxRight) > (metrics.width + 1))
> +				   testFailed('metrics.actualBoundingBoxLeft +
metrics.actualBoundingBoxRight must be about the same as width' + condition)

I think for each of these it would be nice to also have PASS/testPassed
results, so that we can confirm changes to the test and the expected results at
the same time.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2384
> +	   // Do nothing.

No need for this comment.

> Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp:68
> +	   // FIXME: Calculate.

What does this mean? Do you mean "Calculate the actual values rather than just
the font's ascent and descent"?