727954 - Port |Bug 717772 - Delay autocomplete of pasted value| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Autocomplete, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1329215161.1329219819.16122.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/02/14 02:26:01
{
19557 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete_delayOnPaste.xul | Test timed out.
}

Need to port
http://hg.mozilla.org/mozilla-central/diff/19a5e75b8ed8/toolkit/content/widgets/autocomplete.xml

Comment 1

[Serge Gautherie (:sgautherie)]

Neil, do you confirm we need to port that change to the XPFE widget?
Or should this test be forced to use the Toolkit widget? (If not already.)
Or ...

Comment 2

[[e-mail]]

(In reply to Serge Gautherie from comment #1)
> Neil, do you confirm we need to port that change to the XPFE widget?
Yeah, I guess so...

> Or should this test be forced to use the Toolkit widget? (If not already.)
Nontrivial, I'm afraid, until LDAP autocomplete is sorted out...

Comment 3

[[e-mail]]

Attachment: Proposed patch [Checked in: Comment 10]

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #0)
> http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1329215161.1329219819.
> 16122.gz
> WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/02/14
> 02:26:01

{
19554 INFO TEST-START | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete_delayOnPaste.xul
### ERROR - unable to create search "simple".
19555 INFO TEST-PASS | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete_delayOnPaste.xul | Clipboard has the correct value
19556 INFO TEST-PASS | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete_delayOnPaste.xul | Value should not be autocompleted immediately - res should equal res
}

[Mozilla/5.0 (Windows NT 5.0; rv:13.0a1) Gecko/20120201 Firefox/13.0a1 SeaMonkey/2.10a1] (nightly, 2012-02-01-00-30-09-comm-central-trunk)

I tested this patch in that build, but it makes no difference:
{
0 INFO SimpleTest START
1 INFO TEST-START | chrome://mochitests/content/chrome/toolkit/content/tests/chr
ome/test_autocomplete_delayOnPaste.xul
search "simple" not found - skipping.
2 INFO TEST-PASS | chrome://mochitests/content/chrome/toolkit/content/tests/chro
me/test_autocomplete_delayOnPaste.xul | Clipboard has the correct value
3 INFO TEST-PASS | chrome://mochitests/content/chrome/toolkit/content/tests/chro
me/test_autocomplete_delayOnPaste.xul | Value should not be autocompleted immedi
ately - res should equal res
}

Is |search "simple" not found - skipping.| an issue (to fix (first))?

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 601712 [details] [diff] [review]
Proposed patch
[Checked in: Comment 10]

(I wouldn't be a reviewer for this code.)

Comment 6

[[e-mail]]

(In reply to Serge Gautherie from comment #4)
> Is |search "simple" not found - skipping.| an issue (to fix (first))?
Yes, unfortunately XPFE autocomplete can't easily defer its initialisation so the workaround is to move autoCompleteSimple.registerFactory() to the top level rather than waiting for the load event.

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) test_autocomplete_delayOnPaste.xul: Support XPFE AutoComplete widget too [Checked in: Comment 12]

Fixes comment 4 timeout/issue (only), per comment 6.

Also, unrelated but safer:
*Execute waitForExplicitFinish() before load event.
*Get $("autocomplete") after load event.
 *Ftr, it also breaks XPFE if executed before registration.

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 601712 [details] [diff] [review]
Proposed patch
[Checked in: Comment 10]

After applying patch Bv1, this fixes what that test is really about ;-)

Comment 9

[[e-mail]]

Comment on attachment 601964 [details] [diff] [review]
(Bv1) test_autocomplete_delayOnPaste.xul: Support XPFE AutoComplete widget too
[Checked in: Comment 12]

>-let gAutoComplete = $("autocomplete");
Oh yes, this is always a bad idea in XUL.

Comment 10

[[e-mail]]

Pushed changeset ace2338e90df to mozilla-central.

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 601712 [details] [diff] [review]
Proposed patch
[Checked in: Comment 10]

[Approval Request Comment]
Regression caused by (bug #): (Port Firefox enhancement to SeaMonkey.)
User impact if declined: (SeaMonkey security related.)
Testing completed (on m-c, etc.): Comment 10.
Risk to taking this patch (and alternatives if risky): None (for FF), NPOfirefoxB.
String changes made by this patch: None.

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 601964 [details] [diff] [review]
(Bv1) test_autocomplete_delayOnPaste.xul: Support XPFE AutoComplete widget too
[Checked in: Comment 12]

https://hg.mozilla.org/mozilla-central/rev/5309b25db6de


[Approval Request Comment]
Regression caused by (bug #): Bug 717772.
User impact if declined: None, but perma-orange (timeout) on SeaMonkey.
Testing completed (on m-c, etc.): This comment.
Risk to taking this patch (and alternatives if risky): None, test-only.
String changes made by this patch: None.

Comment 13

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1331027114.1331031690.5242.gz&fulltext=1
Linux comm-central-trunk debug test mochitest-other on 2012/03/06 01:45:14
{
19476 INFO TEST-END | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete_delayOnPaste.xul | finished in 2457ms
}

V.Fixed

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 601712 [details] [diff] [review]
Proposed patch
[Checked in: Comment 10]

[Approval Request Comment]
See comment 12.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 601964 [details] [diff] [review]
(Bv1) test_autocomplete_delayOnPaste.xul: Support XPFE AutoComplete widget too
[Checked in: Comment 12]

approving, test-only change.

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 601712 [details] [diff] [review]
Proposed patch
[Checked in: Comment 10]

This is an m-c patch, thus clearing comm approval flag. Please re-request accordingly. [AFAICS you had that initially, then why in the world did you change it?]

Since I don't know whether the parts may be checked in individually I'll also clear the checkin-needed flag until that is clarified.

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #16)

> This is an m-c patch, thus clearing comm approval flag. Please re-request
> accordingly. [AFAICS you had that initially, then why in the world did you
> change it?]

Because it touches an Xpfe part that is (now) used by SeaMonkey only, though it still lives in m-c:
it occurred to me that c-* approval should be enough nowadays.

Yet, I'll request approval(s) again, and ask for a confirmation on both side.

> Since I don't know whether the parts may be checked in individually

The two parts are separate issues.

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 601712 [details] [diff] [review]
Proposed patch
[Checked in: Comment 10]

[Approval Request Comment]
Please see comment 12 and comment 17:
I would also like to get an explicit "future rs+" on mozilla-* side for such Xpfe code which is (now) SeaMonkey specific but has not (yet) moved to comm-*. Thanks.

Comment 19

[Justin Wood (:Callek)]

Comment on attachment 601712 [details] [diff] [review]
Proposed patch
[Checked in: Comment 10]

its NPOTB for Firefox/B2G/Mobile, thus it can inherently land there, but as it is POTB for SeaMonkey, I'd like to still have us approve it until/unless it moves.

Its kinda a shady area of gray, and I'm sorry I didn't explicitly say why I was approving it myself when I did so. I don't think a rs+ is worthwhile, but I also don't think that we need explicit Mozilla-Driver approvals for the suite-only code here.

(We still, of course, have to follow tree rules elsewhere for these landings, like closed for maintenance, etc.)

I would mark a-m-a+ if I could, since I cant' its a-c-a+

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to Justin Wood (:Callek) from comment #19)
> its NPOTB for Firefox/B2G/Mobile, thus it can inherently land there, but as
> it is POTB for SeaMonkey, I'd like to still have us approve it until/unless
> it moves.

Then, fwiw, I think we should record this unusual case, for example, on
http://www.seamonkey-project.org/dev/project-areas
 add an Xpfe line in "Cross-suite components"
and
http://www.seamonkey-project.org/dev/review-and-flags
 add a comment in "approval-comm-XXX"

Comment 21

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c6e9c2534b04
https://hg.mozilla.org/releases/mozilla-aurora/rev/1481424a289b

Comment 22

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1331290603.1331295719.18112.gz
OS X 10.6 comm-aurora debug test mochitest-other on 2012/03/09 02:56:43

firefox12 and seamonkey2.9: verified.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8a1a95f644d4
Delay autocomplete of pasted value r=IanNDONTBUILD