Refactor out JS handler compilation work to DefaultJsCompiler and ClosureJsCompiler. Respectively, they are for build-time and run-time compile. The later is incomplete (ie: need to plug in the actual JS compiler). This change paves way for system-wide runtime compilation, which will initially 1) avoid global namespace pollution, and 2) while public APIs are allowed to escape (and made available) to global via exportJs. Tests also added.
codereview doesn't like me, so forgive the wonky copy-and-paste job.
FeatureRegistry.java
----
572 public List<String> getJsExports() {
(Draft) 2011/03/11 03:06:12rather than these 2 methods you could just reduce
the method to one:public List<String> getApis(ApiDirective.Type type,
boolean isExports) {same filter impl, generalized}
----
597 boolean isValid(ApiDirective api);
(Draft) 2011/03/11 03:06:27nit: s/isValid/matches/
-----------
DynamicJsCompiler.java (no inline editing, so line numbers lost -- grep
should be fine for the preceding line though)
----
+ builder.append("\n/* Section start for feature
'").append(bundle.getName()).append("' */\n");
(Draft) 2011/03/11 02:49:57I'd prefer to see this as small as possible (esp.
since we might keep it incompiled output), ie. "/* feature=foo */"
----
+ builder.append("}) ();");
(Draft) 2011/03/11 02:51:10meganit: space before ();
----
+ builder.append("\n/* Section end for feature
'").append(bundle.getName()).append("' */\n");
(Draft) 2011/03/11 02:50:52@see above
----
+ return new Result(builder.toString());
(Draft) 2011/03/11 02:52:13I believe you mean to do:return
reallyCompile(content, externs);...here.nit: recommend method named
doCompilation(...);
----
+ return resource.getDebugContent();
(Draft) 2011/03/11 02:54:01seems a little odd to provide this as a concrete
impl without providing a realcompiler -- it's more of an abstract class.
Perhaps make this class abstract,with no doCompilation(...) impl (but tested
w/ a does-nothing instantiation)?
----
+ if (jsUri.getLibs().contains(bundle.getName())) {
(Draft) 2011/03/11 02:54:46nit: could implement this if/else as a filtered
set of exports to use.
--------
JsCompiler.java
----
40 String preprocess(JsUri jsUri, FeatureBundle bundle);
(Draft) 2011/03/11 03:04:59I'd recommend naming this something like
getJsContent() to make it more clearwhat it's doing.
----
JsHandler.java
----
112 "JS Compilation error: " + Joiner.on(", ").join(result.getErrors()));
(Draft) 2011/03/11 02:48:02consider Logger.log(Level.ERROR... rather than
System.err. Same general effect,better structure.
On Wed, Mar 9, 2011 at 5:59 PM, <mhermanto@gmail.com> wrote:
> Update patch
>
> http://codereview.appspot.com/4266054/
>
On Thu, Mar 10, 2011 at 7:18 PM, John Hjelmstad <fargo@google.com> wrote:
> codereview doesn't like me, so forgive the wonky copy-and-paste job.
>
> Np. I can do with this. Thx for the review.
> FeatureRegistry.java
> ----
> 572 public List<String> getJsExports() {
> (Draft) 2011/03/11 03:06:12rather than these 2 methods you could just
> reduce the method to one:public List<String> getApis(ApiDirective.Type type,
> boolean isExports) {same filter impl, generalized}
>
Done. I like, less code.
----
> 597 boolean isValid(ApiDirective api);
> (Draft) 2011/03/11 03:06:27nit: s/isValid/matches/
>
Removed. No need anymore for now.
-----------
>
> DynamicJsCompiler.java (no inline editing, so line numbers lost -- grep
> should be fine for the preceding line though)
> ----
> + builder.append("\n/* Section start for feature
> '").append(bundle.getName()).append("' */\n");
> (Draft) 2011/03/11 02:49:57I'd prefer to see this as small as possible
> (esp. since we might keep it incompiled output), ie. "/* feature=foo */"
>
Done. You got it.
> ----
> + builder.append("}) ();");
> (Draft) 2011/03/11 02:51:10meganit: space before ();
>
Done.
> ----
> + builder.append("\n/* Section end for feature
> '").append(bundle.getName()).append("' */\n");
> (Draft) 2011/03/11 02:50:52@see above
>
Done.
> ----
> + return new Result(builder.toString());
> (Draft) 2011/03/11 02:52:13I believe you mean to do:return
> reallyCompile(content, externs);...here.nit: recommend method named
> doCompilation(...);
>
Done. Yes.
> ----
> + return resource.getDebugContent();
> (Draft) 2011/03/11 02:54:01seems a little odd to provide this as a concrete
> impl without providing a realcompiler -- it's more of an abstract class.
> Perhaps make this class abstract,with no doCompilation(...) impl (but tested
> w/ a does-nothing instantiation)?
>
Agree on a little odd, but if used as is (without realcompiler), the class
mostly yields a correctly-working JS. Maybe it should just renamed to
WrappingJsCompiler. I decided on not Abstract because I want to avoid
another level of inheritance to give us : JsCompiler -> DefaultJsCompiler ->
DynamicJsCompiler -> (at Google) ClosureJsCompiler -> ToolJsCompiler.
> ----
> + if (jsUri.getLibs().contains(bundle.getName())) {
> (Draft) 2011/03/11 02:54:46nit: could implement this if/else as a filtered
> set of exports to use.
>
Not following on filtered set of exports. We rely on real jscomp.Compiler
(to remove unused props) for JsCompileMode.EXPLICIT_RUN_TIME. It doesn't
look like it now, so this mode may just be scrapped completely.
> --------
>
> JsCompiler.java
> ----
> 40 String preprocess(JsUri jsUri, FeatureBundle bundle);
> (Draft) 2011/03/11 03:04:59I'd recommend naming this something like
> getJsContent() to make it more clearwhat it's doing.
>
Done. It was called "preprocess" to mean the first step in C/C++ 2steps
compilation, but I see that you want to make the connection between this
step and compile() more clear.
> ----
> JsHandler.java
> ----
> 112 "JS Compilation error: " + Joiner.on(", ").join(result.getErrors()));
> (Draft) 2011/03/11 02:48:02consider Logger.log(Level.ERROR... rather than
> System.err. Same general effect,better structure.
>
>
Done. I toned it down to Level.WARNING, since it is not fatal, wdyt?
> On Wed, Mar 9, 2011 at 5:59 PM, <mhermanto@gmail.com> wrote:
>
>> Update patch
>>
>> http://codereview.appspot.com/4266054/
>>
>
>
Comments inline, then taking a look @ the code again in Rietveld.
On Fri, Mar 11, 2011 at 12:21 PM, Michael Hermanto <mhermanto@gmail.com>wrote:
>
>
> On Thu, Mar 10, 2011 at 7:18 PM, John Hjelmstad <fargo@google.com> wrote:
>
>> codereview doesn't like me, so forgive the wonky copy-and-paste job.
>>
>> Np. I can do with this. Thx for the review.
>
>
>> FeatureRegistry.java
>> ----
>> 572 public List<String> getJsExports() {
>> (Draft) 2011/03/11 03:06:12rather than these 2 methods you could just
>> reduce the method to one:public List<String> getApis(ApiDirective.Type type,
>> boolean isExports) {same filter impl, generalized}
>>
>
> Done. I like, less code.
>
> ----
>> 597 boolean isValid(ApiDirective api);
>> (Draft) 2011/03/11 03:06:27nit: s/isValid/matches/
>>
>
> Removed. No need anymore for now.
>
> -----------
>>
>> DynamicJsCompiler.java (no inline editing, so line numbers lost -- grep
>> should be fine for the preceding line though)
>> ----
>> + builder.append("\n/* Section start for feature
>> '").append(bundle.getName()).append("' */\n");
>> (Draft) 2011/03/11 02:49:57I'd prefer to see this as small as possible
>> (esp. since we might keep it incompiled output), ie. "/* feature=foo */"
>>
>
> Done. You got it.
>
>
>> ----
>> + builder.append("}) ();");
>> (Draft) 2011/03/11 02:51:10meganit: space before ();
>>
>
> Done.
>
>
>> ----
>> + builder.append("\n/* Section end for feature
>> '").append(bundle.getName()).append("' */\n");
>> (Draft) 2011/03/11 02:50:52@see above
>>
>
> Done.
>
>
>> ----
>> + return new Result(builder.toString());
>> (Draft) 2011/03/11 02:52:13I believe you mean to do:return
>> reallyCompile(content, externs);...here.nit: recommend method named
>> doCompilation(...);
>>
>
> Done. Yes.
>
>
>> ----
>> + return resource.getDebugContent();
>> (Draft) 2011/03/11 02:54:01seems a little odd to provide this as a
>> concrete impl without providing a realcompiler -- it's more of an abstract
>> class. Perhaps make this class abstract,with no doCompilation(...) impl (but
>> tested w/ a does-nothing instantiation)?
>>
>
> Agree on a little odd, but if used as is (without realcompiler), the class
> mostly yields a correctly-working JS. Maybe it should just renamed to
> WrappingJsCompiler. I decided on not Abstract because I want to avoid
> another level of inheritance to give us : JsCompiler -> DefaultJsCompiler ->
> DynamicJsCompiler -> (at Google) ClosureJsCompiler -> ToolJsCompiler.
>
I think a rename sounds right. WrappingJsCompiler works for me.
>
>
>> ----
>> + if (jsUri.getLibs().contains(bundle.getName())) {
>> (Draft) 2011/03/11 02:54:46nit: could implement this if/else as a filtered
>> set of exports to use.
>>
>
> Not following on filtered set of exports. We rely on real jscomp.Compiler
> (to remove unused props) for JsCompileMode.EXPLICIT_RUN_TIME. It doesn't
> look like it now, so this mode may just be scrapped completely.
>
w/ the latest round of changes, disregard my filter comment.
>
>
>> --------
>>
>> JsCompiler.java
>> ----
>> 40 String preprocess(JsUri jsUri, FeatureBundle bundle);
>> (Draft) 2011/03/11 03:04:59I'd recommend naming this something like
>> getJsContent() to make it more clearwhat it's doing.
>>
>
> Done. It was called "preprocess" to mean the first step in C/C++ 2steps
> compilation, but I see that you want to make the connection between this
> step and compile() more clear.
>
>
>> ----
>> JsHandler.java
>> ----
>> 112 "JS Compilation error: " + Joiner.on(", ").join(result.getErrors()));
>> (Draft) 2011/03/11 02:48:02consider Logger.log(Level.ERROR... rather than
>> System.err. Same general effect,better structure.
>>
>>
> Done. I toned it down to Level.WARNING, since it is not fatal, wdyt?
>
Seems reasonable as a default impl. Some scenarios I could imagine wanting
to error out though. Maybe put the log.warn in a protected method that
others can override if desired.
>
>
>
>> On Wed, Mar 9, 2011 at 5:59 PM, <mhermanto@gmail.com> wrote:
>>
>>> Update patch
>>>
>>> http://codereview.appspot.com/4266054/
>>>
>>
>>
>
On Fri, Mar 11, 2011 at 12:21 PM, Michael Hermanto <mhermanto@gmail.com>wrote:
>
>
> On Thu, Mar 10, 2011 at 7:18 PM, John Hjelmstad <fargo@google.com> wrote:
>
>> codereview doesn't like me, so forgive the wonky copy-and-paste job.
>>
>> Np. I can do with this. Thx for the review.
>
>
>> FeatureRegistry.java
>> ----
>> 572 public List<String> getJsExports() {
>> (Draft) 2011/03/11 03:06:12rather than these 2 methods you could just
>> reduce the method to one:public List<String> getApis(ApiDirective.Type type,
>> boolean isExports) {same filter impl, generalized}
>>
>
> Done. I like, less code.
>
> ----
>> 597 boolean isValid(ApiDirective api);
>> (Draft) 2011/03/11 03:06:27nit: s/isValid/matches/
>>
>
> Removed. No need anymore for now.
>
> -----------
>>
>> DynamicJsCompiler.java (no inline editing, so line numbers lost -- grep
>> should be fine for the preceding line though)
>> ----
>> + builder.append("\n/* Section start for feature
>> '").append(bundle.getName()).append("' */\n");
>> (Draft) 2011/03/11 02:49:57I'd prefer to see this as small as possible
>> (esp. since we might keep it incompiled output), ie. "/* feature=foo */"
>>
>
> Done. You got it.
>
>
>> ----
>> + builder.append("}) ();");
>> (Draft) 2011/03/11 02:51:10meganit: space before ();
>>
>
> Done.
>
>
>> ----
>> + builder.append("\n/* Section end for feature
>> '").append(bundle.getName()).append("' */\n");
>> (Draft) 2011/03/11 02:50:52@see above
>>
>
> Done.
>
>
>> ----
>> + return new Result(builder.toString());
>> (Draft) 2011/03/11 02:52:13I believe you mean to do:return
>> reallyCompile(content, externs);...here.nit: recommend method named
>> doCompilation(...);
>>
>
> Done. Yes.
>
Ignore my comment here. I've removed this overridable reallyCompile() in
favor of extension with substitution. Changes updated.
>
>
>> ----
>> + return resource.getDebugContent();
>> (Draft) 2011/03/11 02:54:01seems a little odd to provide this as a
>> concrete impl without providing a realcompiler -- it's more of an abstract
>> class. Perhaps make this class abstract,with no doCompilation(...) impl (but
>> tested w/ a does-nothing instantiation)?
>>
>
> Agree on a little odd, but if used as is (without realcompiler), the class
> mostly yields a correctly-working JS. Maybe it should just renamed to
> WrappingJsCompiler. I decided on not Abstract because I want to avoid
> another level of inheritance to give us : JsCompiler -> DefaultJsCompiler ->
> DynamicJsCompiler -> (at Google) ClosureJsCompiler -> ToolJsCompiler.
>
>
>> ----
>> + if (jsUri.getLibs().contains(bundle.getName())) {
>> (Draft) 2011/03/11 02:54:46nit: could implement this if/else as a filtered
>> set of exports to use.
>>
>
> Not following on filtered set of exports. We rely on real jscomp.Compiler
> (to remove unused props) for JsCompileMode.EXPLICIT_RUN_TIME. It doesn't
> look like it now, so this mode may just be scrapped completely.
>
>
>> --------
>>
>> JsCompiler.java
>> ----
>> 40 String preprocess(JsUri jsUri, FeatureBundle bundle);
>> (Draft) 2011/03/11 03:04:59I'd recommend naming this something like
>> getJsContent() to make it more clearwhat it's doing.
>>
>
> Done. It was called "preprocess" to mean the first step in C/C++ 2steps
> compilation, but I see that you want to make the connection between this
> step and compile() more clear.
>
>
>> ----
>> JsHandler.java
>> ----
>> 112 "JS Compilation error: " + Joiner.on(", ").join(result.getErrors()));
>> (Draft) 2011/03/11 02:48:02consider Logger.log(Level.ERROR... rather than
>> System.err. Same general effect,better structure.
>>
>>
> Done. I toned it down to Level.WARNING, since it is not fatal, wdyt?
>
>
>
>> On Wed, Mar 9, 2011 at 5:59 PM, <mhermanto@gmail.com> wrote:
>>
>>> Update patch
>>>
>>> http://codereview.appspot.com/4266054/
>>>
>>
>>
>
LGTM
...with one note about pulling log.warn into a method, from previous mail.
On 2011/03/11 20:35:21, mhermanto wrote:
> On Fri, Mar 11, 2011 at 12:21 PM, Michael Hermanto <mhermanto@gmail.com>wrote:
>
> >
> >
> > On Thu, Mar 10, 2011 at 7:18 PM, John Hjelmstad <mailto:fargo@google.com>
wrote:
> >
> >> codereview doesn't like me, so forgive the wonky copy-and-paste job.
> >>
> >> Np. I can do with this. Thx for the review.
> >
> >
> >> FeatureRegistry.java
> >> ----
> >> 572 public List<String> getJsExports() {
> >> (Draft) 2011/03/11 03:06:12rather than these 2 methods you could just
> >> reduce the method to one:public List<String> getApis(ApiDirective.Type
type,
> >> boolean isExports) {same filter impl, generalized}
> >>
> >
> > Done. I like, less code.
> >
> > ----
> >> 597 boolean isValid(ApiDirective api);
> >> (Draft) 2011/03/11 03:06:27nit: s/isValid/matches/
> >>
> >
> > Removed. No need anymore for now.
> >
> > -----------
> >>
> >> DynamicJsCompiler.java (no inline editing, so line numbers lost -- grep
> >> should be fine for the preceding line though)
> >> ----
> >> + builder.append("\n/* Section start for feature
> >> '").append(bundle.getName()).append("' */\n");
> >> (Draft) 2011/03/11 02:49:57I'd prefer to see this as small as possible
> >> (esp. since we might keep it incompiled output), ie. "/* feature=foo */"
> >>
> >
> > Done. You got it.
> >
> >
> >> ----
> >> + builder.append("}) ();");
> >> (Draft) 2011/03/11 02:51:10meganit: space before ();
> >>
> >
> > Done.
> >
> >
> >> ----
> >> + builder.append("\n/* Section end for feature
> >> '").append(bundle.getName()).append("' */\n");
> >> (Draft) 2011/03/11 02:50:52@see above
> >>
> >
> > Done.
> >
> >
> >> ----
> >> + return new Result(builder.toString());
> >> (Draft) 2011/03/11 02:52:13I believe you mean to do:return
> >> reallyCompile(content, externs);...here.nit: recommend method named
> >> doCompilation(...);
> >>
> >
> > Done. Yes.
> >
>
> Ignore my comment here. I've removed this overridable reallyCompile() in
> favor of extension with substitution. Changes updated.
>
>
> >
> >
> >> ----
> >> + return resource.getDebugContent();
> >> (Draft) 2011/03/11 02:54:01seems a little odd to provide this as a
> >> concrete impl without providing a realcompiler -- it's more of an abstract
> >> class. Perhaps make this class abstract,with no doCompilation(...) impl
(but
> >> tested w/ a does-nothing instantiation)?
> >>
> >
> > Agree on a little odd, but if used as is (without realcompiler), the class
> > mostly yields a correctly-working JS. Maybe it should just renamed to
> > WrappingJsCompiler. I decided on not Abstract because I want to avoid
> > another level of inheritance to give us : JsCompiler -> DefaultJsCompiler ->
> > DynamicJsCompiler -> (at Google) ClosureJsCompiler -> ToolJsCompiler.
> >
> >
> >> ----
> >> + if (jsUri.getLibs().contains(bundle.getName())) {
> >> (Draft) 2011/03/11 02:54:46nit: could implement this if/else as a filtered
> >> set of exports to use.
> >>
> >
> > Not following on filtered set of exports. We rely on real jscomp.Compiler
> > (to remove unused props) for JsCompileMode.EXPLICIT_RUN_TIME. It doesn't
> > look like it now, so this mode may just be scrapped completely.
> >
> >
> >> --------
> >>
> >> JsCompiler.java
> >> ----
> >> 40 String preprocess(JsUri jsUri, FeatureBundle bundle);
> >> (Draft) 2011/03/11 03:04:59I'd recommend naming this something like
> >> getJsContent() to make it more clearwhat it's doing.
> >>
> >
> > Done. It was called "preprocess" to mean the first step in C/C++ 2steps
> > compilation, but I see that you want to make the connection between this
> > step and compile() more clear.
> >
> >
> >> ----
> >> JsHandler.java
> >> ----
> >> 112 "JS Compilation error: " + Joiner.on(", ").join(result.getErrors()));
> >> (Draft) 2011/03/11 02:48:02consider Logger.log(Level.ERROR... rather than
> >> System.err. Same general effect,better structure.
> >>
> >>
> > Done. I toned it down to Level.WARNING, since it is not fatal, wdyt?
> >
> >
> >
> >> On Wed, Mar 9, 2011 at 5:59 PM, <mailto:mhermanto@gmail.com> wrote:
> >>
> >>> Update patch
> >>>
> >>> http://codereview.appspot.com/4266054/
> >>>
> >>
> >>
> >
On Fri, Mar 11, 2011 at 12:34 PM, John Hjelmstad <fargo@google.com> wrote:
> Comments inline, then taking a look @ the code again in Rietveld.
>
> On Fri, Mar 11, 2011 at 12:21 PM, Michael Hermanto <mhermanto@gmail.com>wrote:
>
>>
>>
>> On Thu, Mar 10, 2011 at 7:18 PM, John Hjelmstad <fargo@google.com> wrote:
>>
>>> codereview doesn't like me, so forgive the wonky copy-and-paste job.
>>>
>>> Np. I can do with this. Thx for the review.
>>
>>
>>> FeatureRegistry.java
>>> ----
>>> 572 public List<String> getJsExports() {
>>> (Draft) 2011/03/11 03:06:12rather than these 2 methods you could just
>>> reduce the method to one:public List<String> getApis(ApiDirective.Type type,
>>> boolean isExports) {same filter impl, generalized}
>>>
>>
>> Done. I like, less code.
>>
>> ----
>>> 597 boolean isValid(ApiDirective api);
>>> (Draft) 2011/03/11 03:06:27nit: s/isValid/matches/
>>>
>>
>> Removed. No need anymore for now.
>>
>> -----------
>>>
>>> DynamicJsCompiler.java (no inline editing, so line numbers lost -- grep
>>> should be fine for the preceding line though)
>>> ----
>>> + builder.append("\n/* Section start for feature
>>> '").append(bundle.getName()).append("' */\n");
>>> (Draft) 2011/03/11 02:49:57I'd prefer to see this as small as possible
>>> (esp. since we might keep it incompiled output), ie. "/* feature=foo */"
>>>
>>
>> Done. You got it.
>>
>>
>>> ----
>>> + builder.append("}) ();");
>>> (Draft) 2011/03/11 02:51:10meganit: space before ();
>>>
>>
>> Done.
>>
>>
>>> ----
>>> + builder.append("\n/* Section end for feature
>>> '").append(bundle.getName()).append("' */\n");
>>> (Draft) 2011/03/11 02:50:52@see above
>>>
>>
>> Done.
>>
>>
>>> ----
>>> + return new Result(builder.toString());
>>> (Draft) 2011/03/11 02:52:13I believe you mean to do:return
>>> reallyCompile(content, externs);...here.nit: recommend method named
>>> doCompilation(...);
>>>
>>
>> Done. Yes.
>>
>>
>>> ----
>>> + return resource.getDebugContent();
>>> (Draft) 2011/03/11 02:54:01seems a little odd to provide this as a
>>> concrete impl without providing a realcompiler -- it's more of an abstract
>>> class. Perhaps make this class abstract,with no doCompilation(...) impl (but
>>> tested w/ a does-nothing instantiation)?
>>>
>>
>> Agree on a little odd, but if used as is (without realcompiler), the class
>> mostly yields a correctly-working JS. Maybe it should just renamed to
>> WrappingJsCompiler. I decided on not Abstract because I want to avoid
>> another level of inheritance to give us : JsCompiler -> DefaultJsCompiler ->
>> DynamicJsCompiler -> (at Google) ClosureJsCompiler -> ToolJsCompiler.
>>
>
> I think a rename sounds right. WrappingJsCompiler works for me.
>
Renamed to ExportJsCompiler, to be a little tad-more clearer.
>
>
>>
>>
>>> ----
>>> + if (jsUri.getLibs().contains(bundle.getName())) {
>>> (Draft) 2011/03/11 02:54:46nit: could implement this if/else as a
>>> filtered set of exports to use.
>>>
>>
>> Not following on filtered set of exports. We rely on real jscomp.Compiler
>> (to remove unused props) for JsCompileMode.EXPLICIT_RUN_TIME. It doesn't
>> look like it now, so this mode may just be scrapped completely.
>>
>
> w/ the latest round of changes, disregard my filter comment.
>
>
>
>>
>>
>>> --------
>>>
>>> JsCompiler.java
>>> ----
>>> 40 String preprocess(JsUri jsUri, FeatureBundle bundle);
>>> (Draft) 2011/03/11 03:04:59I'd recommend naming this something like
>>> getJsContent() to make it more clearwhat it's doing.
>>>
>>
>> Done. It was called "preprocess" to mean the first step in C/C++ 2steps
>> compilation, but I see that you want to make the connection between this
>> step and compile() more clear.
>>
>>
>>> ----
>>> JsHandler.java
>>> ----
>>> 112 "JS Compilation error: " + Joiner.on(", ").join(result.getErrors()));
>>> (Draft) 2011/03/11 02:48:02consider Logger.log(Level.ERROR... rather than
>>> System.err. Same general effect,better structure.
>>>
>>>
>> Done. I toned it down to Level.WARNING, since it is not fatal, wdyt?
>>
>
> Seems reasonable as a default impl. Some scenarios I could imagine wanting
> to error out though. Maybe put the log.warn in a protected method that
> others can override if desired.
>
>
>>
>>
>>
>>> On Wed, Mar 9, 2011 at 5:59 PM, <mhermanto@gmail.com> wrote:
>>>
>>>> Update patch
>>>>
>>>> http://codereview.appspot.com/4266054/
>>>>
>>>
>>>
>>
>
On Fri, Mar 11, 2011 at 12:42 PM, Michael Hermanto <mhermanto@gmail.com>wrote:
>
>
> On Fri, Mar 11, 2011 at 12:34 PM, John Hjelmstad <fargo@google.com> wrote:
>
>> Comments inline, then taking a look @ the code again in Rietveld.
>>
>> On Fri, Mar 11, 2011 at 12:21 PM, Michael Hermanto
<mhermanto@gmail.com>wrote:
>>
>>>
>>>
>>> On Thu, Mar 10, 2011 at 7:18 PM, John Hjelmstad <fargo@google.com>wrote:
>>>
>>>> codereview doesn't like me, so forgive the wonky copy-and-paste job.
>>>>
>>>> Np. I can do with this. Thx for the review.
>>>
>>>
>>>> FeatureRegistry.java
>>>> ----
>>>> 572 public List<String> getJsExports() {
>>>> (Draft) 2011/03/11 03:06:12rather than these 2 methods you could just
>>>> reduce the method to one:public List<String> getApis(ApiDirective.Type
type,
>>>> boolean isExports) {same filter impl, generalized}
>>>>
>>>
>>> Done. I like, less code.
>>>
>>> ----
>>>> 597 boolean isValid(ApiDirective api);
>>>> (Draft) 2011/03/11 03:06:27nit: s/isValid/matches/
>>>>
>>>
>>> Removed. No need anymore for now.
>>>
>>> -----------
>>>>
>>>> DynamicJsCompiler.java (no inline editing, so line numbers lost -- grep
>>>> should be fine for the preceding line though)
>>>> ----
>>>> + builder.append("\n/* Section start for feature
>>>> '").append(bundle.getName()).append("' */\n");
>>>> (Draft) 2011/03/11 02:49:57I'd prefer to see this as small as possible
>>>> (esp. since we might keep it incompiled output), ie. "/* feature=foo */"
>>>>
>>>
>>> Done. You got it.
>>>
>>>
>>>> ----
>>>> + builder.append("}) ();");
>>>> (Draft) 2011/03/11 02:51:10meganit: space before ();
>>>>
>>>
>>> Done.
>>>
>>>
>>>> ----
>>>> + builder.append("\n/* Section end for feature
>>>> '").append(bundle.getName()).append("' */\n");
>>>> (Draft) 2011/03/11 02:50:52@see above
>>>>
>>>
>>> Done.
>>>
>>>
>>>> ----
>>>> + return new Result(builder.toString());
>>>> (Draft) 2011/03/11 02:52:13I believe you mean to do:return
>>>> reallyCompile(content, externs);...here.nit: recommend method named
>>>> doCompilation(...);
>>>>
>>>
>>> Done. Yes.
>>>
>>>
>>>> ----
>>>> + return resource.getDebugContent();
>>>> (Draft) 2011/03/11 02:54:01seems a little odd to provide this as a
>>>> concrete impl without providing a realcompiler -- it's more of an abstract
>>>> class. Perhaps make this class abstract,with no doCompilation(...) impl
(but
>>>> tested w/ a does-nothing instantiation)?
>>>>
>>>
>>> Agree on a little odd, but if used as is (without realcompiler), the
>>> class mostly yields a correctly-working JS. Maybe it should just renamed to
>>> WrappingJsCompiler. I decided on not Abstract because I want to avoid
>>> another level of inheritance to give us : JsCompiler -> DefaultJsCompiler ->
>>> DynamicJsCompiler -> (at Google) ClosureJsCompiler -> ToolJsCompiler.
>>>
>>
>> I think a rename sounds right. WrappingJsCompiler works for me.
>>
>
> Renamed to ExportJsCompiler, to be a little tad-more clearer.
>
>>
>>
>>>
>>>
>>>> ----
>>>> + if (jsUri.getLibs().contains(bundle.getName())) {
>>>> (Draft) 2011/03/11 02:54:46nit: could implement this if/else as a
>>>> filtered set of exports to use.
>>>>
>>>
>>> Not following on filtered set of exports. We rely on real jscomp.Compiler
>>> (to remove unused props) for JsCompileMode.EXPLICIT_RUN_TIME. It doesn't
>>> look like it now, so this mode may just be scrapped completely.
>>>
>>
>> w/ the latest round of changes, disregard my filter comment.
>>
>>
>>
>>>
>>>
>>>> --------
>>>>
>>>> JsCompiler.java
>>>> ----
>>>> 40 String preprocess(JsUri jsUri, FeatureBundle bundle);
>>>> (Draft) 2011/03/11 03:04:59I'd recommend naming this something like
>>>> getJsContent() to make it more clearwhat it's doing.
>>>>
>>>
>>> Done. It was called "preprocess" to mean the first step in C/C++ 2steps
>>> compilation, but I see that you want to make the connection between this
>>> step and compile() more clear.
>>>
>>>
>>>> ----
>>>> JsHandler.java
>>>> ----
>>>> 112 "JS Compilation error: " + Joiner.on(",
>>>> ").join(result.getErrors()));
>>>> (Draft) 2011/03/11 02:48:02consider Logger.log(Level.ERROR... rather
>>>> than System.err. Same general effect,better structure.
>>>>
>>>>
>>> Done. I toned it down to Level.WARNING, since it is not fatal, wdyt?
>>>
>>
>> Seems reasonable as a default impl. Some scenarios I could imagine wanting
>> to error out though. Maybe put the log.warn in a protected method that
>> others can override if desired.
>>
>
Done, sure thing.
>
>>
>>>
>>>
>>>
>>>> On Wed, Mar 9, 2011 at 5:59 PM, <mhermanto@gmail.com> wrote:
>>>>
>>>>> Update patch
>>>>>
>>>>> http://codereview.appspot.com/4266054/
>>>>>
>>>>
>>>>
>>>
>>
>
Issue 4266054: Wrap debug JS in anonymous function, while public APIs are exportJs'ed
(Closed)
Created 15 years ago by mhermanto
Modified 15 years ago
Reviewers: fargo
Base URL: http://svn.apache.org/repos/asf/shindig/trunk/
Comments: 0
