Closed
Bug 1160114
Opened 9 years ago
Closed 9 years ago
Strict mode warnings on faceted search
Categories
(Thunderbird :: General, defect)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 40.0
People
(Reporter: aleth, Assigned: aleth)
Details
Attachments
(1 file, 1 obsolete file)
3.55 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
JavaScript strict warning: components/glautocomp.js, line 128: ReferenceError: reference to undefined property thing.subject JavaScript strict warning: components/glautocomp.js, line 86: ReferenceError: reference to undefined property this._results[aIndex] JavaScript strict warning: chrome://messenger/content/glodaFacetBindings.xml, line 301: ReferenceError: reference to undefined property this.trueValues JavaScript strict warning: chrome://messenger/content/glodaFacetBindings.xml, line 305: ReferenceError: reference to undefined property this.trueValues
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8599826 -
Flags: review?(acelists)
Comment on attachment 8599826 [details] [diff] [review] glodawarnings.diff Review of attachment 8599826 [details] [diff] [review]: ----------------------------------------------------------------- So is it expected that those object attributes do not exist? Depending on what they are, I think it would be cleaner to initialize them to null where they are created. Or can we test them with ("attr" in object) ? I am not very used to these (reference1 || reference2) JS games ;) They may be correct, I just do not have enough of a feel for JS yet to review them. Please try somebody else.
Attachment #8599826 -
Flags: review?(acelists)
Assignee | ||
Updated•9 years ago
|
Attachment #8599826 -
Flags: review?(mkmelin+mozilla)
Comment 4•9 years ago
|
||
Comment on attachment 8599826 [details] [diff] [review] glodawarnings.diff Review of attachment 8599826 [details] [diff] [review]: ----------------------------------------------------------------- For the trueValues, I think you should instead move this.trueValues = []; up the function where it's set, so we never return before getting there. For consistency, best to move realTrueGroups, falseValues and selectNodes too. r=mkmelin with that
Attachment #8599826 -
Flags: review?(mkmelin+mozilla) → review+
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Magnus Melin from comment #4) > Comment on attachment 8599826 [details] [diff] [review] > glodawarnings.diff > > For the trueValues, I think you should instead move > this.trueValues = []; > up the function where it's set, so we never return before getting there. For > consistency, best to move realTrueGroups, falseValues and selectNodes too. Is it possible you were looking at the wrong context? There's no realTrueGroups, falseValues, or selectNodes in the facet-boolean binding. I decided against moving the this.trueValues = [] line since it is not clear to me this should happen every time the build method is called - it may overwrite an existing value. And if one just adds it inside the |if (aFirstTime)| clause to ensure the property always exists, one probably still has to keep it where it is now as that line may be being used to reset the value too. I'm not confident about making assumptions about this code.
Comment 6•9 years ago
|
||
Right, that was the wrong context. So adding it in the |if (aFirstTime)| sounds like a plan, no?
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Magnus Melin from comment #6) > Right, that was the wrong context. > So adding it in the |if (aFirstTime)| sounds like a plan, no? OK.
Attachment #8599826 -
Attachment is obsolete: true
Attachment #8600714 -
Flags: review?(mkmelin+mozilla)
Updated•9 years ago
|
Assignee: nobody → aleth
Updated•9 years ago
|
Attachment #8600714 -
Flags: review?(mkmelin+mozilla) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 8•9 years ago
|
||
https://hg.mozilla.org/comm-central/rev/b10df189b89b
Assignee | ||
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
You need to log in
before you can comment on or make changes to this bug.
Description
•