1403819 - Remove nsIDOMHTMLCanvasElement

Status: RESOLVED FIXED

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

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Continuing post-addon-deprecation XPCOM interface cleanup

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1403819 - Remove nsIDOMHTMLCanvasElement;

Removes the XPCOM interface for nsIDOMHTMLCanvasElement, replacing it
with binding class usage.

Review commit: https://reviewboard.mozilla.org/r/184716/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184716/

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8913370 [details]
Bug 1403819 - Remove nsIDOMHTMLCanvasElement;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184716/diff/1-2/

Comment 3

[Boris Zbarsky [: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.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8913370 [details]
Bug 1403819 - Remove nsIDOMHTMLCanvasElement;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184716/diff/2-3/

Comment 5

[Mozilla Autoland]

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 6

[Code Review Bot [:reviewbot]]

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 7

[Jan Keromnes [:janx]]

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 {                                                                   \
      ^

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yeah, no prob, figured it was in the cycle collection macros. I'll file a followup to clean that up anyways.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8913370 [details]
Bug 1403819 - Remove nsIDOMHTMLCanvasElement;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184716/diff/3-4/

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8913370 [details]
Bug 1403819 - Remove nsIDOMHTMLCanvasElement;

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

Comment 11

[Code Review Bot [:reviewbot]]

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]

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/959244bbe99d
Remove nsIDOMHTMLCanvasElement; r=bz

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for windows build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=135260444&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/b8f4938f37d9a2ac2aadeec28fd45bbf12362234

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yeah, already taking care of it. I forgot to run a try on windows while changing windows only code. >.>

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d16d9e53b9e
Remove nsIDOMHTMLCanvasElement; r=bz

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9d16d9e53b9e