382996 - (Re)move G_ObjectSafeMap

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Simon Bünzli]

It's only used in one place (inside the to-be-removed-as-well G_Preferences) and should - without the need for abstraction - rather have been implemented in place.

OTOH the core parts of it (get, set and del) could also be factored out into a .jsm as there's at least one other place where such functionality has been implemented: SessionStore. I'm not sure whether that's really worth the code overhead.

Comment 1

[Simon Bünzli]

Attachment: remove G_ObjectHashMap

As I read it, the code currently didn't even behave as expected (when re-registering a pref-observer callback, you got two observers instead of just the one). Correcting the code would take about as many lines with an ObjectHashMap as without it. I tend to use the solution with less code overall in this situation: removing the hash map without moving it elsewhere.

Comment 2

[Tony Chang (Google)]

Comment on attachment 267031 [details] [diff] [review]
remove G_ObjectHashMap

Rather than having two parallel arrays, why not have a single array of objects like {callback: callback, observer:observer}.  You wouldn't be able to use indexOf during removal, but having a loop or Array.filter seems ok.

If the observer already exists, should we throw an error?  It seems like that would be unintentional.  What do pref observers normally do in this case?

Comment 3

[Simon Bünzli]

(In reply to comment #2)
> why not have a single array of objects

Why have one? Using .indexOf gives us slightly more compact (and faster) code. OTOH it doesn't really matter anyway, as those bits will go away completely once bug 382635 is fixed.

> What do pref observers normally do in this case?

They do what this code does: replace the observer (resp. ignoring the addition). But again: those bits should go away completely in bug 382635.

Comment 4

[Tony Chang (Google)]

Comment on attachment 267031 [details] [diff] [review]
remove G_ObjectHashMap

r+ only because it's going away soon.

Comment 5

[Tony Chang (Google)]

I'll try and check in tomorrow unless someone beats me to it.

Comment 6

[Tony Chang (Google)]

on trunk

Removing toolkit/components/url-classifier/content/moz/objectsafemap.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/moz/objectsafemap.js,v  <--  objectsafemap.js
new revision: delete; previous revision: 1.3
done
Checking in toolkit/components/url-classifier/content/moz/preferences.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/moz/preferences.js,v  <--  preferences.js
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierLib.js;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierLib.js,v  <--  nsUrlClassifierLib.js
new revision: 1.9; previous revision: 1.8
done
Checking in browser/components/safebrowsing/content/application.js;
/cvsroot/mozilla/browser/components/safebrowsing/content/application.js,v  <--  application.js
new revision: 1.11; previous revision: 1.10
done
Checking in browser/components/safebrowsing/src/nsSafebrowsingApplication.js;
/cvsroot/mozilla/browser/components/safebrowsing/src/nsSafebrowsingApplication.js,v  <--  nsSafebrowsingApplication.js
new revision: 1.8; previous revision: 1.7
done
Checking in mail/components/phishing/nsPhishingProtectionApplication.js;
/cvsroot/mozilla/mail/components/phishing/nsPhishingProtectionApplication.js,v  <--  nsPhishingProtectionApplication.js
new revision: 1.2; previous revision: 1.1
done