Last Comment Bug 730340 - Don't use expando properties for storing data on places nodes
: Don't use expando properties for storing data on places nodes
Status: RESOLVED FIXED
: addon-compat, perf
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
Mentors:
Depends on: 760940 761027 761494
Blocks: 714689 569127
  Show dependency treegraph
 
Reported: 2012-02-24 09:46 PST by Marco Bonardo [::mak]
Modified: 2012-07-18 09:05 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
part 1 - _DOMElement (12.24 KB, patch)
2012-05-28 14:26 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
part 1 - _DOMElement (18.62 KB, patch)
2012-05-29 02:00 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
part 2 - "transparent" property bag for when it's really necessary (test included) (8.78 KB, patch)
2012-05-29 04:32 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
part 3 - feedURI using this new api (wip, toolbar and menu views not fixed) (11.88 KB, patch)
2012-05-29 04:50 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
Remove expandos support (4.80 KB, patch)
2012-05-29 05:01 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: review+
asaf: checkin+
Details | Diff | Review
Fix test_sort-date-site-grouping test (829 bytes, patch)
2012-05-29 05:11 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: review+
asaf: checkin+
Details | Diff | Review
part 1 - _DOMElement (18.51 KB, patch)
2012-06-01 06:50 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: review+
Details | Diff | Review
part 2 - cellProperties, plainContainer (6.39 KB, patch)
2012-06-01 10:01 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: review+
Details | Diff | Review
part 1 + 2 as checked in (25.15 KB, patch)
2012-06-01 13:04 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
asaf: checkin+
Details | Diff | Review
part 3, livemarks (24.89 KB, patch)
2012-06-01 14:40 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
part 3 - livemarks, addressing neil's comments (28.68 KB, patch)
2012-06-01 15:52 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: review+
Details | Diff | Review
part 3 - livemarks, as checked in (29.53 KB, patch)
2012-06-02 06:56 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
asaf: checkin+
Details | Diff | Review
Combined front-end changes for SeaMonkey (47.93 KB, patch)
2012-06-02 07:44 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
Fix typo found by Neil, remove some useless comments (2.99 KB, patch)
2012-06-02 13:25 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
asaf: checkin+
Details | Diff | Review
_cutting (4.24 KB, patch)
2012-06-02 14:33 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: review+
asaf: checkin+
Details | Diff | Review
Combined front-end changes for SeaMonkey, as checked in (47.92 KB, patch)
2012-06-02 14:53 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
asaf: checkin+
Details | Diff | Review

Description Marco Bonardo [::mak] 2012-02-24 09:46:28 PST
Mano suggested this to me yesterday, and sounds like an awesome idea.

Currently we attach data to nodes through expando properties, and this adds some complexity to our code.
Moreover it may increase the work needed for GC+CC, since we have cycles like placesNode._DOMElement._placesNode.
Comment 1 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-02-26 05:10:53 PST
I think this idea is very good.

The WeakMap references in MDN describes that current WeakMap implementation is not stable and we do not rely on it for production code.
(https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/WeakMap)

We have to wait that WeakMap will be implemented as stable?
Comment 2 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-02-26 06:35:28 PST
As far as I understand, the implementation is stable, but the API isn't, but that's not such a concern for in-tree code.

Taking.
Comment 3 Bobby Holley (PTO through June 13) 2012-05-01 03:10:50 PDT
Nominating for tracking per Mano's suggestion, since compartment-per-global makes this more of a problem.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-02 00:16:41 PDT
Tracking flags aren't a way to ask for bugs to be prioritized, they're just a way to indicate that release-management folks need to monitor the bug before that train ships, either because they're new features that may need disabling, regressions, critical security issues, etc. None of that seems to apply here.
Comment 5 Bobby Holley (PTO through June 13) 2012-05-02 01:02:20 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)
> Tracking flags aren't a way to ask for bugs to be prioritized, they're just
> a way to indicate that release-management folks need to monitor the bug
> before that train ships, either because they're new features that may need
> disabling, regressions, critical security issues, etc. None of that seems to
> apply here.

The issue is that places is doing things that are not compartment-per-global safe (putting expandos on per-compartment XPCWNs), and we're landing CPG. The tests are green, but we could be turning on a whole class of hidden bugs (and therefore regressions) here. So I thought we should make sure that we actually followed through with this before the next release.

