Issue 4309044: Separate out base osapi from its transports (Closed)
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update patch #Patch Set 3 : Add just batch.js #
MessagesTotal messages: 7
Good start, next I'd imagine we could provide osapi.gadgetsrpc and osapi.jsonrpc providing exclusively one or the other transport, for situations in which ppl know that they'll use only one (right now it's config-driven but the JS is included for both at all times). Few small comments below. http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... File features/src/main/javascript/features/osapi.base/feature.xml (right): http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... features/src/main/javascript/features/osapi.base/feature.xml:23: <dependency>core.log</dependency> if I recall correctly, core.log isn't used http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... features/src/main/javascript/features/osapi.base/feature.xml:26: <script src="batch.js"></script> file missing in this patch
Sign in to reply to this message.
Update patch
Sign in to reply to this message.
On Wed, Mar 23, 2011 at 7:43 PM, <johnfargo@gmail.com> wrote: > Good start, next I'd imagine we could provide osapi.gadgetsrpc and > osapi.jsonrpc providing exclusively one or the other transport, for > situations in which ppl know that they'll use only one (right now it's > config-driven but the JS is included for both at all times). > > Yes, my thinking was along the same line. > Few small comments below. > > > > http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... > File features/src/main/javascript/features/osapi.base/feature.xml > (right): > > > http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... > features/src/main/javascript/features/osapi.base/feature.xml:23: > <dependency>core.log</dependency> > if I recall correctly, core.log isn't used > It used in osapi.js:77 > > > http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... > features/src/main/javascript/features/osapi.base/feature.xml:26: <script > src="batch.js"></script> > file missing in this patch > > Repatched. > > http://codereview.appspot.com/4309044/ >
Sign in to reply to this message.
On Thu, Mar 24, 2011 at 9:34 AM, Michael Hermanto <mhermanto@gmail.com>wrote: > > > On Wed, Mar 23, 2011 at 7:43 PM, <johnfargo@gmail.com> wrote: > >> Good start, next I'd imagine we could provide osapi.gadgetsrpc and >> osapi.jsonrpc providing exclusively one or the other transport, for >> situations in which ppl know that they'll use only one (right now it's >> config-driven but the JS is included for both at all times). >> >> Yes, my thinking was along the same line. > > >> Few small comments below. >> >> >> >> http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... >> File features/src/main/javascript/features/osapi.base/feature.xml >> (right): >> >> >> http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... >> features/src/main/javascript/features/osapi.base/feature.xml:23: >> <dependency>core.log</dependency> >> if I recall correctly, core.log isn't used >> > > It used in osapi.js:77 > > >> >> >> http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... >> features/src/main/javascript/features/osapi.base/feature.xml:26: <script >> src="batch.js"></script> >> file missing in this patch >> >> Repatched. > Well, it turns out that codereview doesn't play friendly. It's now there in a separate patch, containing just this file. > >> http://codereview.appspot.com/4309044/ >> > >
Sign in to reply to this message.
LGTM Codereview still looks wonky, but so long as osapi.base/batch.js is added things look fine. On 2011/03/24 16:39:13, mhermanto wrote: > On Thu, Mar 24, 2011 at 9:34 AM, Michael Hermanto <mhermanto@gmail.com>wrote: > > > > > > > On Wed, Mar 23, 2011 at 7:43 PM, <mailto:johnfargo@gmail.com> wrote: > > > >> Good start, next I'd imagine we could provide osapi.gadgetsrpc and > >> osapi.jsonrpc providing exclusively one or the other transport, for > >> situations in which ppl know that they'll use only one (right now it's > >> config-driven but the JS is included for both at all times). > >> > >> Yes, my thinking was along the same line. > > > > > >> Few small comments below. > >> > >> > >> > >> > http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... > >> File features/src/main/javascript/features/osapi.base/feature.xml > >> (right): > >> > >> > >> > http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... > >> features/src/main/javascript/features/osapi.base/feature.xml:23: > >> <dependency>core.log</dependency> > >> if I recall correctly, core.log isn't used > >> > > > > It used in osapi.js:77 > > > > > >> > >> > >> > http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... > >> features/src/main/javascript/features/osapi.base/feature.xml:26: <script > >> src="batch.js"></script> > >> file missing in this patch > >> > >> Repatched. > > > > Well, it turns out that codereview doesn't play friendly. It's now there in > a separate patch, containing just this file. > > > > >> http://codereview.appspot.com/4309044/ > >> > > > >
Sign in to reply to this message.
On 2011/03/24 17:02:20, johnfargo wrote: > LGTM > > Codereview still looks wonky, but so long as osapi.base/batch.js is added things > look fine. > Submitted. svn correctly commits the file. > On 2011/03/24 16:39:13, mhermanto wrote: > > On Thu, Mar 24, 2011 at 9:34 AM, Michael Hermanto <mhermanto@gmail.com>wrote: > > > > > > > > > > > On Wed, Mar 23, 2011 at 7:43 PM, <mailto:johnfargo@gmail.com> wrote: > > > > > >> Good start, next I'd imagine we could provide osapi.gadgetsrpc and > > >> osapi.jsonrpc providing exclusively one or the other transport, for > > >> situations in which ppl know that they'll use only one (right now it's > > >> config-driven but the JS is included for both at all times). > > >> > > >> Yes, my thinking was along the same line. > > > > > > > > >> Few small comments below. > > >> > > >> > > >> > > >> > > > http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... > > >> File features/src/main/javascript/features/osapi.base/feature.xml > > >> (right): > > >> > > >> > > >> > > > http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... > > >> features/src/main/javascript/features/osapi.base/feature.xml:23: > > >> <dependency>core.log</dependency> > > >> if I recall correctly, core.log isn't used > > >> > > > > > > It used in osapi.js:77 > > > > > > > > >> > > >> > > >> > > > http://codereview.appspot.com/4309044/diff/1/features/src/main/javascript/fea... > > >> features/src/main/javascript/features/osapi.base/feature.xml:26: <script > > >> src="batch.js"></script> > > >> file missing in this patch > > >> > > >> Repatched. > > > > > > > Well, it turns out that codereview doesn't play friendly. It's now there in > > a separate patch, containing just this file. > > > > > > > >> http://codereview.appspot.com/4309044/ > > >> > > > > > >
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||
