We have these flags for type descriptors in our typelib format, but there only ever seems to be one valid configuration for each type. So about 90% of our usage of these flags ends up just being validation that the flags are what we think they should be. I'm reasonable confident that we can come up with a few macros that answer any questions we might have been trying to answer with these flags, and kill the flags from the typelib format. Nuking these will put us in further violation of our own xpt spec, but I don't think anybody cares. This will make life easier in bug 665409.

I don't think we should be incompatibly changing the XPT file format unless we absolutely have to: it will be a royal PITA for a certain class of extension developer.

Just talked to bsmedberg and khuey about this on IRC. There are really two changes to make here - removing gecko's dependency on these bits, and removing these bits from the typelib. Doing the former won't break anything. Doing the latter might, and the reason we'd want to do that would be to use those bits for something else. We currently have one free bit, which I can use for bug 665409. So for now, I'm going to only remove the _use_ of those bits from gecko, and leave them in the typelib. Then we forget about this for a while while we generate a good series of future-compatible gecko releases. We can come back to removing the bits from the typelib later. ;-)

16 patches on github: https://github.com/bholley/mozilla-central/tree/xpt_ptr A lot of this is more or less a continuation of my refactoring work. XPCWrappedNative is starting to get pretty manageable! ;-)

Flagging mrbkap for review. On the off chance that anyone was looking at the branch before, please refresh - I just updated it a little bit.

Comment on attachment 566374 [details] github URL (placeholder) This is going to need significant rebasing on top of bug 688012, so I'm canceling review for the time being.

Comment on attachment 566374 [details] github URL (placeholder) This is now ready for mrbkap's review. It applies on top of bug 677788, and passes the test coverage in bug 693341.

https://github.com/bholley/mozilla-central/commit/98283373b2aea09f42bf009edcf1e79f2b649856 https://github.com/bholley/mozilla-central/commit/0c4b1804919c5f6c7124b39c5ce56d8a65793aec https://github.com/bholley/mozilla-central/commit/319ea23acafc7d5800b92009a87d1bdfe9c0eb9a These need some 'if(' -> 'if (' love.

(In reply to Ms2ger from comment #7) > https://github.com/bholley/mozilla-central/commit/ > 98283373b2aea09f42bf009edcf1e79f2b649856 > https://github.com/bholley/mozilla-central/commit/ > 0c4b1804919c5f6c7124b39c5ce56d8a65793aec > https://github.com/bholley/mozilla-central/commit/ > 319ea23acafc7d5800b92009a87d1bdfe9c0eb9a > > These need some 'if(' -> 'if (' love. Should all be fixed now - thanks! :-)

Comment on attachment 566374 [details] github URL (placeholder) This looks pretty good. I haven't done the case analyses that you have, so I'm trusting you there. Overall, this looks like a really nice cleanup. I do have one question though: - // The xpidl compiler ensures this. We reaffirm it for safety. - if (arg_type.IsPointer() || arg_type.TagPart() != nsXPTType::T_U32) - return JS_FALSE; + // This should be enforced by the xpidl compiler, but it's not. + // See bug 695235. + NS_ABORT_IF_FALSE(arg_type.TagPart() == nsXPTType::T_U32, + "size_is references parameter of invalid type."); should this happen before this patch lands? It seems like it might be bad news if someone accidentally does this with no enforcement at all...

(In reply to Blake Kaplan (:mrbkap) from comment #9) > Comment on attachment 566374 [details] > github URL (placeholder) > > This looks pretty good. I haven't done the case analyses that you have, so > I'm trusting you there. Overall, this looks like a really nice cleanup. I do > have one question though: > > - // The xpidl compiler ensures this. We reaffirm it for safety. > - if (arg_type.IsPointer() || arg_type.TagPart() != nsXPTType::T_U32) > - return JS_FALSE; > + // This should be enforced by the xpidl compiler, but it's not. > + // See bug 695235. > + NS_ABORT_IF_FALSE(arg_type.TagPart() == nsXPTType::T_U32, > + "size_is references parameter of invalid type."); > > should this happen before this patch lands? It seems like it might be bad > news if someone accidentally does this with no enforcement at all... Well, we still have enforcement in debug builds thanks to the NS_ABORT_IF_FALSE, and I think it's pretty unlikely that anyone would be writing new IDL and never testing it in such a configuration. It'd certainly be enforced on tinderbox. Still, it'd be good to get those enforcement bugs in sooner rather than later. Maybe khuey can be persuaded to do them? :-)

Landed to mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/0408a35a67f9 (and ancestors)

https://hg.mozilla.org/mozilla-central/rev/c428312abbc7 https://hg.mozilla.org/mozilla-central/rev/42ab175e8d81 https://hg.mozilla.org/mozilla-central/rev/e8df70ae53e2 https://hg.mozilla.org/mozilla-central/rev/8790687685ee https://hg.mozilla.org/mozilla-central/rev/4d417032d887 https://hg.mozilla.org/mozilla-central/rev/a03dd8e80c34 https://hg.mozilla.org/mozilla-central/rev/a8300adcea72 https://hg.mozilla.org/mozilla-central/rev/47047b832f46 https://hg.mozilla.org/mozilla-central/rev/206075dcdf40 https://hg.mozilla.org/mozilla-central/rev/74a834deb8a2 https://hg.mozilla.org/mozilla-central/rev/d7e55d8251a6 https://hg.mozilla.org/mozilla-central/rev/c1bc4c098814 https://hg.mozilla.org/mozilla-central/rev/2c2730b0cbf7 https://hg.mozilla.org/mozilla-central/rev/89e610a1014c https://hg.mozilla.org/mozilla-central/rev/911074d770a2 https://hg.mozilla.org/mozilla-central/rev/0408a35a67f9

This bug is a good example of why we should not do code reviews on github. :(

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #13) > This bug is a good example of why we should not do code reviews on github. :( Yeah. :-( Blake and I stopped doing that shortly after this, I think. If you have any questions about this bug, I'm happy to answer them. :-)

(In reply to comment #14) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #13) > > This bug is a good example of why we should not do code reviews on github. :( > > Yeah. :-( Blake and I stopped doing that shortly after this, I think. > > If you have any questions about this bug, I'm happy to answer them. :-) It's OK, I did have questions but figured them out with more hunting around. That was just a rant. :-)