1357578 - Convert PRCList usage in nsPluginHost to mozilla::LinkedList

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Description

[Eric Rahm [:erahm]]

We would like to update PRCList usage in nsPluginHost to use mozilla::LinkedList.

- Convert |sListHead| [1], |sModuleListHead| [2] to be a mozilla::LinkedList.
- Update |PluginDestructionGuard| [3], |PluginModuleMapping| [4] to inherit from mozilla::LinkedListElement
- Update PRCList calls to LinkedList equivalents:
  - remove PR_INIT_CLIST
  - PR_INSERT_BEFORE -> LinkedList::insertBack
  - PR_INSERT_AFTER/PR_APPEND_LINK -> LinkedList::insertFront
  - remove PR_REMOVE_LINK 
  - PR_CLIST_IS_EMPTY -> LinkedList::isEmpty
  - convert 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/nsPluginHost.h#456
[2] http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/dom/plugins/ipc/PluginModuleParent.cpp#354
[3] http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/dom/plugins/base/nsPluginHost.h#419
[4] http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/dom/plugins/ipc/PluginModuleParent.cpp#216

Comment 1

[Eric Rahm [:erahm]]

This got a little mixed up, the proper names are:

- Convert |sListHead| [1], |sRunnableListHead| [2] to be a mozilla::LinkedList.
- Update |PluginDestructionGuard| [3], |nsPluginDestroyRunnable| [4] to inherit from mozilla::LinkedListElement

[1] http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/dom/plugins/base/nsPluginHost.h#456
[2] http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/dom/plugins/base/nsPluginHost.cpp#4036
[3] http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/dom/plugins/base/nsPluginHost.h#419
[4] http://searchfox.org/mozilla-central/rev/214345204f1e7d97abb571b7992b6deedb5ff98f/dom/plugins/base/nsPluginHost.cpp#3977

Comment 2

[Łukasz Kędziorski]

Hello, I'd like to work on this bug

Comment 3

[Eric Rahm [:erahm]]

(In reply to Łukasz Kędziorski from comment #2)
> Hello, I'd like to work on this bug

Great! Let me know if you need any help getting started.

Comment 4

[Łukasz Kędziorski]

Attachment: 1357578 v1

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8863283 [details] [diff] [review]
1357578 v1

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

Thank you Łukasz, this looks great! There are just few small changes I'd like you to make.

::: dom/plugins/base/nsPluginHost.cpp
@@ +4005,5 @@
>      }
>  
> +	nsPluginDestroyRunnable *r = sRunnableListHead.getFirst();
> +
> +    while (r != sRunnableListHead.getLast()->getNext()) {

This can be simplified to a range-based for loop, ie:

> for (auto r : sRunnableListHead)

@@ +4031,4 @@
>  protected:
>    RefPtr<nsNPAPIPluginInstance> mInstance;
>  
> +  static mozilla::LinkedList<nsPluginDestroyRunnable> sRunnableListHead;

We can rename these lists now. The 'Head' portion isn't necessary, so just 'sRunnableList'.

@@ +4084,5 @@
>    // destroy upon destruction.
>  
> +  PluginDestructionGuard *g = sListHead.getFirst();
> +
> +  while (g != sListHead.getLast()->getNext()) {

This can also be simplified to a range-based for loop:

> for (auto g : sList)

::: dom/plugins/base/nsPluginHost.h
@@ +6,4 @@
>  #ifndef nsPluginHost_h_
>  #define nsPluginHost_h_
>  
> +#include "mozilla/LinkedList.h"

We can remove the "prclist.h" include as well now.

@@ +452,5 @@
>  
>    RefPtr<nsNPAPIPluginInstance> mInstance;
>    bool mDelayedDestroy;
>  
> +  static mozilla::LinkedList<PluginDestructionGuard> sListHead;

This can be renamed to just 'sList'.

Comment 6

[Łukasz Kędziorski]

Attachment: 1357578 v2

I did all changes, but due to removal of #include I had to modify PluginModuleMapping.

Comment 7

[Eric Rahm [:erahm]]

Comment on attachment 8863928 [details] [diff] [review]
1357578 v2

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

This is super close, we just have a few minor changes.

Thank you for fixing PluginModuleParent.cpp as well, but that code is going to be removed soon so we don't want to change it (bug 1357582). Can you please revert your changes to PluginModuleParent.cpp and just add a #include "prclist.h" to PluginModuleParent.cpp instead? I think that should fix your build issues.

::: dom/plugins/base/nsPluginHost.cpp
@@ +4009,5 @@
>          // There's another runnable scheduled to tear down
>          // instance. Let it do the job.
>          return NS_OK;
>        }
> +      r = r->getNext();

Sorry for not being clearer! You don't need this anymore. The range-based for loop [1] takes care of it for you.

[1] http://en.cppreference.com/w/cpp/language/range-for

@@ +4086,5 @@
>        g->mDelayedDestroy = true;
>  
>        return true;
>      }
> +    g = g->getNext();

Same here.

Comment 8

[Łukasz Kędziorski]

Attachment: 1357578 v3

Comment 9

[Eric Rahm [:erahm]]

Comment on attachment 8863946 [details] [diff] [review]
1357578 v3

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

Thank you this looks perfect. We just need to get sign-off from a plugin peer now.

bsemdberg, can you take a look at this. I've already gone through a few iterations and it looks good to me.

Comment 10

[Benjamin Smedberg]

Comment on attachment 8863946 [details] [diff] [review]
1357578 v3

great, thank you for taking this Łukasz!

Comment 11

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6c824580042a056a77eebd7edeb433ccd7b0ab9
Bug 1357578 - Convert PRCList usage in nsPluginHost to mozilla::LinkedList. r=erahm,r=bsemdberg

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e6c824580042