313149 - the code of findBar.js should be protected in a prototype

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, enhancement, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, Firefox's FastFind/FAYT's codes are included by some files.
But the functions and variables are defined on global.
This is not safety. We should protect these functions and variables with prototype.

# See bug 312913.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Everyone:

I'm working on this. Please don't allow to check-in to findBar.js on Trunk for several days.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0(-u8pw)

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

NOTE:
I didn't change any logic.
I only wrapped all variables and functions with gFindBar prototype.

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 201441 [details] [diff] [review]
Patch rv1.0(-u8pw)

prevailing style uses this.foo() instead of gFindBar.foo() where possible.

Anyway, this is good cleanup overall for trunk (and possibly Fx2)

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #5)
> (From update of attachment 201441 [details] [diff] [review] [edit])
> prevailing style uses this.foo() instead of gFindBar.foo() where possible.
> 
> Anyway, this is good cleanup overall for trunk (and possibly Fx2)
> 

Mike:

See my comment in the patch. If we use this.foo(), when the method is called by timers or event listeners, |this| keyword doesn't refer to |gFindBar|. Then this.foo() is broken.
I link this.foo(). But if I rewrite so, the patch doesn't work fine :-(

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

I provide another way that is using this.foo() style.
But in this patch, we need global functions for event handlers and for timer handlers. However, these name always have "findBar_" prefix.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0(-u8pw)

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Mike:

Please choose from these patches.

rv1.0 uses gFindBar.foo() style, But it doesn't have global functions.
rv2.0 uses this.foo() style, But it has global functions that is having "findBar_" prefix in its name.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.1

We need more changing for browser.js.
If you grant to rv1.0, I will change it too.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.1(-u8pw)

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 201518 [details] [diff] [review]
Patch rv2.1

I like this approach better, thanks!

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in to Trunk. Thanks.


NOTE:

This patch is very big. So, it may have regression.
But don't reopen this bug. Please file a new bug and adding to CC me.

Comment 14

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

This patch is suitable for the branch, right? All found regressions have been fixed, from what I can tell? Masayuki, since the branch and trunk are going to be synced, it would be great if you could ask for approval and land this on the 1.8 branch as well (with the patches from bug 314986 and bug 328036).

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #14)
> This patch is suitable for the branch, right? All found regressions have been
> fixed, from what I can tell? Masayuki, since the branch and trunk are going to
> be synced, it would be great if you could ask for approval and land this on the
> 1.8 branch as well (with the patches from bug 314986 and bug 328036).
> 

We need to work for checking-in to 1.8 branch. Because 1.8's code is changed by Places...
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/toolkit/components/typeaheadfind/content/findBar.js&rev=HEAD&mark=1.35
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/toolkit/components/typeaheadfind/content/findBar.js&rev=MOZILLA_1_8_BRANCH&mark=1.21.2.11

Comment 16

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

(In reply to comment #15)
> We need to work for checking-in to 1.8 branch. Because 1.8's code is changed
> by Places...

How hard would it be to do this merging? If you don't have enough time to do it, let me know and I will.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If we also fix bug 313653 and bug 321481 they are fixed on trunk after this, it is easy work. I don't have much time. If you can work on this, please do it.

Comment 18

[Phil Ringnalda (:philor)]

Attachment: Branch rollup

Rollup with fixes from bug 313653, bug 314986, bug 321481, and bug 328036.

Comment 19

[Mike Connor [:mconnor]]

Comment on attachment 220272 [details] [diff] [review]
Branch rollup

Phil, can you point pkasting to this bug, since he's doing other things here.

Comment 20

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

Attachment 220272 [details] [diff] checked in on the 1.8 branch.

Comment 21

[Scott MacGregor]

Hmm could this fix have broken Thunderbird's findbar on the 1.8.1 branch?

See Bug 338185