1086693 - Use autogenerated JNI bindings for SDK classes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Bug 1014614 checked in some code that was based on autogenerated JNI code, but had to be changed in order to make usable. We should actually generate that code instead.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Use generated JNI bindings for MediaCodec and SurfaceTexture

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

This patch actually checks in pre-generated output because I can't figure out how to export a generated header. :gps, can you help? widget/android/bindings/Makefile.in has the rules for generating stuff

Comment 3

[Chris Kitching [:ckitching]]

Quick comment: It looks like you copypasta'd a ton of code from the annotation processor to make SDKGenerator.

Wouldn't it make sense to share more of that?

After all, all you *really* need in this patch is a way to provide input to the generation step of the annotation processor that doesn't rely on annotations.

... I'll give you a proper review when I'm not on a train. (I seem to be on trains a lot recently)

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Chris Kitching [:ckitching] from comment #3)
> Quick comment: It looks like you copypasta'd a ton of code from the
> annotation processor to make SDKGenerator.
> 
> Wouldn't it make sense to share more of that?
> 
> After all, all you *really* need in this patch is a way to provide input to
> the generation step of the annotation processor that doesn't rely on
> annotations.
> 
> ... I'll give you a proper review when I'm not on a train. (I seem to be on
> trains a lot recently)

Yeah, quite a bit of that can/should be factored out. I can do that too, I guess, just trying to get stuff working :)

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8508824 [details] [diff] [review]
Use generated JNI bindings for MediaCodec and SurfaceTexture

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

This should not be checked in as-is because it doesn't meet build system standards. But getting there isn't too much effort.

To handle generated files, you should define GENERATED_FILES in a moz.build. For each file in that list, make will evaluate the rule for that file during the "export" target. See media/libjpeg and jpeg_nbits_table.h for an example. Eventually we'll have something better where we can declare the commands used to perform generation, all from moz.build. See bug 883954.

Ideally you could also define generated files in EXPORTS to have them copied to the objdir automatically. But I'm pretty sure that will fail. So, you need an INSTALL_TARGETS rule in your Makefile to install things to $(DIST)/include.

Please request review from a build peer when these changes are made.

Comment 6

[Nick Alexander :nalexander [he/him]]

I can help with the generated files bits.  ckitching: can you sketch a roadmap for landing this relative to the other changes you have in the works?

Comment 7

[Chris Kitching [:ckitching]]

(In reply to Nick Alexander :nalexander from comment #6)
> ckitching: can you sketch a
> roadmap for landing this relative to the other changes you have in the works?

The Java changes here will need to be altered to compensate for Bug 1067217. I have a mild preference for landing this after Bug 1067217, simply because I don't especially want to rebase the ridiculous patch in that bug. I really should automate my hg rebasing more.

If there is no urgency on landing this one, I suggest simply assigning it to me and moving on to other things. I'll make the necessary changes to make it play nice with the other stuff that's in flight.


On another note why not just stick the new classes in the existing GeneratedJNIWrappers.cpp? That way, you don't need to bother writing your own output routines, and can simplify this even further.

An advantage of doing what you've done is that it doesn't experience the cyclic dependency that led to the other code being in the VCS - the library is invariant, so you can generate this stuff whenever you like, really. 

This leads us nicely to the more general problem that gps pointed out.
Our current (pre-Bug 1067217) JNI wrapper generation pipeline looks like this:

1) Compile the generated C++ code.
2) Compile the generator program.
3) Compile the Java code.
4) Run the generator on the compiled Java code to generate the C++ code.
5) Compare the generated C++ code to the stuff we compiled earlier (that was found in the VCS). If they differ, fail and instruct the user to copy one to the other.

Since we generate the C++ code in a later step than we want to use it, we're in trouble.
The "correct" solution is to reorder the build process so the Java stuff happens before the C++ stuff. I gather from Nick that this is nontrivial and generally not worth the hassle.
So, we stick the generated C++ in the VCS and error out when it changes.

After Bug 1067217, the situation looks like this:

1) Compile the generated C++ code.
2) Compile the Java code with an annotation processor that generates the C++ code shortly after the parsing step
3) Compare the generated C++ code as before.

The difference is subtle, but important. We no longer require the result of compiling the Java code to generate the C++ code.
We're now running an actual, proper, Java annotation processor (See: http://docs.oracle.com/javase/7/docs/technotes/tools/windows/javac.html#processing ). This is invoked by javac during compilation (shortly after parsing) and handed chunks of the AST which have the desired annotations. This runs faster than scanning the bytecode for annotations, as well as being prettier, but the real reason we care (in this context, anyway) is:

* Javac can be instructed to abort immediately following annotation processing. 
* Annotation processing can complete provided it parses. It need not type check, nor be capable of actually producing valid bytecode. This avoids some of the potential dependency problems associated with radically-reordering the build process.

As a result, we could then do:

1) Run annotation processing on Fennec's Java source. (Generating our C++ code)
2) Compile the C++ we just generated (and whatever else)
3) Compile Fennec's Java code with the code-generating annotation processor turned off.
4) Party, because GPS no longer wants to throw us out the window.

And make the madness go away.
Is the second step worth it? Maybe not, but at least it stops being impossible now.

Comment 8

[Nick Alexander :nalexander [he/him]]

Trying to sort through ckitching's comment and discussions in #mobile, I conclude:

* this is not an AP at all; it's a code generation step that we want to add to widget/android;
* this is entangled with the existing AP code, but that's just because it's convenient.

ckitching and I discussed this and he's happy to get the build system integration of this up to par, land it, and then continue his AP work on top of these changes.

I'm going to dig in to the build ordering problems snorp was hitting now.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8508824 [details] [diff] [review]
Use generated JNI bindings for MediaCodec and SurfaceTexture

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

Clearing the review until Kitching's concerns are resolved

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Add SDKProcessor. r=ckitching

