[Vimperator] [Proposal]To avoid memory-leak risk on 'hlsearch'

[teramako]

diff -r b6d5a4df3137 common/content/finder.js
--- a/common/content/finder.js Thu Apr 19 21:45:23 2012 +1000
+++ b/common/content/finder.js Sat Apr 21 22:43:03 2012 +0900
@@ -34,137 +34,6 @@
this._lastSearchBackwards = false; // like "backwards", but for the last search, so if you cancel a search with <esc> this is not set
this._caseSensitive = false; // search string is case sensitive
this._linksOnly = false; // search is limited to link text only
-
- /* Stolen from toolkit.jar in Firefox, for the time being. The private
- * methods were unstable, and changed. The new version is not remotely
- * compatible with what we do.
- * The following only applies to this object, and may not be
- * necessary, or accurate, but, just in case:
- * The Original Code is mozilla.org viewsource frontend.
- *
- * The Initial Developer of the Original Code is
- * Netscape Communications Corporation.
- * Portions created by the Initial Developer are Copyright (c) 2003
- * by the Initial Developer. All Rights Reserved.
- *
- * Contributor(s):
- * Blake Ross <blake at cs.stanford.edu> (Original Author)
- * Masayuki Nakano <masayuki at d-toybox.com>
- * Ben Basson <contact at cusser.net>
- * Jason Barnabe <jason_barnabe at fastmail.fm>
- * Asaf Romano <mano at mozilla.com>
- * Ehsan Akhgari <ehsan.akhgari at gmail.com>
- * Graeme McCutcheon <graememcc_firefox at graeme-online.co.uk>
- */
- this._highlighter = {
-
- doc: null,
-
- spans: [],
-
- search: function (aWord, matchCase) {
- var finder = services.create("find");
- if (matchCase !== undefined)
- self._caseSensitive = matchCase;
-
- var range;
- while ((range = finder.Find(aWord, this.searchRange, this.startPt, this.endPt)))
- yield range;
- },
-
- highlightDoc: function highlightDoc(win, aWord) {
- this.doc = content.document; // XXX
- Array.forEach(win.frames, function (frame) this.highlightDoc(frame, aWord), this);
-
- var doc = win.document;
- if (!doc || !(doc instanceof HTMLDocument))
- return;
-
- if (!aWord) {
- let elems = this._highlighter.spans;
- for (let i = elems.length; --i >= 0;) {
- let elem = elems[i];
- let docfrag = doc.createDocumentFragment();
- let next = elem.nextSibling;
- let parent = elem.parentNode;
-
- let child;
- while ((child = elem.firstChild))
- docfrag.appendChild(child);
-
- parent.removeChild(elem);
- parent.insertBefore(docfrag, next);
- parent.normalize();
- }
- return;
- }
-
- var baseNode = <span highlight="Search"/>;
- baseNode = util.xmlToDom(baseNode, window.content.document);
-
- var body = doc.body;
- var count = body.childNodes.length;
- this.searchRange = doc.createRange();
- this.startPt = doc.createRange();
- this.endPt = doc.createRange();
-
- this.searchRange.setStart(body, 0);
- this.searchRange.setEnd(body, count);
-
- this.startPt.setStart(body, 0);
- this.startPt.setEnd(body, 0);
- this.endPt.setStart(body, count);
- this.endPt.setEnd(body, count);
-
- liberator.interrupted = false;
- let n = 0;
- for (let retRange in this.search(aWord, this._caseSensitive)) {
- // Highlight
- var nodeSurround = baseNode.cloneNode(true);
- var node = this.highlight(retRange, nodeSurround);
- this.startPt = node.ownerDocument.createRange();
- this.startPt.setStart(node, node.childNodes.length);
- this.startPt.setEnd(node, node.childNodes.length);
- if (n++ % 20 == 0)
- liberator.threadYield(true);
- if (liberator.interrupted)
- break;
- }
- },
-
- highlight: function highlight(aRange, aNode) {
- var startContainer = aRange.startContainer;
- var startOffset = aRange.startOffset;
- var endOffset = aRange.endOffset;
- var docfrag = aRange.extractContents();
- var before = startContainer.splitText(startOffset);
- var parent = before.parentNode;
- aNode.appendChild(docfrag);
- parent.insertBefore(aNode, before);
- this.spans.push(aNode);
- return aNode;
- },
-
- /**
- * Clears all search highlighting.
- */
- clear: function () {
- this.spans.forEach(function (span) {
- if (span.parentNode) {
- let el = span.firstChild;
- while (el) {
- span.removeChild(el);
- span.parentNode.insertBefore(el, span);
- el = span.firstChild;
- }
- span.parentNode.removeChild(span);
- }
- });
- this.spans = [];
- },
-
- isHighlighted: function (doc) this.doc == doc && this.spans.length > 0
- };
},

// set searchString, searchPattern, caseSensitive, linksOnly
@@ -354,26 +223,20 @@
if (config.name == "Muttator")
return;