It's your call though. Re-nominating for tracking, not because I necessarily disagree with tracking-, but just because I want to make sure we're on the same page (and because the bug previously lacked context).
Comment 6 Marco Bonardo [::mak] 2012-05-02 06:17:06 PDT
We need to better track prioritization of bugs, instead tracking should be set on the CPG bug, and the idea is that the new feature is disabled/backed-out, rather than "the bug X caused by new feature Y should be fixed before we ship Y", especially in the rapid release process.  I can see how backing out CPG may be problematic and block a bunch of awesome work, so may not be the perfect choice here.

Regarding this bug, fixing it involves rewriting a bunch of code in the views, that may as well introduce a bunch of hidden and new bugs, so no doubt both situations involve regression risks.  I don't think should be a blocker to CPG, though I think its importance grows quite a bit and should be considered a P1 in Places (unfortunately we don't have a good history of using Importance fields).  We may need some more human resources, btw will check availability with Mano and figure out something.
Comment 7 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-07 16:30:44 PDT
> The tests are green, but we could be turning on a whole class of hidden bugs
> (and therefore regressions) here. So I thought we should make sure that we
> actually followed through with this before the next release.
> 

Tracking for 15 so we can check in again later on bholley's statements here and ensure that they are addressed prior to the release of 15.
Comment 8 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-05-28 14:26:55 PDT
Created attachment 627791 [details] [diff] [review]
part 1 - _DOMElement

I think we should land this in pieces. 

This is the easiest, though largest, part of this change.
Comment 9 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-05-29 02:00:19 PDT
Created attachment 627884 [details] [diff] [review]
part 1 - _DOMElement
Comment 10 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-05-29 04:32:19 PDT
Created attachment 627911 [details] [diff] [review]
part 2 - "transparent" property bag for when it's really necessary (test included)

Expose has/get/set/delete (similar to the Map api). I deliberately chose to hide the semantics of nsIPropertyBag, as they don't work so well for js.

This is one option. Another options is to expose a "nodesProperties" *js Map* getter, which would just return a js Map. I'm not sure which one I dislike more...
Comment 11 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-05-29 04:50:45 PDT
Created attachment 627912 [details] [diff] [review]
part 3 - feedURI using this new api (wip, toolbar and menu views not fixed)
Comment 12 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-05-29 05:01:10 PDT
Created attachment 627917 [details] [diff] [review]
Remove expandos support

This will be the last part to land (to be clear, few pieces are still missing).
Comment 13 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-05-29 05:11:46 PDT
Created attachment 627918 [details] [diff] [review]
Fix test_sort-date-site-grouping test

Expandos-removeal revealed a bug in this test.
Comment 14 Marco Bonardo [::mak] 2012-05-30 08:11:14 PDT
Comment on attachment 627884 [details] [diff] [review]
part 1 - _DOMElement

Review of attachment 627884 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/browserPlacesViews.js
@@ +100,5 @@
> +    let node = this._domNodes.get(aPlacesNode, null);
> +    if (!node && aThrowIfNotSet !== false)
> +      throw new Error("No DOM node set for aPlacesNode");
> +    return node;
> +  },

Wouldn't be better to have 2 separate helpers instead of a blind boolean argument that everytime I will have to check to know if it means to throw or not and what's the default? this kind of boolean arguments are error-prone imo.

Looks like the "don't throw" use-case is so rare that adding an _hasDOMNodeForPlacesNode shouldn't have bad consequences.
Probably in the end I'm not sure I like hiding the code flow into a getter, the previous code was repetitive, but clear (just think of NS_ENSURE_SUCCESS that hides the fact it warns and returns).
So imo, either we go back to the old explicit flow, or we just implement a throwing get and use .has() below where we should not throw.

@@ +1032,5 @@
>        }
>      }
>  
>      button._placesNode = aChild;
> +    if (!this._getDOMNodeForPlacesNode(aChild, false))

may use .has
Comment 15 Marco Bonardo [::mak] 2012-05-31 07:35:53 PDT
Comment on attachment 627911 [details] [diff] [review]
part 2 - "transparent" property bag for when it's really necessary (test included)

Review of attachment 627911 [details] [diff] [review]:
-----------------------------------------------------------------

maybe a dumb question, but I don't remember. If we expose this, may someone pass an xpcom object as a value? cause the propertybag has a setAsInterface, but I think for our needs we should only allow natives, to avoid possible leaks. Basically I think we should disallow passing an nsISupports.

::: toolkit/components/places/nsINavHistoryService.idl
@@ +158,5 @@
> +   *        the property name
> +   * @return the value for the property aName, if set, "undefined" otherwise
> +   * (void-type nsIVariant).
> +   */
> +  nsIVariant get(in AString aName);

I wonder if we should make these methods clearer, while on a map get,set,has,delete is clear, on a places node their effect is completely unclear.
My guess is that we should at least make these setProperty, hasProperty and so on... property is not exactly correct but can't find a better naming atm.
Or as discussed on irc having a .properties.set/has/get/delete

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +358,5 @@
> +nsNavHistoryResultNode::Set(const nsAString& aName, nsIVariant* aValue) {
> +  if (!mPropertyBag) {
> +    nsresult rv;
> +    mPropertyBag = do_CreateInstance("@mozilla.org/hash-property-bag;1", &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);

nit: may use NS_ENSURE_TRUE(mPropertyBag and avoid rv

@@ +375,5 @@
> +
> +NS_IMETHODIMP
> +nsNavHistoryResultNode::Delete(const nsAString& aName) {
> +  if (mPropertyBag)
> +    mPropertyBag->DeleteProperty(aName);

please brace. Ideally we should always brace both in cpp and js, but at least we should in cpp.

::: toolkit/components/places/tests/unit/test_node_properties.js
@@ +16,5 @@
> +  do_check_false(node.has("prop"))
> +  do_check_true(node.get("prop") === undefined);
> +
> +  // Make sure this doesn't throw.
> +  node.delete("prop");

please close the node, for the usual fact add-ons copy our tests code :)

::: toolkit/components/places/tests/unit/xpcshell.ini
@@ +121,5 @@
>  [test_PlacesUtils_asyncGetBookmarkIds.js]
>  [test_PlacesUtils_lazyobservers.js]
>  [test_placesTxn.js]
>  [test_telemetry.js]
> +[test_node_properties.js]

please keep tests alphabetical ordered
Comment 16 Marco Bonardo [::mak] 2012-05-31 07:39:09 PDT
Comment on attachment 627912 [details] [diff] [review]
part 3 - feedURI using this new api (wip, toolbar and menu views not fixed)

Review of attachment 627912 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/controller.js
@@ +1038,5 @@
>          addData(PlacesUtils.TYPE_X_MOZ_PLACE, i);
>  
>          // Drop the feed uri for livemark containers
> +        if (node.has("feedURI")) {
> +          addURIData(i, node.get("feedURI").QueryInterface(Ci.nsIURI).spec);

store the spec so we avoid the QI

@@ +1112,5 @@
>          return;
>        if (PlacesUtils.nodeIsFolder(node))
>          copiedFolders.push(node);
>  
> +      let overrideURI = node.has("feedURI") ? node.get("feedURI").QueryInterface(Ci.nsIURI).spec : null;

ditto

::: browser/components/places/content/treeView.js
@@ +856,5 @@
>        PlacesUtils.livemarks.getLivemark(
>          { id: aNode.itemId },
>          (function (aStatus, aLivemark) {
>            if (Components.isSuccessCode(aStatus)) {
> +            aNode.set("feedURI", aLivemark.feedURI)

let's rather store the .spec and call it feedURL
Comment 17 Marco Bonardo [::mak] 2012-05-31 07:41:59 PDT
Comment on attachment 627917 [details] [diff] [review]
Remove expandos support

Review of attachment 627917 [details] [diff] [review]:
-----------------------------------------------------------------

please, do a tryrun once you have all the updated patches. Off-hand I don't remember other things supported by having the result implement classInfo, but with time we added so many things...
Comment 18 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-01 06:50:24 PDT
Created attachment 629170 [details] [diff] [review]
part 1 - _DOMElement
Comment 19 Marco Bonardo [::mak] 2012-06-01 09:02:35 PDT
Comment on attachment 629170 [details] [diff] [review]
part 1 - _DOMElement

Review of attachment 629170 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/browserPlacesViews.js
@@ +92,5 @@
> +  _getDOMNodeForPlacesNode:
> +  function PVB__getDOMNodeForPlacesNode(aPlacesNode) {
> +    let node = this._domNodes.get(aPlacesNode, null);
> +    if (!node)
> +      throw new Error("No DOM node set for aPlacesNode");

wonder if would be nice to print out something about the node, like its type and parent (to check it's alive)... something useful for debugging
Comment 20 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-01 10:00:45 PDT
Comment on attachment 627911 [details] [diff] [review]
part 2 - "transparent" property bag for when it's really necessary (test included)

Not going with this approach after all.
Comment 21 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-01 10:01:51 PDT
Created attachment 629229 [details] [diff] [review]
part 2 - cellProperties, plainContainer
Comment 22 Marco Bonardo [::mak] 2012-06-01 12:44:18 PDT
Comment on attachment 629229 [details] [diff] [review]
part 2 - cellProperties, plainContainer

Review of attachment 629229 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/treeView.js
@@ +113,1 @@
>              nodeType != Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT);

the parenthesis are useless now

@@ +853,5 @@
>          { id: aNode.itemId },
>          (function (aStatus, aLivemark) {
>            if (Components.isSuccessCode(aStatus)) {
>              aNode._feedURI = aLivemark.feedURI;
> +            let properties = this._cellProperties.get(aNode, null);

fwiw, weakmap returns undefined if a defaultValue is not suggested (the second arg to get is optional indeed), did you always pass null for consistency?

@@ +1210,3 @@
>      }
> +    for (let property of properties)
> +      aProperties.AppendElement(property);

please always brace loops, at least :)
Comment 23 Marco Bonardo [::mak] 2012-06-01 12:47:57 PDT
Comment on attachment 629170 [details] [diff] [review]
part 1 - _DOMElement

Review of attachment 629170 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/browserPlacesViews.js
@@ +1025,5 @@
>        }
>      }
>  
>      button._placesNode = aChild;
> +    if (!this._domNodes.has(aChild, button))

just as a reminder, NeilAway found this typo, to be fixed.
Comment 24 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-01 13:04:45 PDT
Created attachment 629307 [details] [diff] [review]
part 1 + 2 as checked in

http://hg.mozilla.org/integration/mozilla-inbound/rev/c4bf270b5b0a
Comment 25 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-01 14:40:10 PDT
Created attachment 629348 [details] [diff] [review]
part 3, livemarks

What a nightmare.
Comment 26 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-01 15:52:38 PDT
Created attachment 629378 [details] [diff] [review]
part 3 - livemarks, addressing neil's comments
Comment 27 Marco Bonardo [::mak] 2012-06-02 03:10:14 PDT
Comment on attachment 629378 [details] [diff] [review]
part 3 - livemarks, addressing neil's comments

Review of attachment 629378 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following fixes (especially the wrong if conditions!)

::: browser/components/places/content/browserPlacesViews.js
@@ +655,3 @@
>              if (Components.isSuccessCode(aStatus)) {
> +              let shouldInvalidate =
> +                this.controller.hasCachedLivemarkInfo(aPlacesNode);

you have lost a ! here... it was "let shouldInvalidate = !aPlacesNode._feedURI;", so should !hasCached

::: browser/components/places/content/controller.js
@@ +1287,5 @@
>        this._view.selectItems(insertedNodeIds, false);
> +  },
> +
> +  /**
> +   * Cache the livemark info for a node.  This allows the controller

allows the controller and the views

@@ +1292,5 @@
> +   * to treat the given node as a livemark.
> +   * @param aNode
> +   *        a places result node.
> +   * @param aLivemarkInfo
> +   *        a livemark-info object.

mozILivemarkInfo

@@ +1299,5 @@
> +    this._cachedLivemarkInfoObjects.set(aNode, aLivemarkInfo);
> +  },
> +
> +  hasCachedLivemarkInfo: function PC_hasCachedLivemarkInfo(aNode)
> +    this._cachedLivemarkInfoObjects.has(aNode),

You added documentation to the other ones, not this one... not that I care that much, it's self documenting, but sounds a bit strange :)

@@ +1302,5 @@
> +  hasCachedLivemarkInfo: function PC_hasCachedLivemarkInfo(aNode)
> +    this._cachedLivemarkInfoObjects.has(aNode),
> +
> +  /**
> +   * Returns the cached livemark info for a node, if set (by cacheLivemarkInfo),

remove parenthesis

@@ +1306,5 @@
> +   * Returns the cached livemark info for a node, if set (by cacheLivemarkInfo),
> +   * null otherwise.
> +   * @param aNode
> +   *        a places result node.
> +   * @return the livemark-info object for aNode, if set, null otherwise.

mozILivemarkInfo

::: browser/components/places/content/treeView.js
@@ +892,3 @@
>            if (Components.isSuccessCode(aStatus)) {
> +            let shouldInvalidate = 
> +              this._controller.hasCachedLivemarkInfo(aNode);

you lost the ! here as well, should be !hasCached
Comment 28 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-02 06:56:07 PDT
Created attachment 629469 [details] [diff] [review]
part 3 - livemarks, as checked in

I've also fixed a bug I introduced in PVB_nodeHistroyDetailsChanged in the previous check-in.
Comment 29 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-02 06:56:53 PDT
Comment on attachment 629469 [details] [diff] [review]
part 3 - livemarks, as checked in

http://hg.mozilla.org/integration/mozilla-inbound/rev/efd698c68283
Comment 30 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-02 06:59:35 PDT
Comment on attachment 627918 [details] [diff] [review]
Fix test_sort-date-site-grouping test

http://hg.mozilla.org/integration/mozilla-inbound/rev/1f359ffe1cdd
Comment 31 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-02 07:44:48 PDT
Created attachment 629472 [details] [diff] [review]
Combined front-end changes for SeaMonkey
Comment 32 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-02 12:09:55 PDT
https://hg.mozilla.org/mozilla-central/rev/c4bf270b5b0a
Comment 34 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-02 13:25:51 PDT
Created attachment 629516 [details] [diff] [review]
Fix typo found by Neil, remove some useless comments

http://hg.mozilla.org/integration/mozilla-inbound/rev/7ee3f3fe90fb
Comment 35 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-02 14:33:13 PDT
Created attachment 629521 [details] [diff] [review]
_cutting

After removing expandos support, this is last change required for tests to pass.

Marco: Fx13 is about to ship with Set support included, thus I think it's safe to use it, finally.

Note: Sometime soon I'm going to make treeView.js the actual places view for tress, and then this kind of forwarding won't be needed.
Comment 36 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-02 14:53:17 PDT
Created attachment 629524 [details] [diff] [review]
Combined front-end changes for SeaMonkey, as checked in

http://hg.mozilla.org/comm-central/rev/58c04223b055
Comment 37 Marco Bonardo [::mak] 2012-06-03 02:16:18 PDT
Comment on attachment 629521 [details] [diff] [review]
_cutting

Review of attachment 629521 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this wrappedJSObject addition sucks, but I assume you did it cause being treeview-only would have been bad to use the shared controller.
I don't have better ideas atm.
I also assume you tested this manually in the library and sidebar.
Comment 38 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-03 04:56:33 PDT
I could avoid this wrappedJSObject thing by simply returning the PlacesTreeView instance without going through treeBoxObject. I didn't do so because changing this |view| getter & setter tended to introduce regression in the past, and we're too close to "release". Anyway, the real fix is getting rid of tree.xml, which I'm going to do sooner than you think... :)
Comment 39 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-03 04:57:09 PDT
Oh, and thanks for reviewing on Sunday :)
Comment 40 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-03 05:02:23 PDT
Comment on attachment 629521 [details] [diff] [review]
_cutting

http://hg.mozilla.org/integration/mozilla-inbound/rev/04682cf84deb
Comment 41 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-06-03 05:51:14 PDT
Comment on attachment 627917 [details] [diff] [review]
Remove expandos support

http://hg.mozilla.org/integration/mozilla-inbound/rev/b1b44a491f6a

Done.
Comment 43 Jorge Villalobos [:jorgev] 2012-07-06 15:03:54 PDT
Should I be looking for something other than _DOMElement to find add-on compatibility issues?
Comment 44 Marco Bonardo [::mak] 2012-07-09 12:26:53 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #43)
> Should I be looking for something other than _DOMElement to find add-on
> compatibility issues?

that's the major change (also ._feedURI, ._siteURI, ._cellProperties), though most of the changed stuff was considered private (but I'm sure someone copied the views code).
Comment 45 Alex Keybl [:akeybl] 2012-07-16 07:47:30 PDT
Should we still be considering this as a possible uplift to FF15, given perceived risk vs reward? We originally tracked because of the possibility of hidden regressions. Does that concern still stand?
Comment 46 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-07-16 09:51:23 PDT
Alex, this has landed for Fx15 already, afaict.

Note You need to log in before you can comment on or make changes to this bug.