Closed
Bug 490028
Opened 14 years ago
Closed 14 years ago
JavaScript strict warning in chrome://messenger-newsblog/content/Feed.js and feed-parser.js
Categories
(MailNews Core :: Feed Reader, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.1
People
(Reporter: mnyromyr, Assigned: mnyromyr)
References
()
Details
Attachments
(4 files, 1 obsolete file)
1019 bytes,
patch
|
myk
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
1.11 KB,
patch
|
myk
:
review+
|
Details | Diff | Splinter Review |
1.50 KB,
patch
|
myk
:
review+
|
Details | Diff | Splinter Review |
2.16 KB,
patch
|
myk
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
On starting up my SeaMonkey trunk (1.9.1) build, I get JavaScript strict warning: chrome://messenger-newsblog/content/Feed.js, line 24: reference to undefined property this.mFeeds[this.normalizeHost(aUrl)] dumped to the shell thrice, although they don't appear in the Error Console...
Assignee | ||
Comment 1•14 years ago
|
||
By now they do, accompanied by Warning: anonymous function does not always return a value Source File: chrome://messenger-newsblog/content/Feed.js Line: 160, Column: 16 Source Code: }, and Warning: assignment to undeclared variable url Source File: chrome://messenger-newsblog/content/feed-parser.js Line: 502
Summary: JavaScript strict warning in chrome://messenger-newsblog/content/Feed.js → JavaScript strict warning in chrome://messenger-newsblog/content/Feed.js and feed-parser.js
Comment 2•14 years ago
|
||
(In reply to comment #1) Let's fix these first. > Warning: anonymous function does not always return a value > Source File: chrome://messenger-newsblog/content/Feed.js > Line: 160, Column: 16 > Source Code: > }, Right, blamed to sayrer: http://hg.mozilla.org/comm-central/rev/cb36617b6a11 > Warning: assignment to undeclared variable url > Source File: chrome://messenger-newsblog/content/feed-parser.js > Line: 502 Right, blamed to rq: http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mail/extensions/newsblog/content/feed-parser.js&mark=1.18
Blocks: 491720
Comment 3•14 years ago
|
||
(In reply to comment #2) > Right, blamed to sayrer: > Right, blamed to rq: Swap sayrer <-> rq.
Comment 4•14 years ago
|
||
Line 160 is not my code: http://hg.mozilla.org/comm-central/annotate/cb36617b6a11/mailnews/extensions/newsblog/content/Feed.js#l160 or am I mistaken?
Comment 5•14 years ago
|
||
(In reply to comment #4) > or am I mistaken? Yes, you are quoting the line from the bare source file, whereas the error reports the line from the "preprocessed" file.
Comment 7•14 years ago
|
||
so, how can I check the preprocessed file?..
Comment 8•14 years ago
|
||
(In reply to comment #7) > so, how can I check the preprocessed file?.. Any build should do. But what is the need? The error and the code are explicit enough, no?
Comment 9•14 years ago
|
||
not for me, no. Plus, looking at the commit link, I don't think i've introduced an anonymous function, did I? Sorry, you'll have to educate me more on that if you want me to fix this. Or just do it yourself... I'm a total noob wrt mozilla inner workings.
Comment 10•14 years ago
|
||
well, unless you just want me to change this line: http://hg.mozilla.org/comm-central/annotate/cb36617b6a11/mailnews/extensions/newsblog/content/Feed.js#l194 to: return feed.parse(); // parse will asynchronously call the download callback when it is done That, I can do :)
Comment 11•14 years ago
|
||
or I could replace this (line 183): return Feed.prototype.onDownloadError(aEvent); with this: { Feed.prototype.onDownloadError(aEvent); return; } so that it never returns anything.
Comment 12•14 years ago
|
||
(In reply to comment #9) > Plus, looking at the commit link, I don't think i've introduced an anonymous > function, did I? You modified 177 onDownloaded: function(aEvent) Noone said you created it. > Sorry, you'll have to educate me more on that if you want me to fix this. 183 return Feed.prototype.onDownloadError(aEvent); (now) returns a value, but 194 feed.parse(); // parse will asynchronously call the download callback when it is done 195 }, (still) does not > Or just do it yourself... I'm a total noob wrt mozilla inner workings. I have no idea what this function should do: let's reopen bug 491720! It's not (only) "mozilla", it's _javascript_ in the first place.
Comment 13•14 years ago
|
||
(In reply to comment #12) > (In reply to comment #9) > > Plus, looking at the commit link, I don't think i've introduced an anonymous > > function, did I? > > You modified > 177 onDownloaded: function(aEvent) > Noone said you created it. > > > Sorry, you'll have to educate me more on that if you want me to fix this. > > 183 return Feed.prototype.onDownloadError(aEvent); > (now) returns a value, > > but > 194 feed.parse(); // parse will asynchronously call the download callback when > it is done > 195 }, > (still) does not > > > Or just do it yourself... I'm a total noob wrt mozilla inner workings. > > I have no idea what this function should do: let's reopen bug 491720! > It's not (only) "mozilla", it's _javascript_ in the first place. yeah, righ. The secret is that Feed.prototype.onDownloadError(aEvent) never returns anything, so basically, if I get it right, returning its result is the same as returning nothing. Thus technically, this function never returns a value. CC'ing Myk. Myk, would you help me with this? Is solution in Comment #11 the right thing to do?
Comment 14•14 years ago
|
||
(In reply to comment #13) > The secret is that Feed.prototype.onDownloadError(aEvent) never returns > anything, As you can see, neither the JSEngine nor me did guess that. > returning its result is the same as returning nothing. Except that asking to return a non-existent value does not make sense in the first place. > Is solution in Comment #11 the right thing to do? It obviously is ;-)
Comment 15•14 years ago
|
||
OK, so I can provide a patch next week, get a review etc. But if someone else does (blink blink), I'd be grateful. :)
Updated•14 years ago
|
Component: Backend → Feed Reader
QA Contact: backend → feed.reader
Hardware: x86 → All
Updated•14 years ago
|
Comment 16•14 years ago
|
||
(In reply to comment #14) > (In reply to comment #13) > Except that asking to return a non-existent value does not make sense in the > first place. > > > Is solution in Comment #11 the right thing to do? > > It obviously is ;-) Yup, I concur, this is the right solution.
Comment 17•14 years ago
|
||
Thanks for waiting, here's the patch I promised.
Attachment #380936 -
Flags: review?(myk)
Comment 18•14 years ago
|
||
Comment on attachment 380936 [details] [diff] [review] [committed] Feed.js patch > if (request.status < 200 || request.status >= 300) >+ { Nits: join the two lines.
Comment 19•14 years ago
|
||
Bah, blame vim. Fixed locally, waiting for r+ to commit.
Comment 20•14 years ago
|
||
Comment on attachment 380936 [details] [diff] [review] [committed] Feed.js patch Looks good! r=myk with Serge's nit fixed.
Attachment #380936 -
Flags: superreview?(bienvenu)
Attachment #380936 -
Flags: review?(myk)
Attachment #380936 -
Flags: review+
Updated•14 years ago
|
Attachment #380936 -
Flags: superreview?(bienvenu) → superreview+
Comment 21•14 years ago
|
||
Comment on attachment 380936 [details] [diff] [review] [committed] Feed.js patch thx!
Comment 22•14 years ago
|
||
http://hg.mozilla.org/comm-central/rev/72082583daf3 Not marking fixed since feed-parser.js is still there, right?
Whiteboard: Feed.js fixed
Comment 23•14 years ago
|
||
I guess this is what feed-parser.js patch should look like, right?
Attachment #381153 -
Flags: superreview?
Attachment #381153 -
Flags: review?
Updated•14 years ago
|
Attachment #380936 -
Attachment description: the patch → [committed] Feed.js patch
Updated•14 years ago
|
Attachment #381153 -
Flags: superreview?(bienvenu)
Attachment #381153 -
Flags: superreview?
Attachment #381153 -
Flags: review?(myk)
Attachment #381153 -
Flags: review?
Comment 24•14 years ago
|
||
Comment on attachment 381153 [details] [diff] [review] feed-parser.js patch you can just return ioService.newURI(alink.baseURI, null, null).resolve(alink.getAttribute('href'));
Comment 25•14 years ago
|
||
Attachment #381153 -
Attachment is obsolete: true
Attachment #381333 -
Flags: superreview?(bienvenu)
Attachment #381333 -
Flags: review?(myk)
Attachment #381153 -
Flags: superreview?(bienvenu)
Attachment #381153 -
Flags: review?(myk)
Updated•14 years ago
|
Attachment #381333 -
Flags: review?(myk) → review+
Comment 26•14 years ago
|
||
Comment on attachment 381333 [details] [diff] [review] [committed] feed-parser.js patch, v.1.1 Great! r=myk
Comment 27•14 years ago
|
||
Comment on attachment 381333 [details] [diff] [review] [committed] feed-parser.js patch, v.1.1 OK, since it's bienvenu's suggestion, I took liberty to commit it without his superreview: http://hg.mozilla.org/comm-central/rev/7f60d259141f
Attachment #381333 -
Attachment description: feed-parser.js patch, v.1.1 → [committed] feed-parser.js patch, v.1.1
Attachment #381333 -
Flags: superreview?(bienvenu)
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Whiteboard: Feed.js fixed
Comment 28•14 years ago
|
||
(In reply to comment #0) > JavaScript strict warning: chrome://messenger-newsblog/content/Feed.js, line > 24: reference to undefined property this.mFeeds[this.normalizeHost(aUrl)] Karsten, are you still seeing this (initial) warning?
Assignee | ||
Comment 29•14 years ago
|
||
(In reply to comment #28) > (In reply to comment #0) > > JavaScript strict warning: chrome://messenger-newsblog/content/Feed.js, line > > 24: reference to undefined property this.mFeeds[this.normalizeHost(aUrl)] > > Karsten, are you still seeing this (initial) warning? Yes, thrice, in the very same line.
Updated•14 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 30•14 years ago
|
||
Code is { 57 getFeed: function (aUrl) 58 { 59 return this.mFeeds[this.normalizeHost(aUrl)]; 60 }, }
Assignee | ||
Comment 31•14 years ago
|
||
The basic problem here is the implementation of FeedCache: mFeeds is initialized as of type Array, but used as an Object. The first time a feed is sought in the cache, everything is fine - the cache is empty and of type Array, the given URL is not a valid index, no problem. Then mFeeds is treated as (and internally turned into) an Object when adding the feed using the normalized URL as an index. Any subsequent call to getFeed with a yet unknown feed URL will result in a strict warning...
Assignee: nobody → mnyromyr
Attachment #381955 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #381955 -
Flags: review? → review?(myk)
Comment 32•14 years ago
|
||
Comment on attachment 381955 [details] [diff] [review] fix the real cause [pushed: comment #33] I haven't built and tested SeaMonkey with this patch, but based on visual inspection and grepping the code for references to mFeeds, this is obviously correct. r=myk Note that progressNotifier::mFeeds in newsblog.js has the same problem of being initialized to an Array while being used as an object.
Attachment #381955 -
Flags: review?(myk) → review+
Assignee | ||
Comment 33•14 years ago
|
||
Pushed as <http://hg.mozilla.org/comm-central/rev/6062bc855a1c>.
Status: NEW → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Attachment #381955 -
Attachment description: fix the real cause → fix the real cause [pushed: comment #33]
Assignee | ||
Updated•14 years ago
|
Whiteboard: [ToDo: comment 0+30]
Comment 34•14 years ago
|
||
(In reply to comment #32) > Note that progressNotifier::mFeeds in newsblog.js has the same problem of being > initialized to an Array while being used as an object. http://mxr.mozilla.org/comm-central/search?string=mFeeds&case=on&find=%2Fnewsblog%5C.js%24 Untested, but seems trivial.
Attachment #382588 -
Flags: review?(myk)
Updated•14 years ago
|
Attachment #382588 -
Flags: superreview?(bienvenu)
Attachment #382588 -
Flags: review?(myk)
Attachment #382588 -
Flags: review+
Comment 35•14 years ago
|
||
Comment on attachment 382588 [details] [diff] [review] (Dv1) Fix newsblog.js too [Checkin: Comment 36] This looks great and seems to behave the same in my tests. r=myk
Updated•14 years ago
|
Attachment #382588 -
Flags: superreview?(bienvenu) → superreview+
Comment 36•14 years ago
|
||
Comment on attachment 382588 [details] [diff] [review] (Dv1) Fix newsblog.js too [Checkin: Comment 36] http://hg.mozilla.org/comm-central/rev/4ee756629083
Attachment #382588 -
Attachment description: (Dv1) Fix newsblog.js too → (Dv1) Fix newsblog.js too
[Checkin: Comment 36]
Updated•14 years ago
|
Target Milestone: --- → mozilla1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•