- if (this._highlighter.isHighlighted(content.document))
- return;
-
- if (!str)
- str = this._lastSearchString;
-
- this._highlighter.highlightDoc(window.content, str);
-
- // recreate selection since highlightDoc collapses the selection
- if (window.content.getSelection().isCollapsed)
- config.browser.fastFind.findAgain(this._backwards, this._linksOnly);
-
- // TODO: remove highlighting from non-link matches (HTML - A/AREA with href attribute; XML - Xlink [type="simple"])
+ var findToolbar = document.getElementById("FindToolbar");
+ if (findToolbar) {
+ findToolbar._highlightDoc(false);
+ findToolbar._highlightDoc(true, str);
+ }
},

/**
* Clears all search highlighting.
*/
clear: function () {
- this._highlighter.clear();
+ var findToolbar = document.getElementById("FindToolbar");
+ if (findToolbar)
+ findToolbar._highlightDoc(false);
}
}, {
}, {
diff -r b6d5a4df3137 common/content/style.js
--- a/common/content/style.js Thu Apr 19 21:45:23 2012 +1000
+++ b/common/content/style.js Sat Apr 21 22:43:03 2012 +0900
@@ -205,13 +205,6 @@
HelpWarning color: red; font-weight: bold;

Logo
-
- Search,,* {
- font-size: inherit;
- padding: 0;
- color: black;
- background-color: yellow;
- }
]]></>.toString();

/**

[teramako]

ooooops, sorry for empty message

Now Vimperator 'hlsearch' uses it's own defined method.
And that method customizes content's DOM and the customized DOM pieces
are stored for restoring the original.

Unfortunately the customized DOM pieces keeps alive even when the tab is closed.
So this has memory-leak risk.

Meanwhile, Firefox's 'Highlight all' in the findbar is using special
selection and this doesn't have the risk.

My suggestion is that replace Vimperator's original code to the
findbar's method.
But this suggestion will lose some qualities:
1.)
can not do :highlight Search .... (can not or difficult to customize colors)
2.)
doing :set nohlsearch (clear highlights) effects on just only the
current tab's content

Best regards

[Martin Stubenschrott]

Hi Teramako,

Everything which makes Vimperator leaner (smaller, simpler) while not
losing essential
functionality is a good thing!

So I am highly in favor of it, and you can please push it! (with a note in
NEWS if possible, however).

Thanks again,
Martin

Post by teramako
ooooops, sorry for empty message
Now Vimperator 'hlsearch' uses it's own defined method.
And that method customizes content's DOM and the customized DOM pieces
are stored for restoring the original.
Unfortunately the customized DOM pieces keeps alive even when the tab is closed.
So this has memory-leak risk.
Meanwhile, Firefox's 'Highlight all' in the findbar is using special
selection and this doesn't have the risk.
My suggestion is that replace Vimperator's original code to the
findbar's method.
1.)
can not do :highlight Search .... (can not or difficult to customize colors)
2.)
doing :set nohlsearch (clear highlights) effects on just only the
current tab's content
Best regards
_______________________________________________
Vimperator mailing list
Vimperator at mozdev.org
https://www.mozdev.org/mailman/listinfo/vimperator

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mozdev.org/pipermail/vimperator/attachments/20120421/b877bda5/attachment.html>

[teramako]

Thanks for reply.

I committed :)

Additionally, I found out the way of changing higtlight colors
set! ui.textHighlightBackground=<colorValue>
set! ui.textHighlightForeground=<colorValue>

http://mxr.mozilla.org/mozilla-central/source/layout/generic/test/test_bug263683.html?force=1#71

Thanks

Post by Martin Stubenschrott
Hi Teramako,
Everything which makes Vimperator leaner (smaller, simpler) while not losing
essential
functionality is a good thing!
So I am highly in favor of it, and you can please push it! (with a note in
NEWS if possible, however).
Thanks again,
Martin

Post by teramako
ooooops, sorry for empty message
Now Vimperator 'hlsearch' uses it's own defined method.
And that method customizes content's DOM and the customized DOM pieces
are stored for restoring the original.
Unfortunately the customized DOM pieces keeps alive even when the tab is closed.
So this has memory-leak risk.
Meanwhile, Firefox's 'Highlight all' in the findbar is using special
selection and this doesn't have the risk.
My suggestion is that replace Vimperator's original code to the
findbar's method.
?1.)
? ?can not do :highlight Search .... (can not or difficult to customize
colors)
?2.)
? ?doing :set nohlsearch (clear highlights) effects on just only the
current tab's content
Best regards
_______________________________________________
Vimperator mailing list
Vimperator at mozdev.org
https://www.mozdev.org/mailman/listinfo/vimperator

_______________________________________________
Vimperator mailing list
Vimperator at mozdev.org
https://www.mozdev.org/mailman/listinfo/vimperator