1473771 - make LinkedList work better when its element type inherits from multiple LinkedListElement<T>s

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Cameron McCormack (:heycam)]

Currently mozilla::dom::MediaQueryList inherits from both LinkedListElement<DOMEventTargetHelper> and LinkedListElement<MediaQueryList>.  This causes some ambiguous name lookups from some LinkedList member functions.  We can make them work with some static_cast<>s.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1473771 - Part 1: Make LinkedList<T>::sizeOfExcludingThis work when element type inherits from multiple LinkedListElement<T>s.

Review commit: https://reviewboard.mozilla.org/r/255182/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255182/

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Bug 1473771 - Part 2: Make LinkedList::Iterator work when element type inherits from multiple LinkedListElement<T>s.

Review commit: https://reviewboard.mozilla.org/r/255184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255184/

Comment 3

[Cameron McCormack (:heycam)]

For the Iterator patch, I was considering whether to leave just a single type parameter on Iterator, and to derive whether we use a const or non-const LinkedListElement<> pointer when static_cast<>ing.  The long std::conditional<> stuff looked a bit ugly though so it was simpler just to pass in the right type to use.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8990166 [details]
Bug 1473771 - Part 2: Make LinkedList::Iterator work when element type inherits from multiple LinkedListElement<T>s.

https://reviewboard.mozilla.org/r/255184/#review262346

::: mfbt/LinkedList.h:436
(Diff revision 1)
>      const Iterator& operator++() {
> -      mCurrent = mCurrent->getNext();
> +      mCurrent = static_cast<Element>(mCurrent)->getNext();
>        return *this;
>      }
>  
> -    bool operator!=(const Iterator<Type>& aOther) const {
> +    bool operator!=(const Iterator<Type, Element>& aOther) const {

Just use `const Iterator& aOther` -- the type parameters are assumed if you don't provide them, here.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8990165 [details]
Bug 1473771 - Part 1: Make LinkedList<T>::sizeOfExcludingThis work when element type inherits from multiple LinkedListElement<T>s.

https://reviewboard.mozilla.org/r/255182/#review262340

I'm super-dubious about the desirability of multiply inheriting LinkedListElement, but this isn't really precisely *extra* code to deal with that, so.  Bleh.

Comment 6

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13f8a986895792b4489803c85c20474ae88a6a09

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8990165 [details]
Bug 1473771 - Part 1: Make LinkedList<T>::sizeOfExcludingThis work when element type inherits from multiple LinkedListElement<T>s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255182/diff/1-2/

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8990166 [details]
Bug 1473771 - Part 2: Make LinkedList::Iterator work when element type inherits from multiple LinkedListElement<T>s.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255184/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56cf99e809bd
Part 1: Make LinkedList<T>::sizeOfExcludingThis work when element type inherits from multiple LinkedListElement<T>s. r=Waldo
https://hg.mozilla.org/integration/autoland/rev/f05677e4a0f7
Part 2: Make LinkedList::Iterator work when element type inherits from multiple LinkedListElement<T>s. r=Waldo

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/56cf99e809bd
https://hg.mozilla.org/mozilla-central/rev/f05677e4a0f7