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)

All
Windows 8.1
enhancement

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: rsilveira, Unassigned)

Details

(Whiteboard: [leave open])

Attachments

(1 file, 1 obsolete file)

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.
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)
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.
Attachment #793777 - Flags: feedback?(jmathies)
Thank you! I was so close.. Cancelled the feedback on that patch I'll loop you in for review when I have it ready.
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 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+
Whiteboard: [leave open]
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.
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
OS: Windows 8 Metro → Windows 8.1
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.

Attachment

General

Created:
Updated:
Size: