1357575 - Remove remaining PRCList references in nsNPAPIPlugin

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Description

[Eric Rahm [:erahm]]

Convert |sPendingAsyncCalls| [1] to a mozilla::LinkedList, update |nsPluginThreadRunnable| to inherit from mozilla::LinkedListElement [2], update PRCList calls to LinkedList equivalents:
  - remove PR_INIT_CLIST
  - PR_APPEND_LINK -> LinkedList::pushBack
  - PR_REMOVE -> LinkedListElement::remove, 
  - PR_CLIST_IS_EMPTY -> LinkedList::isEmpty
  - do/while loops w/ PR_LIST_HEAD/PR_NEXT_LINK -> range-based for (auto elm : list)

[1] http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/dom/plugins/base/nsNPAPIPlugin.cpp#180
[2] http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/dom/plugins/base/nsNPAPIPlugin.cpp#462

Comment 1

[Horatiu Lazu]

Hi, I'd like to work on this bug, would this be possible? I'll get straight to working on it.

Comment 2

[Horatiu Lazu]

Actually, if someone else wants to work on it they can, I might not start until later. Sorry

Comment 3

[Eric Rahm [:erahm]]

(In reply to Horatiu Lazu from comment #2)
> Actually, if someone else wants to work on it they can, I might not start
> until later. Sorry

No worries, let me know if you have any questions if you find time to work on it.

Comment 4

[jelliott15]

We are a group of Computer Science students at Western Oregon University and we would like to claim this bug to work on as our contribution for our Open Source Software class.  Can we please be assigned this bug?

Comment 5

[Eric Rahm [:erahm]]

(In reply to jelliott15 from comment #4)
> We are a group of Computer Science students at Western Oregon University and
> we would like to claim this bug to work on as our contribution for our Open
> Source Software class.  Can we please be assigned this bug?

Sure thing, welcome to the project! Please needinfo me if you need any help getting started. You can find more details on getting setup to develop Firefox on MDN [1]. For a first time contributor I suggest just using |hg| and |mqueue| [2] (ignore the giant warning on that page), but feel free to follow whatever flow is easiest for you.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Mercurial/Queues

Comment 6

[Eric Rahm [:erahm]]

I'll be out for a few days, please ni? :bsmedberg if you need assistance.

Comment 7

[jelliott15]

sPendingAsyncCalls only shows up a few times in the linked code so I we were wondering if you could hint at the the first step of how to convert that to mozilla::LinkedList?  We just need a pointer to get started.

Comment 8

[Eric Rahm [:erahm]]

(In reply to jelliott15 from comment #7)
> sPendingAsyncCalls only shows up a few times in the linked code so I we were
> wondering if you could hint at the the first step of how to convert that to
> mozilla::LinkedList?  We just need a pointer to get started.

Comment 0 has some basic details, more specifically:

  - |sPendingAsyncCalls| holds is a list of |nsPluginThreadRunnable| instances. We want to convert it from a PRCList to a mozilla::LinkedList, so we need to change the declaration [1] from |PRCList| to mozilla::LinkedList<nsPluginThreadRunnable>

  - We need to make it so that |nsPluginThreadRunnable| instances can be held in the mozilla::LinkedList, to do that we need to make |nsPluginThreadRunnable| [2] inherit from mozilla::LinkedListElement<nsPluginThreadRunnable> instead of |PRCList|.

  - Remove the call to PR_INIT_CLIST [3]
  - Convert PR_APPEND_LINK calls to LinkedList::insertBack
  - Convert PR_REMOVE_LINK calls to LinkedListElement::remove
  - Convert PR_CLIST_IS_EMPTY calls to LinkedList::isEmpty
  - Convert loops iterating over the list w/ PR_LIST_HEAD/PR_NEXT_LINK to range-based for loops [4], ie:
> for (auto r : sPendingAsyncCalls)

[1] http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/dom/plugins/base/nsNPAPIPlugin.cpp#180
[2] http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/dom/plugins/base/nsNPAPIPlugin.cpp#462
[3] http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/dom/plugins/base/nsNPAPIPlugin.cpp#572
[4] http://en.cppreference.com/w/cpp/language/range-for

Comment 9

[koll]

Attachment: bug1357575 - 'bug1357575' <Eric Rahm>

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

Comment 10

[Eric Rahm [:erahm]]

Comment on attachment 8866444 [details]
bug1357575 - 'bug1357575' <Eric Rahm>

https://reviewboard.mozilla.org/r/138066/#review141248

This is a really good start! Please see the comments below on what needs to be changed.

Also can you please make sure you [compile the code](https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build) before you push your next review? If you're having trouble working out compilation issues feel free to push a new review and ask for more direction.

::: commit-message-a3491:1
(Diff revision 1)
> +bug1357575 - 'bug1357575' r=<Eric Rahm>

Can you update your commit message to follow our [conventions](https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions)?

Something like the follow would suffice:
> Bug 1357575 - Convert PRCList usage in nsNPAPIPlugin to mozilla::LinkedList. r=erahm

::: dom/plugins/base/nsNPAPIPlugin.cpp:178
(Diff revision 1)
>    _urlredirectresponse,
>    _initasyncsurface,
>    _finalizeasyncsurface,
>    _setcurrentasyncsurface
>  };
>  

You'll need to `#include "mozilla/LinkedList.h"` at the top of this file, you can also remove `#include "prclist.h"` if it's there as well.

::: dom/plugins/base/nsNPAPIPlugin.cpp:180
(Diff revision 1)
>    _finalizeasyncsurface,
>    _setcurrentasyncsurface
>  };
>  
>  static Mutex *sPluginThreadAsyncCallLock = nullptr;
> -static PRCList sPendingAsyncCalls = PR_INIT_STATIC_CLIST(&sPendingAsyncCalls);
> +static mozilla::LinkedList<nsPluginThreadRunnable> sPendingAsyncCalls = PR_INIT_STATIC_CLIST(&sPendingAsyncCalls);

You can remove the ` = PR_INIT_STATIC_CLIST...` part now that it's a LinkedList.

::: dom/plugins/base/nsNPAPIPlugin.cpp:572
(Diff revision 1)
>      mFunc = nullptr;
>  
>      return;
>    }
>  
> -  PR_INIT_CLIST(this);
> +  //PR_INIT_CLIST(this);