This is one part of snorp's monolithic patch.  I think ckitching is
okay with this, but might as well have his stamp.

Comment 11

[Chris Kitching [:ckitching]]

Comment on attachment 8512320 [details] [diff] [review]
Part 1: Add SDKProcessor. r=ckitching

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

Very nice. We can kill off most of this slightly annoying code generation later (as mentioned on IRC)

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Generate and build Android SDK JNI wrappers. r=gps

The generated sources and headers are built during the export tier.
Since export is fully parallel, a new recurse dependency is needed to
make build/annotationProcessors visible to widget/android/bindings.

This is part 2 of 3 of snorp's original patch.

Comment 13

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Use generated SDK bindings. r=blassey

This is part 3 of snorp's original patch.  I'll let snorp flag blassey
for review when he's tested and is ready.

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8512325 [details] [diff] [review]
Part 2: Generate and build Android SDK JNI wrappers. r=gps

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

Just nits. Nothing that I suspect you can't handle without rereview.

::: config/recurse.mk
@@ +140,5 @@
>  ifeq (.,$(DEPTH))
>  # Interdependencies for parallel export.
>  js/xpconnect/src/export: dom/bindings/export xpcom/xpidl/export
>  accessible/xpcom/export: xpcom/xpidl/export
> +widget/android/bindings/export: build/annotationProcessors/export

Please document this. (Yes, the existing ones should be documented too.)

::: widget/android/bindings/Makefile.in
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +ANNOTATION_PROCESSOR_JAR_FILES := $(DEPTH)/build/annotationProcessors/annotationProcessors.jar

bonus points if you rename the Makefile.in-local variables to lowercase. I try to enforce the convention that UPPERCASE variables are special and have rules in rules.mk, etc. But I haven't been having much luck with that. Ignore this request if you are lazy.

@@ +6,5 @@
> +
> +MediaCodec.cpp: $(ANDROID_SDK)/android.jar mediacodec-classes.txt
> +	$(JAVA) -classpath $(ANNOTATION_PROCESSOR_JAR_FILES) org.mozilla.gecko.annotationProcessors.SDKProcessor $(ANDROID_SDK)/android.jar $(srcdir)/mediacodec-classes.txt $(CURDIR) MediaCodec
> +
> +MediaCodec.h: MediaCodec.cpp ;

Kill the semicolon.

@@ +11,5 @@
> +
> +SurfaceTexture.cpp: $(ANDROID_SDK)/android.jar surfacetexture-classes.txt
> +	$(JAVA) -classpath $(ANNOTATION_PROCESSOR_JAR_FILES) org.mozilla.gecko.annotationProcessors.SDKProcessor $(ANDROID_SDK)/android.jar $(srcdir)/surfacetexture-classes.txt $(CURDIR) SurfaceTexture
> +
> +SurfaceTexture.h: SurfaceTexture.cpp ;

Kill semicolon.

@@ +20,5 @@
> +  SurfaceTexture.h \
> +  $(NULL)
> +
> +INSTALL_TARGETS += bindings_exports
> +bindings_exports_FILES := $(bindings_exports)

You could just make the assignment inline. The extra variable adds nothing except confusion.

Comment 15

[Nick Alexander :nalexander [he/him]]

