Implement NPN_PluginThreadAsyncCall

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
Plug-ins
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: jst, Assigned: jst)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9alpha8
dev-doc-complete
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

11 years ago
Sun requested and specified this for a new version of the Java plugin that they're working on. Patch coming up.
(Assignee)

Comment 1

11 years ago
Created attachment 270788 [details] [diff] [review]
Proposed fix.

This implements the new API.
Attachment #270788 - Flags: review?(joshmoz)
(Assignee)

Updated

11 years ago
Flags: blocking1.9+

Comment 2

11 years ago
Comment on attachment 270788 [details] [diff] [review]
Proposed fix.

 void    NP_LOADDS NPN_PushPopupsEnabledState(NPP instance, NPBool enabled);
 void    NP_LOADDS NPN_PopPopupsEnabledState(NPP instance);
+void    NP_LOADDS NPN_PluginThreadAsyncCall(NPP plugin, void (*func) (void *),
+                                            void *userData);

Perhaps we should call the first argument "instance" instead of "plugin" to be consistent with the functions above?

+    return mNpp == npp;

I think this would be more clear with parentheses around the test like ('(mNpp == npp)').

+  if (!sPluginThreadAsyncCallLock) {
+    // Failed to create lock, not much we can do here then...
+
+    mFunc = nsnull;
+
+    return;
+  }

Seems like the two blank lines are unnecessary.
Attachment #270788 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 3

11 years ago
Created attachment 271283 [details] [diff] [review]
Updated fix.

Brendan, wanna look over my locking code once to make sure I didn't do anything stupid here?
Attachment #271283 - Flags: superreview?(brendan)
Attachment #271283 - Flags: review+
Comment on attachment 271283 [details] [diff] [review]
Updated fix.


>+static PRCList *sPendingAsyncCalls = nsnull;

This should be a list head object (PRCList), not a pointer (PRCList *), if you want FIFO order. Looks like order does not matter on the circular, head-less list of all runnables, but using a real list head could help code size and complexity (see below) a bit.

>+    if (sPendingAsyncCalls) {
>+      PR_INSERT_LINK(this, sPendingAsyncCalls);
>+    } else {
>+      sPendingAsyncCalls = this;
>+    }
>+  }

So first runnable goes at head of list, then 2nd goes after first (PR_INSERT_LINK(e,l) inserts e after l), then 3rd goes after 1st and before 2nd, etc. -- reversing after the first. Does this matter?

>+    if (PR_CLIST_IS_EMPTY(this)) {
>+      if (sPendingAsyncCalls == this) {
>+        sPendingAsyncCalls = nsnull;
>+      }
>+    } else {
>+      if (sPendingAsyncCalls == this) {
>+        sPendingAsyncCalls = PR_NEXT_LINK(this);
>+      }
>+
>+      PR_REMOVE_LINK(this);
>+    }

With sPendingAsyncCalls a PRCList header, you just need PR_REMOVE_LINK(this) here and everything is nicely kept consistent. So a list head seems worth it to me.

/be
(Assignee)

Comment 5

11 years ago
Created attachment 271912 [details] [diff] [review]
Make sPendingAsyncCalls a clist head.

Yeah, that totally does make sense (and no, order is not important here).
Attachment #270788 - Attachment is obsolete: true
Attachment #271283 - Attachment is obsolete: true
Attachment #271912 - Flags: superreview?(brendan)
Attachment #271283 - Flags: superreview?(brendan)
Comment on attachment 271912 [details] [diff] [review]
Make sPendingAsyncCalls a clist head.

>+static PRCList sPendingAsyncCalls;
[snip...]
>+  if (!PR_NEXT_LINK(&sPendingAsyncCalls)) {
>+    PR_INIT_CLIST(&sPendingAsyncCalls);
>+  }

So you could just statically initialize sPendingAsyncCalls = PR_INIT_STATIC_CLIST(&sPendingAsyncCalls); and avoid the runtime initialization (unless you need to re-init, but I didn't see anything that nulls the PRCList header's next link). sr=me with this taken into account,

/be
Attachment #271912 - Flags: superreview?(brendan) → superreview+
(Assignee)

Comment 7

11 years ago
Created attachment 271929 [details] [diff] [review]
Fix that was checked in (uses PR_INIT_STATIC_CLIST).

Yup, that makes things even simpler.
(Assignee)

Comment 8

11 years ago
Fix landed.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 9

11 years ago
doesn't compile on Solaris

"ns4xPlugin.cpp", line 2289: Warning: function void(_NPP*,void(*)(void*),void*) overloads extern "C" void(_NPP*,extern "C" void(*)(void*),void*) because of different language linkages.
"ns4xPlugin.cpp", line 2352: Error: The function _pluginthreadasynccall(_NPP*, extern "C" void(*)(void*), void*) has not had a body defined.
1 Error(s) and 13 Warning(s) detected.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 10

11 years ago
Created attachment 271977 [details] [diff] [review]
patch for solaris compiler (landed with name change)

I don't know how good it is.

I got this idea from http://www.glenmccl.com/ansi028.htm
Attachment #271977 - Flags: review?(jst)
(Assignee)

Comment 11

11 years ago
Thanks Ginn, I landed that (with a name change) on the trunk just now.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Keywords: dev-doc-complete
Version: unspecified → Trunk

Updated

11 years ago
Attachment #271977 - Attachment description: patch for solaris compiler → patch for solaris compiler (landed with name change)
Attachment #271977 - Flags: review?(jst)
Flags: in-testsuite?

Updated

8 years ago
Duplicate of this bug: 64794
You need to log in before you can comment on or make changes to this bug.