The private members are currently declared using leading underscores. Alternatively, we can declare the functionality as non-exported top-level objects, however, margaret agreed on IRC that the encapsulation with closures is probably preferable.

Example private member in the code: https://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Home.jsm?rev=46c932e276ea#167

I take this one, and I will wait until bug 965023 landed.

Comment on attachment 8373196 [details] [diff] [review] bug968378.patch Review of attachment 8373196 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/modules/Home.jsm @@ +67,5 @@ > break; > + } > + }, > + > + _handleGet: function() { Sorry if I was unclear but these private functions should also not be in the returned object as calling them could change internal state. @@ +157,2 @@ > // Valid layouts for a panel. > + let Layout = { This needs to be a part of the returned object as it's part of the API. Assume any reference that does not start with an underscore should be public and anything with the underscore should be private. [1]: https://wiki.mozilla.org/Mobile/Projects/Third-party_service_integration_MVP#JS_APIs @@ +161,3 @@ > > // Valid types of views for a dataset. > + let View = { Same: public.

Thank you :) I wasn't aware of that before.

Comment on attachment 8374013 [details] [diff] [review] bug968378-V2.patch Review of attachment 8374013 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/modules/Home.jsm @@ +215,5 @@ > + throw "Home.panels: Can't create a home panel without an id and title!"; > + } > + > + // Bail if the panel already exists > + if (panel.id in this._panels) { `this` refers to the object on which this method was called (unless altered by `bind()` and similar methods, but let's not worry about this). In this case, `this` refers to the return object which has not `_panels` attribute, meaning `this._panels` is null, and this fails. (I believe) removing `this` should fix the issue. Can you update this here and where ever else it is an issue? Sorry I didn't mention this before - I didn't realize it would be an issue until running the code! :P In testing, check the logcat output to ensure there are no errors in Home.jsm, and no errors in the addon. If "Replace doc w/ feed contents" appears in the overflow menu (hamburger button), the addon has generally loaded successfully. @@ +255,5 @@ > + sendMessageToJava({ > + type: "HomePanels:Remove", > + panel: this._panelToJSON(panel) > + }); > + } nit: Excess end of line whitespace.

Should work this time. Thanks

Comment on attachment 8374757 [details] [diff] [review] bug968378-V3.patch Review of attachment 8374757 [details] [diff] [review]: ----------------------------------------------------------------- Nice!

Thanks!

Bitrotted, please rebase :(

sorry, my bad.

Sorry, this just bitrotted against bug 968573.

thank you