(In reply to Gregory Szorc [:gps] from comment #14)
> Comment on attachment 8512325 [details] [diff] [review]
> Part 2: Generate and build Android SDK JNI wrappers. r=gps
> 
> Review of attachment 8512325 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just nits. Nothing that I suspect you can't handle without rereview.
> 
> ::: config/recurse.mk
> @@ +140,5 @@
> >  ifeq (.,$(DEPTH))
> >  # Interdependencies for parallel export.
> >  js/xpconnect/src/export: dom/bindings/export xpcom/xpidl/export
> >  accessible/xpcom/export: xpcom/xpidl/export
> > +widget/android/bindings/export: build/annotationProcessors/export
> 
> Please document this. (Yes, the existing ones should be documented too.)

Is a comment in this file explaining why the dependency exists sufficient, such as:

# build/annotationProcessors produces a code generating JAR during its export tier that is used by widget/android/bindings during its export tier.

> ::: widget/android/bindings/Makefile.in
> @@ +1,5 @@
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +ANNOTATION_PROCESSOR_JAR_FILES := $(DEPTH)/build/annotationProcessors/annotationProcessors.jar
> 
> bonus points if you rename the Makefile.in-local variables to lowercase. I
> try to enforce the convention that UPPERCASE variables are special and have
> rules in rules.mk, etc. But I haven't been having much luck with that.
> Ignore this request if you are lazy.

I like this convention and will follow it.

> @@ +6,5 @@
> > +
> > +MediaCodec.cpp: $(ANDROID_SDK)/android.jar mediacodec-classes.txt
> > +	$(JAVA) -classpath $(ANNOTATION_PROCESSOR_JAR_FILES) org.mozilla.gecko.annotationProcessors.SDKProcessor $(ANDROID_SDK)/android.jar $(srcdir)/mediacodec-classes.txt $(CURDIR) MediaCodec
> > +
> > +MediaCodec.h: MediaCodec.cpp ;
> 
> Kill the semicolon.

Damn memory.  The semi is necessary for empty dependency lists.

> @@ +11,5 @@
> > +
> > +SurfaceTexture.cpp: $(ANDROID_SDK)/android.jar surfacetexture-classes.txt
> > +	$(JAVA) -classpath $(ANNOTATION_PROCESSOR_JAR_FILES) org.mozilla.gecko.annotationProcessors.SDKProcessor $(ANDROID_SDK)/android.jar $(srcdir)/surfacetexture-classes.txt $(CURDIR) SurfaceTexture
> > +
> > +SurfaceTexture.h: SurfaceTexture.cpp ;
> 
> Kill semicolon.
> 
> @@ +20,5 @@
> > +  SurfaceTexture.h \
> > +  $(NULL)
> > +
> > +INSTALL_TARGETS += bindings_exports
> > +bindings_exports_FILES := $(bindings_exports)
> 
> You could just make the assignment inline. The extra variable adds nothing
> except confusion.

Yeah, copy-pasta.

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Part 3: Don't generate members that are above a given API version

This adds stuff to the SDK generator such that it avoids members that are above a specified API level.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Part 4: Add a lazy initialization mode to CodeGenerator, and use it for SDK bindings

This allows us to initialize methods (and fields, etc) only when they are called, avoiding problems with things that are only available at certain API versions.

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Part 5: Add a 'catchException' mode to JNI generator

This is a new exception handling mode that allows the caller to easily check if something went wrong. It could be enhanced in the future to return the exception instance or something, but this is good enough for now.

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Part 6: Use generated bindings for MediaCodec and SurfaceTexture

Comment 20

[Chris Kitching [:ckitching]]

Comment on attachment 8519132 [details] [diff] [review]
Part 3: Don't generate members that are above a given API version

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

Seems mostly sane, though relies on some apparently magical Android Lint API I didn't know existed.

Provided you've tested it and it does what it's supposed to I'm a fan. Most of my comments relate to code cleanup (in particular your filtering function seems like it can be simplified a fair bit).

::: build/annotationProcessors/SDKProcessor.java
@@ +4,5 @@
>  
>  package org.mozilla.gecko.annotationProcessors;
>  
> +import com.android.tools.lint.checks.ApiLookup;
> +import com.android.tools.lint.LintCliClient;

I find my inability to find Javadocs for these classes quite disturbing.

... Presumably it all works...

@@ +68,5 @@
> +        System.out.println("MediaCodec version: " + sApiLookup.getClassVersion("android/media/MediaCodec"));
> +        System.out.println("MediaCodec.getInputBuffer version: " +
> +            sApiLookup.getCallVersion("android/media/MediaCodec",
> +                                      "getInputBuffer",
> +                                      "(I)Ljava/nio/ByteBuffer;"));

Isn't this debugging code?

@@ +147,5 @@
>          long e = System.currentTimeMillis();
>          System.out.println("SDK processing complete in " + (e - s) + "ms");
>      }
>  
> +    private static Member[] sortAndFilterMembers(Member[] members) {

It seems like you're only calling this from the bit a little later on where you do:
generator.generateMembers(sortAndFilterMembers(clazz.getDeclaredConstructors()));
... etc.


This means that each argument will be either a Constructor[], a Method[], or a Field[].
You should therefore probably refactor this to avoid the evil runtime type checks you've had to put into it a little later on.

You might well end up with two specialised (and about three-line) filter functions and a Member[]-accepting sort function (prettymuch the one you had before).

@@ +155,5 @@
>                  return a.getName().compareTo(b.getName());
>              }
>          });
>  
> +        ArrayList<Member> list = new ArrayList<Member>();

Use diamonds!

ArrayList<Member> list = new ArrayList<>();

Type inference for the win.

@@ +181,5 @@
> +        }
> +
> +        Member[] result = new Member[list.size()];
> +        list.toArray(result);
> +        return result;

It might be clearer to squish all three of these lines into one:

return list.toArray(new Member[list.size()]);

(I have a general distaste of relying on toArray's side effect)

::: build/annotationProcessors/utils/Utils.java
@@ +319,5 @@
>      }
>  
> +    public static String getTypeSignatureStringForClass(Class<?> clazz) {
> +        return clazz.getCanonicalName().replace('.', '/');
> +    }

It seems like this is what I really wanted to do when I wrote this:

http://mxr.mozilla.org/mozilla-central/source/build/annotationProcessors/utils/Utils.java#592

You should probably do the obvious refactor of getStartupLineForClass to use your new utility function.

Likewise:
http://mxr.mozilla.org/mozilla-central/source/build/annotationProcessors/utils/Utils.java#344

Comment 21

[Chris Kitching [:ckitching]]

Comment on attachment 8519135 [details] [diff] [review]
Part 4: Add a lazy initialization mode to CodeGenerator, and use it for SDK bindings

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

It'd be much easier to review this were it not for the fact that the vast majority of the patch seems to be simply renaming jEnv to env.

... Really?

I'm a massive fan of ridiculously huge and aggressive refactoring patches (and routinely throw >300kb ones at rnewman), but please separate the refactors from the stuff that I have to actually turn my brain on to read. "Spot the real changes amongst the huge amount of variable renamings in Splinter" is not a fun game to play.

I'm not sure that renaming this variable is a useful thing to bother with, but if you really want to then go for it... just don't make reviewing the interesting stuff harder by munging it all together.

::: build/annotationProcessors/CodeGenerator.java
@@ +46,5 @@
>      public CodeGenerator(Class<?> aClass, String aGeneratedName) {
> +        this(aClass, aGeneratedName, false);
> +    }
> +
> +    public CodeGenerator(Class<?> aClass, String aGeneratedName, boolean aLazyInit) {

The "aSomethingSomething" naming convention for parameters has fallen out of favour at Mozilla since the original code here was written. If you fancy another  aggressive refactoring patch, feel free to go through absolutely all the generator code exterminating it.

::: build/annotationProcessors/SDKProcessor.java
@@ +190,5 @@
>                                        StringBuilder implementationFile,
>                                        StringBuilder headerFile) {
>          String generatedName = clazz.getSimpleName();
>  
> +        CodeGenerator generator = new CodeGenerator(clazz, generatedName, true);

Don't you want finer-grained control of what gets lazily-initialised?
The initialisation is sort of expensive, so doing this at the granularity of the CodeGenerator seems a little sledgehammer-esque.

More generally, might this program benefit from a config file where you can specify options for generated classes? There's a bunch of little things we might want to set slightly differently (the exception handling, optional parameters, etc.), and inventing a new kludge every time we want a new feature doesn't seem like it'll scale well.

Some sort of config file format where you can specify classes to generate, members to exclude (or include), and options for each... Perhaps?

::: widget/android/AndroidBridge.cpp
@@ +158,5 @@
>      sBridge = bridge;
>  }
>  
>  bool
> +AndroidBridge::Init(JNIEnv *env)

