Closed Bug 1052288 Opened 10 years ago Closed 10 years ago

Move page actions to a jsm

Categories

(Firefox for Android Graveyard :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: wesj, Assigned: wesj)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
We should move pageactions to a jsm. That will make it easier for outsiders to use without needing access to NativeWindow. It will also help us keep the code better encapsulated, and save us loading/parsing some JS on first run.
Comment on attachment 8471385 [details] [diff] [review]
Patch

This does the work to move this to a JSM. It also moves all of our current callers over.
Attachment #8471385 - Flags: review?(bnicholson)
Comment on attachment 8471385 [details] [diff] [review]
Patch


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

> // Lazily-loaded JS modules that use observer notifications
> [
>   ["Home", ["HomeBanner:Get", "HomePanels:Get", "HomePanels:Authenticate", "HomePanels:RefreshView",
>             "HomePanels:Installed", "HomePanels:Uninstalled"], "resource://gre/modules/Home.jsm"],
>   ["Toast", ["Toast:Click", "Toast:Hidden"], "resource://gre/modules/Toast.jsm"],
>+  ["PageActions", ["PageActions:Clicked", "PageActions:LongClicked"], "resource://gre/modules/PageActions.jsm"],


Again, I don't like doing this. It breaks encapsulation and I think we can avoid it like we can with Toast.jsm

>+XPCOMUtils.defineLazyModuleGetter(this, "PageActions",
>+                                  "resource://gre/modules/PageActions.jsm");
>+
>+XPCOMUtils.defineLazyModuleGetter(NativeWindow, "pageactions",
>+                                  "resource://gre/modules/PageActions.jsm", "PageActions");

Again, this backward compatibility wrapper needs a comment. Just one for all of them would be enough.
Attachment #8471385 - Attachment is patch: true
Attached patch Patch v2Splinter Review
Updated. I actually made the old API's complain if you use them, but only the first time they're accessed. I'm still debating if thats enough, but its a start.
Attachment #8471385 - Attachment is obsolete: true
Attachment #8471385 - Flags: review?(bnicholson)
Attachment #8471798 - Flags: review?(bnicholson)
Comment on attachment 8471798 [details] [diff] [review]
Patch v2

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

::: mobile/android/chrome/content/browser.js
@@ +2679,5 @@
> +    Cu.reportError(err);
> +
> +    let sandbox = {};
> +    Services.scriptloader.loadSubScript(script, sandbox);
> +    NativeWindow[name] = sandbox[exprt];

defineLazyGetter is already designed to replace the getter with the returned value, so this line is unnecessary.
Attachment #8471798 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/7bd97e67037c
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
# LOCALIZATION NOTE (nativeWindow.deprecated):
# This string is shown in the console when someone uses deprecated NativeWindow apis.
# %1$S=name of the api that's deprecated, %2$S=New API to use. This may be a url to
# a file they should import or the name of an api.
nativeWindow.deprecated=%S is deprecated. Please use %S instead

Can we switch to ordered arguments here or there's a technical limitation in Android? Also, the difference between comment and string is confusing.
(In reply to Francesco Lodolo [:flod] from comment #7)
> Can we switch to ordered arguments here or there's a technical limitation in
> Android? Also, the difference between comment and string is confusing.

No problem. Can you file a follow up bug?
Blocks: 1054177
Depends on: 1055212
Depends on: 1098657
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: