On Mon, Aug 23, 2010 at 9:48 AM, P T Withington <[email protected]>
wrote:
Issues:
1) I like the idea of a unified place to parse lzoptions (getLzOption)
as you have done in the server, and I think that works just right: if there
is an lzoption parameter, then you only accept new-style options; otherwise
you fall back to the old style. But I don't think it is right for
getInitArg on the client to be looking into lzoptions. getInitArg ought to
be _only_ for user code, for user parameters. I think you should add
getLzOption to the client code and find all the places in the LFC that are
looking for options and update them to use getLzOption.
I made embed.js put the parsed lzoptions into a separate slot called
"options", and am storing that persistently along with
how initargs is stored now.
I made a client method called getLzOption, which calls out to lz.embed and
looks in the options list first, and if it doesn't find
an entry, it then looks in initargs for back compatibility.
1a) If the client uses `lzr` it should get updated to use `runtime` and
you should include the translation as you do in the server when you have to
fall back. Are there other names that should be cleaned up? Like
`lzbacktrace` could become just `backtrace`?
LzHTTPUtils.java (server), LzBrowser.lzs (client): I made a mapping
table for new->old names for the server and client implementations of
getLzOption; so for example 'runtime' is looked up, and if that fails,
it looks for 'lzr' in the query. Same for "backtrace"->"lzbacktrace", and
"proxied"->"lzproxied".
2) I _disagree_ with Max's suggestion of using the shorter name `lz`,
because that is more likely to collide with a user program query arg. I
think we should use a longer name to reduce the possibility of a collision.
I left the name as "lzoptions".
3) In the server code, you should only look for `runtime` in the new
options and only look for `lzr` in the old params. I think the code is a
little confused there.
Updated LZHTTPUtils.getLzOption to do this.
4) In embednew, ca. line 605, where it is deciding what lps vars to copy
to the query string, this is the whole point of lzoptions. It should just
copy all the lzoptions into the query string. That way, you can add new
options and never have to edit this code again. [I have to admit, I can't
really understand the difference between flashvars, query, options, and
initargs, but lzoptions should simplify this code, not make it more
complex.]
The lzoptions are parsed out into a "options" table, and stored on lz.embed,
so the client can access them at runtime.
Questions:
1) swf/LZDebug looks specifically for the new options. It should use
the common getLzOptions call.
I had to special case this because after much grief I found that lz.embed is
not reliably initialized yet when that
code in LzDebug runs (too early in app startup I guess). It's just looking
for the damned lzconsoledebug flag, so
I left that out of lzoptions, and it's still a plain old discrete query arg.
2) Is it really worthwhile to check flex version? Maybe there should
just be a simple passthrough --additionalFlexCompilerArguments or
something. Then we don't have to adjust the compiler each time a customer
comes up with a need to tweak the flex compiler?
That's a good idea, but I think I'll file an improvement for it, since it
will take a little more design.
3) I'd really like it if we could "improve" the options names. I know
this is more work, but rather than just move `lzr` from the query params to
lzoptions, I'd like to also rename it to be more mnemonic, say `runtime`.
Maybe we could just have a mapping table from new to old in the getLzOption
method that knows what to look for in the query arg if there is no lzoptions
specified?
Done, see 1a) above.
Nits:
1) `substring(i,i+1)` is more conventionally written `charAt(i)`
fixed, I'm now using regexps to split the string, after Max pointed out that
Andre's implementation works in swf8.
2) I like how you shared the flexversion string between sc/Compiler and
compiler/CompilationEnvironment. I guess we should do that for all those
shared flags.
On Tue, Aug 24, 2010 at 3:55 PM, P T Withington <[email protected]>wrote:
> Can you elaborate on how you have responded to Max and my previous review
> comments on this change?
>
> I find Max's technique, where he quotes the review comments and explains
> point-by-point how he addressed (or why he is not addressing) each one as
> part of the updated change request to be really helpful. It makes it so the
> reviewer does not have to play "hunt the wumpus" to figure out if his
> comments have been addressed or not.
>
> Thanks!
>
> On 2010-08-24, at 15:31, Henry Minsky wrote:
>
> > Max, is it possible for you test this change with Webtop, to see if it
> still operates? The query arg handling should be back compatible
> > but I'm guessing there might be some configuration I didn't cover in
> testing
> >
> > (If you're too busy to fire up Webtop, I can try to do it on my machine..
> )
> >
> >
> >
> > Change hqm-20100824-ocl by [email protected] on 2010-08-24 11:29:17
> EDT
> > in /Users/hqm/openlaszlo/trunk-clean
> > for http://svn.openlaszlo.org/openlaszlo/trunk
> >
> > Summary: add unified 'lzoptions' support
> >
> > New Features:
> >
> > Bugs Fixed: LPP-3479
> >
> > Technical Reviewer: ptw
> > QA Reviewer: max
> > Doc Reviewer: (pending)
> >
> > Documentation:
> >
> > Release Notes:
> >
> > Overview:
> >
> > + Added "flexversion" compiler option to select between flash 10.0 and
> 10.1
> >
> > + Added support for passing compiler options in unified "lzoptions"
> string
> >
> > + modified "lzc" command line interpreter to accept 'flexversion' arg
> >
> > Details:
> >
> >
> > WEB-INF/lps/lfc/kernel/swf/LzBrowserKernel.lzs
> > WEB-INF/lps/lfc/kernel/swf9/LzBrowserKernel.lzs
> > added getLzOption method, which looks at the
> lz.embed.applications.options field
> >
> > WEB-INF/lps/lfc/kernel/LzKernelUtils.lzs
> > Added routine to parse lzoptions string
> >
> > WEB-INF/lps/config/lps.properties
> > add default value for flex version = 10.0
> >
> > WEB-INF/lps/server/src/org/openlaszlo/utils/LZHttpUtils.java
> > for back compatibility treat "runtime" as a synonym for "lzr" in
> lzoptions
> >
> > WEB-INF/lps/server/src/org/openlaszlo/sc/SWF9External.java
> > pass "flex_version" option to flex compiler (targets 10.0 vs 10.1)
> >
> > WEB-INF/lps/server/src/org/openlaszlo/sc/Compiler.java
> > Add "flexversion" constant
> >
> > WEB-INF/lps/server/src/org/openlaszlo/server/LPS.java
> > add method to return default value for flex version
> >
> >
> WEB-INF/lps/server/src/org/openlaszlo/servlets/responders/ResponderCompile.java
> > use accessor LZHttpUtils.getLzOption to get arg values instead of
> request.getParameter
> >
> >
> WEB-INF/lps/server/src/org/openlaszlo/servlets/responders/ResponderAPP_CONSOLE.java
> > formats the lzoptions values into the XML data format used by the
> dev-console.lzx app
> >
> >
> WEB-INF/lps/server/src/org/openlaszlo/compiler/CompilationEnvironment.java
> > WEB-INF/lps/server/src/org/openlaszlo/compiler/Compiler.java
> > WEB-INF/lps/server/src/org/openlaszlo/compiler/Main.java
> > WEB-INF/lps/server/src/org/openlaszlo/compiler/SWF9Writer.java
> > add support for 'flex version' arg
> >
> >
> > lps/includes/source/embednew.js
> > lps/includes/source/build.xml
> > add parser for lzoptions string
> > store parsed options in "options" slot on application, similar to
> initargs
> >
> > lps/admin/dev-console.lzx
> > generate a URL string for the app being loaded that encodes args
> into lzoptions format
> >
> > + I did not modify "lzc" command line processor to accept the new
> lzoptions
> > format yet. It still requires the old style discrete flags.
> >
> > + solo deploy wizards still use old style compiler options in query
> > string when pulling data from the server, I will create a JIRA task to
> > update them
> >
> > Tests:
> > smokecheck in swf10
> > test/lfc/data in swf10
> >
> > TLF bidi tests
> > test/tlf/text-test.lzx?lzr=swf10
> > test/tlf/focus-input.lzx?lzr=swf10
> >
> > fetching test/hello.lzx using lzoptions
> >
> http://127.0.0.1:8080/trunk-clean/test/hello.lzx?lzoptions=lzr(swf10),flexversion(10.1)<http://127.0.0.1:8080/trunk-clean/test/hello.lzx?lzoptions=lzr%28swf10%29,flexversion%2810.1%29>
> >
> http://127.0.0.1:8080/trunk3/test/hello.lzx?lzoptions=runtime(swf8),debug<http://127.0.0.1:8080/trunk3/test/hello.lzx?lzoptions=runtime%28swf8%29,debug>
> >
> http://127.0.0.1:8080/trunk3/test/hello.lzx?lzoptions=runtime(dhtml),debug,lzbacktrace<http://127.0.0.1:8080/trunk3/test/hello.lzx?lzoptions=runtime%28dhtml%29,debug,lzbacktrace>
> >
> > verify that dev-console displays correct button and checkbox settings
> for the selected options
> > (e.g., runtime and debug/backtrace flags, remote console debug)
> >
> > confirm by inspection of as files in compiler apache temp build
> > directory 'lzswf9', that flex compiler is getting passed the arg
> "-target-player=10.1":
> >
> /openlaszlo/lib/apache-tomcat-5.5.29/temp/lzswf9/Users/hqm/openlaszlo/trunk-clean/test/build/hello/build.sh
> >
> >
> > test/data/jdata.lzx:
> > modified to test proxied flag, it displays the value of canvas proxied
> flag
> > test with urls
> > test/data/jdata.lzx?lzoptions=proxied(false) // new lzoptions arg
> > test/data/jdata.lzx?lzoptions=proxied(true)
> > test/data/jdata.lzx?lzproxied=false // old style query arg
> > test/data/jdata.lzx?lzproxied=true
> >
> >
> > Files:
> >
> > M test/data/jdata.lzx
> > M WEB-INF/lps/lfc/kernel/swf/LzBrowserKernel.lzs
> > M WEB-INF/lps/lfc/kernel/dhtml/LzBrowserKernel.lzs
> > M WEB-INF/lps/lfc/kernel/swf9/LzBrowserKernel.lzs
> > M WEB-INF/lps/lfc/kernel/LzKernelUtils.lzs
> > M WEB-INF/lps/lfc/services/LzBrowser.lzs
> > M WEB-INF/lps/lfc/debugger/platform/swf/LzDebug.as
> > M WEB-INF/lps/lfc/debugger/platform/swf9/LzFlashRemote.as
> > M WEB-INF/lps/config/lps.properties
> > M WEB-INF/lps/server/src/org/openlaszlo/cm/CompilationManager.java
> > M WEB-INF/lps/server/src/org/openlaszlo/utils/LZHttpUtils.java
> > M WEB-INF/lps/server/src/org/openlaszlo/cache/DataCache.java
> > M WEB-INF/lps/server/src/org/openlaszlo/sc/SWF9External.java
> > M WEB-INF/lps/server/src/org/openlaszlo/sc/Compiler.java
> > M WEB-INF/lps/server/src/org/openlaszlo/server/LPS.java
> > M
> WEB-INF/lps/server/src/org/openlaszlo/servlets/responders/ResponderCompile.java
> > M
> WEB-INF/lps/server/src/org/openlaszlo/servlets/responders/ResponderAPP_CONSOLE.java
> > M
> WEB-INF/lps/server/src/org/openlaszlo/servlets/responders/ResponderOBJECT.java
> > M
> WEB-INF/lps/server/src/org/openlaszlo/servlets/responders/TemplateResponder.java
> > M
> WEB-INF/lps/server/src/org/openlaszlo/servlets/responders/ResponderLFC.java
> > M
> WEB-INF/lps/server/src/org/openlaszlo/compiler/CompilationEnvironment.java
> > M WEB-INF/lps/server/src/org/openlaszlo/compiler/Compiler.java
> > M WEB-INF/lps/server/src/org/openlaszlo/compiler/Main.java
> > M WEB-INF/lps/server/src/org/openlaszlo/compiler/SWF9Writer.java
> > M lps/includes/source/embednew.js
> > M lps/admin/dev-console.lzx.swf
> > M lps/admin/dev-console.lzx
> > M lps/admin/lps/includes/lfc/LFCdhtml.js
> > M lps/admin/solo-deploy.jsp
> > M lps/admin/dev-console.lzx.js
> > M lps/admin/solo-dhtml-deploy.jsp
> >
> > Changeset:
> http://svn.openlaszlo.org/openlaszlo/patches/hqm-20100824-ocl.tar
>
>
--
Henry Minsky
Software Architect
[email protected]
