Bug 36906 – (non-generated) code should only use CanvasRenderingContext::canvas as a CanvasSurface.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[David Levin]

"non-generated" because this change will not touch the idl.

[David Levin]

Created attachment 52342 [details]
Proposed fix.

[Early Warning System Bot]

Attachment 52342 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1568160

[David Levin]

Created attachment 52344 [details]
Proposed fix.

Fixes the qt build problem (and any platform on which dashboard support isn't enabled).

[Darin Adler]

Comment on attachment 52344 [details]
Proposed fix.

> +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

[David Levin]

Created attachment 52443 [details]
Proposed fix.

[David Levin]

Created attachment 52444 [details]
Proposed fix.

[Darin Adler]

Comment on attachment 52443 [details]
Proposed fix.

You forgot to set review? on this. Or was it intentional? This is only a diff from what you previously posted. Is that to help me see what you did in response to my suggestions?

> +    RenderStyle* computedStyle() {  return HTMLElement::computedStyle(); }

There's an extra space here after the "{" character.

> -    CSSParser parser(!m_inCompatMode); // Use the parse mode of the canvas' document when parsing CSS.
> +    CSSParser parser(!m_usesCSSCompatibilityParseMode);

Heh, now that I see this I wish we had reversed the sense so it was strict, but that would be inconvenient for the caller, so I retract that suggestion.

[Darin Adler]

I see now. Heh, jumped the gun.

[Darin Adler]

Comment on attachment 52444 [details]
Proposed fix.

> +    RenderStyle* computedStyle() {  return HTMLElement::computedStyle(); }

Extra space after the "{".

[David Levin]

(In reply to comment #7)
> (From update of attachment 52443 [details])
> You forgot to set review? on this. Or was it intentional? This is only a diff
> from what you previously posted. Is that to help me see what you did in
> response to my suggestions?

No, I goofed when I told git the revision number to do the diff from. Sorry about that (but the change is completely there). (I had a minor error in the ChangeLog that I caught last time when I removed the r?.)

I'll fix the space issue and commit. Thx!

[David Levin]

Committed as http://trac.webkit.org/changeset/57020

[WebKit Review Bot]

http://trac.webkit.org/changeset/57020 might have broken Chromium Win Release, Chromium Mac Release, and Chromium Linux Release