386773 - Implement NPN_PluginThreadAsyncCall

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Johnny Stenback (:jst)]

Sun requested and specified this for a new version of the Java plugin that they're working on. Patch coming up.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Proposed fix.

This implements the new API.

Comment 2

[Josh Aas]

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.

Comment 3

[Johnny Stenback (:jst)]

Attachment: Updated fix.

Brendan, wanna look over my locking code once to make sure I didn't do anything stupid here?

Comment 4

[Brendan Eich [:brendan]]

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

Comment 5

[Johnny Stenback (:jst)]

Attachment: Make sPendingAsyncCalls a clist head.

Yeah, that totally does make sense (and no, order is not important here).

Comment 6

[Brendan Eich [: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

Comment 7

[Johnny Stenback (:jst)]

Attachment: Fix that was checked in (uses PR_INIT_STATIC_CLIST).

Yup, that makes things even simpler.

Comment 8

[Johnny Stenback (:jst)]

Fix landed.

Comment 9

[Ginn Chen]

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.

Comment 10

[Ginn Chen]

Attachment: 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

Comment 11

[Johnny Stenback (:jst)]

Thanks Ginn, I landed that (with a name change) on the trunk just now.