747668 - Port |Bug 495277 - autocomplete.xml should not use new Function()| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Autocomplete, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1334784642.1334790794.21467.gz
WINNT 5.2 comm-aurora debug test mochitest-other on 2012/04/18 14:30:42
{
19647 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete5.xul | onsearchbegin handler has been correctly called. - got false, expected true
19650 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete5.xul | onsearchbegin handler has been correctly called. - got false, expected true
}

*****

(In reply to [e-mail] from bug 707039 comment #6)
> (In reply to Serge Gautherie from bug 707039 comment #5)
> > Should XPFE implement more/all of 'nsIAutoCompleteInput'?
> This is really designed so that the C++ backend can communicate with the
> XBL; we've already implemented all we need for form fill to work, so
> anything else might not be worthwhile.

It looks like we do miss at least some more of this interface afterall.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) XPFE autocomplete.xml: Regroup nsIAutoCompleteInput properties, Fix nits

Cf
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsIAutoCompleteInput.idl

First step to (further) resync' with Toolkit.
In addition to moving code, I simplified 2 onget() which used to be:
{
      <property name="maxRows"
                onget="var t = parseInt(this.getAttribute('maxrows')); return t ? t : 0;"/>

      <property name="timeout"
                onget="var t = parseInt(this.getAttribute('timeout')); return t ? t : 0;"/>
}

NB: I hope following work will (help to) fix some other autocomplete test failures/hangs already filed...

Comment 2

[[e-mail]]

Comment on attachment 617248 [details] [diff] [review]
(Av1) XPFE autocomplete.xml: Regroup nsIAutoCompleteInput properties, Fix nits

>+      <!-- =================== nsIAccessibleProvider =================== -->
> 
>-      <!-- nsIAccessibleProvider --> 
This is a subheading of the main public properties heading, so it makes sense to have it as a "smaller" heading. You might want to add "properties" though.
 
>+      <!-- XXX: Work in progress to support "at least as much as Toolkit does" of nsIAutoCompleteInput... -->
"XXX: This implementation is currently incomplete."

>-      <property name="editable" readonly="true" onget="return true;" />  
>+      <property name="editable" readonly="true"
>+                onget="return true;" />
Why these changes?

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #2)

> >+      <!-- =================== nsIAccessibleProvider =================== -->
> > 
> >-      <!-- nsIAccessibleProvider --> 
> This is a subheading of the main public properties heading, so it makes
> sense to have it as a "smaller" heading. You might want to add "properties"
> though.

Would using '-' instead of '=' be an acceptable compromise?
'=' is more like Toolkit, which doesn't have the "PROPERTIES" headings.
Current (bare) comments are not very/enough noticeable, I think.

I'm not sure we want to add "properties": there can be methods too, for example.
I would agree to add 'interface' if you think it's worth it.

> >+      <!-- XXX: Work in progress to support "at least as much as Toolkit does" of nsIAutoCompleteInput... -->
> "XXX: This implementation is currently incomplete."

As you prefer.
Hopefully, it's just temporary, until we (can) add this interface to 'implements='.

> >-      <property name="editable" readonly="true" onget="return true;" />  
> >+      <property name="editable" readonly="true"
> >+                onget="return true;" />
> Why these changes?

No other reason than make things more alike, within the file and wrt Toolkit.

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) XPFE autocomplete.xml: Regroup nsIAutoCompleteInput properties, Fix some nits [Checked in: Comments 10 and 16]

Av1, with your comment,
and interfaces moved to top (as in Toolkit), per IRC.

Comment 5

[Serge Gautherie (:sgautherie)]

Ftr/Fwiw,
{
<NeilAway>	fyi, form fill doesn't use that part of autocomplete, which is why we didn't add the interface to the implements
}

Comment 6

[[e-mail]]

Notes on missing attributes:

