Closed Bug 747668 Opened 7 years ago Closed 7 years ago

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

Categories

(SeaMonkey :: Autocomplete, defect, P2, major)

Tracking

(seamonkey2.8 wontfix, seamonkey2.9 wontfix, seamonkey2.10 fixed, seamonkey2.11 fixed)

RESOLVED FIXED
seamonkey2.12
Tracking Status
seamonkey2.8 --- wontfix
seamonkey2.9 --- wontfix
seamonkey2.10 --- fixed
seamonkey2.11 --- fixed

People

(Reporter: sgautherie, Assigned: sgautherie)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fixed in SM 2.10: Av1a; SM 2.11: [B-G]v1, Hv1a; SM 2.12: [C-G]v1, Hv1a; , SM 2.17a: Iv2a] [perma-orange])

Attachments

(9 files, 4 obsolete files)

17.07 KB, patch
neil
: review+
Details | Diff | Splinter Review
1.92 KB, patch
neil
: review+
Details | Diff | Splinter Review
2.70 KB, patch
neil
: review+
Details | Diff | Splinter Review
1.69 KB, patch
neil
: review+
Details | Diff | Splinter Review
1.91 KB, patch
neil
: review+
Details | Diff | Splinter Review
2.38 KB, patch
neil
: review+
iann_bugzilla
: approval-comm-aurora+
Details | Diff | Splinter Review
1.38 KB, patch
neil
: review+
iann_bugzilla
: approval-comm-aurora+
Details | Diff | Splinter Review
1.89 KB, patch
neil
: review+
iann_bugzilla
: approval-comm-aurora+
Details | Diff | Splinter Review
2.87 KB, patch
neil
: review+
Details | Diff | Splinter Review
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 neil@parkwaycc.co.uk 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.
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...
Attachment #617248 - Flags: review?(neil)
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?
(In reply to neil@parkwaycc.co.uk 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.
Av1, with your comment,
and interfaces moved to top (as in Toolkit), per IRC.
Attachment #617248 - Attachment is obsolete: true
Attachment #617248 - Flags: review?(neil)
Attachment #617258 - Flags: review?(neil)
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
}
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
Attachment #617258 - Flags: review?(neil) → review+
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 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...
Attachment #617258 - Flags: approval-comm-beta?
Attachment #617258 - Flags: approval-comm-aurora?
This patch fixes comment 0 test failures :-)

(In reply to neil@parkwaycc.co.uk 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.
Attachment #617276 - Flags: review?(neil)
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
Attachment #617258 - Attachment description: (Av1a) XPFE autocomplete.xml: Regroup nsIAutoCompleteInput properties, Fix some nits → (Av1a) XPFE autocomplete.xml: Regroup nsIAutoCompleteInput properties, Fix some nits [Checked in: Comment 10]
Whiteboard: [perma-orange]
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.)
Attachment #617276 - Flags: review?(neil) → review+
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
Attachment #617276 - Attachment description: (Bv1) Port |Bug 418712 - nsIAutoCompleteInput should fire an event when a search begins| to SeaMonkey → (Bv1) Port |Bug 418712 - nsIAutoCompleteInput should fire an event when a search begins| to SeaMonkey [Checked in: Comment 12]
Depends on: 446678
Follow-up to SM bug 446678 and older code(s).