Yay, noise!

Comment 22

[Chris Kitching [:ckitching]]

Comment on attachment 8519136 [details] [diff] [review]
Part 5: Add a 'catchException' mode to JNI generator

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

Seems a vaguely useful feature. Mostly whinging about code-cleanup-related things again.

::: build/annotationProcessors/CodeGenerator.java
@@ +124,5 @@
>          writeMethodBody(implementationSignature, theMethod, mClassToWrap,
>                          aMethodTuple.mAnnotationInfo.isMultithreaded,
>                          aMethodTuple.mAnnotationInfo.noThrow,
> +                        aMethodTuple.mAnnotationInfo.narrowChars,
> +                        aMethodTuple.mAnnotationInfo.catchException);

This is starting to get silly. Perhaps we should refactor this to just take the AnnotationInfo object and have done with it.

Don't bother for now, though: refactoring this as an annotation processor makes much of this madness go away.

@@ +460,5 @@
> +                .append("            *aResult = NS_ERROR_FAILURE;\n")
> +                .append("        }\n")
> +                .append("    } else if (aResult) {\n")
> +                .append("        *aResult = NS_OK;\n")
> +                .append("    }\n\n");

Javac can't constant fold across these calls.

Your performance will be somewhat better if you refactor this as a single append call:

.append("    if (env->ExceptionCheck()) {\n" +
        "        env->ExceptionClear();\n" +
        ... etc...


That way, javac can constant fold your use of + into one big string and you get one append call at runtime (what you have right now will actually compile to the eight calls you actually *wrote*, even though that doesn't make any kind of sense)

@@ +557,5 @@
> +                .append("    } else if (aResult) {\n")
> +                .append("        *aResult = NS_OK;\n")
> +                .append("    }\n\n");
> +        }
> +

Such magnificent copypasta!

You might want to not duplicate this code....

::: build/annotationProcessors/utils/GeneratableElementIterator.java
@@ +101,5 @@
> +                        // Determine if we should catch exceptions
> +                        final Method catchExceptionMethod = annotationType.getDeclaredMethod("catchException");
> +                        catchExceptionMethod.setAccessible(true);
> +                        catchException = (Boolean) catchExceptionMethod.invoke(annotation);
> +

*twitch*

I hate this API so very, very much.

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Chris Kitching [:ckitching] from comment #21)
> Comment on attachment 8519135 [details] [diff] [review]
> Part 4: Add a lazy initialization mode to CodeGenerator, and use it for SDK
> bindings
> 
> Review of attachment 8519135 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It'd be much easier to review this were it not for the fact that the vast
> majority of the patch seems to be simply renaming jEnv to env.
> 
> ... Really?
> 
> I'm a massive fan of ridiculously huge and aggressive refactoring patches
> (and routinely throw >300kb ones at rnewman), but please separate the
> refactors from the stuff that I have to actually turn my brain on to read.
> "Spot the real changes amongst the huge amount of variable renamings in
> Splinter" is not a fun game to play.
> 
> I'm not sure that renaming this variable is a useful thing to bother with,
> but if you really want to then go for it... just don't make reviewing the
> interesting stuff harder by munging it all together.

Do you want to lay off the hostility just a little bit? This kind of thing is completely uncalled for.

I didn't change the variable name just for the hell of it. The generator code uses the macros in AndroidBridgeUtilties, which (until this patch), assumes 'jEnv' as the JNIEnv*. We were using 'env' in the generated method bodies, so something needed changing. We use 'env' almost everywhere else, so I changed to that. I realize now we could easily skip macros altogether in the generator. That would be a much smaller patch, so I'll make that change.

> 
> ::: build/annotationProcessors/CodeGenerator.java
> @@ +46,5 @@
> >      public CodeGenerator(Class<?> aClass, String aGeneratedName) {
> > +        this(aClass, aGeneratedName, false);
> > +    }
> > +
> > +    public CodeGenerator(Class<?> aClass, String aGeneratedName, boolean aLazyInit) {
> 
> The "aSomethingSomething" naming convention for parameters has fallen out of
> favour at Mozilla since the original code here was written. If you fancy
> another  aggressive refactoring patch, feel free to go through absolutely
> all the generator code exterminating it.
> 
> ::: build/annotationProcessors/SDKProcessor.java
> @@ +190,5 @@
> >                                        StringBuilder implementationFile,
> >                                        StringBuilder headerFile) {
> >          String generatedName = clazz.getSimpleName();
> >  
> > +        CodeGenerator generator = new CodeGenerator(clazz, generatedName, true);
> 
> Don't you want finer-grained control of what gets lazily-initialised?
> The initialisation is sort of expensive, so doing this at the granularity of
> the CodeGenerator seems a little sledgehammer-esque.

Maybe, but this is 1.0. No need to get fancy right off the bat. The current approach works fine for what I need.

> 
> More generally, might this program benefit from a config file where you can
> specify options for generated classes? There's a bunch of little things we
> might want to set slightly differently (the exception handling, optional
> parameters, etc.), and inventing a new kludge every time we want a new
> feature doesn't seem like it'll scale well.
> 
> Some sort of config file format where you can specify classes to generate,
> members to exclude (or include), and options for each... Perhaps?

Maybe, but again, this is 1.0. I don't see the options really changing much as far as binding SDK classes. They should all be callable from all threads, they all can throw exceptions, etc. I guess maybe you could want wide strings.

> 
> ::: widget/android/AndroidBridge.cpp
> @@ +158,5 @@
> >      sBridge = bridge;
> >  }
> >  
> >  bool
> > +AndroidBridge::Init(JNIEnv *env)
> 
> Yay, noise!

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Chris Kitching [:ckitching] from comment #20)
> ::: build/annotationProcessors/SDKProcessor.java
> @@ +4,5 @@
> >  
> >  package org.mozilla.gecko.annotationProcessors;
> >  
> > +import com.android.tools.lint.checks.ApiLookup;
> > +import com.android.tools.lint.LintCliClient;
> 
> I find my inability to find Javadocs for these classes quite disturbing.
> 
> ... Presumably it all works...

Yeah, it's definitely a little shady. It's part of the SDK tools, and could presumably change at any time. Hopefully we won't have too many problems.

> 
> @@ +68,5 @@
> > +        System.out.println("MediaCodec version: " + sApiLookup.getClassVersion("android/media/MediaCodec"));
> > +        System.out.println("MediaCodec.getInputBuffer version: " +
> > +            sApiLookup.getCallVersion("android/media/MediaCodec",
> > +                                      "getInputBuffer",
> > +                                      "(I)Ljava/nio/ByteBuffer;"));
> 
> Isn't this debugging code?

Oops, yes it is.

> 
> @@ +147,5 @@
> >          long e = System.currentTimeMillis();
> >          System.out.println("SDK processing complete in " + (e - s) + "ms");
> >      }
> >  
> > +    private static Member[] sortAndFilterMembers(Member[] members) {
> 
> It seems like you're only calling this from the bit a little later on where
> you do:
> generator.generateMembers(sortAndFilterMembers(clazz.
> getDeclaredConstructors()));
> ... etc.
> 
> 
> This means that each argument will be either a Constructor[], a Method[], or
> a Field[].
> You should therefore probably refactor this to avoid the evil runtime type
> checks you've had to put into it a little later on.
> 
> You might well end up with two specialised (and about three-line) filter
> functions and a Member[]-accepting sort function (prettymuch the one you had
> before).

Yup, probably good. I'll change it.

> 
> @@ +155,5 @@
> >                  return a.getName().compareTo(b.getName());
> >              }
> >          });
> >  
> > +        ArrayList<Member> list = new ArrayList<Member>();
> 
> Use diamonds!
> 
> ArrayList<Member> list = new ArrayList<>();
> 
> Type inference for the win.
> 
> @@ +181,5 @@
> > +        }
> > +
> > +        Member[] result = new Member[list.size()];
> > +        list.toArray(result);
> > +        return result;
> 
> It might be clearer to squish all three of these lines into one:
> 
> return list.toArray(new Member[list.size()]);
> 
> (I have a general distaste of relying on toArray's side effect)

Ah, I didn't even realize it also returned the Array. Yes, that's much nicer.

> 
> ::: build/annotationProcessors/utils/Utils.java
> @@ +319,5 @@
> >      }
> >  
> > +    public static String getTypeSignatureStringForClass(Class<?> clazz) {
> > +        return clazz.getCanonicalName().replace('.', '/');
> > +    }
> 
> It seems like this is what I really wanted to do when I wrote this:
> 
> http://mxr.mozilla.org/mozilla-central/source/build/annotationProcessors/
> utils/Utils.java#592
> 
> You should probably do the obvious refactor of getStartupLineForClass to use
> your new utility function.
> 
> Likewise:
> http://mxr.mozilla.org/mozilla-central/source/build/annotationProcessors/
> utils/Utils.java#344