controller: we don't use one, the autocomplete controls itself.
popupOpen: trivial
completeSelectedIndex: rename our autoFill to match?
showImageColumn: feasible
searchParam: trivial
searchCount: feasible
getSearchAt: feasible
textValue: trivial
selectionStart: inherited
selectionEnd: inherited
selectTextRange: trivial, but should be inherited setSelectionRange instead
onSearchBegin: no idea
onSearchComplete: no idea
onTextEntered: no idea
onTextReverted: trivial
consumeRollupEvent: feasible

Comment 7

[Serge Gautherie (:sgautherie)]

Ftr,
Bug 495277 (port) itself is targeted at SM 2.10(+),
yet (at least some of) XPFE autocomplete.xml resync' would apply to previous branches,
but we're too close to SM 2.9 release so the latter is just WontFix there :-|

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 617258 [details] [diff] [review]
(Av1a) XPFE autocomplete.xml: Regroup nsIAutoCompleteInput properties, Fix some nits
[Checked in: Comments 10 and 16]

[Approval Request Comment]
No risk, 98% just moving code around.

Reminder: this file lives in mozilla-*, but is SeaMonkey specific.

NB:
I would like a blanket-approval for upcoming patches to resync' autocomple.xml wrt comment 6 too.
I'm requesting both c-a and c-b because SM 2.10 (= the target) will very soon move from the former to the latter...

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Port |Bug 418712 - nsIAutoCompleteInput should fire an event when a search begins| to SeaMonkey [Checked in: Comment 12]

This patch fixes comment 0 test failures :-)

(In reply to [e-mail] from comment #6)

> Notes on missing attributes:

Thanks: that helps wrt what I had/would manage to figure out :-)

> controller: we don't use one, the autocomplete controls itself.

Copy of Toolkit nsAutoCompleteController::StartSearch() code to XPFE autocomplete.xml startLookup().

> onSearchBegin: no idea

Trivial copy of Toolkit code.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 617258 [details] [diff] [review]
(Av1a) XPFE autocomplete.xml: Regroup nsIAutoCompleteInput properties, Fix some nits
[Checked in: Comments 10 and 16]

https://hg.mozilla.org/mozilla-central/rev/4bd6c048e63a

Comment 11

[[e-mail]]

Comment on attachment 617276 [details] [diff] [review]
(Bv1) Port |Bug 418712 - nsIAutoCompleteInput should fire an event when a search begins| to SeaMonkey
[Checked in: Comment 12]

Aha, so in fact we already fire a searchcomplete event, we just don't have a separate method for it. (It got added in bug 446678, so I need to work out which version of SeaMonkey/Thunderbird that event landed on, so that I can update MDN. Also let me know which version this event lands on, or update MDN.)

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 617276 [details] [diff] [review]
(Bv1) Port |Bug 418712 - nsIAutoCompleteInput should fire an event when a search begins| to SeaMonkey
[Checked in: Comment 12]

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

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) XPFE autocomplete.xml: Regroup code into onSearchComplete() [Checked in: Comment 18]

Follow-up to SM bug 446678 and older code(s).

(In reply to [e-mail] from comment #6)

> controller: we don't use one, the autocomplete controls itself.

Ftr, Toolkit call is in nsAutoCompleteController::PostSearchCleanup().

> onSearchComplete: no idea

Regroup our code from XPFE autocomplete.xml postSearchCleanup().
I added '"' to setAttribute() value, as Toolkit has, though I'm not sure which syntax is the best.

Comment 14

[Justin Wood (:Callek)]

Comment on attachment 617258 [details] [diff] [review]
(Av1a) XPFE autocomplete.xml: Regroup nsIAutoCompleteInput properties, Fix some nits
[Checked in: Comments 10 and 16]

a=me for SeaMonkey 2.10/Mozilla-13

Comment 15

[Serge Gautherie (:sgautherie)]

Callek, what about the following?

(In reply to Serge Gautherie (:sgautherie) from comment #8)
> I would like a blanket-approval for upcoming patches to resync'
> autocomple.xml wrt comment 6 too.

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 617258 [details] [diff] [review]
(Av1a) XPFE autocomplete.xml: Regroup nsIAutoCompleteInput properties, Fix some nits
[Checked in: Comments 10 and 16]

http://hg.mozilla.org/releases/mozilla-aurora/rev/39dbe3601ed8

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 617276 [details] [diff] [review]
(Bv1) Port |Bug 418712 - nsIAutoCompleteInput should fire an event when a search begins| to SeaMonkey
[Checked in: Comment 12]

[Approval Request Comment]
Low risk, Adds a missing event.

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 617359 [details] [diff] [review]
(Cv1) XPFE autocomplete.xml: Regroup code into onSearchComplete()
[Checked in: Comment 18]

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


[Approval Request Comment]
No risk, 99% just moving code around.

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) XPFE autocomplete.xml: Move code into onTextReverted() [Checked in: Comment 20]

(In reply to [e-mail] from comment #6)

Follow-up to old (cvs) code.

> controller: we don't use one, the autocomplete controls itself.

Ftr, Toolkit call is in nsAutoCompleteController::RevertTextValue().

> onTextReverted: trivial

Move our code from XPFE autocomplete.xml undoAutoComplete().

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 618873 [details] [diff] [review]
(Dv1) XPFE autocomplete.xml: Move code into onTextReverted()
[Checked in: Comment 20]

https://hg.mozilla.org/mozilla-central/rev/9edaf29eefe6


[Approval Request Comment]
No risk, simply moving code around.

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) XPFE autocomplete.xml: Move code into searchParam [Checked in: Comment 22]

(In reply to [e-mail] from comment #6)

Follow-up to old (cvs) code.

> controller: we don't use one, the autocomplete controls itself.

Ftr, Toolkit call is in nsAutoCompleteController::StartSearch().

> searchParam: trivial

Move our code from XPFE autocomplete.xml mAutoCompleteSession.

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 620261 [details] [diff] [review]
(Ev1) XPFE autocomplete.xml: Move code into searchParam
[Checked in: Comment 22]

https://hg.mozilla.org/mozilla-central/rev/765f5d166172


[Approval Request Comment]
No risk, simply moving code around.

Comment 23

[Justin Wood (:Callek)]

Comment on attachment 617276 [details] [diff] [review]
(Bv1) Port |Bug 418712 - nsIAutoCompleteInput should fire an event when a search begins| to SeaMonkey
[Checked in: Comment 12]

a+ but this needs to land in m-beta ;-)

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1) XPFE autocomplete.xml: Move selectBy() into nsIAutoCompletePopup section [Checked in: Comment 27]

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsIAutoCompletePopup.idl
This interface is fully implemented :-)


[Approval Request Comment]
No risk, simply moving code around.

Comment 25

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.popupOpen() [Checked in: See comment 29]

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 627475 [details] [diff] [review]
(Gv1) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.popupOpen()
[Checked in: See comment 29]