(In reply to neil@parkwaycc.co.uk 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.
Attachment #617359 - Flags: review?(neil)
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
Attachment #617258 - Flags: approval-comm-beta?
Attachment #617258 - Flags: approval-comm-beta-
Attachment #617258 - Flags: approval-comm-aurora?
Attachment #617258 - Flags: approval-comm-aurora+
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.
Keywords: checkin-needed
Whiteboard: [perma-orange] → [c-n: 4bd6c048e63a to c-a] [perma-orange]
Whiteboard: [c-n: 4bd6c048e63a to c-a] [perma-orange] → [c-n: 4bd6c048e63a to m-a] [perma-orange]
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
Attachment #617258 - Attachment description: (Av1a) XPFE autocomplete.xml: Regroup nsIAutoCompleteInput properties, Fix some nits [Checked in: Comment 10] → (Av1a) XPFE autocomplete.xml: Regroup nsIAutoCompleteInput properties, Fix some nits [Checked in: Comments 10 and 16]
Keywords: checkin-needed
Whiteboard: [c-n: 4bd6c048e63a to m-a] [perma-orange] → [perma-orange]
Attachment #617258 - Flags: approval-comm-beta-
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.
Attachment #617276 - Flags: approval-comm-beta?
Whiteboard: [perma-orange] → [fixed in SM 2.10: Av1a; SM 2.11: Bv1] [perma-orange]
Target Milestone: seamonkey2.11 → seamonkey2.12
Attachment #617359 - Flags: review?(neil) → review+
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.
Attachment #617359 - Attachment description: (Cv1) XPFE autocomplete.xml: Regroup code into onSearchComplete() → (Cv1) XPFE autocomplete.xml: Regroup code into onSearchComplete() [Checked in: Comment 18]
Attachment #617359 - Flags: approval-comm-beta?
Attachment #617359 - Flags: approval-comm-aurora?
Whiteboard: [fixed in SM 2.10: Av1a; SM 2.11: Bv1] [perma-orange] → [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1] [perma-orange]
(In reply to neil@parkwaycc.co.uk 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().
Attachment #618873 - Flags: review?(neil)
Attachment #618873 - Flags: review?(neil) → review+
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.
Attachment #618873 - Attachment description: (Dv1) XPFE autocomplete.xml: Move code into onTextReverted() → (Dv1) XPFE autocomplete.xml: Move code into onTextReverted() [Checked in: Comment 20]
Attachment #618873 - Flags: approval-comm-beta?
Attachment #618873 - Flags: approval-comm-aurora?
Whiteboard: [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1] [perma-orange] → [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1, Dv1] [perma-orange]
(In reply to neil@parkwaycc.co.uk 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.
Attachment #620261 - Flags: review?(neil)
Attachment #620261 - Flags: review?(neil) → review+
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.
Attachment #620261 - Attachment description: (Ev1) XPFE autocomplete.xml: Move code into searchParam → (Ev1) XPFE autocomplete.xml: Move code into searchParam [Checked in: Comment 22]
Attachment #620261 - Flags: approval-comm-beta?
Attachment #620261 - Flags: approval-comm-aurora?
Whiteboard: [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1, Dv1] [perma-orange] → [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1, Dv1, Ev1] [perma-orange]
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 ;-)
Attachment #617276 - Flags: approval-comm-beta? → approval-comm-beta+
Attachment #617359 - Flags: approval-comm-beta?
Attachment #617359 - Flags: approval-comm-beta+
Attachment #617359 - Flags: approval-comm-aurora?
Attachment #617359 - Flags: approval-comm-aurora+
Attachment #618873 - Flags: approval-comm-beta?
Attachment #618873 - Flags: approval-comm-beta+
Attachment #618873 - Flags: approval-comm-aurora?
Attachment #618873 - Flags: approval-comm-aurora+
Attachment #620261 - Flags: approval-comm-beta?
Attachment #620261 - Flags: approval-comm-beta+
Attachment #620261 - Flags: approval-comm-aurora?
Attachment #620261 - Flags: approval-comm-aurora+
Keywords: checkin-needed
Whiteboard: [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1, Dv1, Ev1] [perma-orange] → [c-n: f4d242fc2534 to m-b, dae9534421a6, 9edaf29eefe6, 765f5d166172 to m-a and m-b] [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1, Dv1, Ev1] [perma-orange]
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.
Attachment #627469 - Flags: review?(neil)
Attachment #627469 - Flags: approval-comm-beta?
Attachment #627469 - Flags: approval-comm-aurora?
Attachment #627469 - Flags: review?(neil) → review+
Comment on attachment 627475 [details] [diff] [review]
(Gv1) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.popupOpen()
[Checked in: See comment 29]

(In reply to neil@parkwaycc.co.uk from comment #6)
> popupOpen: trivial

Simple port of Toolkit code.
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
Attachment #627469 - Attachment description: (Fv1) XPFE autocomplete.xml: Move selectBy() into nsIAutoCompletePopup section → (Fv1) XPFE autocomplete.xml: Move selectBy() into nsIAutoCompletePopup section [Checked in: Comment 27]
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.]
Attachment #627475 - Flags: review?(neil) → review+
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).
Attachment #627475 - Attachment description: (Gv1) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.popupOpen() → (Gv1) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.popupOpen() [Checked in: See comment 29]
Attachment #627475 - Flags: approval-comm-beta?
Attachment #627475 - Flags: approval-comm-aurora?
(In reply to neil@parkwaycc.co.uk 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
Attachment #627517 - Flags: review?(neil)
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?
Attachment #627469 - Flags: approval-comm-aurora? → approval-comm-aurora+
Attachment #627475 - Flags: approval-comm-aurora? → approval-comm-aurora+
Whiteboard: [c-n: f4d242fc2534 to m-b, dae9534421a6, 9edaf29eefe6, 765f5d166172 to m-a and m-b] [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1, Dv1, Ev1] [perma-orange] → [c-n: f4d242fc2534 to m-b; dae9534421a6, 9edaf29eefe6, 765f5d166172 to m-a and m-b; cff5b4470690, 2ea752a2c768 to m-a] [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1, Dv1, Ev1] [perma-orange]
(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 neil@parkwaycc.co.uk 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.
(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.
Hv1, with comment 31 suggestion(s).
Attachment #627517 - Attachment is obsolete: true
Attachment #627517 - Flags: review?(neil)
Attachment #628359 - Flags: review?(neil)
Whiteboard: [c-n: f4d242fc2534 to m-b; dae9534421a6, 9edaf29eefe6, 765f5d166172 to m-a and m-b; cff5b4470690, 2ea752a2c768 to m-a] [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1, Dv1, Ev1] [perma-orange] → [c-n: f4d242fc2534 to m-b; dae9534421a6, 9edaf29eefe6, 765f5d166172 to m-a and m-b; cff5b4470690, 2ea752a2c768 to m-a] [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1, Dv1, Ev1, Fv1, Gv1a] [perma-orange]
Attachment #628359 - Flags: review?(neil) → review+
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.
Attachment #628359 - Attachment description: (Hv1a) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.textValue → (Hv1a) XPFE autocomplete.xml: Implement nsIAutoCompleteInput.textValue [Checked in: Comment 35]
Attachment #628359 - Flags: approval-comm-beta?
Attachment #628359 - Flags: approval-comm-aurora?
Whiteboard: [c-n: f4d242fc2534 to m-b; dae9534421a6, 9edaf29eefe6, 765f5d166172 to m-a and m-b; cff5b4470690, 2ea752a2c768 to m-a] [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1, Dv1, Ev1, Fv1, Gv1a] [perma-orange] → [c-n: f4d242fc2534 to m-b; dae9534421a6, 9edaf29eefe6, 765f5d166172 to m-a and m-b; cff5b4470690, 2ea752a2c768 to m-a] [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1, Dv1, Ev1, Fv1, Gv1a, Hv1a] [perma-orange]
(In reply to neil@parkwaycc.co.uk from comment #6)
> searchCount: feasible
> getSearchAt: feasible

I adapted Toolkit code.
Attachment #628428 - Flags: review?(neil)
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.)
Attachment #628428 - Flags: review?(neil)
Attachment #628359 - Flags: approval-comm-aurora? → approval-comm-aurora+
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)
Attachment #617276 - Flags: approval-comm-beta+ → approval-comm-beta-
Attachment #617359 - Flags: approval-comm-beta+ → approval-comm-beta-
Attachment #618873 - Flags: approval-comm-beta+ → approval-comm-beta-
Attachment #620261 - Flags: approval-comm-beta+ → approval-comm-beta-
Attachment #627469 - Flags: approval-comm-beta? → approval-comm-beta-
Attachment #627475 - Flags: approval-comm-beta? → approval-comm-beta-
Attachment #628359 - Flags: approval-comm-beta? → approval-comm-beta-
Whiteboard: [c-n: f4d242fc2534 to m-b; dae9534421a6, 9edaf29eefe6, 765f5d166172 to m-a and m-b; cff5b4470690, 2ea752a2c768 to m-a] [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1, Dv1, Ev1, Fv1, Gv1a, Hv1a] [perma-orange] → [c-n: dae9534421a6, 9edaf29eefe6, 765f5d166172, cff5b4470690, 2ea752a2c768, f28d1ec8bd33 to m-a] [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1, Dv1, Ev1, Fv1, Gv1a, Hv1a] [perma-orange]
Iv1, with comment 37 suggestion(s).
Attachment #628428 - Attachment is obsolete: true
Attachment #629145 - Flags: review?(neil)
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.]
Depends on: 760455
(In reply to neil@parkwaycc.co.uk 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.]
(In reply to Serge Gautherie from comment #41)
> (In reply to neil@parkwaycc.co.uk 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.
Keywords: checkin-needed
Whiteboard: [c-n: dae9534421a6, 9edaf29eefe6, 765f5d166172, cff5b4470690, 2ea752a2c768, f28d1ec8bd33 to m-a] [fixed in SM 2.10: Av1a; SM 2.11: Bv1; SM 2.12: Cv1, Dv1, Ev1, Fv1, Gv1a, Hv1a] [perma-orange] → [fixed in SM 2.10: Av1a; SM 2.11: [B-G]v1, Hv1a; SM 2.12: [C-G]v1, Hv1a] [perma-orange]
Attachment #629223 - Flags: review?(neil) → review+
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/1a1dbdad81da

Time to stick a fork in this bug.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in SM 2.10: Av1a; SM 2.11: [B-G]v1, Hv1a; SM 2.12: [C-G]v1, Hv1a] [perma-orange] → [fixed in SM 2.10: Av1a; SM 2.11: [B-G]v1, Hv1a; SM 2.12: [C-G]v1, Hv1a; , SM 2.17a: Iv2a] [perma-orange]
Attachment #629223 - Attachment description: (Iv2a) XPFE autocomplete.xml: Implement (nsIAutoCompleteInput) searchCount and getSearchAt(), Nits → (Iv2a) XPFE autocomplete.xml: Implement (nsIAutoCompleteInput) searchCount and getSearchAt(), Nits [Checked in: Comment 45]
Pushed by frgrahl@gmx.net:
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.
You need to log in before you can comment on or make changes to this bug.