Closed Bug 1290614 Opened 4 years ago Closed 4 years ago

XPCNativeInterfaces are always inserted at the end of an XPCNativeSet

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(7 files)

There is a lot of code to deal with inserting interfaces into the middle of a set, but we only ever add them at the very end. I wrote a patch to simplify this code. I also made explicit some invariants about XPCNativeSetKeys.
Summary: XPCNativeInterface are always inserted at the end of an XPCNativeSet → XPCNativeInterfaces are always inserted at the end of an XPCNativeSet
A few of these patches are a little tricky, but they hopefully should not depend on prior knowledge of XPCNativeSetKey.
Depends on: 1289137
Comment on attachment 8779372 [details]
Bug 1290614, part 1 - Pass around XPCNativeSetKeys to better encapsulate argument invariants.

https://reviewboard.mozilla.org/r/70354/#review67876
Attachment #8779372 - Flags: review?(mrbkap) → review+
Comment on attachment 8779373 [details]
Bug 1290614, part 2 - Split XPCNativeSetKey into three constructors.

https://reviewboard.mozilla.org/r/70356/#review67878

r=me with or without my nit.

::: js/xpconnect/src/xpcprivate.h:1280
(Diff revision 1)
>  class XPCNativeSetKey final
>  {
>  public:
> -    explicit XPCNativeSetKey(XPCNativeSet* baseSet = nullptr,
> -                             XPCNativeInterface* addition = nullptr,
> -                             uint16_t position = 0)
> +    // This represents an existing set |baseSet|.
> +    explicit XPCNativeSetKey(XPCNativeSet* baseSet)
> +        : mBaseSet(baseSet), mAddition(nullptr), mPosition(0)

You could just call the other constructor in the two new ones:

: XPCNativeSetKey(baseSet, nullptr, 0)

to avoid a tiny amount of duplication.
Attachment #8779373 - Flags: review?(mrbkap) → review+
Comment on attachment 8779374 [details]
Bug 1290614, part 3 - The last argument to the third XPCNativeSetKey ctor is always the interface count.

https://reviewboard.mozilla.org/r/70358/#review67880
Attachment #8779374 - Flags: review?(mrbkap) → review+
Comment on attachment 8779375 [details]
Bug 1290614, part 4 - Stop storing mPosition in XPCNativeSetKey.

https://reviewboard.mozilla.org/r/70360/#review67882

It's crazy how much simpler this is to understand without mPosition.
Attachment #8779375 - Flags: review?(mrbkap) → review+
Comment on attachment 8779376 [details]
Bug 1290614, part 5 - Split out the last iteration of the XPCNativeSetKey loops.

https://reviewboard.mozilla.org/r/70362/#review67874
Attachment #8779376 - Flags: review?(mrbkap) → review+
Comment on attachment 8779377 [details]
Bug 1290614, part 6 - Hoist out the common loop over all interfaces in XPCNativeSetKey::Hash().

https://reviewboard.mozilla.org/r/70364/#review67884
Attachment #8779377 - Flags: review?(mrbkap) → review+
Comment on attachment 8779378 [details]
Bug 1290614, part 7 - Further refactor XPCNativeSetKey::Hash.

https://reviewboard.mozilla.org/r/70366/#review67886
Attachment #8779378 - Flags: review?(mrbkap) → review+
(In reply to Blake Kaplan (:mrbkap) from comment #11)
> You could just call the other constructor in the two new ones:

Oh right, I always forget about calling one ctor from another. Anyways, in this particular case I'm just going to leave it as is, because I like having it so that the three cases of keys are three separate constructors. I could generalize the asserts a little for the third ctor, but I think it would muddle the waters.

(In reply to Blake Kaplan (:mrbkap) from comment #13)
> It's crazy how much simpler this is to understand without mPosition.

Yeah. I wonder if that generality was ever used. Probably not.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36fb54f13084
part 1 - Pass around XPCNativeSetKeys to better encapsulate argument invariants. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/40e671c1ac3b
part 2 - Split XPCNativeSetKey into three constructors. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/ff0feedf164c
part 3 - The last argument to the third XPCNativeSetKey ctor is always the interface count. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/3c8bce6d54b7
part 4 - Stop storing mPosition in XPCNativeSetKey. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/12e085578d94
part 5 - Split out the last iteration of the XPCNativeSetKey loops. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/7433a7c06701
part 6 - Hoist out the common loop over all interfaces in XPCNativeSetKey::Hash(). r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/79769d046c43
part 7 - Further refactor XPCNativeSetKey::Hash. r=mrbkap
You need to log in before you can comment on or make changes to this bug.