Noticed this go by in the logcat spew, while running a debug Fennec build today: { GeckoConsole(25873): [JavaScript Warning: "in strict mode code, functions may be declared only at top level or immediately within another function" {file: "chrome://browser/content/Readability.js" line: 436 column: 15 source: " function purgeNode(node) { E/GeckoConsole(25873): "}] } Link to code: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Readability.js?rev=f932b3812c63#436 The issue is that this function is declared inside the scope of a "while" loop, rather than at the top level scope of the enclosing function. For more, see the first answer here: http://stackoverflow.com/questions/5758042/which-js-function-declaration-syntax-is-correct-according-to-the-standard [hg blame says this was introduced by bug 779796 -- adding dependency]

This is still present in latest m-c code.

Hi, Is this bug still open? I can't find the file Readability.js in my local repository, instead there is a Reader.js.

(In reply to shreyas from comment #4) > Hi, > > Is this bug still open? I can't find the file Readability.js in my local > repository, instead there is a Reader.js. Yes, this bug is still open. The Readability.js file is located here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/content/Readability.js This is shared by desktop Firefox and Firefox for Android, so you can make either a desktop build or an Android build to fix this bug.

Comment on attachment 8554471 [details] [diff] [review] function initialization added Review of attachment 8554471 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/reader/content/Readability.js @@ +432,5 @@ > * JSDOMParser returns static node lists, not live ones. When we remove > * an element from the document, we need to manually remove it - and all > * of its children - from the allElements array. > */ > + purgeNode(node); I'm a bit confused by this change.. could you explain what you're trying to do here? This warning is about where the purgeNode function is declared, so I don't think calling it elsewhere will fix things.

I tried to apply what the thread on bug-1071877 meant

Thanks -- but I don't think bug 1071877 has any relevance to this particular bug here. (And even if it did -- you're adding a call to a function, which changes behavior. This bug is just about a coding-style issue, which triggers a warning -- so any fix for this should be sure not to change the code's actual behavior.)

As noted in comment 0, I think the correct fix for this is to move this whole function -- 'function purgeNode(node) { ..etc.. }' -- to be up just before the "while" loop opens, & maybe add a comment just before the function in that new spot, to explain why it's there, saying something like: // Helper function, used below in 'while' loop:

Comment on attachment 8555059 [details] [diff] [review] bug958735_jswarning4.diff Review of attachment 8555059 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/reader/content/Readability.js @@ +416,5 @@ > + purgeNode(node.childNodes[i]); > + } > + if (node._index !== undefined && allElements[node._index] == node) > + delete allelements[node._index]; > + } Much better, thanks! Please just update this patch to use 2-space indentation (we use 2-space indentation in our JS files), and then I'll mark the new patch as r+ and ready for check-in :)

Indentation updated.

Comment on attachment 8555187 [details] [diff] [review] bug958735_jswarning4.diff Review of attachment 8555187 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/reader/content/Readability.js @@ +415,5 @@ > + for (let i = node.childNodes.length; --i >= 0;) { > + purgeNode(node.childNodes[i]); > + } > + if (node._index !== undefined && allElements[node._index] == node) > + delete allelements[node._index]; Oops, looks like a typo here - should be allElements (capital E). Also, looking more closely at the function bode here, I just realized that this won't work, since allElements is a local variable defined within the while loop. To fix this, you can update this purgeNode helper function to also take allElements as a parameter. Sorry for not catching this earlier! I believe we should hit this _grabArticle function any time that we try to parse an article in reader mode, so you should make sure you test a page in reader mode to make sure there aren't any errors. If you make a desktop Firefox build, you can test reader mode by flipping "reader.parse-on-load.enabled" to true in about:config to make a reader mode button appear in the toolbar. You could also test on Firefox for Android, where this feature is enabled by default.

I couldn't test it. The reader mode button doesn't seem to appear in my location bar in nightly.

(In reply to shreyas from comment #15) > Created attachment 8555385 [details] [diff] [review] > bug958735_jswarning4.diff > > I couldn't test it. The reader mode button doesn't seem to appear in my > location bar in nightly. Did you try flipping the about:config pref that Margaret indicated in comment 14? I confirmed that I get the icon, if I flip that pref and visit e.g. https://blog.mozilla.org/blog/2015/01/13/firefox-enables-you-to-experience-and-share-more-on-the-web/

I had to add the pref in my about:config in nightly. The pref is set to true. I visited https://blog.mozilla.org/blog/2015/01/13/firefox-enables-you-to-experience-and-share-more-on-the-web/ but couldn't find the reader mode button in my location bar.

(Weird -- the pref (reader.parse-on-load.enabled) already exists in about:config for me -- no need to add it. I can see it by just typing "reader" into the about:config search bar, and then I can double-click it to change it from false to true. Maybe you mistyped when searching for it / adding it?)

No I checked the spelling, even copy pasted it. I had to manually add it.

Something must be wrong then... We've got it set it in all.js (the main place where pref defaults is set), and then we override it in firefox.js (to make it default to off). Each of those should make it show up in about:config. MXR search, for reference: http://mxr.mozilla.org/mozilla-central/search?string=reader.parse-on-load.enabled Perhaps your Nightly is out of date? It looks like these lines in all.js and firefox.js only got added last week, in this push: http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=ad3867772421 (Help|About should give you a datestamp to see your Nightly's datestamp.)

