1478655 - factor out a PrepareAndDispatch helper for xptcall

Status: NEW

(Core :: XPCOM, enhancement)

Description

[Nathan Froyd [:froydnj]]

PrepareAndDispatch implementions for xptcstubs all tend to look about
the same; the real differences come in architecture-specific details
about sign-extension, where arguments are passed, etc.  Let's try to
factor out a lot of the similar-looking stuff into a helper function
which takes care of all xptcall-specific stuff, so PrepareAndDispatch
can focus a little more on the architecture-specific stuff.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: factor out a PrepareAndDispatch helper for xptcall

This change was inspired by jandem's recent foray into xpconnect-land.  I don't
know whether it would have made jandem's job any easier, but maybe it will make
somebody else's job a little easier.

This change is based on an old tree, and I know there's outstanding patches
that will conflict pretty hard with this change.  I'm happy to let those
patches get sorted out first.

I have not tried converting all of the PrepareAndDispatch implementations.  I
thought I would get feedback on the reasonableness of this before proceeding.
WDYT?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8995185 [details] [diff] [review]
factor out a PrepareAndDispatch helper for xptcall

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

::: xpcom/reflect/xptcall/md/unix/xptcstubs_x86_64_linux.cpp
@@ +39,2 @@
>  
> +    return PADHelper(self, methodIndex,

Don't love the name PADHelper, but I understand whereabouts it's coming from. maybe XPTC_PrepareAndDispatch(...)?

@@ +39,5 @@
>  
> +    return PADHelper(self, methodIndex,
> +                     [&](bool isJSContext,
> +                         const nsXPTParamInfo& param,
> +                         const nsXPTType& type,

Why bother passing in the type when it's literally always just `param.Type()`? - the first two params here are actually the exact same pointer (as param stores its flags in the nsXPTType's free bits).

::: xpcom/reflect/xptcall/xptcprivate.h
@@ +57,5 @@
>  #undef SENTINEL_ENTRY
>  
> +template<typename F>
> +nsresult
> +PADHelper(nsXPTCStubBase* self, uint32_t methodIndex, F&& f)

What if we made this a method on nsXPTCStubBase so we don't have to pass `self` in explicitly?

@@ +61,5 @@
> +PADHelper(nsXPTCStubBase* self, uint32_t methodIndex, F&& f)
> +{
> +  const size_t PARAM_BUFFER_COUNT = 16;
> +  nsXPTCMiniVariant paramBuffer[PARAM_BUFFER_COUNT];
> +  nsXPTCMiniVariant* dispatchParams = nullptr;

Let's use an AutoTArray here instead of doing it this way perhaps?

@@ +84,5 @@
> +    const nsXPTParamInfo& param = info->GetParam(i);
> +    const nsXPTType& type = param.GetType();
> +    nsXPTCMiniVariant* dp = &dispatchParams[i];
> +
> +    f(i == indexOfJSContext, param, type, dp);

I think it would be cleaner here to just fabricate a TD_VOID nsXPTParamInfo entry for the JSContext here, e.g:

  static const nsXPTParamInfo JSCTXT_PARAM = { { TD_VOID, 1, 0, 0, 0, 0, 0 } };

which you just pass in when at the JSContext argument. We'd then also point the output at a dummy nsXPTCMiniVariant so we effectively just throw out the parameter.

It'd look something like:


    static const nsXPTParamInfo JSCTXT = { { TD_VOID, 1 } };
  
    // We could also support stuff like OptArgc this way:
    // static const nsXPTParamInfo OPTARGC = { { TD_UINT32, 1 } };

    nsXPTCMiniVariant junk;
    for (uint32_t i = 0; i < paramCount; ++i) {
      // If we're looking at the JSContext, skip it.
      if (i == indexOfJSContext) {
        f(JSCTXT, JSCTXT.Type(), &junk);
      }

      f(info->Param(i), info->Param(i).Type(), &dispatchParams[i]);
    } 

I just really don't love the isJSContext argument, and I would like to move as much of this cross-platform reasoning out of the platform specific code, and this seems like a reasonable way to do so.

@@ +92,5 @@
> +					 dispatchParams);
> +
> +  if (dispatchParams != paramBuffer) {
> +    delete[] dispatchParams;
> +  }

Again, I would prefer an AutoTArray here if possible :-)

Comment 3

[Nathan Froyd [:froydnj]]

I understand the AutoTArray thing and have desired to do something similar myself.  At this point, however, I'd like to make minimal changes.  If this plan does work out, then changing things to AutoTArray will be much more straightforward than otherwise.

Everything else sounds pretty good, especially putting this as a method on nsXPTCStubBase.  Thanks for the feedback!

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.