(In reply to [e-mail] from comment #6)
> popupOpen: trivial

Simple port of Toolkit code.

Comment 27

[Serge Gautherie (:sgautherie)]

Comment on attachment 627469 [details] [diff] [review]
(Fv1) XPFE autocomplete.xml: Move selectBy() into nsIAutoCompletePopup section
[Checked in: Comment 27]

http://hg.mozilla.org/mozilla-central/rev/cff5b4470690

Comment 28

[[e-mail]]

Comment on attachment 627475 [details] [diff] [review]
(Gv1) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.popupOpen()
[Checked in: See comment 29]

>+                onset="if (val) { this.openPopup(); } else { this.closePopup(); }"/>
No {}s please, just copy the toolkit version. r=me with that fixed.
[In theory we should return val; too. I don't mind if you don't do that.]

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 627475 [details] [diff] [review]
(Gv1) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.popupOpen()
[Checked in: See comment 29]

http://hg.mozilla.org/mozilla-central/rev/2ea752a2c768
Gv1, with comment 28 suggestion(s).

Comment 30

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.textValue

(In reply to [e-mail] from comment #6)
> textValue: trivial

Simple port of Toolkit code.

Can setTextValue() be merged into onset()?
http://mxr.mozilla.org/comm-central/search?string=setTextValue

Comment 31

[[e-mail]]

Comment on attachment 627517 [details] [diff] [review]
(Hv1) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.textValue

>+                onset="this.setTextValue(val); return this.value;"/>
Nit: please return val;

>+          this.value = aValue;
>+
>           // Completing a result should simulate the user typing the result,
>           // so fire an input event.
>-          this.value = aValue;
Why this change?

Comment 32

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #30)
> Can setTextValue() be merged into onset()?
> http://mxr.mozilla.org/comm-central/search?string=setTextValue

Neil, what about this question?


(In reply to [e-mail] from comment #31)
> Comment on attachment 627517 [details] [diff] [review]
> (Hv1) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.textValue
> 
> >+                onset="this.setTextValue(val); return this.value;"/>
> Nit: please return val;

Agreed.
Toolkit returns 'this.value' too, but that might just be an "error", or would need a comment...

> >+          this.value = aValue;
> >+
> >           // Completing a result should simulate the user typing the result,
> >           // so fire an input event.
> >-          this.value = aValue;
> Why this change?

Only to "unconfuse" comment and related code.

Comment 33

[[e-mail]]

(In reply to Serge Gautherie from comment #30)
> Can setTextValue() be merged into onset()?
It could, but it's not worth changing it.

(In reply to Serge Gautherie from comment #32)
> Toolkit returns 'this.value' too, but that might just be an "error", or
> would need a comment...
It was originally "return this.value = val;" but when bryner added the event he forgot that "this.value = val; return this.value;" is subtly different.

> Only to "unconfuse" comment and related code.
Fair enough.

Comment 34

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1a) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.textValue [Checked in: Comment 35]

Hv1, with comment 31 suggestion(s).

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 628359 [details] [diff] [review]
(Hv1a) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.textValue
[Checked in: Comment 35]

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


[Approval Request Comment]
No risk, property addition.

Comment 36

[Serge Gautherie (:sgautherie)]

Attachment: (Iv1) XPFE autocomplete.xml: Implement (nsIAutoCompleteInput) searchCount and getSearchAt()

(In reply to [e-mail] from comment #6)
> searchCount: feasible
> getSearchAt: feasible

I adapted Toolkit code.

Comment 37

[[e-mail]]

Comment on attachment 628428 [details] [diff] [review]
(Iv1) XPFE autocomplete.xml: Implement (nsIAutoCompleteInput) searchCount and getSearchAt()

I'm not sure I want to go to such lengths until after we move ldap to toolkit autocomplete, and therefore can rip out all the XPFE autocomplete code, and therefore can simplify the code, such as making search sessions lazy.

But you could return sessionCount for the searchCount property, or maybe just rename sessionCount to searchCount (although you wouldn't be able to ensure it was readonly in that case.)

Comment 38

[Justin Wood (:Callek)]

Comment on attachment 617276 [details] [diff] [review]
(Bv1) Port |Bug 418712 - nsIAutoCompleteInput should fire an event when a search begins| to SeaMonkey
[Checked in: Comment 12]

After internal discussion and being this late in the game, I'm only beta-approving regression fixes and heavy crashers, this can still land for 2.11 (will apply to all the beta approvals I marked, your whiteboard is confusing, will leave you to untangle)

Comment 39

[Serge Gautherie (:sgautherie)]

Attachment: (Iv2) XPFE autocomplete.xml: Implement (nsIAutoCompleteInput) searchCount and getSearchAt(), Nits

Iv1, with comment 37 suggestion(s).

Comment 40

[[e-mail]]

Comment on attachment 629145 [details] [diff] [review]
(Iv2) XPFE autocomplete.xml: Implement (nsIAutoCompleteInput) searchCount and getSearchAt(), Nits

>+          throw Components.results.NS_ERROR_INVALID_ARG;
Nit: toolkit method doesn't throw.

>-          var idx = 0;
>+          var idx = -1;
>           for (var name in this.mSessions) {
>-            if (idx == aIndex)
>+            if (++idx == aIndex)
>               return this.mSessions[name];
>-            ++idx;
>           }
Was there any particular reason you changed the style?
[Nit: {}s are no longer necessary.]

Comment 41

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #40)
> Comment on attachment 629145 [details] [diff] [review]

> >+          throw Components.results.NS_ERROR_INVALID_ARG;
> Nit: toolkit method doesn't throw.

Yes, I noticed: Bug 760455 Submitted.

The equivalent of Toolkit behavior would be to "return undefined;".
Do you prefer that (ftb)?

> >-          var idx = 0;
> >+          var idx = -1;
> >           for (var name in this.mSessions) {
> >-            if (idx == aIndex)
> >+            if (++idx == aIndex)
> >               return this.mSessions[name];
> >-            ++idx;
> >           }
> Was there any particular reason you changed the style?

It just seemed less code and more sync' between idx and name :-|

> [Nit: {}s are no longer necessary.]

Comment 42

[[e-mail]]

(In reply to Serge Gautherie from comment #41)
> (In reply to [e-mail] from comment #40)
> > (From update of attachment 629145 [details] [diff] [review])
> > >+          throw Components.results.NS_ERROR_INVALID_ARG;
> > Nit: toolkit method doesn't throw.
> The equivalent of Toolkit behavior would be to "return undefined;".
> Do you prefer that (ftb)?
My preference would be to return one of "" or null.

Comment 43

[Serge Gautherie (:sgautherie)]

Attachment: (Iv2a) XPFE autocomplete.xml: Implement (nsIAutoCompleteInput) searchCount and getSearchAt(), Nits [Checked in: Comment 45]

Iv2, with comment 40 and comment 42 suggestion(s).

Comment 44

[Jens Hatlak (:InvisibleSmiley)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/11ac8b585ae3
http://hg.mozilla.org/releases/mozilla-aurora/rev/8c479f0b1edb
http://hg.mozilla.org/releases/mozilla-aurora/rev/ad5fab74c8fd
http://hg.mozilla.org/releases/mozilla-aurora/rev/f4f10979fdf8
http://hg.mozilla.org/releases/mozilla-aurora/rev/0dae08b004a1
http://hg.mozilla.org/releases/mozilla-aurora/rev/60318baf2bc3

Comment 45

[Philip Chee]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/1a1dbdad81da

Time to stick a fork in this bug.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cb26ffb49e69
(Av1a) XPFE autocomplete.xml: Regroup nsIAutoCompleteInput properties, Fix some nits. r=neil.
https://hg.mozilla.org/comm-central/rev/13ae00e9b517
(Bv1) Port |Bug 418712 - nsIAutoCompleteInput should fire an event when a search begins| to SeaMonkey. r=neil.
https://hg.mozilla.org/comm-central/rev/86b125b9cad9
(Cv1) XPFE autocomplete.xml: Regroup code into onSearchComplete(). r=neil.
https://hg.mozilla.org/comm-central/rev/775954d54d20
(Dv1) XPFE autocomplete.xml: Move code into onTextReverted(). r=neil.
https://hg.mozilla.org/comm-central/rev/9a9b4f7a44f5
(Ev1) XPFE autocomplete.xml: Move code into searchParam. r=neil.
https://hg.mozilla.org/comm-central/rev/a7e6e3d7bcfb
(Fv1) XPFE autocomplete.xml: Move selectBy() into nsIAutoCompletePopup section. r=neil.
https://hg.mozilla.org/comm-central/rev/0aa7d3704949
(Gv1a) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.popupOpen(). r=neil.
https://hg.mozilla.org/comm-central/rev/3f5c7f2c7b70
(Hv1a) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.textValue. r=neil.
https://hg.mozilla.org/comm-central/rev/8288831a9dd7
(Iv2a) XPFE autocomplete.xml: Implement (nsIAutoCompleteInput) searchCount and getSearchAt(), Nits. r=Neil.