:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Description • 11 years ago

      No description provided.

	(no longer active)
	Comment 1 • 11 years ago

Note https://www.w3.org/Bugs/Public/show_bug.cgi?id=21043.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 8 • 11 years ago

Comment on attachment 720623 [details] [diff] [review]
part 1 - renaming HTMLMediaElement

Review of attachment 720623 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/public/nsHTMLMediaElement.h
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifndef mozilla_dom_HTMLMediaElement_h_
> +#define mozilla_dom_HTMLMediaElement_h_

No trailing _

@@ +43,5 @@
> +namespace dom {
> +
> +class HTMLMediaElement : public nsGenericHTMLElement,
> +                         public nsIObserver,
> +                         public mozilla::MediaDecoderOwner,

No mozilla::

@@ +58,3 @@
>    typedef mozilla::AudioStream AudioStream;
>    typedef mozilla::MediaDecoder MediaDecoder;
>    typedef mozilla::DOMMediaStream DOMMediaStream;

Drop all these typedefs (use layers:: below if necessary)

@@ +58,5 @@
>    typedef mozilla::AudioStream AudioStream;
>    typedef mozilla::MediaDecoder MediaDecoder;
>    typedef mozilla::DOMMediaStream DOMMediaStream;
>  
>    mozilla::CORSMode GetCORSMode() {

Drop mozilla::. Please check below too.

::: content/html/content/public/Makefile.in
@@ +38,5 @@
>  EXPORTS_NAMESPACES = mozilla/dom
>  
>  EXPORTS_mozilla/dom = \
>  		HTMLCanvasElement.h \
> +		HTMLMediaElement.h \

(Hmm, I think this should only be used if MOZ_MEDIA is defined, but that probably doesn't work anyway... Just keep what you have.)

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +122,5 @@
>  #include "mozilla/Preferences.h"
>  
>  #include "nsIPermissionManager.h"
>  
>  using namespace mozilla;

Drop this

::: content/media/MediaDecoderOwner.h
@@ +40,2 @@
>    // HTML media element, and null otherwise.
> +  virtual mozilla::dom::HTMLMediaElement* GetMediaElement()

No need for the mozilla:: here

::: content/media/VideoFrameContainer.h
@@ +40,5 @@
>    typedef mozilla::layers::Image Image;
>  
>    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(VideoFrameContainer)
>  
> +  VideoFrameContainer(mozilla::dom::HTMLMediaElement* aElement,

Just dom:: here and below

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 15 • 11 years ago

Comment on attachment 720624 [details] [diff] [review]
part 2 - HTMLMediaElement to WebIDL

Review of attachment 720624 [details] [diff] [review]:
-----------------------------------------------------------------

First batch of comments:

Add 'concrete': False to Bindings.conf.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +537,5 @@
> +already_AddRefed<MediaError>
> +HTMLMediaElement::GetError() const
> +{
> +  nsRefPtr<MediaError> error = static_cast<MediaError*>(mError.get());
> +  return error.forget();

Please fix mError to be an nsRefPtr<MediaError> to avoid this cast. Also return a raw pointer.

@@ +544,3 @@
>  NS_IMETHODIMP HTMLMediaElement::GetError(nsIDOMMediaError * *aError)
>  {
>    NS_IF_ADDREF(*aError = mError);

And then make this NS_IF_ADDREF(*aError = GetError());

@@ +553,5 @@
> +HTMLMediaElement::Ended()
> +{
> +  if (mSrcStream) {
> +    return GetSrcMediaStream()->IsFinished();
> +  } else if (mDecoder) {

Drop the else after a return

@@ +758,5 @@
>  }
>  
>  /* void load (); */
> +void
> +HTMLMediaElement::Load(ErrorResult& aRv)

This doesn't throw, does it? You probably can keep using the XPCOM version

@@ +1261,5 @@
>    return NS_OK;
>  }
>  
> +void
> +HTMLMediaElement::MozLoadFrom(mozilla::dom::HTMLMediaElement& aOther, ErrorResult& aRv)

No mozilla::dom::

@@ +1265,5 @@
> +HTMLMediaElement::MozLoadFrom(mozilla::dom::HTMLMediaElement& aOther, ErrorResult& aRv)
> +{
> +  // Make sure we don't reenter during synchronous abort events.
> +  if (mIsRunningLoadMethod)
> +    return;

{}

@@ +1271,5 @@
> +  AbortExistingLoads();
> +  mIsRunningLoadMethod = false;
> +
> +  if (!aOther.mDecoder)
> +    return;

{}

@@ +1294,5 @@
>    nsCOMPtr<nsIContent> content = do_QueryInterface(aOther);
>    HTMLMediaElement* other = static_cast<HTMLMediaElement*>(content.get());
> +
> +  ErrorResult rv;
> +  MozLoadFrom(*other, rv);

other can be null here. Check that and return some error code. And add a test; I suspect the following would fail with just patches 1-2 applied:

<script>
document.createElement("video").mozLoadFrom({});
</script>

@@ +1326,5 @@
> +HTMLMediaElement::CurrentTime()
> +{
> +  if (mSrcStream) {
> +    return MediaTimeToSeconds(GetSrcMediaStream()->GetCurrentTime());
> +  } else if (mDecoder) {

No else

@@ +1375,4 @@
>    }
>  
>    // Detect for a NaN and invalid values.
>    if (aCurrentTime != aCurrentTime) {

This can't happen with the new bindings; move the check to the XPCOM implementation and add a MOZ_ASSERT to the top of this method.

@@ +1412,5 @@
> +HTMLMediaElement::Duration()
> +{
> +  if (mSrcStream) {
> +    return std::numeric_limits<double>::infinity();
> +  } else if (mDecoder) {

As before

@@ +1446,5 @@
>  
>  /* readonly attribute boolean paused; */
>  NS_IMETHODIMP HTMLMediaElement::GetPaused(bool *aPaused)
>  {
>    *aPaused = mPaused;

Paused()

::: dom/webidl/HTMLMediaElement.webidl
@@ +21,5 @@
> +  [SetterThrows]
> +           attribute DOMString src;
> +  readonly attribute DOMString currentSrc;
> +  [SetterThrows]
> +           attribute DOMString crossOrigin;

So I guess we keep the lower case version for now.

@@ +126,5 @@
> +  // Mozilla extension: return embedded metadata from the stream as a
> +  // JSObject with key:value pairs for each tag. This can be used by
> +  // player interfaces to display the song title, artist, etc.
> +  [Throws]
> +  any mozGetMetadata();

Return object. (Or object? if that doesn't work for the throwing case.)

	Andrea Marchesini [:baku] Assignee
	Comment 16 • 11 years ago

there was a memory leak.

	Andrea Marchesini [:baku] Assignee
	Comment 17 • 11 years ago

Comment on attachment 721138 [details] [diff] [review]
part 2 - HTMLMediaElement to WebIDL

I did'nt apply your comments yet.

	Andrea Marchesini [:baku] Assignee
	Comment 18 • 11 years ago

> Add 'concrete': False to Bindings.conf.

If I do this, HTMLMediaElement must be marked as chromeOnly.

> @@ +1294,5 @@
> >    nsCOMPtr<nsIContent> content = do_QueryInterface(aOther);
> >    HTMLMediaElement* other = static_cast<HTMLMediaElement*>(content.get());
> > +
> > +  ErrorResult rv;
> > +  MozLoadFrom(*other, rv);
> 
> other can be null here. Check that and return some error code. And add a
> test; I suspect the following would fail with just patches 1-2 applied:

We have |NS_ENSURE_ARG_POINTER(aOther);|... this should be enough. is it?
Then aOther cannot be null in the webIDL method.

	Andrea Marchesini [:baku] Assignee
	Comment 19 • 11 years ago

> We have |NS_ENSURE_ARG_POINTER(aOther);|... this should be enough. is it?
> Then aOther cannot be null in the webIDL method.

No... it's not enough :)

	Andrea Marchesini [:baku] Assignee
	Comment 20 • 11 years ago

The only comment that I didn't apply was the concrete: False.
Doing that, it complains about ChromeOnly attribute missing .. and I don't think we want to have MediaElement chrome only.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 11 years ago

> Doing that, it complains about ChromeOnly attribute missing 

You can't mark leaf interfaces as "concrete: False", because that's nonsensical.

But once you have an interface inheriting from HTMLMediaElement, you should mark it concrete:False.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 22 • 11 years ago

Comment on attachment 720639 [details] [diff] [review]
part 1 - renaming HTMLMediaElement

Review of attachment 720639 [details] [diff] [review]:
-----------------------------------------------------------------

Just some drive-by comments. Thanks for taking this on!

::: content/html/content/public/nsHTMLMediaElement.h
@@ +49,3 @@
>  {
>  public:
> +  typedef TimeStamp TimeStamp;

Most of these typedefs can be removed right?

::: content/media/webaudio/MediaBufferDecoder.cpp
@@ +424,5 @@
>    MOZ_ASSERT(!mBufferDecoder);
>    mBufferDecoder = new BufferDecoder(resource);
>  
>    // If you change this list to add support for new decoders, please consider
> +  // updating HTMLMediaElement::CreateDecoder as well.

Note: this bitrotten on m-c due to landing of bug 834172.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 23 • 11 years ago

Comment on attachment 721178 [details] [diff] [review]
part 2 - HTMLMediaElement to WebIDL

Review of attachment 721178 [details] [diff] [review]:
-----------------------------------------------------------------

Generally looks good, but I've got enough comments I'd like to see the patch one more time. An interdiff would be nice ;)

::: content/html/content/public/HTMLMediaElement.h
@@ +323,3 @@
>    virtual void FireTimeUpdate(bool aPeriodic) MOZ_FINAL MOZ_OVERRIDE;
>  
>    MediaStream* GetSrcMediaStream()

If you make this const, CurrentTime() and Duration() can be const too.

@@ +330,5 @@
>  
> +  // WebIDL
> +
> +  // Mark this as resultNotAddRefed to return raw pointers
> +  already_AddRefed<MediaError> GetError() const;

MediaError* GetError() const
{
  return mError;
}

@@ +335,5 @@
> +
> +  // XPCOM GetSrc() is OK
> +  void SetSrc(const nsAString& aSrc, ErrorResult& aRv)
> +  {
> +    aRv = SetAttrHelper(nsGkAtoms::src, aSrc);

SetHTMLAttr(nsGkAtoms::src, aSrc, aRv);

@@ +343,5 @@
> +
> +  // XPCOM GetCrossorigin() is OK
> +  void SetCrossorigin(const nsAString& aValue, ErrorResult& aRv)
> +  {
> +    aRv = SetAttr(kNameSpaceID_None, nsGkAtoms::crossorigin, nullptr, aValue, true);

SetHTMLAttr(nsGkAtoms::crossorigin, aValue, aRv);

@@ +354,5 @@
> +
> +  // XPCOM GetPreload() is OK
> +  void SetPreload(const nsAString& aValue, ErrorResult& aRv)
> +  {
> +    aRv = SetAttrHelper(nsGkAtoms::preload, aValue);

SetHTMLAttr(nsGkAtoms::preload, aValue, aRv);

@@ +361,5 @@
> +  already_AddRefed<TimeRanges> Buffered() const;
> +
> +  // XPCOM Load() is OK
> +
> +  void CanPlayType(const nsAString& aType, nsAString& aResult, ErrorResult& aRv);

Can't throw; use the XPCOM version.

@@ +376,5 @@
> +  void SetCurrentTime(double aCurrentTime, ErrorResult& aRv);
> +
> +  double Duration();
> +
> +  bool Paused()

const

@@ +408,5 @@
> +  }
> +
> +  void SetAutoplay(bool aValue, ErrorResult& aRv)
> +  {
> +    aRv = SetBoolAttr(nsGkAtoms::autoplay, aValue);

SetHTMLBoolAttr(nsGkAtoms::autoplay, aValue, aRv);

@@ +418,5 @@
> +  }
> +
> +  void SetLoop(bool aValue, ErrorResult& aRv)
> +  {
> +    aRv = SetBoolAttr(nsGkAtoms::loop, aValue);

Same

@@ +432,5 @@
> +  }
> +
> +  void SetControls(bool aValue, ErrorResult& aRv)
> +  {
> +    aRv = SetBoolAttr(nsGkAtoms::controls, aValue);

Same

@@ +447,5 @@
> +  {
> +    return mMuted;
> +  }
> +
> +  void SetMuted(bool aMuted, ErrorResult& aRv);

This can't throw. Use the XPCOM version instead.

@@ +456,5 @@
> +  }
> +
> +  void SetDefaultMuted(bool aMuted, ErrorResult& aRv)
> +  {
> +    aRv = SetBoolAttr(nsGkAtoms::muted, aMuted);

Same as before

@@ +470,5 @@
> +  {
> +    return mPreservesPitch;
> +  }
> +
> +  void SetMozPreservesPitch(bool aPreservesPitch, ErrorResult& aRv);

This can't throw; use the XPCOM version.

@@ +477,5 @@
> +  {
> +    return mAutoplayEnabled;
> +  }
> +
> +  // Mark this as resultNotAddRefed to return raw pointers

Probably better this way. Please remove the comment.

@@ +480,5 @@
> +
> +  // Mark this as resultNotAddRefed to return raw pointers
> +  already_AddRefed<DOMMediaStream> MozCaptureStream(ErrorResult& aRv);
> +
> +  // Mark this as resultNotAddRefed to return raw pointers

Same

@@ +494,5 @@
> +  uint32_t GetMozSampleRate(ErrorResult& aRv) const;
> +
> +  uint32_t GetMozFrameBufferLength(ErrorResult& aRv) const;
> +
> +  void SetMozFrameBufferLength(uint32_t arg, ErrorResult& aRv);

aFoo

@@ +496,5 @@
> +  uint32_t GetMozFrameBufferLength(ErrorResult& aRv) const;
> +
> +  void SetMozFrameBufferLength(uint32_t arg, ErrorResult& aRv);
> +
> +  JSObject* MozGetMetadata(JSContext* cx, ErrorResult& aRv);

aCx

@@ +498,5 @@
> +  void SetMozFrameBufferLength(uint32_t arg, ErrorResult& aRv);
> +
> +  JSObject* MozGetMetadata(JSContext* cx, ErrorResult& aRv);
> +
> +  void MozLoadFrom(HTMLMediaElement& other, ErrorResult& aRv);

aOther

@@ +506,5 @@
> +  // XPCOM GetMozAudioChannelType() is OK
> +
> +  void SetMozAudioChannelType(const nsAString& aValue, ErrorResult& aRv)
> +  {
> +    SetAttrHelper(nsGkAtoms::mozaudiochannel, aValue);

SetHTMLAttr(nsGkAtoms::mozaudiochannel, aValue, aRv);

::: content/html/content/src/HTMLMediaElement.cpp
@@ +540,5 @@
> +}
> +
> +NS_IMETHODIMP HTMLMediaElement::GetError(nsIDOMMediaError** aError)
> +{
> +  NS_IF_ADDREF(*aError = GetError().get());

The only reason this doesn't leak is because you're compensating for a use-after-free from GetError().

@@ +557,5 @@
> +  if (mDecoder) {
> +    return mDecoder->IsEnded();
> +  }
> +
> +  return true;

Make this false; that's what we returned for a null mDecoder before roc broke this case in bug 664918.

@@ +581,2 @@
>  {
>    *aNetworkState = mNetworkState;

NetworkState()

@@ +1274,5 @@
> +  ChangeDelayLoadStatus(true);
> +
> +  mLoadingSrc = aOther.mLoadingSrc;
> +  aRv = InitializeDecoderAsClone(aOther.mDecoder);
> +  if (NS_FAILED(aRv.ErrorCode())) {

aRv.Failed()

@@ +1303,5 @@
> +
> +/* readonly attribute unsigned short readyState; */
> +NS_IMETHODIMP HTMLMediaElement::GetReadyState(uint16_t* aReadyState)
> +{
> +  *aReadyState = mReadyState;

ReadyState()

@@ +1328,5 @@
>  {
> +  if (mSrcStream) {
> +    return MediaTimeToSeconds(GetSrcMediaStream()->GetCurrentTime());
> +  }
> + 

Trailing whitespace

@@ +1358,5 @@
>    }
>  
>    if (mCurrentPlayRangeStart != -1.0) {
>      double rangeEndTime = 0;
>      GetCurrentTime(&rangeEndTime);

CurrentTime()

@@ +1420,2 @@
>    }
> + 

Trailing whitespace

@@ +1478,2 @@
>      double now = 0.0;
>      GetCurrentTime(&now);

CurrentTime()

@@ +1500,5 @@
>  {
>    if (mNetworkState == nsIDOMHTMLMediaElement::NETWORK_EMPTY) {
>      LOG(PR_LOG_DEBUG, ("Loading due to Pause()"));
> +    aRv = Load();
> +    if (NS_FAILED(aRv.ErrorCode())) {

aRv.Failed()

@@ +1532,5 @@
> +
> +/* attribute double volume; */
> +NS_IMETHODIMP HTMLMediaElement::GetVolume(double* aVolume)
> +{
> +  *aVolume = mVolume;

Volume()

@@ +1672,5 @@
>  {
> +  ErrorResult rv;
> +
> +  JSObject* obj = MozGetMetadata(cx, rv);
> +  if (NS_SUCCEEDED(rv.ErrorCode())) {

!rv.Failed()

@@ +1673,5 @@
> +  ErrorResult rv;
> +
> +  JSObject* obj = MozGetMetadata(cx, rv);
> +  if (NS_SUCCEEDED(rv.ErrorCode())) {
> +    *aValue = OBJECT_TO_JSVAL(obj);

MOZ_ASSERT(obj);
*aValue = JS::ObjectValue(*obj);

should work, I think

@@ +1702,3 @@
>    // The framebuffer (via MozAudioAvailable events) is only available
>    // when reading vs. writing audio directly.
>    if (!mDecoder) {

What happened here?

@@ +1733,2 @@
>  {
>    *aMuted = mMuted;

Muted()

@@ +1838,2 @@
>  {
>    *aCaptured = mAudioCaptured;

MozAudioCaptured()

@@ +2086,5 @@
>    SetPlayedOrSeeked(true);
>  
>    if (mNetworkState == nsIDOMHTMLMediaElement::NETWORK_EMPTY) {
> +    aRv = Load();
> +    if (NS_FAILED(aRv.ErrorCode())) {

aRv.Failed()

@@ +2101,5 @@
>        SetCurrentTime(0);
>      }
>      if (!mPausedForInactiveDocumentOrChannel) {
> +      aRv = mDecoder->Play();
> +      if (NS_FAILED(aRv.ErrorCode())) {

Same

@@ +3759,5 @@
>  
>  /* attribute double defaultPlaybackRate; */
>  NS_IMETHODIMP HTMLMediaElement::GetDefaultPlaybackRate(double* aDefaultPlaybackRate)
>  {
>    *aDefaultPlaybackRate = mDefaultPlaybackRate;

DefaultPlaybackRate()

@@ +3785,5 @@
>  
>  /* attribute double playbackRate; */
>  NS_IMETHODIMP HTMLMediaElement::GetPlaybackRate(double* aPlaybackRate)
>  {
>    *aPlaybackRate = mPlaybackRate;

PlaybackRate()

@@ +3825,5 @@
>  
>  /* attribute bool mozPreservesPitch; */
>  NS_IMETHODIMP HTMLMediaElement::GetMozPreservesPitch(bool* aPreservesPitch)
>  {
>    *aPreservesPitch = mPreservesPitch;

MozPreservesPitch()

@@ +3923,5 @@
> +JSObject*
> +HTMLMediaElement::WrapNode(JSContext* aCx, JSObject* aScope,
> +                           bool* aTriedToWrap)
> +{
> +    return HTMLMediaElementBinding::Wrap(aCx, aScope, this, aTriedToWrap);

2 spaces

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 24 • 11 years ago

(In reply to :Ms2ger from comment #23)
> @@ +3923,5 @@
> > +JSObject*
> > +HTMLMediaElement::WrapNode(JSContext* aCx, JSObject* aScope,
> > +                           bool* aTriedToWrap)
> > +{
> > +    return HTMLMediaElementBinding::Wrap(aCx, aScope, this, aTriedToWrap);
> 
> 2 spaces

Actually, you don't want to override WrapNode on MediaElement; just remove this.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 25 • 11 years ago

Comment on attachment 720659 [details] [diff] [review]
part 3 - HTMLAudioElement renamed

Review of attachment 720659 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/public/nsHTMLAudioElement.h
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifndef mozilla_dom_HTMLAudioElement_h_
> +#define mozilla_dom_HTMLAudioElement_h_

Nit: drop the remaining trailing underscore.

@@ +15,5 @@
>  
> +namespace mozilla {
> +namespace dom {
> +
> +class HTMLAudioElement : public mozilla::dom::HTMLMediaElement,

Drop the mozilla::dom

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 26 • 11 years ago

Comment on attachment 720660 [details] [diff] [review]
part 4 - HTMLAudioElement to WebIDL

Review of attachment 720660 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/public/HTMLAudioElement.h
@@ +56,5 @@
> +
> +  // WebIDL
> +
> +  static already_AddRefed<HTMLAudioElement> Audio(const GlobalObject& global, ErrorResult& aRv);
> +  static already_AddRefed<HTMLAudioElement> Audio(const GlobalObject& global, const nsAString& src, ErrorResult& aRv);

Wrap at 80 columns

::: content/html/content/src/HTMLAudioElement.cpp
@@ +128,5 @@
> +    return nullptr;
> +  }
> +
> +  nsRefPtr<HTMLAudioElement> audio = new HTMLAudioElement(nodeInfo.forget());
> +  return audio.forget();

// Audio elements created using "new Audio(...)" should have
// 'preload' set to 'auto' (since the script must intend to
// play the audio)

Don't we have tests for that? :)

@@ +168,5 @@
>    }
>  
>    mAudioStream = AudioStream::AllocateStream();
> +  aRv = mAudioStream->Init(aChannels, aRate, mAudioChannelType);
> +  if (NS_FAILED(aRv.ErrorCode())) {

aRv.Failed()

@@ +213,5 @@
>    if (JS_IsFloat32Array(darray)) {
>      tsrc = darray;
>    } else if (JS_IsArrayObject(aCx, darray)) {
>      JSObject* nobj = JS_NewFloat32ArrayFromArray(aCx, darray);
>      if (!nobj) {

Ugh. Definitely want a followup for this crap, if you don't want to do it in this bug.

@@ +244,5 @@
>    // AudioDataValue is 'float', but it's not worth it for this deprecated API.
>    nsAutoArrayPtr<AudioDataValue> audioData(new AudioDataValue[writeLen * mChannels]);
>    ConvertAudioSamples(frames, audioData.get(), writeLen * mChannels);
> +  aRv = mAudioStream->Write(audioData.get(), writeLen);
> +  if (NS_FAILED(aRv.ErrorCode())) {

You know by now :)

@@ +317,5 @@
> +JSObject*
> +HTMLAudioElement::WrapNode(JSContext* aCx, JSObject* aScope,
> +                           bool* aTriedToWrap)
> +{
> +    return HTMLAudioElementBinding::Wrap(aCx, aScope, this, aTriedToWrap);

Two spaces

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 27 • 11 years ago

Comment on attachment 720662 [details] [diff] [review]
part 5 - renaming HTMLVideoElement

Review of attachment 720662 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/public/nsHTMLVideoElement.h
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_HTMLVideoElement_h_
> +#define mozilla_dom_HTMLVideoElement_h_

Underscore

@@ +12,5 @@
>  
> +namespace mozilla {
> +namespace dom {
> +
> +class HTMLVideoElement : public mozilla::dom::HTMLMediaElement,

No mozilla::dom::

	Andrea Marchesini [:baku] Assignee
	Comment 31 • 11 years ago

> // Audio elements created using "new Audio(...)" should have
> // 'preload' set to 'auto' (since the script must intend to
> // play the audio)
> 
> Don't we have tests for that? :)

Can you file a bug for this?


> >    if (JS_IsFloat32Array(darray)) {
> >      tsrc = darray;
> >    } else if (JS_IsArrayObject(aCx, darray)) {
> >      JSObject* nobj = JS_NewFloat32ArrayFromArray(aCx, darray);
> >      if (!nobj) {


Bug 849637

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 35 • 11 years ago

Comment on attachment 723214 [details] [diff] [review]
interdiff for HTMLMediaElement to WebIDL

Review of attachment 723214 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

::: content/html/content/public/HTMLMediaElement.h
@@ +381,2 @@
>  
>    bool Paused()

const

@@ +475,2 @@
>  
>    void SetMozPreservesPitch(bool aPreservesPitch, ErrorResult& aRv);

This can't throw; use the XPCOM version.

	Andrea Marchesini [:baku] Assignee
	Comment 36 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=cb33d998ed61

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 38 • 11 years ago

Comment on attachment 723218 [details] [diff] [review]
part 4 - HTMLAudioElement to WebIDL

Review of attachment 723218 [details] [diff] [review]:
-----------------------------------------------------------------

You're still regressing the preload thing.

::: content/html/content/src/HTMLAudioElement.cpp
@@ +119,5 @@
> +    return nullptr;
> +  }
> +
> +  nsCOMPtr<nsINodeInfo> nodeInfo =
> +    doc->NodeInfoManager()->GetNodeInfo(nsGkAtoms::option, nullptr,

audio, not option.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 40 • 11 years ago

Comment on attachment 723352 [details] [diff] [review]
part 4 - HTMLAudioElement to WebIDL

Review of attachment 723352 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/public/HTMLAudioElement.h
@@ +69,5 @@
> +  uint64_t MozCurrentSampleOffset(ErrorResult& aRv);
> +
> +protected:
> +  virtual JSObject* WrapNode(JSContext* aCx, JSObject* aScope,
> +                             bool* aTriedToWrap) MOZ_OVERRIDE;

(This will be broken)

::: content/html/content/src/HTMLAudioElement.cpp
@@ +137,5 @@
> +  return audio.forget();
> +}
> +
> +already_AddRefed<HTMLAudioElement>
> +HTMLAudioElement::Audio(const GlobalObject& aGlobal, const nsAString& src, ErrorResult& aRv)

aSrc

::: dom/webidl/HTMLAudioElement.webidl
@@ +11,5 @@
> + * and create derivative works of this document.
> + */
> +
> +[NamedConstructor=Audio(),
> + NamedConstructor=Audio(DOMString src)]

You can use an optional DOMString if you want; your call.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 41 • 11 years ago

Comment on attachment 723221 [details] [diff] [review]
part 6 - HTMLVideoElement to WebIDL

Review of attachment 723221 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/public/HTMLVideoElement.h
@@ +94,5 @@
> +
> +  // XPCOM GetPoster is OK
> +  void SetPoster(const nsAString& aValue, ErrorResult& aRv)
> +  {
> +    aRv = SetAttrHelper(nsGkAtoms::poster, aValue);

SetHTMLAttr

@@ +111,5 @@
> +  bool MozHasAudio() const;
> +
> +protected:
> +  virtual JSObject* WrapNode(JSContext* aCx, JSObject* aScope,
> +                             bool* aTriedToWrap) MOZ_OVERRIDE;

Again

::: content/html/content/src/HTMLVideoElement.cpp
@@ +158,5 @@
>  NS_IMPL_URI_ATTR(HTMLVideoElement, Poster, poster)
>  
> +uint32_t HTMLVideoElement::MozParsedFrames() const
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");

Make those MOZ_ASSERT, please

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 42 • 11 years ago

And please add a patch on top of this stack to mark HTMLMediaElement as concrete:False in Bindings.conf.

	Andrea Marchesini [:baku] Assignee
	Comment 45 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=6a6114bef82f

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 46 • 11 years ago

Comment on attachment 725315 [details] [diff] [review]
part 7 - concrete

Review of attachment 725315 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

	Andrea Marchesini [:baku] Assignee
	Comment 54 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=9482405b6db5

	Andrea Marchesini [:baku] Assignee
	Comment 57 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=0e5abcc1de87

	Andrea Marchesini [:baku] Assignee
	Comment 58 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/59f21770c4da
https://hg.mozilla.org/integration/mozilla-inbound/rev/333f330e2952
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5884ca6c0d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a12afcc09bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e90c1e76bd30
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbb0c6eb4bc7
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d464c665d4a

	Ed Morley [:emorley]
	Comment 59 • 11 years ago

This is causing windows-only m3 mochitest failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=20824198&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=20824213&tree=Mozilla-Inbound

{
07:06:31 INFO - 148 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/test_bug771202.html | uncaught exception - TypeError: plugin.checkObjectValue is not a function at http://mochi.test:8888/tests/dom/plugins/test/test_bug771202.html:28 
}

	Ed Morley [:emorley]
	Comment 60 • 11 years ago

khuey landed a build dependency fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/750ceb3d7faa

	Ryan VanderMeulen [:RyanVM]
	Comment 61 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/59f21770c4da
https://hg.mozilla.org/mozilla-central/rev/333f330e2952
https://hg.mozilla.org/mozilla-central/rev/d5884ca6c0d7
https://hg.mozilla.org/mozilla-central/rev/8a12afcc09bb
https://hg.mozilla.org/mozilla-central/rev/e90c1e76bd30
https://hg.mozilla.org/mozilla-central/rev/bbb0c6eb4bc7
https://hg.mozilla.org/mozilla-central/rev/6d464c665d4a