997638 - JavaScript strict warning: chrome://messenger/content/virtualFolderProperties.js, line 71: assignment to undeclared variable folderNameField

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: Suggested Patch

During running |make mozmill| test suite of full debug build of 
comm-central TB, I have begun seeing the following warning for some time now.

JavaScript strict warning: chrome://messenger/content/virtualFolderProperties.js, line 71: assignment to undeclared variable folderNameField

I checked and found out that
folderNameField is not properly declared and used in the file, 
mailnews/base/content/virtualFolderProperties.js

I am not sure if the variable is shared among the
routines in the file, but I assumed so, and
created the patch attached.

This patch has eliminated the "undeclared variable folderNameField" message.

TIA

Comment 1

[ISHIKAWA, Chiaki]

Comment on attachment 8408076 [details] [diff] [review]
Suggested Patch

Requesting a review to [e-mail].

TIA

Comment 2

[alta88]

Comment on attachment 8408076 [details] [diff] [review]
Suggested Patch

Review of attachment 8408076 [details] [diff] [review]:
-----------------------------------------------------------------

The var name refers to 2 different dom nodes even though the same name is reused, so it's not really a global.  I think the better solution is to just declare 

let folderNameField =

in line 71.

r+ with that (I'm probably ok to r for this nit).

Comment 3

[ISHIKAWA, Chiaki]

Attachment: Patch: carrying over r+ = [e-mail]

> The var name refers to 2 different dom nodes even though the same name is
> reused, so it's not really a global.  I think the better solution is to just
> declare 
> 
> let folderNameField =
> 
> in line 71.
> 
> r+ with that (I'm probably ok to r for this nit).

Thank you for the clarification.

Does this look OK?

Comment 4

[alta88]

yes, thanks!

Comment 5

[ISHIKAWA, Chiaki]

(In reply to alta88 from comment #4)
> yes, thanks!

Thank you.
I am going to put checkin-needed keyword.
Maybe Ryan will figure out if we need someone else's review.

TIA

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Why do I need to be the one to tell you that a module peer needs to review this?

Comment 7

[ISHIKAWA, Chiaki]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #6)
> Why do I need to be the one to tell you that a module peer needs to review
> this?

Oops. Sorry about this. My attention span was not covering this bugzilla after reading comment 2 :-)

According to https://wiki.mozilla.org/Modules/Thunderbird 
it seems only 
Owner: 	David Bienvenu (:bienvenu), Mark Banner (:standard8),
can answer this?
Hard to believe, but there is  no "(recommended)" mark appearing next to the
box shown for requesting review. So I am putting Mark Banner in the field.

TIA

Comment 8

[ISHIKAWA, Chiaki]

Thank you.
I put the checkin-needed keyword.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/2888ec23050f

Comment 10

[:aceman]

(In reply to ISHIKAWA, Chiaki from comment #7)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #6)
> > Why do I need to be the one to tell you that a module peer needs to review
> > this?
> 
> Oops. Sorry about this. My attention span was not covering this bugzilla
> after reading comment 2 :-)
> 
> According to https://wiki.mozilla.org/Modules/Thunderbird 

You do not need to escalate to the top level owners for small stuff like this. You first looks into the various modules/components listed there.
Also, as this stuff was in /mailnews, you need to look at https://wiki.mozilla.org/Modules/MailNews_Core .
The virtual folders are a subset of MailNews Core::Search .