Closed Bug 1142497 Opened 9 years ago Closed 7 years ago

mDOMMediaQueryLists should use mozilla::LinkedList

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: Ms2ger, Unassigned, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=c++][good first bug])

Attachments

(1 file, 5 obsolete files)

Fix for bug 1142497 - change mDOMMediaQueryLists to use mozilla::LinkedList;
59 bytes, text/x-review-board-request
erahm
: review+
baku
: review+
Details 
See <http://mxr.mozilla.org/mozilla-central/ident?i=mDOMMediaQueryLists&filter=>. This should be a fairly simple replacement.
I'd like try for my first C++ contribution.

It's about replacing PRCList by mozilla::LinkedList that's it ?
From what I read, mozilla::LinkedList implementation is only interesting if we want to access next/previous/remove from the LinkedListItem, which doesn't seem to be the case here.

On the other hand, std::list would let us use for ranged loops and doesn't need to update MediaQueryList. Moreover, by using the standard library, the code will be less complex to read, as the exact behavior of std::list is well known.

Is there any guideline against the use of the standard library in favor of custom class/methods?
Yeah; we're not very comfortable using the STL in general.
patch 1/3 - update MediaQueryList class
Attachment #8577594 - Flags: review?(Ms2ger)
patch 2/3 update nsIDocument / nsDocument to use a LinkedList
Attachment #8577595 - Flags: review?(Ms2ger)
patch 3/3 fixed the use of nsDocument.MediaQueryLists in nsPresContext
Attachment #8577596 - Flags: review?(Ms2ger)
Could you squash these into a single patch?
Here you are
Attachment #8577594 - Attachment is obsolete: true
Attachment #8577595 - Attachment is obsolete: true
Attachment #8577596 - Attachment is obsolete: true
Attachment #8577594 - Flags: review?(Ms2ger)
Attachment #8577595 - Flags: review?(Ms2ger)
Attachment #8577596 - Flags: review?(Ms2ger)
Attachment #8577720 - Flags: review?(Ms2ger)
Flags: needinfo?(Ms2ger)
Comment on attachment 8577720 [details] [diff] [review]
Bug-1142497-using-LinkedList-for-mDOMMediaQueryLists.patch

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

I'm sorry, I lost track of this patch. Thanks for reminding me.

r=me with the comments below addressed.

