968378 - Hide private members in Home.jsm using closures

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Ideal private member example

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.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 2

[Peiyong Lin[:lpy](UTC+8)]

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

Comment 3

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug968378.patch

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 5

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug968378-V2.patch

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

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 7

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug968378-V3.patch

Should work this time. Thanks

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8374757 [details] [diff] [review]
bug968378-V3.patch

Review of attachment 8374757 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 9

[Peiyong Lin[:lpy](UTC+8)]

Thanks!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Bitrotted, please rebase :(

Comment 11

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug968378-V4.patch

sorry, my bad.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Sorry, this just bitrotted against bug 968573.

Comment 13

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug968378-V5.patch

thank you

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ecf14acd56be

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ecf14acd56be