Yup

Comment 25

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Chris Kitching [:ckitching] from comment #22)
> Comment on attachment 8519136 [details] [diff] [review]
> Part 5: Add a 'catchException' mode to JNI generator
> 
> Review of attachment 8519136 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems a vaguely useful feature. Mostly whinging about code-cleanup-related
> things again.
> 
> ::: build/annotationProcessors/CodeGenerator.java
> @@ +124,5 @@
> >          writeMethodBody(implementationSignature, theMethod, mClassToWrap,
> >                          aMethodTuple.mAnnotationInfo.isMultithreaded,
> >                          aMethodTuple.mAnnotationInfo.noThrow,
> > +                        aMethodTuple.mAnnotationInfo.narrowChars,
> > +                        aMethodTuple.mAnnotationInfo.catchException);
> 
> This is starting to get silly. Perhaps we should refactor this to just take
> the AnnotationInfo object and have done with it.
> 
> Don't bother for now, though: refactoring this as an annotation processor
> makes much of this madness go away.

Yeah, I agree it is pretty silly. Don't you already have a patch that fixes it?

> 
> @@ +460,5 @@
> > +                .append("            *aResult = NS_ERROR_FAILURE;\n")
> > +                .append("        }\n")
> > +                .append("    } else if (aResult) {\n")
> > +                .append("        *aResult = NS_OK;\n")
> > +                .append("    }\n\n");
> 
> Javac can't constant fold across these calls.
> 
> Your performance will be somewhat better if you refactor this as a single
> append call:
> 
> .append("    if (env->ExceptionCheck()) {\n" +
>         "        env->ExceptionClear();\n" +
>         ... etc...
> 
> 
> That way, javac can constant fold your use of + into one big string and you
> get one append call at runtime (what you have right now will actually
> compile to the eight calls you actually *wrote*, even though that doesn't
> make any kind of sense)

Not sure we care about that too much here, but sure, why not.

> 
> @@ +557,5 @@
> > +                .append("    } else if (aResult) {\n")
> > +                .append("        *aResult = NS_OK;\n")
> > +                .append("    }\n\n");
> > +        }
> > +
> 
> Such magnificent copypasta!
> 
> You might want to not duplicate this code....

Indeed, I thought I had fixed that earlier, but I guess not.

> 
> ::: build/annotationProcessors/utils/GeneratableElementIterator.java
> @@ +101,5 @@
> > +                        // Determine if we should catch exceptions
> > +                        final Method catchExceptionMethod = annotationType.getDeclaredMethod("catchException");
> > +                        catchExceptionMethod.setAccessible(true);
> > +                        catchException = (Boolean) catchExceptionMethod.invoke(annotation);
> > +
> 
> *twitch*
> 
> I hate this API so very, very much.

