Closed Bug 1407040 Opened 2 years ago Closed 2 years ago

Remove nsIDOMHTMLMediaElement

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: qdot, Assigned: qdot)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(5 files)

Continuing post-addon-deprecation XPCOM interface cleanup
Priority: -- → P3
Kyle, do you have time to pick this up again?  I don't think this depends on bug 1387178 -- we can leave the shim map entry while removing the interface everywhere else.
Flags: needinfo?(kyle)
Last try run I ran on a rebased version of the patches to this bug wasn't pretty.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f4c6daf8a95582864c14f26673a9543bc971304

Haven't had a chance to diagnose what I blew up yet though. Was trying to just land the MIDI core (now waiting on final review so basically done), and am now yanked off onto site isolation.
Flags: needinfo?(kyle)
And of course, right after I say that and take a quick look, found a logic flaw in the nsDocument patches that may be causing a good bit of the crashes. Gonna poke at this a bit more and retry, will let you know.
The other thing I noticed in the patches is that getClassName() will never be "HTMLMediaElement".  It will be "HTMLVideoElement" or "HTMLAudioElement".
Even better, notice that in HTMLMediaElement.h I put NS_IMPL_FROMCONTENT_HTML_WITH_TAG(HTMLMediaElement, media), then I do a bunch of calls to the resulting FromContent functions, which will never work.

I really wish I knew what it is about my subconscious that loves creating new HTML tags. Fixing now.
Comment on attachment 8921658 [details]
Bug 1407040 - Change HTMLMediaElement XPCOM Enums to WebIDL Enums;

https://reviewboard.mozilla.org/r/192652/#review226352

::: dom/canvas/ImageBitmap.cpp:25
(Diff revision 4)
>  #include "ImageUtils.h"
>  #include "imgTools.h"
>  
>  using namespace mozilla::gfx;
>  using namespace mozilla::layers;
> +using mozilla::dom::HTMLMediaElementBinding::NETWORK_EMPTY;

Should be including mozilla/dom/HTMLMediaElementBinding.h in this file, right?

::: dom/html/HTMLMediaElement.cpp:6385
(Diff revision 4)
>    //   playback has not stopped due to errors,
>    //   and the element has not paused for user interaction
>    return
>      !mPaused &&
> -    (mReadyState == nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA ||
> -    mReadyState == nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA) &&
> +    (mReadyState == HAVE_ENOUGH_DATA ||
> +    mReadyState == HAVE_FUTURE_DATA) &&

The indent here looks off (preexisting, I know).

::: dom/media/AutoplayPolicy.cpp:56
(Diff revision 4)
>      return true;
>    }
>  
>    // Media has already loaded metadata and doesn't contain audio track
>    if (aElement->IsVideo() &&
> -      aElement->ReadyState() >= nsIDOMHTMLMediaElement::HAVE_METADATA &&
> +      aElement->ReadyState() >= HTMLMediaElementBinding::HAVE_METADATA &&

Should include mozilla/dom/HTMLMediaElementBinding.h

::: layout/base/nsLayoutUtils.cpp:160
(Diff revision 4)
>  using namespace mozilla::dom;
>  using namespace mozilla::image;
>  using namespace mozilla::layers;
>  using namespace mozilla::layout;
>  using namespace mozilla::gfx;
> +using mozilla::dom::HTMLMediaElementBinding::HAVE_NOTHING;

Should include the header.
Attachment #8921658 - Flags: review?(bzbarsky) → review+
Comment on attachment 8951139 [details]
Bug 1407040 - Change nsIDOMHTMLMediaElement QIs to Media/Video type checks;

https://reviewboard.mozilla.org/r/220396/#review226354

::: dom/webbrowserpersist/WebBrowserPersistLocalDocument.cpp:479
(Diff revision 1)
>  
>      if (content->IsSVGElement(nsGkAtoms::img)) {
>          return OnWalkAttribute(aNode, "href", "http://www.w3.org/1999/xlink");
>      }
>  
> -    nsCOMPtr<nsIDOMHTMLMediaElement> nodeAsMedia = do_QueryInterface(aNode);
> +    if (content->IsHTMLElement(nsGkAtoms::audio) ||

content->IsAnyOfHTMLElements() would be better here.

::: dom/webbrowserpersist/WebBrowserPersistLocalDocument.cpp:999
(Diff revision 1)
>              FixupAttribute(*aNodeOut, "src");
>          }
>          return rv;
>      }
>  
> -    nsCOMPtr<nsIDOMHTMLMediaElement> nodeAsMedia = do_QueryInterface(aNodeIn);
> +    if (content->IsHTMLElement(nsGkAtoms::audio) ||

Again, IsAnyOfHTMLElements()

::: mobile/android/chrome/content/browser.js:6209
(Diff revision 1)
>  
>    // extend _getLink to pickup html5 media links.
>    _getMediaLink: function(aElement) {
>      let uri = NativeWindow.contextmenus._getLink(aElement);
> -    if (uri == null && aElement.nodeType == Ci.nsIDOMNode.ELEMENT_NODE && (aElement instanceof Ci.nsIDOMHTMLMediaElement)) {
> +    if (uri == null &&
> +        aElement.nodeType == Ci.nsIDOMNode.ELEMENT_NODE &&

You can probably drop this nodeType check, since the classname better ensure that anyway...
Attachment #8951139 - Flags: review?(bzbarsky) → review+
Comment on attachment 8951140 [details]
Bug 1407040 - Change HTMLMediaElement QIs to FromContent calls;

https://reviewboard.mozilla.org/r/220398/#review226356

r=me if we drop the manual FromContent bits in favor of using the macro with the already-existing predicate.

::: dom/base/nsNodeUtils.cpp:608
(Diff revision 1)
>          }
>        }
>      }
>  
>      if (wasRegistered && oldDoc != newDoc) {
> -      nsCOMPtr<nsIDOMHTMLMediaElement> domMediaElem(do_QueryInterface(aNode));
> +      nsCOMPtr<nsIContent> content(do_QueryInterface(aNode));

Could use AsContent/IsContent here and avoid the virtual calls and refcounting...

::: dom/html/HTMLMediaElement.h:153
(Diff revision 1)
> +  static HTMLMediaElement* FromContent(nsIContent* aContent);
> +  static const HTMLMediaElement* FromContent(const nsIContent* aContent);
> +  static HTMLMediaElement* FromContentOrNull(nsIContent* aContent);
> +  static const HTMLMediaElement* FromContentOrNull(const nsIContent* aContent);

How about replacing all that, and the bits in the .cpp, with:

    NS_IMPL_FROMCONTENT_HELPER(HTMLMediaElement,
      IsAnyOfHTMLElements(nsGkAtoms::video,
                          nsGkAtoms::audio))
                          
(with reasonable indentation).

::: layout/generic/nsVideoFrame.cpp:826
(Diff revision 1)
>  
>    nsContainerFrame::OnVisibilityChange(aNewVisibility, aNonvisibleAction);
>  }
>  
>  bool nsVideoFrame::HasVideoElement() {
> -  nsCOMPtr<nsIDOMHTMLMediaElement> mediaDomElement = do_QueryInterface(mContent);
> +  HTMLMediaElement* mediaDomElement = HTMLMediaElement::FromContentOrNull(GetContent());

GetContent() can't be null here.  Also, why the change from using mContent?

::: layout/generic/nsVideoFrame.cpp:827
(Diff revision 1)
>    nsContainerFrame::OnVisibilityChange(aNewVisibility, aNonvisibleAction);
>  }
>  
>  bool nsVideoFrame::HasVideoElement() {
> -  nsCOMPtr<nsIDOMHTMLMediaElement> mediaDomElement = do_QueryInterface(mContent);
> +  HTMLMediaElement* mediaDomElement = HTMLMediaElement::FromContentOrNull(GetContent());
> +  if (!mediaDomElement) {

Can't happen.  And nsVideoFrame is guaranteed to have an HTML audio or video as mContent.

In fact, if you wanted to you could just replace all these FromContent with static_cast...

::: layout/generic/nsVideoFrame.cpp:845
(Diff revision 1)
>    return size != nsIntSize(0,0);
>  }
>  
>  void nsVideoFrame::UpdateTextTrack()
>  {
> -  HTMLMediaElement* element = static_cast<HTMLMediaElement*>(GetContent());
> +  HTMLMediaElement* element = HTMLMediaElement::FromContentOrNull(GetContent());

The static_cast was in fact safe.

::: layout/generic/nsVideoFrame.cpp:846
(Diff revision 1)
>  }
>  
>  void nsVideoFrame::UpdateTextTrack()
>  {
> -  HTMLMediaElement* element = static_cast<HTMLMediaElement*>(GetContent());
> +  HTMLMediaElement* element = HTMLMediaElement::FromContentOrNull(GetContent());
>    if (element) {

And this can't be null.
Attachment #8951140 - Flags: review?(bzbarsky) → review+
Comment on attachment 8951141 [details]
Bug 1407040 - Remove C++ usage of nsIDOMHTMLMediaElement;

https://reviewboard.mozilla.org/r/220400/#review226358

r=me with the various bits below fixed.

::: commit-message-b7618:1
(Diff revision 1)
> +Bug 1407040 - Replace HTMLMediaElement XPCOM methods to WebIDL; r=bz

Maybe "Stop using nsIDOMHTMLMediaElement from C++"?

::: dom/html/HTMLMediaElement.h:450
(Diff revision 1)
>  
>    // WebIDL
>  
>    MediaError* GetError() const;
>  
> -  void SetSrc(const nsAString& aSrc, nsIPrincipal* aTriggeringPrincipal, ErrorResult& aRv)
> +  void GetSrc(nsAString& aSrc, nsIPrincipal&)

You don't need a GetSrc taking a principal.  No one calls such a thing on these elements.

::: dom/html/HTMLMediaElement.h:458
(Diff revision 1)
> +  }
> +  void GetSrc(nsAString& aSrc)
> +  {
> +    GetURIAttr(nsGkAtoms::src, nullptr, aSrc);
> +  }
> +  void SetSrc(const nsAString& aSrc)

Who calls this one-arg SetSrc?  I don't see any consumers.

::: dom/html/HTMLMediaElement.h:472
(Diff revision 1)
> +  void SetSrc(const nsAString& aSrc, nsIPrincipal* aTriggeringPrincipal, ErrorResult& aError)
>    {
> -    SetHTMLAttr(nsGkAtoms::src, aSrc, aTriggeringPrincipal, aRv);
> +    SetHTMLAttr(nsGkAtoms::src, aSrc, aTriggeringPrincipal, aError);
>    }
>  
> -  // XPCOM GetCurrentSrc() is OK
> +  void GetCurrentSrc(nsAString & aCurrentSrc);

No spsce before '&' please.

::: dom/html/HTMLMediaElement.h:528
(Diff revision 1)
>  
>    void SetCurrentTime(double aCurrentTime, ErrorResult& aRv);
> +  void SetCurrentTime(double aCurrentTime)
> +  {
> +    IgnoredErrorResult res;
> +    SetCurrentTime(aCurrentTime, res);

Just pass IgnoreErrors() for the second arg, and no need for the IgnoredErrorResult.

::: dom/html/HTMLMediaElement.h:608
(Diff revision 1)
>  
>    void Pause(ErrorResult& aRv);
> +  void Pause()
> +  {
> +    IgnoredErrorResult res;
> +    Pause(res);

Again, pass IgnoreErrors().

::: dom/html/HTMLMediaElement.cpp:976
(Diff revision 1)
>      }
>  
>      SetSuspended(aSuspend);
>      if (aSuspend == nsISuspendedTypes::SUSPENDED_PAUSE ||
>          aSuspend == nsISuspendedTypes::SUSPENDED_PAUSE_DISPOSABLE) {
> -        nsresult rv = mOwner->Pause();
> +        ErrorResult rv;

This should be IgnoredErrorResult, or you need to SuppressException() on it in the failure-return case.

::: dom/html/HTMLMediaElement.cpp:991
(Diff revision 1)
>    void
>    Stop()
>    {
>      SetSuspended(nsISuspendedTypes::NONE_SUSPENDED);
> -    mOwner->Pause();
> +    IgnoredErrorResult rv;
> +    mOwner->Pause(rv);

We have a no-arg Pause() signature (that ignores errors), no?

::: dom/html/HTMLMediaElement.cpp:1714
(Diff revision 1)
> -
> -NS_IMETHODIMP HTMLMediaElement::GetCurrentSrc(nsAString & aCurrentSrc)
>  {
>    nsAutoCString src;
>    GetCurrentSpec(src);
>    aCurrentSrc = NS_ConvertUTF8toUTF16(src);

Preexisting, but:

    CopyUTF8toUTF16(src, aCurrentSrc);
    
is more efficient.

::: dom/html/HTMLMediaElement.cpp:2000
(Diff revision 1)
>  
> +  IgnoredErrorResult rv;
>    SetPlayedOrSeeked(false);
>    mIsRunningLoadMethod = true;
>    AbortExistingLoads();
> -  SetPlaybackRate(mDefaultPlaybackRate);
> +  SetPlaybackRate(mDefaultPlaybackRate, rv);

Pass IgnoreErrors(), drop the IgnoredErrorResult.

::: dom/html/HTMLMediaElement.cpp:2631
(Diff revision 1)
>      ChangeDelayLoadStatus(false);
>      return rv;
>    }
>  
> -  SetPlaybackRate(mDefaultPlaybackRate);
> +  IgnoredErrorResult res;
> +  SetPlaybackRate(mDefaultPlaybackRate, res);

IgnoreErrors()

::: dom/html/HTMLMediaElement.cpp:2695
(Diff revision 1)
>  }
>  
>  void
>  HTMLMediaElement::SetCurrentTime(double aCurrentTime, ErrorResult& aRv)
>  {
> +  // Detect for a NaN and invalid values.

Shouldn't happen.  Webidl says "double", so bindings enforce the "not a NaN" bit.  Which is why only the check was only in the XPCOM version.

Or can we end up with NaNs in parser.GetStartTime() or mDefaultPlaybackStartPosition?  If so, it would be best if we just did this NaN check in the one-arg version of this method, with documentation for why it's needed.

::: dom/html/HTMLMediaElement.cpp:3839
(Diff revision 1)
>      mSrcStreamTracksAvailable(false),
>      mSrcStreamPausedCurrentTime(-1),
>      mShutdownObserver(new ShutdownObserver),
>      mSourcePointer(nullptr),
>      mNetworkState(NETWORK_EMPTY),
> +    mReadyState(HAVE_NOTHING),

Then take out the `= nsIDOMHTMLMediaElement::HAVE_NOTHING` bit where this member is declared?

::: dom/html/HTMLMediaElement.cpp:4080
(Diff revision 1)
>    // Even if we just did Load() or ResumeLoad(), we could already have a decoder
>    // here if we managed to clone an existing decoder.
>    if (mDecoder) {
>      if (mDecoder->IsEnded()) {
> -      SetCurrentTime(0);
> +      IgnoredErrorResult res;
> +      SetCurrentTime(0, res);

Why not keep using the one-arg version, since we have one?  If not (e.g. because we know 0 is not NaN), please IgnoreErrors().

::: dom/html/HTMLMediaElement.cpp:6791
(Diff revision 1)
>      mDecoder->SetPlaybackRate(ClampPlaybackRate(mPlaybackRate));
>    }
>    DispatchAsyncEvent(NS_LITERAL_STRING("ratechange"));
>  }
>  
> -NS_IMETHODIMP HTMLMediaElement::SetPlaybackRate(double aPlaybackRate)
> +void HTMLMediaElement::SetMozPreservesPitch(bool aPreservesPitch)

Break after void.

::: dom/html/HTMLVideoElement.h:43
(Diff revision 1)
>    virtual bool ParseAttribute(int32_t aNamespaceID,
>                                nsAtom* aAttribute,
>                                const nsAString& aValue,
>                                nsIPrincipal* aMaybeScriptedPrincipal,
>                                nsAttrValue& aResult) override;
> -  NS_IMETHOD_(bool) IsAttributeMapped(const nsAtom* aAttribute) const override;
> +  virtual bool IsAttributeMapped(const nsAtom* aAttribute) const override;

This should still be NS_IMETHOD_(bool), I would think, since that's the signature of the thing it's overriding.

::: dom/html/HTMLVideoElement.cpp:110
(Diff revision 1)
>  {
>    nsGenericHTMLElement::MapImageSizeAttributesInto(aAttributes, aData);
>    nsGenericHTMLElement::MapCommonAttributesInto(aAttributes, aData);
>  }
>  
> -NS_IMETHODIMP_(bool)
> +bool

And `NS_IMETHODIMP_(bool)`

::: dom/html/VideoDocument.cpp:119
(Diff revision 1)
> -  element->SetControls(true);
> +  element->SetAutoplay(true, res);
> +  element->SetControls(true, res);

You can't reuse IgnoredErrorResult like that.  Which is why using IgnoreErrors() is better.  ;)
Attachment #8951141 - Flags: review?(bzbarsky) → review+
Comment on attachment 8951142 [details]
Bug 1407040 - Remove nsIDOMHTMLMediaElement;

https://reviewboard.mozilla.org/r/220402/#review226362

::: js/xpconnect/tests/mochitest/test_bug790732.html
(Diff revision 1)
>    is(Ci.nsIDOMMouseScrollEvent, MouseScrollEvent);
>    is(Ci.nsIDOMMutationEvent, MutationEvent);
>    // XXX We can't test this here because it's only exposed to chrome
>    //is(Ci.nsIDOMSimpleGestureEvent, SimpleGestureEvent);
>    is(Ci.nsIDOMUIEvent, UIEvent);
> -  is(Ci.nsIDOMHTMLMediaElement, HTMLMediaElement);

This should stay, pending telemetry for removal.  Luckily, it does not depend on having the xpidl or anything.  ;)

::: xpcom/reflect/xptinfo/ShimInterfaceInfo.cpp
(Diff revision 1)
>  #include "mozilla/dom/HTMLButtonElementBinding.h"
>  #include "mozilla/dom/HTMLFormElementBinding.h"
>  #include "mozilla/dom/HTMLFrameSetElementBinding.h"
>  #include "mozilla/dom/HTMLHtmlElementBinding.h"
>  #include "mozilla/dom/HTMLInputElementBinding.h"
> -#include "mozilla/dom/HTMLMediaElementBinding.h"

This needs to stay.

::: xpcom/reflect/xptinfo/ShimInterfaceInfo.cpp
(Diff revision 1)
>    DEFINE_SHIM(FormData),
>    DEFINE_SHIM_WITH_CUSTOM_INTERFACE(nsIFrameLoader, FrameLoader),
>    DEFINE_SHIM_WITH_CUSTOM_INTERFACE(nsIDOMGeoPositionError, PositionError),
>    DEFINE_SHIM(HTMLFormElement),
>    DEFINE_SHIM(HTMLInputElement),
> -  DEFINE_SHIM(HTMLMediaElement),

So does this.  Again, it doesn't depend on the actual nsIDOMHTMLMediaElement interface in any way.
Attachment #8951142 - Flags: review?(bzbarsky) → review-
Comment on attachment 8951142 [details]
Bug 1407040 - Remove nsIDOMHTMLMediaElement;

https://reviewboard.mozilla.org/r/220402/#review226362

> This needs to stay.

Er, I confused it with the other shim.  This part is fine.

> So does this.  Again, it doesn't depend on the actual nsIDOMHTMLMediaElement interface in any way.

Again, this part is fine.
Comment on attachment 8951142 [details]
Bug 1407040 - Remove nsIDOMHTMLMediaElement;

https://reviewboard.mozilla.org/r/220402/#review226624

r=me with the test bit restored.
Attachment #8951142 - Flags: review- → review+
Comment on attachment 8951141 [details]
Bug 1407040 - Remove C++ usage of nsIDOMHTMLMediaElement;

https://reviewboard.mozilla.org/r/220400/#review226358

> Shouldn't happen.  Webidl says "double", so bindings enforce the "not a NaN" bit.  Which is why only the check was only in the XPCOM version.
> 
> Or can we end up with NaNs in parser.GetStartTime() or mDefaultPlaybackStartPosition?  If so, it would be best if we just did this NaN check in the one-arg version of this method, with documentation for why it's needed.

GetStartTime() defaults to 0, and mDefaultPlaybackStartPosition isn't set in a way that I can see would result in NaN, so I think this can just be removed.
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14f3eecff54f
Change HTMLMediaElement XPCOM Enums to WebIDL Enums; r=bz
https://hg.mozilla.org/integration/autoland/rev/9485ece3ea15
Change nsIDOMHTMLMediaElement QIs to Media/Video type checks; r=bz
https://hg.mozilla.org/integration/autoland/rev/a3587827db59
Change HTMLMediaElement QIs to FromContent calls; r=bz
https://hg.mozilla.org/integration/autoland/rev/ce66cbae301e
Remove C++ usage of nsIDOMHTMLMediaElement; r=bz
https://hg.mozilla.org/integration/autoland/rev/20f5d2bcf89b
Remove nsIDOMHTMLMediaElement; r=bz
You need to log in before you can comment on or make changes to this bug.