Closed
Bug 1105099
Opened 8 years ago
Closed 3 years ago
JavaScript strict warning: resource:///modules/DirectoryLinksProvider.jsm, line 399: ReferenceError: reference to undefined property link.enhancedImageURI
Categories
(Firefox :: New Tab Page, defect)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: dholbert, Assigned: Mardak)
References
Details
Attachments
(1 file, 2 obsolete files)
1.34 KB,
patch
|
adw
:
review+
|
Details | Diff | Splinter Review |
Noticed this go by in my debug-spew when starting Firefox (debug build, fresh profile): { JavaScript strict warning: resource:///modules/DirectoryLinksProvider.jsm, line 399: ReferenceError: reference to undefined property link.enhancedImageURI } Code quote: > 395 return rawLinks.filter(link => { > 396 // Make sure the link url is allowed and images too if they exist > 397 return this.isURLAllowed(link.url, ALLOWED_LINK_SCHEMES) && > 398 this.isURLAllowed(link.imageURI, ALLOWED_IMAGE_SCHEMES) && > 399 this.isURLAllowed(link.enhancedImageURI, ALLOWED_IMAGE_SCHEMES); Looks like this line was added in bug 1088729. Mardak, should we be sanity-checking for a missing "enhancedImageURI" here? (Or should all 'link' variables here have a defined enhancedImageURI, in which case this is a busted 'link' object?)
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(edilee)
Assignee | ||
Comment 1•8 years ago
|
||
Not all link objects have an enhancedImageURI and isURLAllowed short circuits to true if there's no value. Not sure what's the common pattern to avoid having that warning trigger.
Flags: needinfo?(edilee)
Reporter | ||
Comment 2•8 years ago
|
||
I'm not sure what the common pattern is either, but based on MDN documentation, it looks like "in" should work relatively elegantly: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/in so I think we'd have e.g.: this.isURLAllowed("enhancedImageURI" in link ? link.enhancedImageURI : null, ALLOWED_IMAGE_SCHEMES)
Assignee | ||
Comment 3•8 years ago
|
||
Reading out values with let {} destructuring seems to not trigger the warning.
Assignee | ||
Comment 4•8 years ago
|
||
Or one fewer total lines by destructuring in the function parameters.
Comment 5•8 years ago
|
||
Comment on attachment 8528779 [details] [diff] [review] v1.1 Review of attachment 8528779 [details] [diff] [review]: ----------------------------------------------------------------- I think link.enhancedImageURI || null would work too.
Attachment #8528779 -
Flags: review+
Updated•8 years ago
|
Attachment #8528777 -
Flags: review?(adw)
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #5) > I think link.enhancedImageURI || null would work too. Oh! You are indeed quite right. Using that instead as it makes things more explicit that we can have an optional enhancedImageURI.
Attachment #8528777 -
Attachment is obsolete: true
Attachment #8528779 -
Attachment is obsolete: true
Comment 7•8 years ago
|
||
Comment on attachment 8529188 [details] [diff] [review] v2 Review of attachment 8529188 [details] [diff] [review]: ----------------------------------------------------------------- OK. To be clear, I liked both your previous patches too, but I'll defer to you to choose the one you like best. :-)
Attachment #8529188 -
Flags: review+
Reporter | ||
Comment 8•8 years ago
|
||
(In reply to Ed Lee :Mardak from comment #6) > > I think link.enhancedImageURI || null would work too. > Oh! You are indeed quite right. Using that instead as it makes things more > explicit that we can have an optional enhancedImageURI. Are you sure that actually silences the warning? I'd assume that would still count as a "reference to undefined property" (since we'd still be trying to grab the property, before we know if it's there). The second-to-last example in this stackoverflow question... https://stackoverflow.com/questions/3789277/javascript-reference-to-undefined-property ...increases my suspicion about that. (The example is: "if (!mySidebar.context.netProgress)", and netProgress isn't defined, and the questioner says that triggers a warning.)
Comment 9•8 years ago
|
||
It does. The rules of strict mode are pretty abstruse. :-/
Comment 10•3 years ago
|
||
Hello!
This bug has been closed due to inactivity and/or the potential for this bug to no longer be an issue with the new Discovery Stream-powered New Tab experience.
Please help us triage by reopening if this issue still persists and should be addressed.
Thanks!
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•