On Tue, Aug 24, 2010 at 10:04 PM, Max Carlson <[email protected]> wrote:
> + 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?
>
I guess that's a bug, I'll have it scrub up the old style args from the
query.
>
> + 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/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%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>
>> <
>> 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
>
--
Henry Minsky
Software Architect
[email protected]
