[webkit-reviews] review denied: [Bug 36906] (non-generated) code should only use CanvasRenderingContext::canvas as a CanvasSurface. : [Attachment 52344] Proposed fix.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied David Levin <levin at chromium.org>'s
request for review:
Bug 36906: (non-generated) code should only use CanvasRenderingContext::canvas
as a CanvasSurface.
https://bugs.webkit.org/show_bug.cgi?id=36906

Attachment 52344: Proposed fix.
https://bugs.webkit.org/attachment.cgi?id=52344&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +RenderBox* CanvasSurface::domRenderBox() const

> +RenderStyle* CanvasSurface::domComputedStyle()

Why do these two function names start with the prefix, DOM? Would the names be
ambiguous or confusing without it? I suggest removing it.

> +CanvasRenderingContext2D::CanvasRenderingContext2D(HTMLCanvasElement*
canvas, bool inCompatMode, bool usesDashbardCompatibilityMode)

I don't think there's enough context here for the names inCompatMode and
m_inCompatMode. In WebCore::Document it's already a pretty weak name, but here
it's even weaker. I think usesCompatParseModeForCSS would be better. You might
be able to think of something even better.

> +    // Silence unused warnings when dashboard support isn't enabled.
> +    ((void) usesDashbardCompatibilityMode);

This should be:

#if !ENABLE(DASHBOARD_SUPPORT)
    ASSERT_UNUSED(usesDashboardCompatibilityMode,
!usesDashboardCompatibilityMode);
#endif

The "cast to void" unused parameter idiom should never be used directly, only
through either the UNUSED_PARAM or ASSERT_UNUSED macro.

review- primarily because of the cast to void