On Mon, Aug 23, 2010 at 9:48 AM, P T Withington <[email protected]>wrote:
> This is great stuff: > > 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. > > 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`? > > 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. > > 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. > Yeah I was waffling about switching to use 'runtime' everywhere in the server. I'll update all the responders code in the server to always ask for the "runtime" option, and have getLzOption look in the query args for "lzr" if it doesn't see a new-style lzoptions string. > > 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.] > > Questions: > > 1) swf/LZDebug looks specifically for the new options. It should use the > common getLzOptions call. > > 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? > > 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? > > Nits: > > 1) `substring(i,i+1)` is more conventionally written `charAt(i)` > > 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 2010-08-20, at 18:34, Henry Minsky wrote: > >
