Closed Bug 1160114 Opened 9 years ago Closed 9 years ago

Strict mode warnings on faceted search

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 40.0

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
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
Attached patch glodawarnings.diff (obsolete) — Splinter Review
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)
Attachment #8599826 - Flags: review?(mkmelin+mozilla)
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+
(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.
Right, that was the wrong context. 
So adding it in the  |if (aFirstTime)| sounds like a plan, no?
(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)
Assignee: nobody → aleth
Attachment #8600714 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
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.