Closed Bug 1034304 Opened 5 years ago Closed 5 years ago

HTMLMediaElement NS_DECL_NSIDOMHTMLMEDIAELEMENTs, but doesn't inherit from nsIDOMHTMLMediaElement

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: khuey, Assigned: khuey)

References

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

This is pretty benign, since the subclasses inherit from this, but it blocks bug 1034302.
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → khuey
Status: NEW → ASSIGNED
Attachment #8450577 - Flags: review?(bugs)
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?
Attachment #8450577 - Flags: review?(bugs) → review-
Attached patch PatchSplinter Review
Attachment #8450577 - Attachment is obsolete: true
Attachment #8451374 - Flags: review?(bugs)
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").
Keywords: addon-compat
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 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();
Attachment #8451374 - Flags: review?(bugs) → review+
(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
dev-doc-needed for the removal of the interfaces.
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/db8c2ad5634a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.