Bug 36906

Summary: (non-generated) code should only use CanvasRenderingContext::canvas as a CanvasSurface.
Product: WebKit Reporter: David Levin <levin>
Component: DOMAssignee: David Levin <levin>
Severity: Normal CC: darin, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 37042    
Bug Blocks: 34909    
Description Flags
Proposed fix.
Proposed fix.
darin: review-
Proposed fix.
Proposed fix. darin: review+, levin: commit-queue-

Description David Levin 2010-03-31 16:22:36 PDT
"non-generated" because this change will not touch the idl.
Comment 1 David Levin 2010-04-01 14:53:54 PDT
Created attachment 52342 [details]
Proposed fix.
Comment 2 Early Warning System Bot 2010-04-01 14:59:32 PDT
Attachment 52342 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1568160
Comment 3 David Levin 2010-04-01 15:22:55 PDT
Created attachment 52344 [details]
Proposed fix.

Fixes the qt build problem (and any platform on which dashboard support isn't enabled).
Comment 4 Darin Adler 2010-04-01 16:25:57 PDT
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:

    ASSERT_UNUSED(usesDashboardCompatibilityMode, !usesDashboardCompatibilityMode);

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
Comment 5 David Levin 2010-04-02 13:14:52 PDT
Created attachment 52443 [details]
Proposed fix.
Comment 6 David Levin 2010-04-02 13:18:09 PDT
Created attachment 52444 [details]
Proposed fix.
Comment 7 Darin Adler 2010-04-02 13:19:49 PDT
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.
Comment 8 Darin Adler 2010-04-02 13:19:59 PDT
I see now. Heh, jumped the gun.
Comment 9 Darin Adler 2010-04-02 13:20:51 PDT
Comment on attachment 52444 [details]
Proposed fix.

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

Extra space after the "{".
Comment 10 David Levin 2010-04-02 13:25:14 PDT
(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!
Comment 11 David Levin 2010-04-02 13:32:25 PDT
Committed as http://trac.webkit.org/changeset/57020
Comment 12 WebKit Review Bot 2010-04-02 13:47:10 PDT
http://trac.webkit.org/changeset/57020 might have broken Chromium Win Release, Chromium Mac Release, and Chromium Linux Release