495277 - autocomplete.xml should not use new Function()

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

autocomplete should not use new Function, especially not in code that's executed each time the user types a key in the urlbar:

  http://hg.mozilla.org/mozilla-central/file/tip/toolkit/content/widgets/autocomplete.xml#l403

fireEvent() is only called from 4 different places, to handle onsearchbegin, onsearchcomplete, ontextreverted, ontextentered.  We should just have properties on autocomplete to store those, and if a string is set we should turn it into a function object once.  Then we should just get rid of fireEvent and just call the right event handling function directly.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Performance-wise, it would be best to just ignore dynamic changes to these attributes (onsearchbegin, onsearchcomplete, ontextreverted, and ontextentered). Would that be an acceptable trade-off?

Comment 2

[Felix Fung (:felix)]

Attachment: Reduce calls to new Function in Autocomplete.xml

- Took to suggestion in comment 1 and ignored setting these properties after autocomplete is constructed. As far as I can tell from searching the codebase, none of these properties are modified dynamically.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 572664 [details] [diff] [review]
Reduce calls to new Function in Autocomplete.xml

>diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml

>       <constructor><![CDATA[
>         mController = Components.classes["@mozilla.org/autocomplete/controller;1"].
>                         getService(Components.interfaces.nsIAutoCompleteController);
>+
>+        mSearchBeginHandler = this.initEventHandler("searchbegin");
>+        mSearchCompleteHandler = this.initEventHandler("searchcomplete");
>+        mTextEnteredHandler = this.initEventHandler("textentered");
>+        mTextRevertedHandler = this.initEventHandler("textreverted");

Use this.mSearchBeginHandler, etc. And fix mController while you're here too?

>+      <method name="initEventHandler">

>+          let handlerString = this.getAttribute("on" + aEventType);
>+          if (handlerString) {
>+            return new Function("eventType", "param", handlerString);

You could return:

(new Function("eventType", "param", handlerString)).bind(this, aEventType);

and then just call the function directly (without .apply, at least for those without additional parameters) from the handlers.

Are there tests for this functionality? There should be, shouldn't be too hard to add.

Cancelling the review request pending response to these comments.

Comment 4

[Felix Fung (:felix)]

Attachment: Reduce calls to new Function in Autocomplete.xml

Additional Changes:
- Addressed above comments

Notes:
- toolkit/content/tests/chrome/test_autocomplete[2..4].xul seem to cover the relevant functionality.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Felix Fung (:felix) from comment #4)
> - toolkit/content/tests/chrome/test_autocomplete[2..4].xul seem to cover the
> relevant functionality.

None of those tests appear to cover e.g. onsearchbegin="foo()" attributes on an xul:autocomplete input, which is what you're changing here.

Comment 6

[Felix Fung (:felix)]

Attachment: Reduce calls to new Function in Autocomplete.xml

Additional Changes:
- Added tests

Comment 7

[Dão Gottwald [:dao]]

Could you please use _ instead of the m prefix for new fields?

Comment 8

[Felix Fung (:felix)]

Attachment: Reduce calls to new Function in Autocomplete.xml

Additional Changes:
- Prefixed new fields with _ instead of m.

Comment 9

[Felix Fung (:felix)]

Where should we comment that making dynamic changes on these properties (e.g. onsearchbegin) does not work as one would expect and instead would do nothing.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

https://developer.mozilla.org/En/XUL/Textbox_%28Toolkit_autocomplete%29

Comment 11

[Felix Fung (:felix)]

Updated docs.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f01f200f798a

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f01f200f798a