Agree this sort of thing should live in Jira as sub tasks for ppl to test. (And yup the CI should help in the future.)
On Wed, Feb 20, 2013 at 1:32 PM, Michal Mocny <mmo...@chromium.org> wrote: > Agree with where this conversation is going. I do think a "call to action" > for review is important (esp given the number of platforms) and perhaps > JIRA isn't the absolute worst way to do it ;) > > Another question: Are there plans to expand CI to other platforms? Support > requesting tests for a remote feature branch? This may remove some strain > on testers for the less popular platforms. > > > On Wed, Feb 20, 2013 at 4:29 PM, Filip Maj <f...@adobe.com> wrote: > >> Good call Mike. Moving this sort of stuff to JIRA (and bringing back to >> list when necessary) makes a lot of sense. >> >> On 2/20/13 1:27 PM, "Michael Brooks" <mich...@michaelbrooks.ca> wrote: >> >> >> >> >> If the "can you guys test my changes" answer is "no", then it'd be >> >>great to >> >> hear a "no" instead of 8 days of silence. That said, I think we'd be >> >>able >> >> to move faster if we just took some time to review/test each others' >> >> changes when necessary. We do this when processing pull requests from >> >> external devs, so why not do this for each other? >> > >> > >> >Totally valid and I support the suggestion of code reviews (as long as its >> >not a hinderance to commit ease). >> > >> >It would be nice if we can bring conversations closer to the code (via >> >GitHub pull requests and code comments) instead of blasting emails at each >> >other. I assume using GitHub more is against the Apache Way? We could >> >bring >> >conversations more into to JIRA. For this particular example, Andrew could >> >create a JIRA issue and assign subtasks for the major platforms to test to >> >branch and report back any issues. >> > >> >Michael >> > >> >On Wed, Feb 20, 2013 at 1:07 PM, Andrew Grieve <agri...@chromium.org> >> >wrote: >> > >> >> I agree with your sentiments, but I think it's impractical in practice. >> >>We >> >> have ~11 platforms, and any change to common js affects them all. >> >> >> >> In this case, I would need to learn how to build & run on webos, tizen, >> >>wp7 >> >> and windowphone, as well as buying the required hardware to do so. A >> >>tall >> >> order for some refactoring changes. >> >> >> >> If the "can you guys test my changes" answer is "no", then it'd be >> >>great to >> >> hear a "no" instead of 8 days of silence. That said, I think we'd be >> >>able >> >> to move faster if we just took some time to review/test each others' >> >> changes when necessary. We do this when processing pull requests from >> >> external devs, so why not do this for each other? >> >> >> >> >> >> >> >> >> >> >> >> >> >> On Wed, Feb 20, 2013 at 3:47 PM, Michael Brooks >> >><mich...@michaelbrooks.ca >> >> >wrote: >> >> >> >> > Agreed. Please take responsibility and test your code on devices >> >>(ideally >> >> > not simulators). >> >> > >> >> > If your change impacts multiple platforms, have it tested on those >> >>before >> >> > pushing to master. >> >> > >> >> > On Wed, Feb 20, 2013 at 12:42 PM, Filip Maj <f...@adobe.com> wrote: >> >> > >> >> > > Andrew did you test it on a device? Don't think "hey guys can you >> >>test >> >> my >> >> > > changes" is a sustainable approach >> >> > > >> >> > > On 2/20/13 12:19 PM, "Andrew Grieve" <agri...@chromium.org> wrote: >> >> > > >> >> > > >Okay, I've interpreted the silence as a "just go ahead and merge it >> >> and >> >> > > >people will complain if it's broken". >> >> > > > >> >> > > >Seeing as we've now cut the next branch, I figured it was a good >> >>time >> >> to >> >> > > >merge these remaining changes in. So, I did. Please let me know if >> >> > symbols >> >> > > >aren't working. >> >> > > > >> >> > > > >> >> > > >On Tue, Feb 12, 2013 at 2:39 PM, Andrew Grieve >> >><agri...@chromium.org> >> >> > > >wrote: >> >> > > > >> >> > > >> I've just merged in most of the changes for this (CB-2227). >> >>Again, >> >> the >> >> > > >> goal here was to move all plugin-specific logic out of common >> >>files. >> >> > > >>It's >> >> > > >> not the end-game solution, but a step in the right direction. >> >> > > >> >> >> > > >> There are still some changes left on the symbolmapping branch >> >>that >> >> > > >>affect >> >> > > >> windows & webos. If there's someone who knows how, it would be >> >>great >> >> > if >> >> > > >>you >> >> > > >> could try merging in the branch and ensure mobile-spec is still >> >> > working >> >> > > >> with the changes. If so, these changes can also be merged into >> >> master. >> >> > > >> >> >> > > >> >> >> > > >> >> >> > > >> On Tue, Jan 29, 2013 at 3:38 PM, Andrew Grieve >> >> > > >><agri...@chromium.org>wrote: >> >> > > >> >> >> > > >>> This is now finished in the branch. There is now *no* plugin >> >>logic >> >> > left >> >> > > >>> in common.js, nor in any platform.js files. >> >> > > >>> >> >> > > >>> >> >> > > >>> >> >> > > >>> >> >> > > >> >> >> https://git-wip-us.apache.org/repos/asf?p=cordova-js.git;a=shortlog;h=re >> >> > > >>>fs/heads/symbolmapping >> >> > > >>> >> >> > > >>> There is one exception, and that's things like the "app" plugin, >> >> > where >> >> > > >>> it's not really a plugin that a platform can live without. >> >> > > >>> >> >> > > >>> Changes of interest: >> >> > > >>> 1. In tests, I've added helpers for stubbing out modules & for >> >> > stubbing >> >> > > >>> out properties. I used this to be able to undo symbol mapping >> >> within >> >> > > >>>tests. >> >> > > >>> >> >> > > >>> var propertyreplacer = require('cordova/propertyreplacer'); >> >> > > >>> propertyreplacer.stub(platform, 'id', 'test'); >> >> > > >>> >> >> > > >>> var modulereplacer = require('cordova/modulereplacer'); >> >> > > >>> modulereplacer.replace('cordova/platform', {id:'test', >> >> > > >>> initialize:createSpy()}); >> >> > > >>> >> >> > > >>> 2. Loading plugins by name (aka, looping through all defined >> >> modules >> >> > > >>>and >> >> > > >>> loading the ones that have names that match a pattern). >> >> > > >>> A) "symbols" Modules that have the name "symbols" are loaded to >> >> > define >> >> > > >>> their plugin's module->JS symbol mappings >> >> (merges/clobbers/defaults). >> >> > > >>>On >> >> > > >>> Blackberry, sub-platform symbol files are called "bbsymbols". >> >> > > >>> B) "plugininit" Modules that have the name "plugininit" are >> >>loaded >> >> to >> >> > > >>> perform any custom start-up logic. >> >> > > >>> C) "*Proxy" On Windows8, modules that end with "Proxy" are >> >>loaded >> >> on >> >> > > >>> start-up. >> >> > > >>> >> >> > > >>> I don't love the looping-through-module-names approach, but >> >>thought >> >> > it >> >> > > >>> was a good initial solution while we talk about better ideas. >> >>To do >> >> > > >>>this, I >> >> > > >>> had to make the moduleMap exported, which it wasn't before. >> >> Certainly >> >> > > >>> interested to hear if this is a really bad idea, and what >> >> > alternatives >> >> > > >>>we >> >> > > >>> could use going forward. >> >> > > >>> >> >> > > >>> >> >> > > >>> >> >> > > >>> >> >> > > >>> >> >> > > >>> >> >> > > >>> On Thu, Jan 17, 2013 at 12:45 PM, Andrew Grieve >> >> > > >>><agri...@chromium.org>wrote: >> >> > > >>> >> >> > > >>>> Pushed up the change with the File plugin being registered in >> >>this >> >> > new >> >> > > >>>> way. Please let me know if you have concerns about it, since >> >>the >> >> > next >> >> > > >>>>step >> >> > > >>>> is moving over other plugin APIs, which is boring work :P. >> >> > > >>>> >> >> > > >>>> Also, let's move any discussion into the JIRA issue: >> >> > > >>>> https://issues.apache.org/jira/browse/CB-2227 >> >> > > >>>> >> >> > > >>>> >> >> > > >>>> >> >> > > >>>> On Wed, Jan 16, 2013 at 4:35 PM, Andrew Grieve >> >> > > >>>><agri...@chromium.org>wrote: >> >> > > >>>> >> >> > > >>>>> Branch started! >> >> > > >>>>> >> >> > > >>>>> I've completed steps 1 & 2. >> >> > > >>>>> >> >> > > >>>>> >> >> > > >>>>> >> >> > > >>>>> >> >> > > >> >>https://git-wip-us.apache.org/repos/asf?p=cordova-js.git;a=shortlog;h= >> >> > > >>>>>refs/heads/symbolmapping >> >> > > >>>>> >> >> > > >>>>> >> >> > > >>>>> On Wed, Jan 16, 2013 at 1:39 PM, Filip Maj <f...@adobe.com> >> >> wrote: >> >> > > >>>>> >> >> > > >>>>>> This all seems reasonable. Shall we start a branch? >> >> > > >>>>>> >> >> > > >>>>>> On 1/15/13 2:47 PM, "Andrew Grieve" <agri...@google.com> >> >>wrote: >> >> > > >>>>>> >> >> > > >>>>>> >Sorry to dump another large email on the list, but I'm >> >>hoping >> >> > this >> >> > > >>>>>> one is >> >> > > >>>>>> >at least less controversial :). I wrote up a plan for moving >> >> > > >>>>>> >module->symbol >> >> > > >>>>>> >mapping out of common.js & platform.js and into individual >> >> > plugins. >> >> > > >>>>>> > >> >> > > >>>>>> >If you have feedback/comments, let me know. >> >> > > >>>>>> > >> >> > > >>>>>> >* Goals: >> >> > > >>>>>> > >> >> > > >>>>>> > - Change from listing module->symbol mapping within >> >> common.js >> >> > & >> >> > > >>>>>> > platform.js, to listing this within the plugins >> >>themselves. >> >> > > >>>>>> > - Support apps that don't want us to clobber global >> >>symbols. >> >> > > >>>>>> > - aka, allow module->symbol mapping to be turned off >> >> > > >>>>>> > - Allow retrieval of clobbered globals >> >> > > >>>>>> > - Currently modules save it themselves when they are >> >> loaded >> >> > > >>>>>> > - This won't work (reliably) for saving references to >> >> > globals >> >> > > >>>>>> > overridden by other modules >> >> > > >>>>>> > - This gets in the way of the idea of lazy-loading >> >> modules >> >> > > >>>>>>via >> >> > > >>>>>> >getters >> >> > > >>>>>> > - Support the use of other module loaders >> >> > > >>>>>> > - So... don't do crazy things at require() time. >> >> > > >>>>>> > >> >> > > >>>>>> > >> >> > > >>>>>> >Requirements: >> >> > > >>>>>> > >> >> > > >>>>>> > - Plugins must be able to declare dependencies >> >> > > >>>>>> > - Plugins must be able to delay onDeviceReady() >> >> > > >>>>>> > - Plugins must be able to run code to initialize >> >> > > >>>>>> > >> >> > > >>>>>> > >> >> > > >>>>>> >Implementation modulemapper.js: >> >> > > >>>>>> > >> >> > > >>>>>> > - clobbers(...) >> >> > > >>>>>> > - merges(...) >> >> > > >>>>>> > - defaults(...) >> >> > > >>>>>> > - mapModules(wnd) >> >> > > >>>>>> > - getOriginalSymbol('FileSystem') >> >> > > >>>>>> > >> >> > > >>>>>> > >> >> > > >>>>>> >Start-up flow: >> >> > > >>>>>> > >> >> > > >>>>>> > 1. Parse all modules >> >> > > >>>>>> > 2. common-bootstrap: >> >> > > >>>>>> > 1. Loads list of modules named "cordova.*/symbols" >> >> > > >>>>>> > 2. Run modulemapper.mapModules(window); >> >> > > >>>>>> > 3. Loads list of modules named "cordova.*/main" >> >> > > >>>>>> > >> >> > > >>>>>> > >> >> > > >>>>>> >symbols.js files: >> >> > > >>>>>> > >> >> > > >>>>>> > - Will make calls to modulemapper instead of exporting >> >> > > >>>>>>{clobbers:} >> >> > > >>>>>> > - This make dependencies work by require()ing >> >>dependent >> >> > > >>>>>>symbols >> >> > > >>>>>> > - We want the to be an evaluated .js file instead of >> >> something >> >> > > >>>>>> listed >> >> > > >>>>>> >in >> >> > > >>>>>> > plugin.xml >> >> > > >>>>>> > - So that it can export based on browser version >> >> > > >>>>>> > >> >> > > >>>>>> > >> >> > > >>>>>> >Implementation Steps >> >> > > >>>>>> > >> >> > > >>>>>> > 1. Expose list of registered modules in >> >>scripts/require.js >> >> so >> >> > > >>>>>>that >> >> > > >>>>>> we >> >> > > >>>>>> > can loop over them >> >> > > >>>>>> > 2. Write modulemapper.js (and have unit tests, of course) >> >> > > >>>>>> > 3. Add logic to bootstrap.js that calls into modulemapper >> >> > > >>>>>> > 4. create $PLUGIN_NAME/symbols.js files for each plugin >> >> within >> >> > > >>>>>> cordova. >> >> > > >>>>>> > 5. Add logic to bootstrap.js that calls into modulemapper >> >> > > >>>>>> > 6. Create main.js files for those that currently have >> >>logic >> >> in >> >> > > >>>>>> their >> >> > > >>>>>> > platform.js files >> >> > > >>>>>> > >> >> > > >>>>>> >* >> >> > > >>>>>> >> >> > > >>>>>> >> >> > > >>>>> >> >> > > >>>> >> >> > > >>> >> >> > > >> >> >> > > >> >> > > >> >> > >> >> >> >>
