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

[bugzilla-daemon at webkit.org]

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

Attachment 316070: Patch

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




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

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

Looks good. Just needs a few small changes.

> Source/WebCore/html/TextMetrics.h:86
> +	   , m_actualBoundingBoxLeft(0)
> +	   , m_actualBoundingBoxRight(0)
> +	   , m_fontBoundingBoxAscent(0)
> +	   , m_fontBoundingBoxDescent(0)
> +	   , m_actualBoundingBoxAscent(0)
> +	   , m_actualBoundingBoxDescent(0)
> +	   , m_emHeightAscent(0)
> +	   , m_emHeightDescent(0)
> +	   , m_hangingBaseline(0)
> +	   , m_alphabeticBaseline(0)
> +	   , m_ideographicBaseline(0)

Get rid of all these and....

> Source/WebCore/html/TextMetrics.h:100
> +    float m_actualBoundingBoxLeft;
> +    float m_actualBoundingBoxRight;
> +    float m_fontBoundingBoxAscent;
> +    float m_fontBoundingBoxDescent;
> +    float m_actualBoundingBoxAscent;
> +    float m_actualBoundingBoxDescent;
> +    float m_emHeightAscent;
> +    float m_emHeightDescent;
> +    float m_hangingBaseline;
> +    float m_alphabeticBaseline;
> +    float m_ideographicBaseline;

... put them here instead. e.g. float m_width { 0 };
then you won't need the TextMetrics constructor at all.

In fact, why not just make this a struct with public member variables.

class TextMetrics {
  float width { 0 };
  float actualBoundingBoxLeft { 0 };
...
}

> Source/WebCore/html/TextMetrics.idl:39
> +    readonly attribute unrestricted float actualBoundingBoxLeft;
> +    readonly attribute unrestricted float actualBoundingBoxRight;
> +    readonly attribute unrestricted float fontBoundingBoxAscent;
> +    readonly attribute unrestricted float fontBoundingBoxDescent;
> +    readonly attribute unrestricted float actualBoundingBoxAscent;
> +    readonly attribute unrestricted float actualBoundingBoxDescent;
> +    readonly attribute unrestricted float emHeightAscent;
> +    readonly attribute unrestricted float emHeightDescent;
> +    readonly attribute unrestricted float hangingBaseline;
> +    readonly attribute unrestricted float alphabeticBaseline;
> +    readonly attribute unrestricted float ideographicBaseline;

Oh. I guess that our bindings code always requires a getter/setter pair, rather
than direct access into variables.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2335
> +    const FontProxy& font = fontProxy();

Use auto.

auto& font = fontProxy();

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2336
> +    const FontMetrics& fontMetrics = font.fontMetrics();

Ditto.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2364
> +    TextAlign align = physicalAlignment();
> +
> +    float xoffset = 0;
> +    switch (align) {

You don't use align other than this, so just switch(physicalAlignment()) ?