Use node ref for HTMLMediaElement::mSourcePointer

RESOLVED FIXED in Firefox 57

Status

()

defect
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: catalinb, Assigned: catalinb)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(4 attachments, 5 obsolete attachments)

This helps with the effort of dropping nsAttrAndChildArray.
Comment on attachment 8886136 [details] [diff] [review]
Use node pointer for HTMLMediaElement::mSourcePointer.

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

Thanks for the patch! This only needs a simple fix to work I think.

::: dom/html/HTMLMediaElement.cpp
@@ +1480,5 @@
>  HTMLMediaElement::ContentInserted(nsIDocument* aDocument,
>                                    nsIContent*  aContainer,
>                                    nsIContent*  aChild,
>                                    int32_t      aIndexInContainer)
> +{ }

Mozilla style guide requires this to be:

{
}

If you run `./mach clang-format`, your patch will automatically be formatted to conform to the Mozilla coding standard. Then your reviewers won't ping you for style violations.  :)

@@ +6448,5 @@
>  {
>    mSourceLoadCandidate = nullptr;
>  
>    while (true) {
> +    if (!mSourcePointer)

Nothing is initializing mSourcePointer to be the first child of the HTMLMediaElement. That means it will be nullptr the first time we enter this loop, and we won't iterate over the child <source> elements of the HTMLMediaElement.

Indeed, if you run `./mach mochitest dom/media/test/test_media_selection.html` you'll see the test times out with this patch.

You'll need to initialize mSourcePointer to nsIContent::GetFirstChild() somewhere, and run the loop in here until mSourcePointer has past nsIContent::GetLastChild().
Attachment #8886136 - Flags: review?(cpearce) → review-
>  HTMLMediaElement::ContentInserted(nsIDocument* aDocument,

I think you can remove this method if you remove this line in the header:
  NS_DECL_NSIMUTATIONOBSERVER_CONTENTINSERTED
Assignee: nobody → catalin.badea392
Attachment #8886136 - Attachment is obsolete: true
I made this into a separate patch because ./mach clang-format rearranged the
entire constructor definition.
Attachment #8890809 - Flags: review?(cpearce)
Attachment #8890809 - Flags: review?(cpearce) → review+
Comment on attachment 8890808 [details] [diff] [review]
Use node pointer for HTMLMediaElement::mSourcePointer.

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

::: dom/html/HTMLMediaElement.cpp
@@ +1488,3 @@
>                                   nsIContent*  aPreviousSibling)
>  {
> +  if (aChild == mSourcePointer) {

I am not familiar with the behaviour of ContentRemoved(), but what happens if aChild is the first child of the media element? It looks like aPreviousSibling would be null [1]. So we'd then not set mSourcePointer to the next candidate, but nullptr, and so not traverse the other candidates?

So if mSourcePointerInitialized==true and aPreviousSibling==nullptr, you should set mSourcePointer to nsINode::GetFirstChild(), right?

You should be able to repro this case by loading a media element with two source children, the first of which is a supported type, but which 404s, and in an error event handler on the first source element remove the source element from the media element. It would good if you could test this case, and even better if you can add a test case for it to test_media_selection.html.

[1] https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/dom/base/nsDocument.cpp#2162

@@ +1775,1 @@
>  

You need to reset mSourcePointerInitialized here too, otherwise loads other than the first on the same element won't work correctly.
Attachment #8890808 - Flags: review?(cpearce) → review-
I tried testing this, but couldn't quite reproduce what you described. The problem I had was
that by the time I'm removing children in the error handler, mSourcePointer would
already point to a latter child. Also, in the debugger I noticed I'm getting some
weird text node removals which I can't explain. Attaching test case.
Attachment #8898585 - Flags: review?(cpearce)
Attachment #8890808 - Attachment is obsolete: true
Posted file test.html
Comment on attachment 8898585 [details] [diff] [review]
Use node pointer for HTMLMediaElement::mSourcePointer.

I have a timeout failure in media/test_load.html with this patch.
Attachment #8898585 - Flags: review?(cpearce)
Slightly different. mSourcePointer will be positioned before the next element
considered for loading and will stop on the last child. This is needed to correctly
load sources that are appended at a later time. Try run looks good.
Attachment #8899480 - Flags: review?(cpearce)
Attachment #8898585 - Attachment is obsolete: true
Comment on attachment 8899480 [details] [diff] [review]
Use node pointer for HTMLMediaElement::mSourcePointer.

> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSourcePointer)

Shouldn't we also add a NS_IMPL_CYCLE_COLLECTION_UNLINK(mSourcePointer)
in the NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED section?

> -  uint32_t mSourcePointer;
> +  nsCOMPtr<nsIContent> mSourcePointer;

The member before this is "uint32_t mCurrentLoadID", so we now
get a 32-bit alignment padding here.  We should probably move
that member down a bit to avoid that.

Other than that, this looks good to me.
Attachment #8899480 - Flags: feedback+
Putting mCurrentLoadID before LoadAlgorithmState reduces the object's size by 8
on my local debug build.
Attachment #8899586 - Flags: review?(mats)
Attachment #8899480 - Attachment is obsolete: true
Attachment #8899480 - Flags: review?(cpearce)
Comment on attachment 8899586 [details] [diff] [review]
Use node pointer for HTMLMediaElement::mSourcePointer.

Thanks, this looks good to me.  I'll leave the formal review to Chris.
Attachment #8899586 - Flags: review?(mats)
Attachment #8899586 - Flags: review?(cpearce)
Attachment #8899586 - Flags: feedback+
Comment on attachment 8899586 [details] [diff] [review]
Use node pointer for HTMLMediaElement::mSourcePointer.

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

Almost made it to r+, just code hygiene issues to resolve now...

Your commit message should succinctly describe what you're doing and why. First line is a summary, then a blank line, then a description of your motivations.

Then it's clear to people looking back over the commit logs in years to come what you did and more critically why.

::: dom/html/HTMLMediaElement.h
@@ +1444,5 @@
>  
> +  // The current media load ID. This is incremented every time we start a
> +  // new load. Async events note the ID when they're first sent, and only fire
> +  // if the ID is unchanged when they come to fire.
> +  uint32_t mCurrentLoadID;

Please split this change out into a separate commit, as it's logically a different change than the rest of your patch.

One commit per idea makes it easier to read and review the code, as it isolates changes into atomic units that do a single thing only. It also makes it easier to back out/uplift only the changes that need to be backed-out/uplift, rather than take everything else as collateral.
Attachment #8899586 - Flags: review?(cpearce) → review-
(In reply to Chris Pearce (:cpearce) from comment #14)
> Comment on attachment 8899586 [details] [diff] [review]
> Use node pointer for HTMLMediaElement::mSourcePointer.
> 
> Review of attachment 8899586 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Almost made it to r+, just code hygiene issues to resolve now...
> 
> Your commit message should succinctly describe what you're doing and why.
> First line is a summary, then a blank line, then a description of your
> motivations.
> 
> Then it's clear to people looking back over the commit logs in years to come
> what you did and more critically why.
Reading the bug comments would provide a better explanation than the commit
message anyway. I personally don't see the point of long commit messages.
(In reply to Cătălin Badea (:catalinb) from comment #15)
> Reading the bug comments would provide a better explanation than the commit
> message anyway. I personally don't see the point of long commit messages.

The reason we require this for patches to the media code is that often the bug is long and confusing and it can take quite a while to read through and understand all the comments to figure out why in the end the author decided to do a thing a certain way. If the commit message summarizes that, it save a lot of time for people looking back over the code in future.

Don't just take my word for it:
https://groups.google.com/d/msg/mozilla.dev.platform/qERnoJniCds/YWmmKqe4EAAJ
Using a pointer instead of an index helps us avoid some costly
operations such as IndexOf and GetChildAt with the upcoming changes from
bug 651120.
Attachment #8903547 - Flags: review?(cpearce)
Attachment #8899586 - Attachment is obsolete: true
This reduces the size of the object by avoiding memory alignment padding.
Attachment #8903548 - Flags: review?(cpearce)
Comment on attachment 8903547 [details] [diff] [review]
Use node pointer for HTMLMediaElement::mSourcePointer.

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

Thanks!
Attachment #8903547 - Flags: review?(cpearce) → review+
Attachment #8903548 - Flags: review?(cpearce) → review+
Pushed by catalin.badea392@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c41ce4850bd
Use node pointer for HTMLMediaElement::mSourcePointer.  r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/6375aa5b3aab
Change the order of member variables in HTMLMediaElement. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/2c41ce4850bd
https://hg.mozilla.org/mozilla-central/rev/6375aa5b3aab
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.