1034304 - HTMLMediaElement NS_DECL_NSIDOMHTMLMEDIAELEMENTs, but doesn't inherit from nsIDOMHTMLMediaElement

Status: RESOLVED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is pretty benign, since the subclasses inherit from this, but it blocks bug 1034302.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I don't understand the setup.
Why not just  public nsIDOMHTMLMediaElement, and then in QI of HTMLMediaElement support
nsIDOMHTMLMediaElement. That would be less-surprising. Or is there something preventing that setup?

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The only two addons using nsIDOMHTMLVideo/AudioElement are NoScript and mfinkle's MobileTools.  It should be straightforward to replace "instanceof Ci.nsIDOMHTMLVideoElement" with "instanceof HTMLVideoElement" (or possibly "instanceof appropriateWindow.HTMLVideoElement").

Comment 5

[Giorgio Maone [:ma1]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #4)
> The only two addons using nsIDOMHTMLVideo/AudioElement are NoScript and

Thanks for the CC.
Fixed in 2.6.8.32rc2, http://noscript.net/getit#devel

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8451374 [details] [diff] [review]
Patch

> namespace mozilla {
> namespace dom {
> 
>-class HTMLAudioElement MOZ_FINAL : public HTMLMediaElement,
>-                                   public nsIDOMHTMLAudioElement
>+class HTMLAudioElement MOZ_FINAL : public HTMLMediaElement
> {
>-public:
>-  HTMLAudioElement(already_AddRefed<mozilla::dom::NodeInfo>& aNodeInfo);
>+private:
>   virtual ~HTMLAudioElement();
> 
>-  // nsISupports
>-  NS_DECL_ISUPPORTS_INHERITED
>+public:
>+  typedef mozilla::dom::NodeInfo NodeInfo;
Do you actually need this typedef? The class is already in mozilla::dom

> 
>-class HTMLVideoElement MOZ_FINAL : public HTMLMediaElement,
>-                                   public nsIDOMHTMLVideoElement
>+class HTMLVideoElement MOZ_FINAL : public HTMLMediaElement
> {
>+private:
>+  virtual ~HTMLVideoElement();
>+
> public:
>-  HTMLVideoElement(already_AddRefed<mozilla::dom::NodeInfo>& aNodeInfo);
>-  virtual ~HTMLVideoElement();
>+  typedef mozilla::dom::NodeInfo NodeInfo;
ditto

>+  NS_IMETHOD_(bool) IsVideo() MOZ_OVERRIDE {

>-  virtual nsresult Clone(mozilla::dom::NodeInfo *aNodeInfo, nsINode **aResult) const MOZ_OVERRIDE;
>+  virtual nsresult Clone(NodeInfo *aNodeInfo, nsINode **aResult) const MOZ_OVERRIDE;
Not really complaining about * usage. (it should go with the type.)
>+NS_IMETHODIMP_(bool)
>+HTMLMediaElement::IsVideo()
>+{
>+  return false;
>+}
I don't understand IsVideo.
Why you add it to .idl?
Just add it to HTMLMediaElement as a virtual method.

>   if (Preferences::GetBool("media.capturestream_hints.enabled")) {
>-    nsCOMPtr<nsIDOMHTMLVideoElement> video = do_QueryObject(this);
>-    if (video && GetVideoFrameContainer()) {
>+    if (this->IsVideo() && GetVideoFrameContainer()) {
drop this->

>-[uuid(1f9393e8-2df0-4072-87b9-c26999b09acc)]
>+[uuid(0e14e6ad-2074-48b7-a247-e545a3a15131)]
> interface nsIDOMHTMLMediaElement : nsISupports
> {
>   // error state
>   readonly attribute nsIDOMMediaError error;
> 
>   // network state
>            attribute DOMString src;
>            attribute nsIDOMMediaStream mozSrcObject;
>@@ -128,9 +128,11 @@ interface nsIDOMHTMLMediaElement : nsISu
>    //   Always plays in speaker, even when headphones are plugged in.
>    //   Use case: Camera shutter sound.
>    attribute DOMString mozAudioChannelType;
> 
>   // In addition the media element has this new events:
>   // * onmozinterruptbegin - called when the media element is interrupted
>   //   because of the audiochannel manager.
>   // * onmozinterruptend - called when the interruption is concluded
>+
>+  [notxpcom] boolean isVideo();
So I wouldn't do these changes

> bool nsVideoFrame::HasVideoElement() {
>-  nsCOMPtr<nsIDOMHTMLVideoElement> videoDomElement = do_QueryInterface(mContent);
>-  return videoDomElement != nullptr;
>+  nsCOMPtr<nsIDOMHTMLMediaElement> mediaDomElement = do_QueryInterface(mContent);
>+  return mediaDomElement->IsVideo();
Here I would just 
return static_cast<mozilla::dom::HTMLMediaElement>(mediaDomElement.get())->IsVideo();

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Olli Pettay [:smaug] from comment #6)
> >+public:
> >+  typedef mozilla::dom::NodeInfo NodeInfo;
> Do you actually need this typedef? The class is already in mozilla::dom

Yes, because without it NodeInfo will pick up nsINode::NodeInfo instead of mozilla::dom::NodeInfo.  This is why I had to fully qualify them when I did the mass replace :/

> I don't understand IsVideo.
> Why you add it to .idl?
> Just add it to HTMLMediaElement as a virtual method.

I didn't want to deal with this.  I'll file a followup for removing internal use of nsIDOMHTMLMediaElement.

https://hg.mozilla.org/integration/mozilla-inbound/rev/db8c2ad5634a

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

dev-doc-needed for the removal of the interfaces.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db8c2ad5634a