Issue 4276046: More improvements/changes for dynamic JS compilation

Issue 4276046: More improvements/changes for dynamic JS compilation (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
15 years ago by mhermanto

Modified:
14 years, 10 months ago

Reviewers:
johnfargo, fargo

Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/

Visibility:
Public.

Description

- Enable common container (and dependent core.io) for dynamic JS compilation. - Streamline RenderingContext and JsCompileMode defaults. - Allow setting of JsCompileMode on JsUri. - Extern JsUri now supports JsCompileMode.

Patch Set 1 : Update #

Total comments: 10

Patch Set 2 : Update patch #

Created: 15 years ago

Download [raw] [tar.bz2]

Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+152 lines, -129 lines)			Patch
features/src/main/javascript/features/container/feature.xml	View		1 chunk	+2 lines, -1 line	0 comments	Download
features/src/main/javascript/features/container/init.js	View		1 chunk	+3 lines, -3 lines	0 comments	Download
features/src/main/javascript/features/container/service.js	View		3 chunks	+10 lines, -10 lines	0 comments	Download
features/src/main/javascript/features/core.io/io.js	View	1	14 chunks	+87 lines, -87 lines	0 comments	Download
features/src/main/javascript/features/osapi/gadgetsrpctransport.js	View	1	2 chunks	+4 lines, -4 lines	0 comments	Download
features/src/main/javascript/features/rpc/feature.xml	View		1 chunk	+0 lines, -1 line	0 comments	Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/JsCompileMode.java	View		1 chunk	+4 lines, -0 lines	0 comments	Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/RenderingContext.java	View		2 chunks	+7 lines, -5 lines	0 comments	Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultJsUriManager.java	View		3 chunks	+8 lines, -2 lines	0 comments	Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/JsUriManager.java	View		6 chunks	+13 lines, -9 lines	0 comments	Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultJsUriManagerTest.java	View		6 chunks	+14 lines, -7 lines	0 comments	Download

Messages

Total messages: 6

Expand All Messages | Collapse All Messages

johnfargo

http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/features/core.io/io.js File features/src/main/javascript/features/core.io/io.js (right): http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/features/core.io/io.js#newcode77 features/src/main/javascript/features/core.io/io.js:77: if (xobj.readyState !== 4) { 'readyState' http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/features/core.io/io.js#newcode81 features/src/main/javascript/features/core.io/io.js:81: if ...

15 years ago (2011-03-14 22:26:52 UTC) #2

mhermanto

On Mon, Mar 14, 2011 at 3:26 PM, <johnfargo@gmail.com> wrote: > > > http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/features/core.io/io.js > ...

15 years ago (2011-03-14 22:47:02 UTC) #3

johnfargo

15 years ago (2011-03-14 22:54:58 UTC) #6

LGTM

Color me impressed that Closure Compiler can trace the provenance of an
object back to its "new" call. I wonder if certain property names are just
not obfuscated?

On Mon, Mar 14, 2011 at 3:47 PM, Michael Hermanto <mhermanto@gmail.com>wrote:

>
>
> On Mon, Mar 14, 2011 at 3:26 PM, <johnfargo@gmail.com> wrote:
>
>>
>>
>>
http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/...
>> File features/src/main/javascript/features/core.io/io.js (right):
>>
>>
>>
http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/...
>> features/src/main/javascript/features/core.io/io.js:77: if
>> (xobj.readyState !== 4) {
>> 'readyState'
>>
>>
> Done.
>
>
>
>>
>>
http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/...
>> features/src/main/javascript/features/core.io/io.js:81: if (xobj.status
>> !== 200) {
>> 'status'
>> ...and others
>>
>> Done.
>
>
>>
>>
http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/...
>> features/src/main/javascript/features/core.io/io.js:136: var txt =
>> xobj.responseText;
>> 'responseText'
>>
>>
> Done.
>
>
>>
>>
http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/...
>> features/src/main/javascript/features/core.io/io.js:152: oauthState =
>> data.oauthState;
>> 'oauthState'
>>
>> Done.
>
>
>>
>>
http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/...
>> features/src/main/javascript/features/core.io/io.js:155: if (data.st) {
>> 'st'
>>
>
> Done.
>
>
>>
>>
>>
http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/...
>> features/src/main/javascript/features/core.io/io.js:156:
>> shindig.auth.updateSecurityToken(data.st);
>> same
>>
>>
>>
http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/...
>> features/src/main/javascript/features/core.io/io.js:204:
>> dom.validateOnParse = false;
>> probably need to quote these too
>>
>
> Don't need. Looks like an already-extern'ed prop for ActiveXObject.
>
>
>>
>>
>>
http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/...
>> features/src/main/javascript/features/core.io/io.js:255:
>> xhr.onreadystatechange = gadgets.util.makeClosure(
>> 'onreadystatechange'
>>
>>
> Don't need. Same reason.
>
>
>>
>>
http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/...
>> features/src/main/javascript/features/core.io/io.js:527:
>> params.rewriteMime ? '&rewriteMime=' +
>> encodeURIComponent(params.rewriteMime) : '';
>> 'rewriteMime'
>>
>
>  Done.
>
>
>>
>>
http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/...
>> File features/src/main/javascript/features/osapi/gadgetsrpctransport.js
>> (right):
>>
>>
>>
http://codereview.appspot.com/4276046/diff/3003/features/src/main/javascript/...
>> features/src/main/javascript/features/osapi/gadgetsrpctransport.js:45:
>> responseMap[response[i].id] = response[i];
>> 'id'
>
>
> Done.
>
>>
>>
>> http://codereview.appspot.com/4276046/
>>
>
>
> Thanks for the thorough comments. I started this just to get common
> container to work. There's likely more missing in other features, but I
> suppose it's more obivous when we get to it.
>

Expand All Messages | Collapse All Messages