[webkit-reviews] review granted: [Bug 222402] REGRESSION(r269957): Empty font names passed to canvas2d cause all text routines to crash : [Attachment 421497] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 222402: REGRESSION(r269957): Empty font names passed to canvas2d cause all
text routines to crash
https://bugs.webkit.org/show_bug.cgi?id=222402

Attachment 421497: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 421497
  --> https://bugs.webkit.org/attachment.cgi?id=421497
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:137
> +    modifiableState().unparsedFont = newFontSafeCopy;
> +
> +    modifiableState().font.initialize(document.fontSelector(), *fontStyle);

I think this code is easier to understand if these two are in a single
paragraph, not two separate ones.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:139
> +    ASSERT(state().font.realized() && state().font.isPopulated());

Why not two separate asserts? That way it’s slightly easier to tell which of
the two failed.

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.h:238
> +	   bool isPopulated() const { return m_font.fonts(); }

It’s a little frustrating to make something like this public just so we can do
an assertion. Minimizing interfaces often makes the code more maintainable in
the long run.