> > 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 > > > >>>>>> > > > > >>>>>> >* > > > >>>>>> > > > >>>>>> > > > >>>>> > > > >>>> > > > >>> > > > >> > > > > > > > > >