Comment 26

[Chris Kitching [:ckitching]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #23)
> Do you want to lay off the hostility just a little bit? This kind of thing
> is completely uncalled for.

That really wasn't intended to come across as hostile. Sorry about that. Apparently I shouldn't try humour in the middle of the night in code reviews (or, quite possibly, ever.).

(In reply to James Willcox (:snorp) ([e-mail]) from comment #25)
> Yeah, I agree it is pretty silly. Don't you already have a patch that fixes
> it?

Yes, I think I sorted this out in one of the other bugs. Mostly just passing comment on my lack of forethought, as various people have bolted new options on since the first implementation and the pattern I started hasn't scaled prettily.


> I didn't change the variable name just for the hell of it. The generator
> code uses the macros in AndroidBridgeUtilties, which (until this patch),
> assumes 'jEnv' as the JNIEnv*. We were using 'env' in the generated method
> bodies, so something needed changing. We use 'env' almost everywhere else,
> so I changed to that. I realize now we could easily skip macros altogether
> in the generator. That would be a much smaller patch, so I'll make that
> change.

That makes a lot more sense. Supporting the macros might be a good idea, mostly a matter of style. Do what you like best.

> Maybe, but again, this is 1.0. I don't see the options really changing much
> as far as binding SDK classes. They should all be callable from all threads,
> they all can throw exceptions, etc. I guess maybe you could want wide
> strings.

True. I guess it's not impossible someone might want to noThrow-ify a library function, but that'd probably not be a good idea anyway.

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8519137 [details] [diff] [review]
Part 6: Use generated bindings for MediaCodec and SurfaceTexture

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

::: dom/media/fmp4/android/AndroidDecoderModule.cpp
@@ +473,5 @@
> +  nsresult res;
> +  mInputBuffers = (jobjectArray) env->NewGlobalRef(mDecoder->GetInputBuffers(&res));
> +  if (NS_FAILED(res)) {
> +    return res;
> +  }

don't you need to return NS_OK here?

@@ +488,5 @@
> +  nsresult res;
> +  mOutputBuffers = (jobjectArray) env->NewGlobalRef(mDecoder->GetOutputBuffers(&res));
> +  if (NS_FAILED(res)) {
> +    return res;
> +  }

don't you need to return NS_OK here?

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Part 3: Don't generate members that are above a given API version

As discussed on IRC, the runtime checks really have the smallest and most readable code, but I did do the other cleanups.

Comment 29

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Part 4: Add a lazy initialization mode to CodeGenerator, and use it for SDK bindings

This version abandons the use of the macros in the generator, which requires far fewer renamings (except in GeneratedJNIWrappers, obviously).

Comment 30

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Part 5: Add a 'catchException' mode to JNI generator

Cleanups, carrying r+ from ckitching

Comment 31

[Chris Kitching [:ckitching]]

Comment on attachment 8520057 [details] [diff] [review]
Part 4: Add a lazy initialization mode to CodeGenerator, and use it for SDK bindings

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

To clarify: I wasn't opposed to your renaming, just suggesting it might've been nicer to have split that out into another patch.

... It was a stupid comment to have made.

I'll leave it up to you to decide if it's a good idea for the generated code to use the macros: certainly, it gets pretty fugly in this patch when you make it stop using them.

Have an r+. I'm also okay with you using the other renaming-heavy version if you want prettier generated code. Apologies for any confusion.

::: widget/android/GeneratedJNIWrappers.cpp
@@ +2391,5 @@
> +    mClipboardClass = AndroidBridge::GetClassGlobalRef(env, "org/mozilla/gecko/util/Clipboard");
> +    jClearText = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "clearText", "()V");
> +    jGetClipboardTextWrapper = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "getText", "()Ljava/lang/String;");
> +    jHasText = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "hasText", "()Z");
> +    jSetClipboardText = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "setText", "(Ljava/lang/CharSequence;)V");

Suddenly, I find myself missing the macros. :/

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Chris Kitching [:ckitching] from comment #31)
> Comment on attachment 8520057 [details] [diff] [review]
> Part 4: Add a lazy initialization mode to CodeGenerator, and use it for SDK
> bindings
> 
> Review of attachment 8520057 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> To clarify: I wasn't opposed to your renaming, just suggesting it might've
> been nicer to have split that out into another patch.
> 
> ... It was a stupid comment to have made.
> 
> I'll leave it up to you to decide if it's a good idea for the generated code
> to use the macros: certainly, it gets pretty fugly in this patch when you
> make it stop using them.
> 
> Have an r+. I'm also okay with you using the other renaming-heavy version if
> you want prettier generated code. Apologies for any confusion.
> 
> ::: widget/android/GeneratedJNIWrappers.cpp
> @@ +2391,5 @@
> > +    mClipboardClass = AndroidBridge::GetClassGlobalRef(env, "org/mozilla/gecko/util/Clipboard");
> > +    jClearText = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "clearText", "()V");
> > +    jGetClipboardTextWrapper = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "getText", "()Ljava/lang/String;");
> > +    jHasText = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "hasText", "()Z");
> > +    jSetClipboardText = AndroidBridge::GetStaticMethodID(env, mClipboardClass, "setText", "(Ljava/lang/CharSequence;)V");
> 
> Suddenly, I find myself missing the macros. :/

Yeah, it's certainly a lot more verbose. I didn't like the renaming either, so I think I prefer this by a little. I guess we could also introduce new macros? Ugh.

