Closed
Bug 1482230
Opened 7 years ago
Closed 6 years ago
Strict errors during test: localName can be undefined
Categories
(Firefox :: Search, defect, P5)
Firefox
Search
Tracking
()
RESOLVED
FIXED
Firefox 63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: mkaply, Assigned: dpino)
Details
(Keywords: good-first-bug)
Attachments
(1 file, 3 obsolete files)
1.43 KB,
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
I'm seeing strict errors in running some tests:
pid:5928 JavaScript strict warning: file:///Users/michaelkaply/Projects/mozilla-unified/obj-x86_64-apple-darwin16.7.0/dist/Nightly.app/Contents/Resources/components/nsSearchService.js, line 1870: ReferenceError: reference to undefined property "localName"
0:01.17 pid:5928 JavaScript strict warning: file:///Users/michaelkaply/Projects/mozilla-unified/obj-x86_64-apple-darwin16.7.0/dist/Nightly.app/Contents/Resources/components/nsSearchService.js, line 1789: ReferenceError: reference to undefined property "localName"
0:01.17 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "localName"" {file: "file:///Users/michaelkaply/Projects/mozilla-unified/obj-x86_64-apple-darwin16.7.0/dist/Nightly.app/Contents/Resources/components/nsSearchService.js" line: 1870}]"
0:01.17 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "localName"" {file: "file:///Users/michaelkaply/Projects/mozilla-unified/obj-x86_64-apple-darwin16.7.0/dist/Nightly.app/Contents/Resources/components/nsSearchService.js" line: 1789}]"
We're assuming localName in certain places in the code, but #text nodes don't have a localName.
Assignee | ||
Comment 1•6 years ago
|
||
I attempted to fix this bug, but I didn't manage to reproduce it. Could you provide more info about what tests to run to reproduce it?
Flags: needinfo?(mozilla)
Reporter | ||
Comment 2•6 years ago
|
||
Sure.
See:
https://bugzilla.mozilla.org/show_bug.cgi?id=1482085
toolkit/components/places/tests/unifiedcomplete/test_PlacesSearchAutocompleteProvider.js
should show it.
Thanks!
Flags: needinfo?(mozilla)
Comment 3•6 years ago
|
||
Yes, run
./mach xpcshell-test toolkit/components/places/tests/unifiedcomplete/test_PlacesSearchAutocompleteProvider.js
and watch the output.
Text nodes don't have an attribute 'localName' so I think this should be changed to 'nodeName'.
Assignee | ||
Comment 4•6 years ago
|
||
Thank you very much for the pointers!
So the main reason for this error was that when iterating the document's nodes (i.e searchplugins/google.xml) for comparing nodes against a tag name ("Param", "MozParam", "ShortName", etc), the iteration was done using 'childNodes', which returns all nodes of a document (including text nodes) [1]. If I recall correctly, an element node always includes a text node, which explains where the text nodes were coming from.
So I changed the function 'childNodes' for 'children' which returns only element nodes. In addition, I replaced all references of 'localName' for 'nodeName', since 'localName' is obsolete according to [2].
[1] https://developer.mozilla.org/en-US/docs/Web/API/Node/childNodes
[2] https://developer.mozilla.org/en-US/docs/Web/API/Node/localName
Attachment #9000171 -
Flags: review?(adw)
Comment 5•6 years ago
|
||
Comment on attachment 9000171 [details] [diff] [review]
Skip text nodes when iterating document nodes
Review of attachment 9000171 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks! This works, and I hate to ask you to change it, but while you're right that localName is obsolete on Node, it's not obsolete on Element: https://developer.mozilla.org/en-US/docs/Web/API/Element/localName We could switch to nodeName, but I don't see a good reason to do that, so would you mind reverting those lines? (_data is set to documentElement, so even in that case, localName is OK.) Switching from childNodes to children is all that's needed to fix this -- and thanks for catching that.
Attachment #9000171 -
Flags: review?(adw)
Updated•6 years ago
|
Assignee: nobody → dpino
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•6 years ago
|
||
I agree if in fact there's no justified reason to change localName for nodeName, it makes to leave it as it is. Patch updated.
Attachment #9000171 -
Attachment is obsolete: true
Assignee | ||
Comment 7•6 years ago
|
||
Adjust commit message to right format.
Attachment #9003032 -
Attachment is obsolete: true
Attachment #9003062 -
Flags: review?(adw)
Comment 8•6 years ago
|
||
Comment on attachment 9003062 [details] [diff] [review]
Bug-1482230-Skip-text-nodes-when-iterating-document-.patch
Review of attachment 9003062 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks!
Attachment #9003062 -
Flags: review?(adw) → review+
Comment 9•6 years ago
|
||
Could you please submit a patch without the
---
toolkit/components/search/nsSearchService.js | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
This text would be committed to our code base.
The two lines above
When iterating document's nodes, use 'children' instead of 'childNodes'
to retrieve element nodes only.
are OK.
When you do, please change the commit message to:
Bug 1482230 - Skip text nodes when iterating document nodes. r=adw.
Thank you.
Comment 10•6 years ago
|
||
Sorry, without the trailing fullstop, so:
Bug 1482230 - Skip text nodes when iterating document nodes. r=adw
Comment 12•6 years ago
|
||
Comment on attachment 9003303 [details] [diff] [review]
Bug-1482230-Skip-text-nodes-when-iterating-document-.patch
You can simply say:
Added reviewer to commit message, carrying forward Drew's (:adw) r+ :-)
Attachment #9003303 -
Flags: review+
Updated•6 years ago
|
Keywords: checkin-needed
Comment 13•6 years ago
|
||
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/394f359ada2d
Skip text nodes when iterating document nodes. r=adw
Keywords: checkin-needed
Comment 14•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in
before you can comment on or make changes to this bug.
Description
•