848137 - Use SDK methods instead of rewriting our own

Status: RESOLVED INCOMPLETE

(Firefox for Metro Graveyard :: General, enhancement, P3)

Description

[Rodrigo Silveira [:rsilveira] [:rodms]]

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

[Sam Foster [:sfoster] (he/him)]

Attachment: Attempt to begin migration of Utils.js to a module

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?

Comment 2

[Jim Mathies [:jimm]]

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.

Comment 3

[Sam Foster [:sfoster] (he/him)]

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

[Sam Foster [:sfoster] (he/him)]

Attachment: Attempt to begin migration of Utils.js to a module

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.

Comment 5

[Matt Brubeck (: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"?

Comment 6

[Sam Foster [:sfoster] (he/him)]

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

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/7e7605f5e1c6

Comment 8

[Rodrigo Silveira [:rsilveira] [:rodms]]

Unassigning as I'm not working on this. I hope one day we can use Object.mixin instead of util.extend.

Comment 9

[Firefox Bug Husbandry Bot]

Mass close of bugs in obsolete product https://bugzilla.mozilla.org/show_bug.cgi?id=1350354