Bug 36906 - (non-generated) code should only use CanvasRenderingContext::canvas as a CanvasSurface.
Summary: (non-generated) code should only use CanvasRenderingContext::canvas as a Canv...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on: 37042
Blocks: 34909
  Show dependency treegraph
 
Reported: 2010-03-31 16:22 PDT by David Levin
Modified: 2010-04-02 13:49 PDT (History)
3 users (show)

See Also:


Attachments
Proposed fix. (14.37 KB, patch)
2010-04-01 14:53 PDT, David Levin
no flags Details | Formatted Diff | Diff
Proposed fix. (14.63 KB, patch)
2010-04-01 15:22 PDT, David Levin
darin: review-
Details | Formatted Diff | Diff
Proposed fix. (10.28 KB, patch)
2010-04-02 13:14 PDT, David Levin
no flags Details | Formatted Diff | Diff
Proposed fix. (10.32 KB, patch)
2010-04-02 13:18 PDT, David Levin
darin: review+
levin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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:

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