Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(62)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 4254051: Exports more symbols via feature directives. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
15 years ago by mhermanto
Modified:
15 years ago
Reviewers:
johnfargo, fargo
Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/
Visibility:
Public.

Patch Set 1 : Update patch #

Patch Set 2 : Update patch #

Unified diffs Side-by-side diffs Delta from patch set Stats (+330 lines, -172 lines) Patch
features/src/main/javascript/features/caja/feature.xml View 1 chunk +4 lines, -0 lines 0 comments Download
features/src/main/javascript/features/cloo/cloo.js View 1 chunk +1 line, -1 line 0 comments Download
features/src/main/javascript/features/cloo/feature.xml View 1 1 chunk +5 lines, -9 lines 0 comments Download
features/src/main/javascript/features/com.google.gadgets.analytics/feature.xml View 1 chunk +3 lines, -0 lines 0 comments Download
features/src/main/javascript/features/container/feature.xml View 1 chunk +36 lines, -0 lines 0 comments Download
features/src/main/javascript/features/core.config.base/feature.xml View 2 chunks +2 lines, -0 lines 0 comments Download
features/src/main/javascript/features/core.config/feature.xml View 1 2 chunks +2 lines, -13 lines 0 comments Download
features/src/main/javascript/features/core.io/feature.xml View 1 1 chunk +13 lines, -6 lines 0 comments Download
features/src/main/javascript/features/core.legacy/feature.xml View 1 chunk +28 lines, -0 lines 0 comments Download
features/src/main/javascript/features/core.legacy/legacy.js View 1 chunk +0 lines, -3 lines 0 comments Download
features/src/main/javascript/features/core.log/feature.xml View 1 1 chunk +2 lines, -16 lines 0 comments Download
features/src/main/javascript/features/core.prefs/feature.xml View 1 chunk +14 lines, -0 lines 0 comments Download
features/src/main/javascript/features/core.util.urlparams/feature.xml View 1 1 chunk +2 lines, -8 lines 0 comments Download
features/src/main/javascript/features/core.util/feature.xml View 1 2 chunks +2 lines, -21 lines 0 comments Download
features/src/main/javascript/features/dynamic-height.height/feature.xml View 1 1 chunk +6 lines, -10 lines 0 comments Download
features/src/main/javascript/features/dynamic-height.height/taming.js View 1 chunk +28 lines, -0 lines 0 comments Download
features/src/main/javascript/features/dynamic-height.util/feature.xml View 1 chunk +5 lines, -2 lines 0 comments Download
features/src/main/javascript/features/dynamic-height.util/taming.js View 1 chunk +28 lines, -0 lines 0 comments Download
features/src/main/javascript/features/dynamic-height/taming.js View 1 chunk +1 line, -3 lines 0 comments Download
features/src/main/javascript/features/exportjs/feature.xml View 1 chunk +2 lines, -0 lines 0 comments Download
features/src/main/javascript/features/flash/feature.xml View 1 1 chunk +10 lines, -6 lines 0 comments Download
features/src/main/javascript/features/globals/feature.xml View 1 1 chunk +2 lines, -10 lines 0 comments Download
features/src/main/javascript/features/globals/globals.js View 1 chunk +3 lines, -3 lines 0 comments Download
features/src/main/javascript/features/jsondom/feature.xml View 1 chunk +3 lines, -0 lines 0 comments Download
features/src/main/javascript/features/minimessage/feature.xml View 1 chunk +8 lines, -0 lines 0 comments Download
features/src/main/javascript/features/oauthpopup/feature.xml View 1 chunk +5 lines, -0 lines 0 comments Download
features/src/main/javascript/features/rpc/feature.xml View 1 2 chunks +2 lines, -31 lines 0 comments Download
features/src/main/javascript/features/setprefs/feature.xml View 1 chunk +5 lines, -0 lines 0 comments Download
features/src/main/javascript/features/settitle/feature.xml View 1 chunk +4 lines, -0 lines 0 comments Download
features/src/main/javascript/features/shindig.auth/feature.xml View 1 1 chunk +7 lines, -6 lines 0 comments Download
features/src/main/javascript/features/shindig.random/feature.xml View 1 1 chunk +2 lines, -8 lines 0 comments Download
features/src/main/javascript/features/shindig.sha1/feature.xml View 1 2 chunks +2 lines, -11 lines 0 comments Download
features/src/main/javascript/features/shindig.uri/feature.xml View 1 1 chunk +21 lines, -5 lines 0 comments Download
features/src/main/javascript/features/shindig.xhrwrapper/feature.xml View 1 chunk +10 lines, -0 lines 0 comments Download
features/src/main/javascript/features/skins/feature.xml View 1 chunk +5 lines, -0 lines 0 comments Download
features/src/main/javascript/features/swfobject/feature.xml View 1 chunk +17 lines, -0 lines 0 comments Download
features/src/main/javascript/features/tabs/feature.xml View 1 chunk +20 lines, -0 lines 0 comments Download
features/src/main/javascript/features/views/feature.xml View 1 chunk +13 lines, -0 lines 0 comments Download
features/src/main/javascript/features/xmlutil/feature.xml View 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 6
mhermanto
15 years ago (2011-03-02 00:54:03 UTC) #1
johnfargo
Excellent to see! http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/features/cloo/feature.xml File features/src/main/javascript/features/cloo/feature.xml (right): http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/features/cloo/feature.xml#newcode29 features/src/main/javascript/features/cloo/feature.xml:29: <container> might as well take the ...
15 years ago (2011-03-02 01:05:15 UTC) #2
mhermanto
Update patch
15 years ago (2011-03-02 01:05:19 UTC) #3
mhermanto
Update patch
15 years ago (2011-03-02 01:16:08 UTC) #4
mhermanto
On Tue, Mar 1, 2011 at 5:05 PM, <johnfargo@gmail.com> wrote: > Excellent to see! > ...
15 years ago (2011-03-02 01:16:12 UTC) #5
johnfargo
15 years ago (2011-03-02 01:18:43 UTC) #6 
LGTM++

