[webkit-reviews] review denied: [Bug 82798] measureText() improvements : [Attachment 187137] patch updated against tot

[bugzilla-daemon at webkit.org]

Alexandru Chiculita <achicu at adobe.com> has denied arno. <arno at renevier.net>'s
request for review:
Bug 82798: measureText() improvements
https://bugs.webkit.org/show_bug.cgi?id=82798

Attachment 187137: patch updated against tot
https://bugs.webkit.org/attachment.cgi?id=187137&action=review

------- Additional Comments from Alexandru Chiculita <achicu at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=187137&action=review


Thanks for working on this patch. I've been long looking for the text metrics
on canvas. I have a few nits below.

Also, not sure if you need a runtime flag for this feature. Did you ask on the
dev list about it? Also, exposing more of the text-metrics and "font" details
might potentially introduce a new leak that might need to be investigated.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2160
> +    switch (state().m_textBaseline) {

Is the drawTextInternal doing the same alignment math? Can you extract a
function from CanvasRenderingContext2D::drawTextInternal and reuse it here?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2177
> +    TextAlign align = physicalAlignment();

ditto.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2417
> +    TextDirection direction = computedStyle ? computedStyle->direction() :
LTR;

Looks like the spec now has a "direction" attribute on the context.

> LayoutTests/fast/canvas/canvas-measureText-2.html:9
> +    var texts = ['Some simple text', 'དབུ་མེད་']; // tibetan
script triggers complex path

The spec has a nice visual representation of the baselines. I think this test
is good, but might be better to also have a visual test with all the
boxes/baselines. Is there a way to make it a ref-test? For example the ref-test
might be using DIV blocks to compare that the baselines are consistent.

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

nit: Might be useful to include the current values of the "for" statements (ie.
text, baseline, alignment).

> LayoutTests/fast/canvas/canvas-measureText-2.html:40
> +		       if (baseline === 'top')
> +			   if (metrics.emHeightAscent !== 0)

nit: combine the nested ifs.

> LayoutTests/fast/canvas/canvas-measureText-2.html:88
> +	   </script>

nit: <script> is not aligned with </script>