965023 - Use Object.freeze on exports of Home.jsm

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

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

Like HomeProvider.jsm.

Comment 1

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

Attachment: bug965023.patch

Comment 2

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

Comment on attachment 8370764 [details] [diff] [review]
bug965023.patch

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

::: mobile/android/modules/Home.jsm
@@ +238,2 @@
>    banner: HomeBanner,
>    panels: HomePanels,

The `HomeBanner` and `HomePanels` objects, and some of their attributes are exported as well so these also need to be frozen.

Don't bother freezing private members - these will be hidden in bug 968378 (you're welcome to work on that one after this too! ;)

Comment 3

[Masatoshi Kimura [:emk]]

(In reply to Michael Comella (:mcomella) from comment #2)
> Comment on attachment 8370764 [details] [diff] [review]
> bug965023.patch
> 
> Review of attachment 8370764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/Home.jsm
> @@ +238,2 @@
> >    banner: HomeBanner,
> >    panels: HomePanels,
> 
> The `HomeBanner` and `HomePanels` objects, and some of their attributes are
> exported as well so these also need to be frozen.

Doesn't Object.freeze() freeze all members recursively?

Comment 4

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

Attachment: bug965023-V2.patch

Comment 5

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

(In reply to Masatoshi Kimura [:emk] from comment #3)
> Doesn't Object.freeze() freeze all members recursively?

The documentation [1] mentions that the freeze is shallow.

[1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze

Comment 6

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

Comment on attachment 8371413 [details] [diff] [review]
bug965023-V2.patch

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

One more level! I can still append to `Home.panels.Layout`. You can test this code with an addon I've been working on [1]. You can get the output with `adb logcat -v time | grep GeckoConsole`.

[1]: https://github.com/mcomella/fennec_rss/tree/obj_freeze

Comment 7

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

Attachment: bug965023-V3.patch

Comment 8

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

Thank you

Comment 9

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

Comment on attachment 8372089 [details] [diff] [review]
bug965023-V3.patch

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

Looks good to me! :)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/42e3d236bebb