Comment 33

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #27)
> Comment on attachment 8519137 [details] [diff] [review]
> Part 6: Use generated bindings for MediaCodec and SurfaceTexture
> 
> Review of attachment 8519137 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/fmp4/android/AndroidDecoderModule.cpp
> @@ +473,5 @@
> > +  nsresult res;
> > +  mInputBuffers = (jobjectArray) env->NewGlobalRef(mDecoder->GetInputBuffers(&res));
> > +  if (NS_FAILED(res)) {
> > +    return res;
> > +  }
> 
> don't you need to return NS_OK here?
> 
> @@ +488,5 @@
> > +  nsresult res;
> > +  mOutputBuffers = (jobjectArray) env->NewGlobalRef(mDecoder->GetOutputBuffers(&res));
> > +  if (NS_FAILED(res)) {
> > +    return res;
> > +  }
> 
> don't you need to return NS_OK here?

Indeed. I also wasn't even checking that return value. New patch coming.

Comment 34

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Part 6: Use generated bindings for AndroidMediaCodec and AndroidSurfaceTexture

Address review comments from Brad

Comment 35

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8520068 [details] [diff] [review]
Part 6: Use generated bindings for AndroidMediaCodec and AndroidSurfaceTexture

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

::: dom/media/fmp4/android/AndroidDecoderModule.cpp
@@ +352,5 @@
>      if (sample) {
>        // We have a sample, try to feed it to the decoder
> +      int inputIndex = mDecoder->DequeueInputBuffer(DECODER_TIMEOUT, &res);
> +      if (NS_FAILED(res)) {
> +        printf_stderr("exiting decoder loop due to exception while dequeuing input\n");

No cleaner logging available here?

::: gfx/gl/AndroidSurfaceTexture.cpp
@@ +105,5 @@
>    aContext->fGenTextures(1, &mTexture);
>  
> +  nsresult res;
> +  mSurfaceTexture->AttachToGLContext(mTexture, &res);
> +  return NS_SUCCEEDED(res);

Must this be a bool? If I were GCC I'd warn about "Possible loss of precision" here.

@@ +182,3 @@
>    if (mSurfaceTexture) {
> +    GeckoAppShell::UnregisterSurfaceTextureFrameListener(mSurfaceTexture->wrappedObject());
> +    delete mSurfaceTexture;

If you're deleting in the destructor, maybe AutoPtr/UniquePtr?

@@ +185,5 @@
>      mSurfaceTexture = nullptr;
>    }
>  
>    if (mSurface) {
> +    delete mSurface;

Same. (But even more so as there's no unregister)

Comment 36

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #35)
> Comment on attachment 8520068 [details] [diff] [review]
> Part 6: Use generated bindings for AndroidMediaCodec and
> AndroidSurfaceTexture
> 
> Review of attachment 8520068 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/fmp4/android/AndroidDecoderModule.cpp
> @@ +352,5 @@
> >      if (sample) {
> >        // We have a sample, try to feed it to the decoder
> > +      int inputIndex = mDecoder->DequeueInputBuffer(DECODER_TIMEOUT, &res);
> > +      if (NS_FAILED(res)) {
> > +        printf_stderr("exiting decoder loop due to exception while dequeuing input\n");
> 
> No cleaner logging available here?

Yeah, I'm going to do something different there in a new patch.

> 
> ::: gfx/gl/AndroidSurfaceTexture.cpp
> @@ +105,5 @@
> >    aContext->fGenTextures(1, &mTexture);
> >  
> > +  nsresult res;
> > +  mSurfaceTexture->AttachToGLContext(mTexture, &res);
> > +  return NS_SUCCEEDED(res);
> 
> Must this be a bool? If I were GCC I'd warn about "Possible loss of
> precision" here.

There isn't really anything you can do to recover from this failing, so bool is the same as NS_OK/NS_ERROR_FAILURE.

> 
> @@ +182,3 @@
> >    if (mSurfaceTexture) {
> > +    GeckoAppShell::UnregisterSurfaceTextureFrameListener(mSurfaceTexture->wrappedObject());
> > +    delete mSurfaceTexture;
> 
> If you're deleting in the destructor, maybe AutoPtr/UniquePtr?

Yup, good catch. I use that in other places, not sure why I didn't here.

> 
> @@ +185,5 @@
> >      mSurfaceTexture = nullptr;
> >    }
> >  
> >    if (mSurface) {
> > +    delete mSurface;
> 
> Same. (But even more so as there's no unregister)

Yup

Comment 37

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Add a Makefile target for updating GeneratedJNIWrappers

Trivial patch that adds a Makefile target for updating GeneratedJNIWrappers.cpp/h and backs up whatever is there currently.

Comment 38

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Part 6: Use generated bindings for AndroidMediaCodec and AndroidSurfaceTexture

I also fixed up some other potential badness where we weren't using a [ns]RefPtr for AndroidSurfaceTexture.

Comment 39

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8520165 [details] [diff] [review]
Add a Makefile target for updating GeneratedJNIWrappers

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

Sure, as you wish.  Why silence the actual commands?  Make your life easier by printing the long file paths to the console so you can copy and paste them.

Comment 40

[Aaron Train [:aaronmt]]

[Tracking Requested - why for this release]: As I understand it, this is needed to fix a top-crash on beta

Comment 41

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Part 6: Use generated bindings for AndroidMediaCodec and AndroidSurfaceTexture

This addresses gcp reviews, and fixes up some RefPtr usage with AndroidSurfaceTexture

Comment 42

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8512320 [details] [diff] [review]
Part 1: Add SDKProcessor. r=ckitching

Approval Request Comment
[Feature/regressing bug #]: bug 1014614
[User impact if declined]: crashes
[Describe test coverage new/current, TBPL]: local only
[Risks and why]: could potentially introduce more crashes, but only in the new MediaCodec backend. Expectation is that things will be better, however.
[String/UUID change made/needed]: none

Comment 43

[James Willcox (:snorp) ([e-mail]) (he/him)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4c759b83892a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/408af1e3b457
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6ea8b9247915
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3adfd767a92f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4b439e7e6c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b67a2e48f281
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6eed18d830d2

Comment 44

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #42)
> Comment on attachment 8512320 [details] [diff] [review]
> Part 1: Add SDKProcessor. r=ckitching
> 
> Approval Request Comment
> [Feature/regressing bug #]: bug 1014614
> [User impact if declined]: crashes
> [Describe test coverage new/current, TBPL]: local only
> [Risks and why]: could potentially introduce more crashes, but only in the
> new MediaCodec backend. Expectation is that things will be better, however.
> [String/UUID change made/needed]: none

Note that I want to land all of the patches on this bug, not just the one.

Comment 45

[James Willcox (:snorp) ([e-mail]) (he/him)]

Backed out due to build failure. Lack of --enable-warnings-as-errors bites me again.

cc1plus: error: MediaCodec.h: not a directory [-Werror]
cc1plus: error: SurfaceTexture.h: not a directory [-Werror]
cc1plus: all warnings being treated as errors 

Nick, any idea what's going on there? I'm looking into it, but thought you may know.

Comment 46

[James Willcox (:snorp) ([e-mail]) (he/him)]

The problem comes from here:

 -IMediaCodec.h -ISurfaceTexture.h

I don't know what part of Makefile.in is causing that

Comment 47

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Part 2: Generate and build Android SDK JNI wrappers. r=gps

I replaced GENERATED_INCLUDES with GENERATED_FILES in widget/android/bindings/moz.build. This gets rid of the incorrect -I flags, but I'm not totally sure it's correct.

Comment 48

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8512320 [details] [diff] [review]
Part 1: Add SDKProcessor. r=ckitching

We're not going to take this fix on Beta and instead will disable the new media backend in bug 1097276.

Comment 49

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8520913 [details] [diff] [review]
Part 2: Generate and build Android SDK JNI wrappers. r=gps

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

Yeah, the LOCAL_INCLUDES was my bad.  This looks fine.

Comment 50

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Generate and build Android SDK JNI wrappers. r=gps

Comment 51

[Nick Alexander :nalexander [he/him]]

Attachment: Part 7: Add a Makefile target for updating GeneratedJNIWrappers. r=nalexander

Comment 52

[Nick Alexander :nalexander [he/him]]

snorp: I would have pushed except these patches look stale.  So instead I've uploaded updated version of Part 2 and Part 7 and you can push.  If you can, add a 

.PHONY: update-generated-wrappers

just below the definition in m/a/b/Makefile.in.  (It's not a big deal, just good form; and I forgot.)

Comment 53

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Nick Alexander :nalexander from comment #52)
> snorp: I would have pushed except these patches look stale.  So instead I've
> uploaded updated version of Part 2 and Part 7 and you can push.  If you can,
> add a 
> 
> .PHONY: update-generated-wrappers
> 
> just below the definition in m/a/b/Makefile.in.  (It's not a big deal, just
> good form; and I forgot.)

Thanks. I added the .PHONY line.

Comment 54

[James Willcox (:snorp) ([e-mail]) (he/him)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4f9f9a469b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/47fd2549de1c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f37293159507
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/352ffda3dc38
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba24360bd0a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc5b7851503
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3cbc514c43f0

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd4f9f9a469b
https://hg.mozilla.org/mozilla-central/rev/47fd2549de1c
https://hg.mozilla.org/mozilla-central/rev/f37293159507
https://hg.mozilla.org/mozilla-central/rev/352ffda3dc38
https://hg.mozilla.org/mozilla-central/rev/9ba24360bd0a
https://hg.mozilla.org/mozilla-central/rev/8dc5b7851503
https://hg.mozilla.org/mozilla-central/rev/3cbc514c43f0

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/af8e75d893b9
https://hg.mozilla.org/releases/mozilla-aurora/rev/41eaf1b65f81
https://hg.mozilla.org/releases/mozilla-aurora/rev/82dc95dab301
https://hg.mozilla.org/releases/mozilla-aurora/rev/6b3f36796e69
https://hg.mozilla.org/releases/mozilla-aurora/rev/f3ce57da55ad
https://hg.mozilla.org/releases/mozilla-aurora/rev/f28a8076587d
https://hg.mozilla.org/releases/mozilla-aurora/rev/2fe98d9bf62b

Comment 57

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8512320 [details] [diff] [review]
Part 1: Add SDKProcessor. r=ckitching

Alright, as discussed, we're going to try this on Beta once again. The patches are on m-c and Aurora and have solved (AFAIK) all of the crashes similar to bug 1091599.

Comment 58

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8512320 [details] [diff] [review]
Part 1: Add SDKProcessor. r=ckitching

As snorp said, we're going to try this in beta10 again. If there is any significant fallout, the plan is to back this out of beta and prepare for an L specific point release. Beta+

Comment 59

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8520055 [details] [diff] [review]
Part 3: Don't generate members that are above a given API version

[Triage Comment]

Comment 60

[James Willcox (:snorp) ([e-mail]) (he/him)]

Could've sworn I posted the beta revision here, but guess not. Anyway, relanded on Beta and sticking this time.

https://hg.mozilla.org/releases/mozilla-beta/rev/2a9f1894f93f
https://hg.mozilla.org/releases/mozilla-beta/rev/6a35dcdb17d6
https://hg.mozilla.org/releases/mozilla-beta/rev/0b896cfeb63a
https://hg.mozilla.org/releases/mozilla-beta/rev/e8970c0331bb
https://hg.mozilla.org/releases/mozilla-beta/rev/d3f0e44fc6b4
https://hg.mozilla.org/releases/mozilla-beta/rev/de696c10bf32
https://hg.mozilla.org/releases/mozilla-beta/rev/01615e872752
https://hg.mozilla.org/releases/mozilla-beta/rev/5610d9bf6c5d
https://hg.mozilla.org/releases/mozilla-beta/rev/058acc4d0acd
https://hg.mozilla.org/releases/mozilla-beta/rev/cc4419d99df3