::: dom/base/nsDocument.cpp
@@ +1626,4 @@
>  
>  nsIDocument::~nsIDocument()
>  {
> +  // assertion already done in ~LinkedList, but here we have a message

I think this comment doesn't need to be here.

@@ +2052,5 @@
>    // We own only the items in mDOMMediaQueryLists that have listeners;
>    // this reference is managed by their AddListener and RemoveListener
>    // methods.
> +  for (MediaQueryList* mql = tmp->mDOMMediaQueryLists.getFirst();
> +      mql != nullptr; mql = mql->getNext()) {

Nit: this should be indented one more space.

@@ +2165,5 @@
>    // We own only the items in mDOMMediaQueryLists that have listeners;
>    // this reference is managed by their AddListener and RemoveListener
>    // methods.
> +  for (MediaQueryList* mql = tmp->mDOMMediaQueryLists.getFirst();
> +      mql != nullptr; mql = mql->getNext()) {

Nit: this should be indented one more space.

@@ +7296,3 @@
>    nsRefPtr<MediaQueryList> result = new MediaQueryList(this, aMediaQueryList);
>  
>    // Insert the new item at the end of the linked list.

This is now readable enough that I don't think we need the comment.

::: layout/base/nsPresContext.cpp
@@ +1924,5 @@
>      // list in the order they were created and, for each list, to the
>      // listeners in the order added.
>      MediaQueryList::NotifyList notifyList;
> +    for (MediaQueryList* mql = mDocument->MediaQueryLists()->getFirst();
> +        mql != nullptr; mql = mql->getNext()) {

Nit: this should be indented one more space.

::: layout/style/MediaQueryList.h
@@ +28,4 @@
>  
>  class MediaQueryList MOZ_FINAL : public nsISupports,
>                                   public nsWrapperCache,
> +                                 public PRCList,

Does this still need to inherit from PRCList?
Attachment #8577720 - Flags: review?(Ms2ger) → review+
Flags: needinfo?(Ms2ger)
Flags: needinfo?(Ms2ger)
Hi, my name is Mukhtar Ali and I am currently studying Open-Source Development at Coventry University. For my first bug, I would like to be assigned to this task.
Is this bug still in existence?
Yes, it is. It would be great if you could finish this.
Flags: needinfo?(Ms2ger)
Attached patch v1.diff (obsolete) — — Splinter Review 
How about this one?
Assignee: nobody → jinank94
Attachment #8577720 - Attachment is obsolete: true
Attachment #8786891 - Flags: review?(Ms2ger)
Comment on attachment 8786891 [details] [diff] [review]
v1.diff

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

Looks good; just two comments:

::: dom/base/nsDocument.cpp
@@ +2023,5 @@
>    // We own only the items in mDOMMediaQueryLists that have listeners;
>    // this reference is managed by their AddListener and RemoveListener
>    // methods.
> +  for (MediaQueryList* mql = tmp->mDOMMediaQueryLists.getFirst();
> +       mql != nullptr; mql = mql->getNext()) {

I would feel better if you kept the somewhat strange control flow where we get the next element before calling RemoveAllListeners().

::: layout/style/MediaQueryList.h
@@ +27,4 @@
>  
>  class MediaQueryList final : public nsISupports,
>                               public nsWrapperCache,
> +                             public PRCList,

I think this can stop inheriting from PRCList now.
Attachment #8786891 - Flags: review?(Ms2ger) → feedback+
We cannot remove PRCList because we need that in MediaQueryList.cpp which still uses PRCList

I did not get what you want to say via first comment?
Flags: needinfo?(Ms2ger)
(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #15)
> Comment on attachment 8786891 [details] [diff] [review]
> v1.diff
> 
> Review of attachment 8786891 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good; just two comments:
> 
> ::: dom/base/nsDocument.cpp
> @@ +2023,5 @@
> >    // We own only the items in mDOMMediaQueryLists that have listeners;
> >    // this reference is managed by their AddListener and RemoveListener
> >    // methods.
> > +  for (MediaQueryList* mql = tmp->mDOMMediaQueryLists.getFirst();
> > +       mql != nullptr; mql = mql->getNext()) {
> 
> I would feel better if you kept the somewhat strange control flow where we
> get the next element before calling RemoveAllListeners().

That is, keep doing

for (MediaQueryList* mql = tmp->mDOMMediaQueryLists.getFirst(); mql;) {
  MediaQueryList* next = mql->getNext();
  ...
  mql = next;
}


> ::: layout/style/MediaQueryList.h
> @@ +27,4 @@
> >  
> >  class MediaQueryList final : public nsISupports,
> >                               public nsWrapperCache,
> > +                             public PRCList,
> 
> I think this can stop inheriting from PRCList now.

The code in MediaQueryList.cpp should be fixed as part of this bug.
Flags: needinfo?(Ms2ger)
Jinank, do you still want to pursue this?
Flags: needinfo?(jinank94)
Hey Eric, I am busy with some other stuff.
Flags: needinfo?(jinank94)
(In reply to Jinank Jain[:jj] from comment #19)
> Hey Eric, I am busy with some other stuff.

Thanks Jinank, I'll open up the bug for another contributor to finish up your existing patch if you don't mind.
Assignee: jinank94 → nobody
Whiteboard: [lang=c++] → [lang=c++][good first bug]
Thanks Eric :)
Ms2ger wasn't accepting review requests, so hopefully someone else can review this.
Apologies if I missed something, I didn't compare my diff with the earlier one. I'll do that in sometime and if I find something I'll update the review request.
Comment on attachment 8855579 [details]
Fix for bug 1142497 - change mDOMMediaQueryLists to use mozilla::LinkedList;

https://reviewboard.mozilla.org/r/127420/#review130122

This looks like a good start! I added some notes on small changes you can make, I'll try to find you a proper reviewer in the meantime.

::: dom/base/nsDocument.cpp:1853
(Diff revision 1)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCSSLoader)
>  
>    // We own only the items in mDOMMediaQueryLists that have listeners;
>    // this reference is managed by their AddListener and RemoveListener
>    // methods.
> -  for (PRCList *l = PR_LIST_HEAD(&tmp->mDOMMediaQueryLists);
> +  for (MediaQueryList* mql = tmp->mDOMMediaQueryLists.getFirst(); mql;) {

You can simplify this for loop now that we're using LinkedList, for example:

> for (auto mql : tmp->mDOMMediaQueryLists) {

::: dom/base/nsDocument.cpp:1854
(Diff revision 1)
>  
>    // We own only the items in mDOMMediaQueryLists that have listeners;
>    // this reference is managed by their AddListener and RemoveListener
>    // methods.
> -  for (PRCList *l = PR_LIST_HEAD(&tmp->mDOMMediaQueryLists);
> -       l != &tmp->mDOMMediaQueryLists; l = PR_NEXT_LINK(l)) {
> +  for (MediaQueryList* mql = tmp->mDOMMediaQueryLists.getFirst(); mql;) {
> +    MediaQueryList *next = mql->getNext();

You shouldn't need to add this.

::: layout/base/nsPresContext.cpp:2103
(Diff revision 1)
>      //
>      // Note that we intentionally send the notifications to media query
>      // list in the order they were created and, for each list, to the
>      // listeners in the order added.
>      nsTArray<MediaQueryList::HandleChangeData> notifyList;
> -    for (PRCList *l = PR_LIST_HEAD(mDocument->MediaQueryLists());
> +    for (MediaQueryList* mql = mDocument->MediaQueryLists()->getFirst(); mql;) {

This can use for (auto ...) loop as well.
Andrea, can you review this or help me find a reviewer? I can also do the review if you're okay with that.
Flags: needinfo?(amarchesini)
Thanks for the quick review Eric!
I had my reservations about using auto here; I wanted to mimic the old code as much as possible. But you're right, it does look a lot simpler! I'll incorporate all your feedback after the final review. :)
Comment on attachment 8855579 [details]
Fix for bug 1142497 - change mDOMMediaQueryLists to use mozilla::LinkedList;

https://reviewboard.mozilla.org/r/127420/#review130190

Looks good to me with these comments applied

::: dom/base/nsDocument.cpp:1853
(Diff revision 1)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCSSLoader)
>  
>    // We own only the items in mDOMMediaQueryLists that have listeners;
>    // this reference is managed by their AddListener and RemoveListener
>    // methods.
> -  for (PRCList *l = PR_LIST_HEAD(&tmp->mDOMMediaQueryLists);
> +  for (MediaQueryList* mql = tmp->mDOMMediaQueryLists.getFirst(); mql;) {

Yes, please, do what Eric suggests here.

::: dom/base/nsDocument.cpp:1957
(Diff revision 1)
>    }
>  
>    // We own only the items in mDOMMediaQueryLists that have listeners;
>    // this reference is managed by their AddListener and RemoveListener
>    // methods.
> -  for (PRCList *l = PR_LIST_HEAD(&tmp->mDOMMediaQueryLists);
> +  for (MediaQueryList* mql = tmp->mDOMMediaQueryLists.getFirst(); mql;) {

Same comment here.

::: dom/base/nsIDocument.h:1866
(Diff revision 1)
>     */
>  
>    already_AddRefed<mozilla::dom::MediaQueryList>
>      MatchMedia(const nsAString& aMediaQueryList);
>  
> -  const PRCList* MediaQueryLists() const {
> +  mozilla::LinkedList<mozilla::dom::MediaQueryList>* MediaQueryLists() {

const mozilla::LinkedList<mozilla::dom::MediaQueryList>& MediaQueryLists() const
{
  return mDOMMediaQueryLists;
}

::: layout/base/nsPresContext.cpp:2074
(Diff revision 1)
>    }
>  
>    mPendingViewportChange = false;
>  
>    if (mDocument->IsBeingUsedAsImage()) {
> -    MOZ_ASSERT(PR_CLIST_IS_EMPTY(mDocument->MediaQueryLists()));
> +    MOZ_ASSERT(mDocument->MediaQueryLists()->isEmpty());

This will be '.' instead '->'

::: layout/base/nsPresContext.cpp:2089
(Diff revision 1)
>    // flushes.  (We're already running off an event.)
>    //
>    // Note that we do this after the new style from media queries in
>    // style sheets has been computed.
>  
> -  if (!PR_CLIST_IS_EMPTY(mDocument->MediaQueryLists())) {
> +  if (!mDocument->MediaQueryLists()->isEmpty()) {

same here.

::: layout/style/MediaQueryList.cpp:28
(Diff revision 1)
> -
>    nsCSSParser parser;
>    parser.ParseMediaList(aMediaQueryList, nullptr, 0, mMediaList);
>  }
>  
> -MediaQueryList::~MediaQueryList()
> +MediaQueryList::~MediaQueryList() {}

{} in a new line
Attachment #8855579 - Flags: review+
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini [:baku] from comment #27)
> ::: dom/base/nsDocument.cpp:1957
> (Diff revision 1)
> >    }
> >  
> >    // We own only the items in mDOMMediaQueryLists that have listeners;
> >    // this reference is managed by their AddListener and RemoveListener
> >    // methods.
> > -  for (PRCList *l = PR_LIST_HEAD(&tmp->mDOMMediaQueryLists);
> > +  for (MediaQueryList* mql = tmp->mDOMMediaQueryLists.getFirst(); mql;) {
> 
> Same comment here.

We can't do that here, mql ends up possibly nuking itself leaving a dead pointer. We need to grab the next pointer before that happens. Also I learned that |NS_RELEASE_THIS| is a thing and I'm terrified :(

> ::: dom/base/nsIDocument.h:1866
> (Diff revision 1)
> >     */
> >  
> >    already_AddRefed<mozilla::dom::MediaQueryList>
> >      MatchMedia(const nsAString& aMediaQueryList);
> >  
> > -  const PRCList* MediaQueryLists() const {
> > +  mozilla::LinkedList<mozilla::dom::MediaQueryList>* MediaQueryLists() {
> 
> const mozilla::LinkedList<mozilla::dom::MediaQueryList>& MediaQueryLists()
> const
> {
>   return mDOMMediaQueryLists;
> }

Since LinkedList elements can remove themselves this probably shouldn't be const, or maybe we need a const and non-const version.
Still a little confused because of the conflicting review:
- Do we need a const version of the MeduaQueryLists() function? My understanding (after looking at LinkedList.h) was that the elements would be able to remove themselves, so const isn't really necessary. But maybe I am not looking at the whole picture.
- Can we safely use auto everywhere? If not, would it be better if we stick to what I have now, instead of having two different kinds of loops throughout the code?

Sorry for being so thick here!
(In reply to Sumit Tiwari from comment #29)
> Still a little confused because of the conflicting review:
> - Do we need a const version of the MeduaQueryLists() function? My
> understanding (after looking at LinkedList.h) was that the elements would be
> able to remove themselves, so const isn't really necessary. But maybe I am
> not looking at the whole picture.
> - Can we safely use auto everywhere? If not, would it be better if we stick
> to what I have now, instead of having two different kinds of loops
> throughout the code?
> 
> Sorry for being so thick here!

No worries, you have dueling reviews going on. I think you want a combo of both of our reviews, I'll add notes in reviewboard.
Comment on attachment 8855579 [details]
Fix for bug 1142497 - change mDOMMediaQueryLists to use mozilla::LinkedList;

https://reviewboard.mozilla.org/r/127420/#review130542

Hopefully this clears things up!

::: dom/base/nsDocument.cpp:1853
(Diff revision 1)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCSSLoader)
>  
>    // We own only the items in mDOMMediaQueryLists that have listeners;
>    // this reference is managed by their AddListener and RemoveListener
>    // methods.
> -  for (PRCList *l = PR_LIST_HEAD(&tmp->mDOMMediaQueryLists);
> +  for (MediaQueryList* mql = tmp->mDOMMediaQueryLists.getFirst(); mql;) {

Still do this.

::: dom/base/nsDocument.cpp:1957
(Diff revision 1)
>    }
>  
>    // We own only the items in mDOMMediaQueryLists that have listeners;
>    // this reference is managed by their AddListener and RemoveListener
>    // methods.
> -  for (PRCList *l = PR_LIST_HEAD(&tmp->mDOMMediaQueryLists);
> +  for (MediaQueryList* mql = tmp->mDOMMediaQueryLists.getFirst(); mql;) {

We need to preserve this behavior because the element removes itself, so keep your original change.

::: dom/base/nsIDocument.h:1866
(Diff revision 1)
>     */
>  
>    already_AddRefed<mozilla::dom::MediaQueryList>
>      MatchMedia(const nsAString& aMediaQueryList);
>  
> -  const PRCList* MediaQueryLists() const {
> +  mozilla::LinkedList<mozilla::dom::MediaQueryList>* MediaQueryLists() {

baku wanted this to be both const *and* a reference, but we can't do const.

So just this:

mozilla::LinkedList<mozilla::dom::MediaQueryList>& MediaQueryLists()
\{
  return mDOMMediaQueryLists;
\}

::: layout/base/nsPresContext.cpp:2074
(Diff revision 1)
>    }
>  
>    mPendingViewportChange = false;
>  
>    if (mDocument->IsBeingUsedAsImage()) {
> -    MOZ_ASSERT(PR_CLIST_IS_EMPTY(mDocument->MediaQueryLists()));
> +    MOZ_ASSERT(mDocument->MediaQueryLists()->isEmpty());

This is fallout from returning a ref instead of a poiter, still do this.

::: layout/base/nsPresContext.cpp:2089
(Diff revision 1)
>    // flushes.  (We're already running off an event.)
>    //
>    // Note that we do this after the new style from media queries in
>    // style sheets has been computed.
>  
> -  if (!PR_CLIST_IS_EMPTY(mDocument->MediaQueryLists())) {
> +  if (!mDocument->MediaQueryLists()->isEmpty()) {

same

::: layout/base/nsPresContext.cpp:2103
(Diff revision 1)
>      //
>      // Note that we intentionally send the notifications to media query
>      // list in the order they were created and, for each list, to the
>      // listeners in the order added.
>      nsTArray<MediaQueryList::HandleChangeData> notifyList;
> -    for (PRCList *l = PR_LIST_HEAD(mDocument->MediaQueryLists());
> +    for (MediaQueryList* mql = mDocument->MediaQueryLists()->getFirst(); mql;) {

Still do this

::: layout/style/MediaQueryList.cpp:28
(Diff revision 1)
> -
>    nsCSSParser parser;
>    parser.ParseMediaList(aMediaQueryList, nullptr, 0, mMediaList);
>  }
>  
> -MediaQueryList::~MediaQueryList()
> +MediaQueryList::~MediaQueryList() {}

Still do this
@Eric,

Thanks for the clearing that up, and sorry for the double review (wanted to clean up my commit message). Hopefully I didn't miss anything this time. :)
Comment on attachment 8855579 [details]
Fix for bug 1142497 - change mDOMMediaQueryLists to use mozilla::LinkedList;

https://reviewboard.mozilla.org/r/127420/#review130932

Thank you, looks good.
Attachment #8855579 - Flags: review?(erahm) → review+
Comment on attachment 8855579 [details]
Fix for bug 1142497 - change mDOMMediaQueryLists to use mozilla::LinkedList;

Andrea can you re-review / clear your issues?
Attachment #8855579 - Flags: review?(amarchesini)
Attachment #8786891 - Attachment is obsolete: true
Comment on attachment 8855579 [details]
Fix for bug 1142497 - change mDOMMediaQueryLists to use mozilla::LinkedList;

https://reviewboard.mozilla.org/r/127420/#review131274

It's perfect. Can you rebase it and land it?

::: dom/base/nsDocument.cpp:1957
(Diff revision 3)
>    // methods.
> -  for (PRCList *l = PR_LIST_HEAD(&tmp->mDOMMediaQueryLists);
> -       l != &tmp->mDOMMediaQueryLists; ) {
> +  for (MediaQueryList* mql = tmp->mDOMMediaQueryLists.getFirst(); mql;) {
> +    MediaQueryList* next = mql->getNext();
> -    PRCList *next = PR_NEXT_LINK(l);
> -    MediaQueryList *mql = static_cast<MediaQueryList*>(l);
>      mql->RemoveAllListeners();

Recently I landed a patch that updates MediaQueryList interface. Probably you need to rebase something. For instance this has been renamed: "Disconnect()"

::: layout/base/nsPresContext.cpp
(Diff revision 3)
>      // Note that we intentionally send the notifications to media query
>      // list in the order they were created and, for each list, to the
>      // listeners in the order added.
>      nsTArray<MediaQueryList::HandleChangeData> notifyList;
> -    for (PRCList *l = PR_LIST_HEAD(mDocument->MediaQueryLists());
> +    for (auto mql : mDocument->MediaQueryLists()) {
> -         l != mDocument->MediaQueryLists(); l = PR_NEXT_LINK(l)) {

Here the code will be:

 for (auto mql : mDocument->MediaQueryLists()) {
   mql->MaybeNotify();
 }

::: layout/style/MediaQueryList.h:29
(Diff revision 3)
>  namespace mozilla {
>  namespace dom {
>  
>  class MediaQueryList final : public nsISupports,
>                               public nsWrapperCache,
> -                             public PRCList
> +                             public mozilla::LinkedListElement<MediaQueryList>

This will be:

class MediaQueryList final : public DOMEventTargetHelper,
                             public mozilla::LinkedListElement<MediaQueryList>
Attachment #8855579 - Flags: review?(amarchesini) → review+
Thanks for the review guys; and thanks for the heads up Andrea. I'll see what needs to be rebased and see it through.
Comment on attachment 8855579 [details]
Fix for bug 1142497 - change mDOMMediaQueryLists to use mozilla::LinkedList;

https://reviewboard.mozilla.org/r/127420/#review133924

I think thi should be good. Thanks again, lets try to land this!
Andrea, can you clear your 'issues' in reviewboard? It won't let us land if they're not marked as resolved.
Flags: needinfo?(amarchesini)
Comment on attachment 8855579 [details]
Fix for bug 1142497 - change mDOMMediaQueryLists to use mozilla::LinkedList;

https://reviewboard.mozilla.org/r/127420/#review133936
I think you just need to upload a new version. Let me know if I have to do anything else.
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini [:baku] from comment #43)
> I think you just need to upload a new version. Let me know if I have to do
> anything else.

He did, mozreview is weird (see bug 1357534) and keeps the issues previously raised around until someone clears them (I can't clear your issues).
Flags: needinfo?(amarchesini)
Done
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/integration/mozilla-inbound/rev/748b099ff0e967750e4fdda279a705a36fd13c0a
Fix for bug 1142497 - change mDOMMediaQueryLists to use mozilla::LinkedList;r=erahm
I ended up pushing this manually, I noticed a rebase issue after looking at the patch in a "squashed" view in mozreview. Rather than go through another round of reviews I just took care of it locally.
Oh man, really? Sorry about the trouble then; I thought everything was in order.
(In reply to Sumit Tiwari from comment #48)
> Oh man, really? Sorry about the trouble then; I thought everything was in
> order.

No worries, it was simple mix up where a comment that was removed got re-added.
https://hg.mozilla.org/mozilla-central/rev/748b099ff0e9
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: