Closed Bug 965023 Opened 6 years ago Closed 6 years ago

Use Object.freeze on exports of Home.jsm

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: mcomella, Assigned: lpy)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mentor=mcomella][lang=js])

Attachments

(1 file, 2 obsolete files)

Like HomeProvider.jsm.
Whiteboard: [mentor=mcomella][lang=js]
Attached patch bug965023.patch (obsolete) — Splinter Review
Assignee: nobody → pylaurent1314
Attachment #8370764 - Flags: review?(michael.l.comella)
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! ;)
Attachment #8370764 - Flags: review?(michael.l.comella) → review-
(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?
Attached patch bug965023-V2.patch (obsolete) — Splinter Review
Attachment #8370764 - Attachment is obsolete: true
Attachment #8371413 - Flags: review?(michael.l.comella)
(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
Status: NEW → ASSIGNED
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
Attachment #8371413 - Flags: review?(michael.l.comella) → review-
Attachment #8371413 - Attachment is obsolete: true
Attachment #8372089 - Flags: review?(michael.l.comella)
Thank you
Comment on attachment 8372089 [details] [diff] [review]
bug965023-V3.patch

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

Looks good to me! :)
Attachment #8372089 - Flags: review?(michael.l.comella) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/42e3d236bebb
Keywords: checkin-needed
Whiteboard: [mentor=mcomella][lang=js] → [mentor=mcomella][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/42e3d236bebb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [mentor=mcomella][lang=js][fixed-in-fx-team] → [mentor=mcomella][lang=js]
Target Milestone: --- → Firefox 30
You need to log in before you can comment on or make changes to this bug.