There are mainly two reasons I want to do this: - Allow to reuse this code in e.g. bug 1214885 - Allow to not duplicate code between the jar manifest handler code and future changes to the handling of FinalTargetFiles in the FasterMake backend wrt GeneratedFiles.

Comment on attachment 8701771 [details] [diff] [review] Part 8 - Associate a Defines instance to each ContextDerived instance Review of attachment 8701771 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/data.py @@ +88,5 @@ > > > +class HostMixin(object): > + @property > + def _defines(self): This is meant to be "def defines(self):"

Comment on attachment 8701771 [details] [diff] [review] Part 8 - Associate a Defines instance to each ContextDerived instance Review of attachment 8701771 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/backend/fastermake.py @@ +59,5 @@ > depfile, > **kwargs) > > def consume_object(self, obj): > + if isinstance(obj, (JARManifest, FinalTargetPreprocessedFiles)): It feels slightly odd that you're not checking the actual base classes that have .defines available. Perhaps hasattr() or getattr() could even be used instead? defines = getattr(obj, 'defines', {}) if defines: defines = defines.defines ::: python/mozbuild/mozbuild/frontend/data.py @@ +79,5 @@ > > @property > + def defines(self): > + defines = self._context.get('DEFINES') > + return Defines(self._context, defines) if defines else None At first glance, an empty Defines instance seems like a better return value than None, as the consumer treats None as an empty dict.

Comment on attachment 8701772 [details] [diff] [review] Part 9 - Add a RenamedSourcePath helper class Review of attachment 8701772 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/frontend/context.py @@ +447,5 @@ > + """ > + def __init__(self, context, value): > + assert isinstance(value, tuple) > + assert len(value) == 2 > + source, self._target_basename = value You can drop the len() assertion since the destructed assignment should raise on length mismatch.

Comment on attachment 8701776 [details] [diff] [review] Part 13 - Move FasterMakeBackend._consume_jar_manifest to CommonBackend Review of attachment 8701776 [details] [diff] [review]: ----------------------------------------------------------------- I didn't look at the actual changes - assumed it was copied. Thanks for the imports cleanup.

(In reply to Gregory Szorc [:gps] from comment #15) > Comment on attachment 8701771 [details] [diff] [review] > Part 8 - Associate a Defines instance to each ContextDerived instance > > Review of attachment 8701771 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: python/mozbuild/mozbuild/backend/fastermake.py > @@ +59,5 @@ > > depfile, > > **kwargs) > > > > def consume_object(self, obj): > > + if isinstance(obj, (JARManifest, FinalTargetPreprocessedFiles)): > > It feels slightly odd that you're not checking the actual base classes that > have .defines available. Perhaps hasattr() or getattr() could even be used > instead? They all have it, and I'm not interested in any other than those two. > ::: python/mozbuild/mozbuild/frontend/data.py > @@ +79,5 @@ > > > > @property > > + def defines(self): > > + defines = self._context.get('DEFINES') > > + return Defines(self._context, defines) if defines else None > > At first glance, an empty Defines instance seems like a better return value > than None, as the consumer treats None as an empty dict. Let's leave it like this for now, eventually I actually want to remove the extra (more or less useless) intermediate object.

https://hg.mozilla.org/integration/mozilla-inbound/rev/12386f9fb717 https://hg.mozilla.org/integration/mozilla-inbound/rev/40ae4d686d46 https://hg.mozilla.org/integration/mozilla-inbound/rev/cce5cfe52a3d https://hg.mozilla.org/integration/mozilla-inbound/rev/7880d2168cf7 https://hg.mozilla.org/integration/mozilla-inbound/rev/666f5a3e18a4 https://hg.mozilla.org/integration/mozilla-inbound/rev/73e69f3c9ea1 https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d167756f2f https://hg.mozilla.org/integration/mozilla-inbound/rev/531617914786 https://hg.mozilla.org/integration/mozilla-inbound/rev/4d191d8983d6 https://hg.mozilla.org/integration/mozilla-inbound/rev/a99b3de8d858 https://hg.mozilla.org/integration/mozilla-inbound/rev/dc3037f69b58 https://hg.mozilla.org/integration/mozilla-inbound/rev/3d25c8199fec https://hg.mozilla.org/integration/mozilla-inbound/rev/70708efd7d3b

https://hg.mozilla.org/mozilla-central/rev/12386f9fb717 https://hg.mozilla.org/mozilla-central/rev/40ae4d686d46 https://hg.mozilla.org/mozilla-central/rev/cce5cfe52a3d https://hg.mozilla.org/mozilla-central/rev/7880d2168cf7 https://hg.mozilla.org/mozilla-central/rev/666f5a3e18a4 https://hg.mozilla.org/mozilla-central/rev/73e69f3c9ea1 https://hg.mozilla.org/mozilla-central/rev/a9d167756f2f https://hg.mozilla.org/mozilla-central/rev/531617914786 https://hg.mozilla.org/mozilla-central/rev/4d191d8983d6 https://hg.mozilla.org/mozilla-central/rev/a99b3de8d858 https://hg.mozilla.org/mozilla-central/rev/dc3037f69b58 https://hg.mozilla.org/mozilla-central/rev/3d25c8199fec https://hg.mozilla.org/mozilla-central/rev/70708efd7d3b