+ I had to explicitly recompile the dev console. Please do this as part
of your changeset.
+ When I launch an app with the old-style query args, e.g.
examples/extensions/rte.lzx?lzr=dhtml, clicking 'compile' in the dev
console still appends the new style URL to the old style:
examples/extensions/rte.lzx?lzr=dhtml&lzoptions=runtime(dhtml),lzusemastersprite(false)
- is that intended?
+ There's a stray Debug statement in LzDebug.as
+ Do we need the changes to LzFlashRemote.as?
+ There are still two copies of the code to parse options - one in
lzembed.js and one in LzKernelUtils.lzs. Can you build a shared
pseudo-class, e.g:
lz.embed.parseOptions = function() {...}
and include that in both places? Maybe you're right, and that's just
too ugly but I fear the method will rot otherwise. At least add a
comment in both places reminding folks to update the other copy...
+ getLzOption() in swf/LzBrowserKernel.lzs has a stray console.log()
Otherwise, it looks good!
On 8/24/10 4:00 PM, Henry Minsky wrote:
On Mon, Aug 23, 2010 at 9:48 AM, P T Withington <[email protected]
<mailto:[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]
<mailto:[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] <mailto:[email protected]>
--
Regards,
Max Carlson
OpenLaszlo.org
