Closed Bug 1403819 Opened 7 years ago Closed 7 years ago

Remove nsIDOMHTMLCanvasElement

Categories

(Core :: DOM: Core & HTML, enhancement)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: qdot, Assigned: qdot)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1403819 - Remove nsIDOMHTMLCanvasElement;
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
Continuing post-addon-deprecation XPCOM interface cleanup
Attachment #8913370 - Flags: review?(bzbarsky)
Comment on attachment 8913370 [details]
Bug 1403819 - Remove nsIDOMHTMLCanvasElement;

https://reviewboard.mozilla.org/r/184716/#review191656

r=me

::: dom/base/nsDOMWindowUtils.cpp:1631
(Diff revision 2)
> -
> -  MOZ_ASSERT(node->IsElement(),
> -             "An nsINode that implements nsIDOMHTMLCanvasElement should "
> -             "be an element.");
>    nsLayoutUtils::SurfaceFromElementResult result =
> -    nsLayoutUtils::SurfaceFromElement(node->AsElement());
> +    nsLayoutUtils::SurfaceFromElement(aCanvas->AsElement());

You don't need the AsElement() bit here.  That will also be a bit more efficient (static vs dynamic dispatch for SurfaceFromElement), though I doubt that matters much here.

::: dom/base/nsDOMWindowUtils.cpp:1650
(Diff revision 2)
>        retVal == nullptr)
>      return NS_ERROR_FAILURE;
>  
> -  RefPtr<DataSourceSurface> img1 = CanvasToDataSourceSurface(aCanvas1);
> -  RefPtr<DataSourceSurface> img2 = CanvasToDataSourceSurface(aCanvas2);
> +  nsCOMPtr<nsIContent> contentCanvas1 = do_QueryInterface(aCanvas1);
> +  nsCOMPtr<nsIContent> contentCanvas2 = do_QueryInterface(aCanvas2);
> +  if (!contentCanvas1 || !contentCanvas2) {

You could just drop this part and use FromContentOrNull to get the HTMLCanvasElement instances.

::: dom/base/nsDOMWindowUtils.cpp:1654
(Diff revision 2)
> -  RefPtr<DataSourceSurface> img2 = CanvasToDataSourceSurface(aCanvas2);
> +  nsCOMPtr<nsIContent> contentCanvas2 = do_QueryInterface(aCanvas2);
> +  if (!contentCanvas1 || !contentCanvas2) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  RefPtr<HTMLCanvasElement> canvas1 = HTMLCanvasElement::FromContent(contentCanvas1);

Should be fine to just use "auto" here.  We don't really need the extra strong refs.

::: dom/interfaces/base/nsIDOMWindowUtils.idl:930
(Diff revision 2)
>     * the maximum difference in a channel.  This will throw an error if
>     * the dimensions of the two canvases are different.
>     *
>     * This method requires chrome privileges.
>     */
> -  uint32_t compareCanvases(in nsIDOMHTMLCanvasElement aCanvas1,
> +  uint32_t compareCanvases(in nsIDOMNode aCanvas1,

Could even just make it take two nsISupports....

::: widget/windows/TaskbarPreview.cpp:351
(Diff revision 2)
>        !mPreview->IsWindowAvailable()) {
>      return NS_ERROR_FAILURE;
>    }
>  
> -  nsCOMPtr<nsIDOMHTMLCanvasElement> domcanvas(do_QueryInterface(aCanvas));
> -  dom::HTMLCanvasElement * canvas = ((dom::HTMLCanvasElement*)domcanvas.get());
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(aCanvas);
> +  if (!content) {

Again, could roll this into the next bit with FromContentOrNull.

::: widget/windows/TaskbarPreview.cpp:355
(Diff revision 2)
> -  nsCOMPtr<nsIDOMHTMLCanvasElement> domcanvas(do_QueryInterface(aCanvas));
> -  dom::HTMLCanvasElement * canvas = ((dom::HTMLCanvasElement*)domcanvas.get());
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(aCanvas);
> +  if (!content) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  RefPtr<dom::HTMLCanvasElement> canvas = dom::HTMLCanvasElement::FromContent(content);

Could use auto.
Attachment #8913370 - Flags: review?(bzbarsky) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 3e80577b62af -d 34fa5d45bc11: rebasing 424266:3e80577b62af "Bug 1403819 - Remove nsIDOMHTMLCanvasElement; r=bz" (tip)
merging dom/canvas/WebGLContext.cpp
merging dom/html/HTMLCanvasElement.cpp
merging dom/html/HTMLCanvasElement.h
merging dom/interfaces/html/moz.build
merging xpcom/reflect/xptinfo/ShimInterfaceInfo.cpp
warning: conflicts while merging dom/interfaces/html/moz.build! (edit, then use 'hg resolve --mark')
warning: conflicts while merging xpcom/reflect/xptinfo/ShimInterfaceInfo.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment on attachment 8913370 [details]
Bug 1403819 - Remove nsIDOMHTMLCanvasElement;

https://reviewboard.mozilla.org/r/184716/#review191678


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/html/HTMLCanvasElement.cpp:407
(Diff revision 3)
>  NS_IMPL_CYCLE_COLLECTION_INHERITED(HTMLCanvasElement, nsGenericHTMLElement,
>                                     mCurrentContext, mPrintCallback,
>                                     mPrintState, mOriginalCanvas,
>                                     mOffscreenCanvas)
>  
> -NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED(HTMLCanvasElement,
> +NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0(HTMLCanvasElement, nsGenericHTMLElement)

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8913370 [details]
Bug 1403819 - Remove nsIDOMHTMLCanvasElement;

https://reviewboard.mozilla.org/r/184716/#review191678

> Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Apologies for this confusing comment, our bot doesn't report macro-related issues too well yet. Please feel free to ignore/drop it.

FYI, the reported "else after return" comes from here:

Note: expanded from macro 'NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0'
Location: obj-x86_64-pc-linux-gnu/dist/include/nsCycleCollectionParticipant.h:973:3

    NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(aClass)                                 \
    ^

Note: expanded from macro 'NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION'
Location: obj-x86_64-pc-linux-gnu/dist/include/nsCycleCollectionParticipant.h:319:5

    NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(_class)
    ^

Note: expanded from macro 'NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION'
Location: obj-x86_64-pc-linux-gnu/dist/include/nsCycleCollectionParticipant.h:311:7

    } else {                                                                   \
      ^
Yeah, no prob, figured it was in the cycle collection macros. I'll file a followup to clean that up anyways.
Comment on attachment 8913370 [details]
Bug 1403819 - Remove nsIDOMHTMLCanvasElement;

https://reviewboard.mozilla.org/r/184716/#review192040


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/html/HTMLCanvasElement.cpp:406
(Diff revision 4)
>  NS_IMPL_CYCLE_COLLECTION_INHERITED(HTMLCanvasElement, nsGenericHTMLElement,
>                                     mCurrentContext, mPrintCallback,
>                                     mPrintState, mOriginalCanvas,
>                                     mOffscreenCanvas)
>  
> -NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED(HTMLCanvasElement,
> +NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0(HTMLCanvasElement, nsGenericHTMLElement)

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/959244bbe99d
Remove nsIDOMHTMLCanvasElement; r=bz
Yeah, already taking care of it. I forgot to run a try on windows while changing windows only code. >.>
Flags: needinfo?(kyle)
https://hg.mozilla.org/mozilla-central/rev/9d16d9e53b9e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: