909639 - Include fewer headers in binding implementation files

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Even near-empty binding files include lots of stuff.  We should stop doing that.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Refactor callForEachType to return a generator so we can use it more easily, and rename it to getAllTypes.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Refactor our computation of whether we need the nsDOMQS header to make it easier to do similar other things.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Move our various conditional headers to the new bindingHeaders setup.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Make previously-unconditional headers conditional.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 795830 [details] [diff] [review]
part 1.  Refactor callForEachType to return a generator so we can use it more easily, and rename it to getAllTypes.

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

::: dom/bindings/Codegen.py
@@ +609,5 @@
>          # Now find all the things we'll need as arguments because we
>          # need to wrap or unwrap them.
>          bindingHeaders = set()
>          declareIncludes = set(declareIncludes)
> +        def addHeadersForType(typeInfo):

Could be

  def addHeadersForType((t, descriptor, dictionary)):

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 795830 [details] [diff] [review]
part 1.  Refactor callForEachType to return a generator so we can use it more easily, and rename it to getAllTypes.

Not a fan of generators, but ok.

And Ms2ger's suggestion is good.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 795831 [details] [diff] [review]
part 2.  Refactor our computation of whether we need the nsDOMQS header to make it easier to do similar other things.


>@@ -852,16 +852,19 @@ def UnionConversions(descriptors, dictio
>             for f in t.flatMemberTypes:
>                 f = f.unroll()
>                 if f.isInterface():
>                     if f.isSpiderMonkeyInterface():
>                         headers.add("jsfriendapi.h")
>                         headers.add("mozilla/dom/TypedArray.h")
>                     elif not f.inner.isExternal():
>                         headers.add(CGHeaders.getDeclarationFilename(f.inner))
>+                    if (f.isGeckoInterface() and
>+                        providers[0].getDescriptor(f.inner.identifier.name).hasXPConnectImpls):
>+                        headers.add("nsDOMQS.h")
[0] could use some comment. It is rather magical and one needs to look at getRelevantProviders to
understand what it means.


> 
>         # Add header includes.
>+        bindingHeaders = [header for (header, include) in
>+                          bindingHeaders.iteritems() if include]
Not exactly easy to parse. This syntax nicely splits the important parts aside; header in the beginning and
'if include' at the end...
But given this is python, I don't expect anything better :)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 795832 [details] [diff] [review]
part 3.  Move our various conditional headers to the new bindingHeaders setup.

nice

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 795833 [details] [diff] [review]
part 4.  Make previously-unconditional headers conditional.


>         curr = CGList([CGForwardDeclarations(config, descriptors,
>                                              mainCallbacks, workerCallbacks,
>                                              dictionaries,
>                                              callbackDescriptors + jsImplemented),
>-                       CGWrapper(CGGeneric("using namespace mozilla::dom;"),
>-                                 defineOnly=True),
Surprising, but if it compiles...

Comment 10

[Boris Zbarsky [:bzbarsky]]

>  def addHeadersForType((t, descriptor, dictionary)):

Oh, spiffy.

> [0] could use some comment.

Will do.

> Surprising, but if it compiles...

It does, because everything after this point is wrapped in "namespace mozilla { namespace dom {".

Comment 11

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/c361190e59d7
   https://hg.mozilla.org/integration/mozilla-inbound/rev/a60adae598b8
   https://hg.mozilla.org/integration/mozilla-inbound/rev/7d19c042913d
   https://hg.mozilla.org/integration/mozilla-inbound/rev/052f30e2182b

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c361190e59d7
https://hg.mozilla.org/mozilla-central/rev/a60adae598b8
https://hg.mozilla.org/mozilla-central/rev/7d19c042913d
https://hg.mozilla.org/mozilla-central/rev/052f30e2182b