Closed
Bug 704088
Opened 13 years ago
Closed 13 years ago
Pass dom::Element to nsLayoutUtils::SurfaceFromElement
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: Ms2ger, Assigned: Ms2ger)
References
Details
Attachments
(1 file)
22.98 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Most callers have an nsIContent, and it could reduce the number of QIs in nsLayoutUtils::SurfaceFromElement.
Comment 1•13 years ago
|
||
Seems reasonable to me. Or even Element?
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #575989 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → Ms2ger
Comment 3•13 years ago
|
||
Comment on attachment 575989 [details] [diff] [review] Patch v1 > WebGLContext::DOMElementToImageSurface(nsIDOMElement *imageOrCanvas, >+ if (!content) { Followup bug to mark nsIDOMElement as builtinclass and remove the nullcheck, assuming imageOrCanvas is known to be non-null here? >+ if (nsHTMLCanvasElement* canvas = nsHTMLCanvasElement::FromContent(content)) { Extra pair of parens around assignment used as condition, please. >+++ b/layout/base/nsLayoutUtils.cpp >+ if (nsHTMLCanvasElement* canvas = nsHTMLCanvasElement::FromContent(aElement)) { Same here. >+ if (nsHTMLVideoElement* video = nsHTMLVideoElement::FromContent(aElement)) { And here. r=me with those nits.
Attachment #575989 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 4•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #3) > Comment on attachment 575989 [details] [diff] [review] [diff] [details] [review] > Patch v1 > > > WebGLContext::DOMElementToImageSurface(nsIDOMElement *imageOrCanvas, > >+ if (!content) { > > Followup bug to mark nsIDOMElement as builtinclass and remove the nullcheck, > assuming imageOrCanvas is known to be non-null here? It can be null, afaict. The custom quickstub calls the _dom functions without checking if the argument it tried to convert to an nsIDOMElement* actually was one: <http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/CustomQS_WebGL.h#314>. (But a followup to unwrap to dom::Element directly?) > > >+ if (nsHTMLCanvasElement* canvas = nsHTMLCanvasElement::FromContent(content)) { > > Extra pair of parens around assignment used as condition, please. This isn't usually done with this pattern, fwiw.
Comment 5•13 years ago
|
||
> But a followup to unwrap to dom::Element directly? Sounds lovely. > This isn't usually done with this pattern, fwiw. Why not? I'd expect a compiler warning on the pattern otherwise....
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #5) > > But a followup to unwrap to dom::Element directly? > > Sounds lovely. Bug 706052. > > This isn't usually done with this pattern, fwiw. > > Why not? I'd expect a compiler warning on the pattern otherwise.... nsHTMLCanvasElement* canvas; if (canvas = nsHTMLCanvasElement::FromContent(content)) would warn, but having the declaration in the condition as well makes it clear I didn't typo '==' (the second time you read it, at least :)).
Comment 7•13 years ago
|
||
Ah, good point. OK, fine.
Assignee | ||
Comment 8•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/881155b89a68
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Summary: Pass nsIContent to nsLayoutUtils::SurfaceFromElement? → Pass dom::Element to nsLayoutUtils::SurfaceFromElement
Target Milestone: --- → mozilla11
You need to log in
before you can comment on or make changes to this bug.
Description
•