Convert PRCList usage in nsPluginHost to mozilla::LinkedList

RESOLVED FIXED in Firefox 55

Status

()

Core
Plug-ins
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: erahm, Assigned: Łukasz Kędziorski, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

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
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
(Assignee)

Comment 2

7 months ago
Hello, I'd like to work on this bug
(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.
(Assignee)

Comment 4

7 months ago
Created attachment 8863283 [details] [diff] [review]
1357578 v1
Attachment #8863283 - Flags: review?(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'.
Attachment #8863283 - Flags: review?(erahm) → feedback+
Assignee: nobody → kedziorski.lukasz
(Assignee)

Comment 6

7 months ago
Created attachment 8863928 [details] [diff] [review]
1357578 v2

I did all changes, but due to removal of #include I had to modify PluginModuleMapping.
Attachment #8863283 - Attachment is obsolete: true
Attachment #8863928 - Flags: review?(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.
Attachment #8863928 - Flags: review?(erahm) → feedback+
(Assignee)

Comment 8

7 months ago
Created attachment 8863946 [details] [diff] [review]
1357578 v3
Attachment #8863928 - Attachment is obsolete: true
Attachment #8863946 - Flags: review?(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.
Attachment #8863946 - Flags: review?(erahm)
Attachment #8863946 - Flags: review?(benjamin)
Attachment #8863946 - Flags: review+

Comment 10

7 months ago
Comment on attachment 8863946 [details] [diff] [review]
1357578 v3

great, thank you for taking this Łukasz!
Attachment #8863946 - Flags: review?(benjamin) → review+
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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e6c824580042
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.