This can be removed now.

::: dom/plugins/base/nsNPAPIPlugin.cpp:585
(Diff revision 1)
>        mFunc = nullptr;
>  
>        return;
>      }
>  
> -    PR_APPEND_LINK(this, &sPendingAsyncCalls);
> +    LinkedList::insertBack(this, &sPendingAsyncCalls);

`insertBack` is not a static function, you can just call the member function on the list object, ie:

```
sPendingAsyncCalls.insertBack(this);
```

::: dom/plugins/base/nsNPAPIPlugin.cpp:598
(Diff revision 1)
>    }
>  
>    {
>      MutexAutoLock lock(*sPluginThreadAsyncCallLock);
>  
> -    PR_REMOVE_LINK(this);
> +    LinkedListElement::remove(this);

Same here.

::: dom/plugins/base/nsNPAPIPlugin.cpp:625
(Diff revision 1)
>    }
>  
>    {
>      MutexAutoLock lock(*sPluginThreadAsyncCallLock);
>  
> -    if (PR_CLIST_IS_EMPTY(&sPendingAsyncCalls)) {
> +    if (LinkedList::isEmpty(&sPendingAsyncCalls)) {

Same here.

::: dom/plugins/base/nsNPAPIPlugin.cpp:630
(Diff revision 1)
> -    if (PR_CLIST_IS_EMPTY(&sPendingAsyncCalls)) {
> +    if (LinkedList::isEmpty(&sPendingAsyncCalls)) {
>        return;
>      }
>  
>      nsPluginThreadRunnable *r =
> -      (nsPluginThreadRunnable *)PR_LIST_HEAD(&sPendingAsyncCalls);
> +      (nsPluginThreadRunnable *)PR_LIST_HEAD(&sPendingAsyncCalls); 

These lines can be removed.

::: dom/plugins/base/nsNPAPIPlugin.cpp:633
(Diff revision 1)
>  
>      nsPluginThreadRunnable *r =
> -      (nsPluginThreadRunnable *)PR_LIST_HEAD(&sPendingAsyncCalls);
> +      (nsPluginThreadRunnable *)PR_LIST_HEAD(&sPendingAsyncCalls); 
>  
> -    do {
> +	for (auto r : sPendingAsyncCalls){
> -      if (r->IsForInstance(instance)) {
> +		      if (r->IsForInstance(instance)) {

Please just use spaces, not tabs.

::: dom/plugins/base/nsNPAPIPlugin.cpp:636
(Diff revision 1)
> -    do {
> +	for (auto r : sPendingAsyncCalls){
> -      if (r->IsForInstance(instance)) {
> +		      if (r->IsForInstance(instance)) {
>          r->Invalidate();
>        }
> -
>        r = (nsPluginThreadRunnable *)PR_NEXT_LINK(r);

This line can be removed now (the range-based for loop takes care of it for us).

::: dom/plugins/base/nsNPAPIPlugin.cpp:638
(Diff revision 1)
>          r->Invalidate();
>        }
> -
>        r = (nsPluginThreadRunnable *)PR_NEXT_LINK(r);
> -    } while (r != &sPendingAsyncCalls);
> +			  
> +		}

The alignment seems off here.

::: dom/plugins/base/nsNPAPIPlugin.cpp:646
(Diff revision 1)
>  
> +
>  void
>  OnShutdown()
>  {
> -  NS_ASSERTION(PR_CLIST_IS_EMPTY(&sPendingAsyncCalls),
> +  NS_ASSERTION(LinkedList::isEmpty(&sPendingAsyncCalls),

Also use the member function here.

Comment 11

[Santiago Paez [:tiago]]

Hi!

Are you guys still interested in solving this bug?

I'm just checking, I'd be happy to give it a try if you're busy :)

Comment 12

[Leo Khodel [:lvk]]

Hi, would it be possible to take this bug?

Comment 13

[Eric Rahm [:erahm]]

(In reply to Leo Khodel [:lvk] from comment #12)
> Hi, would it be possible to take this bug?

Sure thing. I just checked and it looks like most of the code referenced in comment 0 has been removed. I believe all that needs to be done is removing the `#include "prclist.h"` in dom/plugins/base/nsNPAPIPlugin.cpp [1]. Please let me know if you need any assistance getting started.

[1] http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/dom/plugins/base/nsNPAPIPlugin.cpp#13

Comment 14

[Leo Khodel [:lvk]]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #13)
> (In reply to Leo Khodel [:lvk] from comment #12)
> > Hi, would it be possible to take this bug?
> 
> Sure thing. I just checked and it looks like most of the code referenced in
> comment 0 has been removed. I believe all that needs to be done is removing
> the `#include "prclist.h"` in dom/plugins/base/nsNPAPIPlugin.cpp [1]. Please
> let me know if you need any assistance getting started.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/dom/plugins/base/nsNPAPIPlugin.
> cpp#13

Ok, I'll do that and try a local build. Which tests should I run? And could you assign the bug to me?

Thanks!

Comment 15

[Eric Rahm [:erahm]]

(In reply to Leo Khodel [:lvk] from comment #14)
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #13)
> > (In reply to Leo Khodel [:lvk] from comment #12)
> > > Hi, would it be possible to take this bug?
> > 
> > Sure thing. I just checked and it looks like most of the code referenced in
> > comment 0 has been removed. I believe all that needs to be done is removing
> > the `#include "prclist.h"` in dom/plugins/base/nsNPAPIPlugin.cpp [1]. Please
> > let me know if you need any assistance getting started.
> > 
> > [1]
> > http://searchfox.org/mozilla-central/rev/
> > 7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/dom/plugins/base/nsNPAPIPlugin.
> > cpp#13
> 
> Ok, I'll do that and try a local build. Which tests should I run? And could
> you assign the bug to me?
> 
> Thanks!

Good question, in this case since we're removing dead code the test is just that it builds :) I'll assign it to you once we get a patch attached to the bug.

Comment 16

[Leo Khodel [:lvk]]

Attachment: Bug 1357575: Remove unused include prclist.h

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

Comment 17

[Leo Khodel [:lvk]]

Comment on attachment 8915385 [details]
Bug 1357575: Remove unused include prclist.h

Who should I flag for review?

Comment 18

[Eric Rahm [:erahm]]

Comment on attachment 8915385 [details]
Bug 1357575: Remove unused include prclist.h

https://reviewboard.mozilla.org/r/186572/#review191654

Pretty simple! Looks good, I'll push to try just in case.

Comment 19

[Leo Khodel [:lvk]]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #18)
> Comment on attachment 8915385 [details]
> Bug 1357575: Remove unused include prclist.h
> 
> https://reviewboard.mozilla.org/r/186572/#review191654
> 
> Pretty simple! Looks good, I'll push to try just in case.

Great, thanks Eric :)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34fa5d45bc11
Remove unused include prclist.h r=erahm

Comment 21

[Eric Rahm [:erahm]]

Try looked good, I've landed your fix. Thank you for your contribution!

Comment 22

[Leo Khodel [:lvk]]

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #21)
> Try looked good, I've landed your fix. Thank you for your contribution!

Great, thank you for your help! :)

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/34fa5d45bc11