Yep, I committed my <all> patch around when Bastian committed his for PHP.

On 2011/03/02 01:16:12, mhermanto wrote:
> On Tue, Mar 1, 2011 at 5:05 PM, <mailto:johnfargo@gmail.com> wrote:
> 
> > Excellent to see!
> >
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > File features/src/main/javascript/features/cloo/feature.xml (right):
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/cloo/feature.xml:29: <container>
> > might as well take the opportunity to remove <container> and change
> > <gadget> to <all> :P
> >
> 
> Yes, didn't realize your CL has been checked in. I'll give all the same
> treatment, when applicable.
> 
> 
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > File features/src/main/javascript/features/core.io/feature.xml (right):
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/core.io/feature.xml:52:
> > <container>
> > same (consolidation opportunity)
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > File
> >
> > features/src/main/javascript/features/dynamic-height.height/feature.xml
> > (right):
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/dynamic-height.height/feature.xml:31:
> > <container>
> > another consolidation opportunity (though arguably shouldn't be :))
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > File
> >
> > features/src/main/javascript/features/dynamic-height.util/feature.xml
> > (right):
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/dynamic-height.util/feature.xml:32:
> > <container>
> > same
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/dynamic-height.util/feature.xml:35:
> > <apis>
> > s/api/apis/ (not an issue when consolidated)
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > File features/src/main/javascript/features/flash/feature.xml (right):
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/flash/feature.xml:36: <container>
> > consolidate
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > File features/src/main/javascript/features/globals/globals.js (right):
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/globals/globals.js:23: var gadgets
> > = window['gadgets'] || {};
> > why is this necessary? for debug JS loaded after prod?
> >
> 
> This doesn't really matter for us, since we override globals (with quotes,
> like this). I did this to 1) give Shindig the same treatment, and 2) with
> variable renaming, the second "gadgets" will be renamed to something else,
> which will possibly refer to something else.
> 
> 
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > File features/src/main/javascript/features/shindig.auth/feature.xml
> > (right):
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/shindig.auth/feature.xml:31:
> > <exports type="js"></exports>
> > ?
> >
> 
> Fixed.
> 
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > File features/src/main/javascript/features/shindig.uri.ext/feature.xml
> > (right):
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/shindig.uri.ext/feature.xml:29:
> > <exports type="js"></exports>
> > needs symbols
> >
> 
> Yes. Done.
> 
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/shindig.uri.ext/feature.xml:32:
> > <container>
> > consolidate
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/shindig.uri.ext/feature.xml:35:
> > <exports type="js"></exports>
> > needs symbols
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > File features/src/main/javascript/features/shindig.uri/feature.xml
> > (right):
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/shindig.uri/feature.xml:28:
> > <exports type="js"></exports>
> > empty
> >
> >
> >
>
http://codereview.appspot.com/4254051/diff/1/features/src/main/javascript/fea...
> > features/src/main/javascript/features/shindig.uri/feature.xml:34:
> > <exports type="js"></exports>
> > empty
> >
> 
> Yes, yes, I meant to send this CL for my own code review first, but you
> jumped on it very quickly. :)
> 
> >
> >
> 
> > http://codereview.appspot.com/4254051/
> >
> 
> 
> Patch updated.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b