Closed
Bug 848137
Opened 12 years ago
Closed 7 years ago
Use SDK methods instead of rewriting our own
Categories
(Firefox for Metro Graveyard :: General, enhancement, P3)
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
People
(Reporter: rsilveira, Unassigned)
Details
(Whiteboard: [leave open])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
Comments from mbrubeck on Code in browser/metro/base/content/Util.js (bug 844370):
"
I wonder if we can use the merge function here:
http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/util/object.js
To make object.js importable as a JSM, we might need to add some module boilerplate like the one in promise.js:
http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/core/promise.js
"
We should also consider making browser/metro/base/content/Util.js into a module to simplify xpcshell testing.
Comment 1•11 years ago
|
||
It turns out in bug 893091 that I needed a new utility for building a document fragment to represent a formatted, localized string. To bring this into AppDialogHelper it kinda needed to be a module. So, to that end I made a start at moving Util.js into a modules/ContentUtil.jsm.
I came unstuck with the resource:///modules/ContentUtil.jsm apparently not resolving as I got a io error when running the xpcshell test:
0:00.39 TEST-UNEXPECTED-FAIL | xpcshell/head.js | [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame:: c:/dev/mc/obj-metro/_tests/xpcshell/browser/metro/base/tests/unit/test_util_extend.js :: run_test :: line 10" data: no]
So.. I could back up and do this some other how, but ISTM that this should work and would be a useful obstacle to remove. Do you have any idea what's going on?
Attachment #793777 -
Flags: feedback?(jmathies)
Comment 2•11 years ago
|
||
xpcshell doesn't have access to metro app resources by default. You can add them by adding a line to the top of xpcshell.ini -
firefox-appdir = metro
That should fix it.
Updated•11 years ago
|
Attachment #793777 -
Flags: feedback?(jmathies)
Comment 3•11 years ago
|
||
Thank you! I was so close.. Cancelled the feedback on that patch I'll loop you in for review when I have it ready.
Comment 4•11 years ago
|
||
I'm not expecting to close this ticket with this patch, its just a stepping stone for bug 893091 that seems to belong here.
My thought is to progressively move methods out of Util.js and into modules that can be tested and imported in isolation. To avoid regression noise I'm still loading Util.js in the unit test and I suggest we continue to load and access these helpers off of Util - which becomes an aggregate of our utility modules. And sure, we can swap out implementations for those from the SDK if that turns out to be useful.
Is there some neat destructuring-assignment trick to get all of ContentUtils onto Utils? Note we /don't/ want to loop over Utils as the getters get invoked. The block-scoped for/in works and is pretty explicit otherwise.
Attachment #793777 -
Attachment is obsolete: true
Attachment #794356 -
Flags: review?(mbrubeck)
Comment 5•11 years ago
|
||
Comment on attachment 794356 [details] [diff] [review]
Attempt to begin migration of Utils.js to a module
Review of attachment 794356 [details] [diff] [review]:
-----------------------------------------------------------------
Just curious - why the name "ContentUtil"?
Attachment #794356 -
Flags: review?(mbrubeck) → review+
Updated•11 years ago
|
Whiteboard: [leave open]
Comment 6•11 years ago
|
||
On fx-team: https://hg.mozilla.org/integration/fx-team/rev/7e7605f5e1c6
Matt: ContentUtil name because: its not download utils, its stuff that does need a DOM, I cant call it Util because we already have a global of that name. I'm open to following up with a rename if you or anyone has suggestions. As we refactor Util.js its likely other/better module names will suggest themselves anyhow.
Comment 7•11 years ago
|
||
Reporter | ||
Comment 8•11 years ago
|
||
Unassigning as I'm not working on this. I hope one day we can use Object.mixin instead of util.extend.
Assignee: rsilveira → nobody
Status: ASSIGNED → NEW
Assignee | ||
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
Comment 9•7 years ago
|
||
Mass close of bugs in obsolete product https://bugzilla.mozilla.org/show_bug.cgi?id=1350354
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
You need to log in
before you can comment on or make changes to this bug.
Description
•