Closed
Bug 1052288
Opened 10 years ago
Closed 10 years ago
Move page actions to a jsm
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 34
People
(Reporter: wesj, Assigned: wesj)
References
Details
Attachments
(1 file, 1 obsolete file)
14.40 KB,
patch
|
bnicholson
:
review+
|
Details | Diff | 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.
Assignee | ||
Comment 1•10 years ago
|
||
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 2•10 years ago
|
||
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
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/7bd97e67037c
Comment 6•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7bd97e67037c
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Comment 7•10 years ago
|
||
# 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.
Assignee | ||
Comment 8•10 years ago
|
||
(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?
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•