Last Comment Bug 451540 - Find highlighting in textboxes grows/decreases when editing matched word
: Find highlighting in textboxes grows/decreases when editing matched word
Status: VERIFIED FIXED
: testcase
Product: Toolkit
Classification: Components
Component: Find Toolbar (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.2a1
Assigned To: Graeme McCutcheon [:graememcc]
:
Mentors:
: 442458 (view as bug list)
Depends on:
Blocks: 279022 263683
  Show dependency treegraph
 
Reported: 2008-08-21 03:58 PDT by Graeme McCutcheon [:graememcc]
Modified: 2014-08-20 16:13 PDT (History)
10 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (425 bytes, text/html)
2008-08-21 03:59 PDT, Graeme McCutcheon [:graememcc]
no flags Details
Patch v1 (16.94 KB, patch)
2008-08-24 12:29 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Patch v2 (9.14 KB, patch)
2008-09-02 06:47 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Patch v3 (17.26 KB, patch)
2008-10-29 06:20 PDT, Graeme McCutcheon [:graememcc]
asaf: review+
Details | Diff | Review
Patch v4 (16.56 KB, patch)
2009-01-23 04:10 PST, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Patch v5 (39.23 KB, patch)
2009-02-05 14:34 PST, Graeme McCutcheon [:graememcc]
asaf: review-
Details | Diff | Review
Patch v6 (38.07 KB, patch)
2009-02-06 04:36 PST, Graeme McCutcheon [:graememcc]
asaf: review+
Details | Diff | Review
hg export for checkin (38.37 KB, patch)
2009-04-03 05:57 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Use the correct key modifier on mac (39.43 KB, patch)
2009-04-06 09:48 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Branch patch (39.25 KB, patch)
2009-05-22 03:20 PDT, Graeme McCutcheon [:graememcc]
mbeltzner: approval1.9.1-
Details | Diff | Review

Description Graeme McCutcheon [:graememcc] 2008-08-21 03:58:25 PDT
If find bar highlighting finds a match within a textbox, then you can edit the match. However, when you do so, the highlight grows/decreases to encompass your edits. It should disappear.
Comment 1 Graeme McCutcheon [:graememcc] 2008-08-21 03:59:05 PDT
Created attachment 334864 [details]
Testcase
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-08-21 04:09:05 PDT
Well, I just confirmed bug 442458, is probably about the same issue.
Comment 3 Graeme McCutcheon [:graememcc] 2008-08-21 15:21:23 PDT
> Well, I just confirmed bug 442458, is probably about the same issue.

Although, how he ran into this in that build is a bit of a mystery. That build predates bug 263683 landing, and with "old" highlighting, you can't get the editing caret into the highlighted area (bug 256989) to change anything.

Note to self: looks like I need to add the findbar as a nsIEditActionListener, and on an EditAction event, check to see if there is still a match in that editable element. 
Comment 4 Graeme McCutcheon [:graememcc] 2008-08-24 12:29:35 PDT
Created attachment 335290 [details] [diff] [review]
Patch v1

This patch makes the findbar add itself as an nsIEditActionObserver on any editable elements it finds matches in, and then performs appropriate tests when relevant edits are made. It removes itself as an observer either when highlighting is turned off, or the text inside the editable element no longer matches.

Underpinning this patch is an assumption that the findbar will outlive any content documents in the browser: hence, the findbar never unregisters itself as an observer on the editors it observes. Otherwise, the findbar would have to maintain references to all the editors it watches, and this could lead to problems with trying to decide when to release those references when the document disappears. 

So, is this a valid assumption?
Comment 5 Graeme McCutcheon [:graememcc] 2008-08-26 11:56:36 PDT
Comment on attachment 335290 [details] [diff] [review]
Patch v1

My hg skills tripped me up - I forgot a qpop, and part of this patch ended up in a patch intended for another bug.

I'll post a new patch for review as soon as I've untangled them...
Comment 6 Graeme McCutcheon [:graememcc] 2008-09-02 06:47:56 PDT
Created attachment 336482 [details] [diff] [review]
Patch v2

This is what I meant to attach earlier
Comment 7 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-09-02 07:55:09 PDT
*** Bug 442458 has been marked as a duplicate of this bug. ***
Comment 8 Graeme McCutcheon [:graememcc] 2008-10-29 06:20:07 PDT
Created attachment 345272 [details] [diff] [review]
Patch v3

Slightly saner patch. Plus I still managed to miss some bits out of the last one.
Comment 9 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-12-13 11:23:16 PST
Comment on attachment 345272 [details] [diff] [review]
Patch v3

>diff --git a/toolkit/content/widgets/findbar.xml b/toolkit/content/widgets/findbar.xml
>--- a/toolkit/content/widgets/findbar.xml
>+++ b/toolkit/content/widgets/findbar.xml
>@@ -355,16 +355,163 @@
>               this._self._updateCaseSensitivity();
>               if (this._self.getElement("highlight").checked)
>                 this._self._setHighlightTimeout();
>               break;
>           }
>         }
>       })]]></field>
> 
>+      <field name="_editorObserver"><![CDATA[({

What's the scoping for? Shouldn't the binding implement the interface?

You should document the rational for this observer somewhere in the file.

>+        QueryInterface: function(aIID) {
>+          if (aIID.equals(Components.interfaces.nsIEditActionListener) ||
>+              aIID.equals(Components.interfaces.nsISupports))
>+            return this;
>+
>+          throw Components.results.NS_ERROR_NO_INTERFACE;
>+        },
>+
>+        getEditor: function(aNode) {

So this should be prefixed with an underscore and so should any other function that is not part of the API. It took me a little while to figure why cannot you cache the editor. Please comment on that here.

>+          var editableType = Components.interfaces.nsIDOMNSEditableElement;
>+          if (aNode instanceof editableType) return aNode;

Please don't use this code-style in either toolkit/ or browser/.

>+          var temp = aNode.parentNode;
>+          while (temp.parentNode && !(temp instanceof editableType))
>+            temp = temp.parentNode;
>+
>+          // This shouldn't happen, but guard against it
>+          if (!temp instanceof editableType)
>+            return null;
>+          return temp.editor;
>+        },
>+
>+        handleTextEdit: function(aTextNode, aOffset) {
>+          var editor = this.getEditor(aTextNode);
>+          if (!editor)
>+            return;

When  could this happen?

>@@ -595,24 +742,28 @@
>        -->
>       <method name="_highlight">
>         <parameter name="aHighlight"/>
>         <parameter name="aRange"/>
>         <parameter name="aController"/>
>         <body><![CDATA[
>           var node = aRange.startContainer;
>           var controller = aController;
>-          var isEditable = this._getEditableNode(node);
>-          if (isEditable)
>-            controller = isEditable.editor.selectionController;
>+          var editableNode = this._getEditableNode(node);
>+          if (editableNode) {
>+            controller = editableNode.editor.selectionController;
>+            editableNode.editor.addEditActionListener(this._editorObserver);
>+          }
>           var findSelection = controller.getSelection(this.nsISelectionController.SELECTION_FIND);
>           if (aHighlight)
>             findSelection.addRange(aRange);
>-          else if (isEditable)
>+          else if (editableNode) {
>             findSelection.removeAllRanges();
>+            editableNode.editor.removeEditActionListener(this._editorObserver);

Don't we need to remove it in the dtor of this binding as well? It's possible to close the browser window without un-highlighting.

r=mano with these resolved. However, if you change the code much (the leak above nerves me a little), please do ask for review again.
Comment 10 Graeme McCutcheon [:graememcc] 2009-01-23 04:10:51 PST
Created attachment 358380 [details] [diff] [review]
Patch v4

Updated per comments.

> Don't we need to remove it in the dtor of this binding as well? It's possible
> to close the browser window without un-highlighting.

So, I think we have three options here:

1) We either have to go with my assumption in comment 4, in which case the approach in the previous patch is still good

2) In the dtor for the findbar, it needs to check every editable element in every document in every tab of its browser element, look to see if there's any highlighting in that element, and try to unregister itself

3) The approach in this patch is a kind of compromise. Create anonymous listener objects, which can take care of unhooking themselves if the match is modified. However, the findbar won't maintain any references to these objects, so will be unable to unhook them when clearing the highlighting (however the listener will quickly realise there's no work to be done). It's a compromise between the potential to leak on destruction versus the potential to create lots of redundant listeners that don't get destroyed until the editor they're listening to gets destroyed. One significant problem would be a user repeatedly toggles highlighting on/off, creating multiple listener objects per editor...
Comment 11 Graeme McCutcheon [:graememcc] 2009-01-23 07:25:38 PST
Thinking about this whilst on the train, sending this from my phone, so apologies for lack of formatting! The problem of redundant observers from approach 3 above could perhaps be lessened slightly by having _handletextedit etc unhook the listener any time it finds that the find selection range count equals 0.
Comment 12 Graeme McCutcheon [:graememcc] 2009-02-01 17:15:28 PST
Comment on attachment 358380 [details] [diff] [review]
Patch v4

Scratch this, I've gone off this approach. New patch coming...
Comment 13 Graeme McCutcheon [:graememcc] 2009-02-05 14:34:29 PST
Created attachment 360805 [details] [diff] [review]
Patch v5

Right, I'm pretty sure I've got it this time.

So, like I said before, unhooking the listeners during findbar destruction could be problematic, when it came to finding a sane way to find which elements we were listening to so the could unhook itself. I was sure the only way to do that would be to hold on to a reference to each editor we are listening to. The problem then becomes how do we prevent the findbar from holding on to a reference to the editor after its owning element has been destroyed.

Enter nsIDocumentStateListener. It turns out that nsEditor sends out a notification when it's about to be destroyed, which is exactly what we need. Thus, by registering ourselves as a document state listener, we will get a notification when the element is going away, and allows us to safely stash copies of the editors we're listening to, as we can just drop them as soon as we get the notification.

So, there are 3 places where the listeners need to be unhooked:

- when the match text is changed, and it's pointless to listen any more
- when the findbar gets destroyed
- when the element gets destroyed

and this patch covers all 3. In particular, the right thing will happen regardless of the order in which the findbar or editable element(s) gets destroyed.

There are some nice additional wins we get from this:

- no longer needing to use the find API for unhighlighting, which in cases where there are a large number of matches should give a perf bump

- _lastHighlightString can go away

- best of all, the editable element listeners will work across location changes (tab switching/bfcache), which will in turn make bug 279022 significantly simpler. Things still work correctly in these situations: if a page with an element we listen to gets stashed in bfcache, and later gets expired from the cache, the document state listener will still get notified, and the findbar can still correctly unhook its listeners; likewise for a tab close.
Comment 14 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-02-05 23:49:57 PST
Comment on attachment 360805 [details] [diff] [review]
Patch v5

>diff --git a/toolkit/content/widgets/findbar.xml b/toolkit/content/widgets/findbar.xml
>--- a/toolkit/content/widgets/findbar.xml
>+++ b/toolkit/content/widgets/findbar.xml
>@@ -8,17 +8,17 @@
>    - the License. You may obtain a copy of the License at
>    - http://www.mozilla.org/MPL/
>    -
>    - Software distributed under the License is distributed on an "AS IS" basis,
>    - WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>    - for the specific language governing rights and limitations under the
>    - License.
>    -
>-   - The Original Code is mozilla.org viewsource frontend.
>+   - The Original Code is mozilla.org find frontend.

No It's Not, don't change that.


>+      <method name="_unhookListenersAtIndex">
>+        <parameter name="aIndex"/>
>+        <body><![CDATA[
>+          this._editors[aIndex].removeEditActionListener(this);
>+          this._editors[aIndex]
>+              .removeDocumentStateListener(this._stateListeners[aIndex]);
>+          this._editors.splice(aIndex, 1);
>+          this._stateListeners.splice(aIndex, 1);
>+          if (this._editors.length == 0) {
>+            this._editors = null;
>+            this._stateListeners = null;

nit: delete this._foo is cleaner.

>+          }
>+        ]]></body>
>+      </method>
>+
>+      <!--
>+        - Remove ourselves as an nsIEditActionListener and
>+        - nsIDocumentStateListener from a given cached editor
>+        - 
>+        - @param aEditor the editor we no longer wish to listen to
>+        -->
>+      <method name="_removeEditorListeners">
>+        <parameter name="aEditor"/>
>+        <body><![CDATA[
>+          // aEditor is an editor that we listen to, so therefore must be
>+          // cached. Find the index of this editor
>+          var idx = 0;
>+          while (idx < this._editors.length) {
>+            if (this._editors[idx] == aEditor)
>+              break;
>+            idx++;
>+          }
>+
>+          // Now unhook ourselves, and remove our cached copy
>+          this._unhookListenersAtIndex(idx);

See my comment on _onEditorDestruction, it applies here too.

>+        ]]></body>
>+      </method>
>+
>+      <!--
>+        - nsIEditActionListener logic follows
>+        - 
>+        - We implement this interface to allow us to catch the case where
>+        - the findbar found a match in a HTML <input> or <textarea>. If the
>+        - user adjusts the text in some way, it will no longer match, so we
>+        - want to remove the highlight, rather than have it expand/contract
>+        - when letters are added or removed.
>+        -->
>+
>+      <!--
>+        - Helper method used to check whether a selection intersects with
>+        - some highlighting
>+        -
>+        - @param aSelectionRange the range from the selection to check
>+        - @param aFindRange the highlighted range to check against
>+        - @returns true if they intersect, false otherwise
>+        -->


>+      <!-- Start of nsIEditActionListener implementations -->
>+
>+      <method name="WillDeleteText">
>+        <parameter name="aTextNode"/>
>+        <parameter name="aOffset"/>
>+        <parameter name="aLength"/>
>+        <body><![CDATA[
>+          var editor = this._getEditableNode(aTextNode).editor;
>+          var controller = editor.selectionController;
>+          var fSelection = controller.getSelection(this._findSelection);
>+          var rangeCount = fSelection.rangeCount;
>+          var rangeidx = 0;
>+          var foundContainingRange = false;
>+          var range = null;
>+
>+          // Check to see if this edit is inside one of our highlights
>+          while (!foundContainingRange && rangeidx < rangeCount) {
>+            range = fSelection.getRangeAt(rangeidx);
>+            if (range.isPointInRange(aTextNode, aOffset)) {
>+              foundContainingRange = true;
>+              break;
>+            }
>+            rangeidx++;
>+          }

Would it be possible to move all of this into a helper methods. which would return the range if it was found? Then you could use it in DidInsertText as well.

>+      <method name="WillInsertText">
>+        <parameter name="aTextNode"/>
>+        <parameter name="aOffset"/>
>+        <parameter name="aString"/>
>+        <body><![CDATA[
>+          // Unimplemented
>+        ]]></body>
>+      </method>

General comment: A common pattern is not to to list the parameters for methods which are not implemented. I don't mind if you keep it though.

>+
>+      <!-- End of nsIEditActionListener implementations -->
>+
>+      <!--
>+        - nsIDocumentStateListener logic follows
>+        -
>+        - When attaching nsIEditActionListeners, there are no guarantees
>+        - as to whether the findbar or the documents in the browser will get
>+        - destructed first. This leads to the potential to either leak, or to
>+        - hold on to a reference an editable element's editor for too long,
>+        - preventing it from being destructed.
>+        -
>+        - However, when an editor's owning node is being destroyed, the editor
>+        - sends out a DocumentWillBeDestroyed notification. We can use this to
>+        - clean up our references to the object, to allow it to be destroyed in a
>+        - timely fashion.
>+        -->
>+

Please reference to this comment within the dtor.

>+      <!--
>+        - Unhook ourselves when one of our state listeners has been called
>+        -

I would list the concrete cases that cause this. For example, i don't know if this is called when leaving a page that will be saved in the bfcache or when switching tabs.

>+        - @param the listener that was invoked
>+        -->
>+      <method name="_onEditorDestruction">
>+        <parameter name="aListener"/>
>+        <body><![CDATA[
>+          // First find the index of the editor the given listener listens to.
>+          // The listeners and editors arrays must always be in sync.
>+          // The listener will be in our array of cached listeners, as this
>+          // method could not have been called otherwise.
>+          var idx = 0;
>+          while (idx < this._stateListeners.length) {
>+            if (this._stateListeners[idx] == aListener)
>+              break;
>+            idx++;
>+          }
>+
>+          // Unhook both listeners
>+          this._unhookListenersAtIndex(idx);

If the listener wasn't found (the code suggests that this might happen, otherwise your loop condition would be different), you're unhooking the last listener.

>+        ]]></body>
>+      </method>
>+
>+      <!--
>+        - Creates a unique document state listener for an editor.
>+        -
>+        - It is not possible to simply have the findbar implement the
>+        - listener interface itself, as it wouldn't have sufficient information
>+        - to work out which editor was being destroyed. Therefore, we create new
>+        - listeners on the fly, and cache them in sync with the editors they
>+        - listen to.
>+        -->

Ugh :( First I was going to suggest that you should change this to include both listeners. Thinking about this more, the problem is with nsIDocumentStateListener. That interface should provide the document in its methods. I believe this is trivial.


>@@ -502,116 +928,126 @@
>         -->
>       <method name="_highlightDoc">
>         <parameter name="aHighlight"/>
>         <parameter name="aWord"/>
>         <parameter name="aWindow"/>
>         <body><![CDATA[
>           var win = aWindow || this.browser.contentWindow;
> 
>-          if (!aWord)
>+          var controller = this._getSelectionController(win);
>+          if (!controller) {
>+            // Without the selection controller,
>+            // we are unable to (un)highlight any matches
>             return false;
>+          }
> 
>           var textFound = false;
> 
>           for (var i = 0; win.frames && i < win.frames.length; i++) {
>             if (this._highlightDoc(aHighlight, aWord, win.frames[i]))
>               textFound = true;
>           }
> 

I know you didn't change this, but this doesn't work for iframes at all, does it?
Comment 15 Graeme McCutcheon [:graememcc] 2009-02-06 04:36:53 PST
Created attachment 360899 [details] [diff] [review]
Patch v6

Updated per comments

> If the listener wasn't found (the code suggests that this might happen,
> otherwise your loop condition would be different), you're unhooking the last
> listener.

Updated the loop conditions to imply that the listener will always be found. Or would you rather I was defensive and handle the case where it isn't (though this couldn't happen - this function being called implies we have a copy of the listener). 

> Ugh :( First I was going to suggest that you should change this to include both
> listeners. Thinking about this more, the problem is with
> nsIDocumentStateListener. That interface should provide the document in its
> methods. I believe this is trivial.

Can we file on this, and revisit this code then? I'm concerned about trying to get an API change in, even if trivial, at this stage in the game, and this bug is such a glaringly obvious bit of polish, I really want to get it in to 3.1...

> I know you didn't change this, but this doesn't work for iframes at all, does
> it?
Heh, I did - in bug 451286. It does work, _getSelectionController will always return the correct controller, allowing us to (un)highlight in iframes. That said, I've switched the code order around, so it doesn't bail out until it's tried to unhighlight subframes.

I'll fix up the test for 451286 to cover unhighlighting.
Comment 16 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-04-02 00:31:53 PDT
Comment on attachment 360899 [details] [diff] [review]
Patch v6

Please file a bug on nsIDocumentStateListener, and a corresponding bug for fixing this code. r=mano otherwise.
Comment 17 Graeme McCutcheon [:graememcc] 2009-04-03 05:57:51 PDT
Created attachment 370845 [details] [diff] [review]
hg export for checkin

For checkin.

Fixed a minor nit: moved "var textFound = false;" up before the highlighting frames code, where it belongs.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-05 21:17:37 PDT
I checked this in but had to back it out because the mochitests all failed on Mac (and possibly other platforms, they haven't cycled yet).

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1238985149.1238989092.20740.gz&fulltext=1
Comment 19 Graeme McCutcheon [:graememcc] 2009-04-06 09:48:00 PDT
Created attachment 371255 [details] [diff] [review]
Use the correct key modifier on mac

> I checked this in but had to back it out because the mochitests all failed on
> Mac (and possibly other platforms, they haven't cycled yet).

Windows and Linux both passed. I think it was just me not being mac-savvy, and not using the correct accelerator key modifier. At the moment, I'm trying (unsuccesfully) to see if anyone in #developers could run this on TryServer to see if the tests pass there first before I pop checkin-needed on this again...
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-04-06 12:53:06 PDT
You should probably file a bug to get yourself commit access.
Comment 21 Graeme McCutcheon [:graememcc] 2009-04-10 14:53:21 PDT
The updated patch passes on all platforms. Gavin kindly ran it on tryserver this morning.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1239354815.1239364313.22077.gz

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1239354815.1239363688.21154.gz

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1239354815.1239365470.25042.gz

The windows leak in Mochitest-plain is bug 478528
Comment 22 Dão Gottwald [:dao] 2009-04-17 02:48:04 PDT
http://hg.mozilla.org/mozilla-central/rev/80b6eb2851be
Comment 23 Henrik Skupin (:whimboo) 2009-04-20 07:30:12 PDT
Graeme, when the highlighting is activated in your testcase the cursor is hidden while navigating through the word. Is it a regression from another fix lately or a still outstanding fix?

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090418 Shiretoko/3.5b4pre ID:20090418032208

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090418 Minefield/3.6a1pre ID:20090418044239
Comment 24 Henrik Skupin (:whimboo) 2009-04-20 07:33:01 PDT
(In reply to comment #23)
> Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
> rv:1.9.1b4pre) Gecko/20090418 Shiretoko/3.5b4pre ID:20090418032208

This should have been Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090420 Minefield/3.6a1pre ID:20090420031158
Comment 25 Graeme McCutcheon [:graememcc] 2009-04-20 13:44:36 PDT
> Graeme, when the highlighting is activated in your testcase the cursor is
> hidden while navigating through the word. Is it a regression from another fix
> lately or a still outstanding fix?

Henrik, that is bug 256989. That bug predates the landing of bug 263683. 263683 managed to preserve that bug, even though it now has a completely different root cause!

I've been meaning to comment on that bug, will hopefully do so later this week -  I need to run some reftests and things so I know what I'm talking about when I comment. The short story is that the lack of cursor is now a layout bug, but the fix is likely to be scarily non-trivial.
Comment 26 Graeme McCutcheon [:graememcc] 2009-05-22 03:20:40 PDT
Created attachment 379103 [details] [diff] [review]
Branch patch

This has baked on trunk since 17th April.

Quite a large patch in terms of LOC, but much of this is boilerplate empty methods for implementing the nsIEditActionListener interface. Great care has been taken to ensure this patch doesn't leak editors, which would be the single biggest risk. Otherwise, this patch fixes a glaring (although admittedly not easily discoverable because of bug 256989) polish bug.

Lastly, apologies for the late nom. It made sense to me to nominate this with bug 279022, as that bug was a significant nice-to-have bug fix, which would require this code to be in the tree as well. However, 279022 is running into problems, and is now clearly not going to be fixed in time for a 3.5 nomination.
Comment 27 Mike Beltzner [:beltzner, not reading bugmail] 2009-05-26 10:49:10 PDT
Comment on attachment 379103 [details] [diff] [review]
Branch patch

I think we'll wait on this, it's a pretty big patch and the risk (although it's been on trunk, I know) of any code change here doesn't really outweigh the value IMO

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