(Or alternately, if you're using your own local build, that's probably using out-of-date source code & needs updating in order for you to be able to test reader mode on Desktop.)

I tried running hg pull and hg update and build it. But there's no change in nightly.

Got it, how should I proceed now?

You got the icon? Great! Now you just want to test it & make sure your patch doesn't somehow break it, I suppose. (circling back to comment 14). i.e. poke around at it without your patch, and then poke around at it with your patch, and make sure it behaves the same. (Margaret may have more concrete suggestions for what to test. I don't imagine you have to be too thorough, since this is unlikely to break stuff.) Also, I'd say you want to make sure that: (a) you can actually see this warning in the Error Console (Ctrl + Shift + J) without your patch, when you visit a page that supports reader mode (like the blog.mozilla.org URL I linked above) (b) the warning goes away after you apply your patch & rebuild. (To see the warning, you'll need to flip the pref "javascript.options.strict" to "true", and you'll have to have "reader.parse-on-load.enabled" = true as well.)

This one works...

Comment on attachment 8556795 [details] [diff] [review] bug958735_jswarning4.diff (For future reference: when you post an updated patch, you generally want to mark the old one as "obsolete". There's a checkbox to do that in the "post attachment" page, when you upload the new version; or, you can mark it as obsolete by viewing the "details" page for the old attachment and clicking "edit details". This will automatically cancel pending review requests on the old version, which is good so that the reviewer knows which version actually needs reviewing. Anyway, I'll mark the old attachment as obsolete, since I'm already commenting. :) but now you know how to do that, too.)

Attachment 8556795 [details] [diff] was the correct one. I am resubmitting the correct patch and making all others obsolete.

Comment on attachment 8556977 [details] [diff] [review] bug958735_jswarning4.diff Review of attachment 8556977 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! Also, thanks dholbert for your mentorship help!

Hi, this failed to apply cleanly: adding 958735 to series file renamed 958735 -> bug958735_jswarning4.diff applying bug958735_jswarning4.diff patching file toolkit/components/reader/content/Readability.js Hunk #1 FAILED at 404 1 out of 3 hunks FAILED -- saving rejects to file toolkit/components/reader/content/Readability.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh bug958735_jswarning4.diff could you take a look (and maybe this need a try run too). Thanks!

Sorry I didn't get it? Could you please explain? Is there a problem in the patch ? Or the patch file?

(In reply to shreyas from comment #30) > Sorry I didn't get it? Could you please explain? Is there a problem in the > patch ? Or the patch file? The patch didn't apply cleanly to the latest trunk. You probably need to rebase it locally. You can do this with 'hg pull --rebase', and then post the rebased version.

If you post a rebased version, I can help make a try run for you, or I can just test it locally and land it myself.

Comment on attachment 8560839 [details] [diff] [review] bug958735_jswarning4.diff This still doesn't apply cleanly to mozilla-central or fx-team. Are you sure you updated your tree before rebasing? Looking at the changelog, the last change was Feb 3: http://hg.mozilla.org/integration/fx-team/filelog/tip/toolkit/components/reader/content/Readability.js

I had done it "hg pull --rebase", I don't know something must have gone wrong. Submitting the same patch again (after doing "hg pull --rebase")

Apparently that patch had some missing lines of code in it. This should work.

Comment on attachment 8562138 [details] [diff] [review] bug958735_jswarning4.diff Review of attachment 8562138 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for updating the patch! Actually, since you started working on this bug, we've moved Readability.js development over to a github repo here: https://github.com/mozilla/readability Would you mind formatting your patch as a pull request against that repo, so that we can land it there? There have been some changes to that file since it was lifted out of mozilla-central, and I worry that if we land your patch here, there will be conflicts when we try to update from this shared library. I know you're just trying to finish a simple bug here, so if this is too complicated, I can also do this for you.

Made a pull request on the github repo.

(In reply to shreyas from comment #38) > Made a pull request on the github repo. Thanks so much! I just merged your change :) I filed bug 1134443 to merge this change back into mozilla-central. I'll close this bug as fixed once I land a patch there. If you're interested, there are lots of other Readability issues being tracked in bug 1102450, and we could definitely use help fixing them! :)

I just landed a patch for bug 1134443, so this will be in mozilla-central soon. I'm just going to close this out as FIXED, since it landed in the main readability repo. Thanks so much for your help, and for your patience while we sort out the workflow around this external library. There are plenty more Readability.js bugs if you're interested in helping out with more!