1008006 - Clean up code duplication in mozJSComponentLoader

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Optimizer was struggling with some of the code duplication nastiness over in bug 1004487. I had a cute idea on how to clean it up, but it was too involved to make someone else do, so I wrote up a patch.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Use a helper to avoid repeated computations of various dependencies. v1

Given how swamped I am, this was probably not the best use of the last hour
and a half. But that's water under the bridge at this point.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Use a helper to avoid repeated computations of various dependencies. v2

Comment 3

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=b833dfec785f

Comment 4

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8419845 [details] [diff] [review]
Use a helper to avoid repeated computations of various dependencies. v2

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

I'm a bit concerned about this patch in general. I honestly don't see the big code improvement that should come with the macros. On one hand I wanted to just quickly review it since another patch is already built on top of it but, on the other that's not the reviewer mentality you told me to follow... so here it goes.

In general I'm not a big fan of macro wizardry, because it saves a few key strokes for the cost of harder debugability and often harder to read/follow simple code. Anyway r+ with the few nits, do as you wish with the macros, but please consider my opinion now and in the future about them.

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +325,5 @@
>      sSelf = this;
>  }
>  
> +#define ENSURE_DEP(name) { nsresult rv = Ensure##name(); NS_ENSURE_SUCCESS(rv, rv); }
> +#define ENSURE_HEAD(self) { \

I find this name confusing, NS_ENSURE means ensure that something is set, and this does the exact opposite. And the HEAD part does not mean a thing for me. How about ENSURE_INIT_ONLY_ONCE

@@ +334,5 @@
> +#define ENSURE_HEAD_WITH_DEPS(self, ...) { \
> +    ENSURE_HEAD(self); \
> +    ENSURE_DEPS(__VA_ARGS__); \
> +}
> +

On one hand this is a nice solution, on the other it took me more time to understand what is going on than without it... so I don't think it worth it. My opinion is that if we want to have a machinery like this, it only makes sense if we use it everywhere consistently (which might or might not be a good idea). Having to understand these macros only for this few simple cases can be frustrating for anyone who reads this code for the first time, and the benefits are questionable. Please consider this point of view.

@@ +342,5 @@
> +
> +    nsIIOService* IOService() { MOZ_ASSERT(mIOService); return mIOService; }
> +    nsresult EnsureIOService() {
> +        ENSURE_HEAD(IOService);
> +        nsresult rv = NS_OK;

Isn't initing it to NS_OK considered to be a bad practice?

@@ +361,5 @@
> +    }
> +
> +    nsIURI* ResolvedURI() { MOZ_ASSERT(mResolvedURI); return mResolvedURI; }
> +    nsresult EnsureResolvedURI() {
> +        ENSURE_HEAD_WITH_DEPS(ResolvedURI, IOService, ScriptChannel);

IOService is not need needed here.

@@ +365,5 @@
> +        ENSURE_HEAD_WITH_DEPS(ResolvedURI, IOService, ScriptChannel);
> +        return mScriptChannel->GetURI(getter_AddRefs(mResolvedURI));
> +    }
> +
> +    nsAutoCString& Key() { return mKey; }

This might worth to add an isSet flag for assertion and for an init once flag.

@@ +381,5 @@
> +    nsAutoCString mKey; // This is safe because we're MOZ_STACK_CLASS
> +};
> +
> +#undef ENSURE_DEPS
> +#undef ENSURE_DEP

Is there a reason to keep the rest of the macros?

@@ +1252,5 @@
>      nsCOMPtr<nsIFile> sourceLocalFile;
>      sourceLocalFile = do_QueryInterface(sourceFile, &rv);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    rv = info.EnsureKey();

missing NS_ENSURE_SUCCESS

Comment 5

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> I'm a bit concerned about this patch in general. I honestly don't see the
> big code improvement that should come with the macros. On one hand I wanted
> to just quickly review it since another patch is already built on top of it
> but, on the other that's not the reviewer mentality you told me to follow...
> so here it goes.

I really appreciate it!
 
> In general I'm not a big fan of macro wizardry, because it saves a few key
> strokes for the cost of harder debugability and often harder to read/follow
> simple code.

That's definitely fair. My biggest concern that I was trying to fix with this patch was the explosion of boilerplate code and the subsequent likelihood of mistakes (like the missing NS_ENSURE_SUCCESS that you pointed out). I'd be less concerned if this were JSAPI (where we can group all of this stuff with &&), but with XPCOM we have to separately propagate each error code. Yuck.

Before I added the macros, EnsureScriptChannel had a bug. Can you spot it?

nsresult EnsureScriptChannel() {
  if (mIOService)
    return NS_OK;
  nsresult rv = EnsureIOService();
  NS_ENSURE_SUCCESS(rv, rv);
  rv = EnsureURI();
  NS_ENSURE_SUCCESS(rv, rv);
  return mIOService->NewChannelFromURI(mURI, getter_AddRefs(mScriptChannel));
}

I didn't, because my eyes glazed over, in a way that they wouldn't have with:

ENSURE_HEAD_WITH_DEPS(IOService, IOService, URI);

So I think the value of macros in situations like this is less about saving keystrokes, and more about filtering out boilerplate so that bugs are easier to spot.

At the same time though, I agree with you about the cost, and TBH I was expecting longer dependency lists than I ended up with. So I could go either way here, especially given the 25 minutes I wasted futzing around with MOZ_FOR_EACH. But the code's done, so I'm just going to roll with it.

> Anyway r+ with the few nits, do as you wish with the macros,
> but please consider my opinion now and in the future about them.

Point taken. Thanks for the feedback! :-)

> 
> ::: js/xpconnect/loader/mozJSComponentLoader.cpp
> @@ +325,5 @@
> >      sSelf = this;
> >  }
> >  
> > +#define ENSURE_DEP(name) { nsresult rv = Ensure##name(); NS_ENSURE_SUCCESS(rv, rv); }
> > +#define ENSURE_HEAD(self) { \
> 
> I find this name confusing, NS_ENSURE means ensure that something is set,
> and this does the exact opposite. And the HEAD part does not mean a thing
> for me. How about ENSURE_INIT_ONLY_ONCE

Yeah. I can honestly just ditch that macro. It's only used once. ;-)

> 
> Isn't initing it to NS_OK considered to be a bad practice?

Not sure. I can remove it.

> IOService is not need needed here.

Good catch.

> This might worth to add an isSet flag for assertion and for an init once
> flag.

I switched it to a Maybe<>, which gets us the assert for free.

> Is there a reason to keep the rest of the macros?

Fixed.

> missing NS_ENSURE_SUCCESS

Fixed.

Comment 6

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e046f7ed043

Comment 7

[Gabor Krizsanits (INACTIVE)]

> I didn't, because my eyes glazed over

I know, I'm a master of glazing over...

> ENSURE_HEAD_WITH_DEPS(IOService, IOService, URI);
> 
> So I think the value of macros in situations like this is less about saving
> keystrokes, and more about filtering out boilerplate so that bugs are easier
> to spot.

Fair point. The end result is kind of neat even...

> But the code's done, so I'm just going to roll with it.

Yeah, that was my end conclusion as well, just wanted to be sure we're
on the same page about this.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e046f7ed043