This bug will implement Feed (Podcast and Video Podcast) Preview, external subscribe and "Pass to web-app" like currently done in Firefox. Spun off from the ever-growing Bug 240393. WIP Attachment at same bug attachment 285847 [details] [diff] [review].

this zip has images that go at: suite/themes/classic/navigator/icons/feedIcon.png suite/themes/classic/navigator/icons/page-livemarks.png

I didn't make subscribeToFeed behave the same as FX. if (!href) href = event.target.getAttribute("feed"); we don't set the "feed" atribute, so code that relies on this will break anyway. At least, it will work if the caller pass href argument. Regarding Neil comments on bug 240393 about: if (/^https?/.test(feedURI.scheme)) href = "feed:" + href; That isn't to filter protocols out, it's to make sure to accept the "feed:" scheme as the href. Examples: feed:http://foo.bar/rss.xml feed:https://foo.com/rss.xml feed://foo.com/rss.xml otherwise, add "feed:" to url. Things missing on this patch: - changes on CSS - handle middle click on the popup, if possible The name choosen for the preferences was "messenger" and I'm using the same string as rss.accountName, but it won't be actually the same for each l10n. I haven't changed the name of the picture right now. If we use that one, we should change its name. Due to security constraints, I had to make navigator content accessible.

Missed: subscribe.dtd, subscribe.properties and feed-subscribe.css on the last patch. Adding those and no further changes. I've just seen that: http://mxr.mozilla.org/comm-central/source/suite/common/pref/pref-applications.js#1262 and http://mxr.mozilla.org/comm-central/source/suite/locales/en-US/chrome/common/pref/pref-applications.properties#23 will have to be changed too.

IANNA and I don't know what I'm talking about, but I'll comment anyway: +++ b/suite/browser/feeds/src/FeedConverter.js @@ -0,0 +1,707 @@ +# -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ +# ***** BEGIN LICENSE BLOCK ***** +# Version: MPL 1.1/GPL 2.0/LGPL 2.1 [...] +# the terms of any one of the MPL, the GPL or the LGPL. +# +# ***** END LICENSE BLOCK ***** */ In Suite we don't normally pre-process out the licence block in the .js files. Please convert to a normal JS comment block. +const Cc = Components.classes; +const Ci = Components.interfaces; +const Cr = Components.results; Suite reviewers don't like the Cc/Ci/Cr constructs. Please use Components.classes etc. + // This means that content-type alone is not enough to determine whether + // or not a feed should be auto-handled. This means that for feeds we need The second "This means that" should be a Thus or a Therefore. + Cc["@mozilla.org/browser/feeds/result-service;1"]. + getService(Ci.nsIFeedResultService); Suite reviewers don't like the trailing dots style. Instead please use: Components.classes["@mozilla.org/browser/feeds/result-service;1"] .getService(Ci.nsIFeedResultService); (In fact you do use that sometimes) + const NS_ERROR_MALFORMED_URI = 0x804B000A; Use Components.results.NS_ERROR_MALFORMED_URI + var ios = + Cc["@mozilla.org/network/io-service;1"]. + getService(Ci.nsIIOService); var ios = Components.classes["@mozilla.org/network/io-service;1"] .getService(Components.interfaces.nsIIOService); + var channel = + ios.newChannelFromURI(uri, null).QueryInterface(Ci.nsIHttpChannel); ditto. +#include GenericFactory.js XPCOMify? +++ b/suite/browser/feeds/src/FeedConverter.js +function LOG(str) { + dump("*** " + str + "\n"); +} +++ b/suite/browser/feeds/src/FeedWriter.js +function LOG(str) { + var prefB = Cc["@mozilla.org/preferences-service;1"]. + getService(Ci.nsIPrefBranch); + + var shouldLog = false; + try { + shouldLog = prefB.getBoolPref("feeds.log"); + } + catch (ex) { + } I think being able to turn on/off logging consistently for the feeds system is good. + try { + return ios.newURI(aURLSpec, aCharset, null); + } catch (ex) { } try { return ios.newURI(aURLSpec, aCharset, null); } catch (ex) { } Several people with varying styles contributed to the original code. You should take the opportunity to standardize the coding style in these files. +function getPrefAppForType(t) { +function getPrefWebForType(t) { +function getPrefActionForType(t) { +function getPrefReaderForType(t) { Some functions are duplicated in FeedConverter.js and FeedWriter.js. Perhaps a shared FeedUtils component? + _mimeSvc : Cc["@mozilla.org/mime;1"]. + getService(Ci.nsIMIMEService), Unnecessary spaces before the ":". + __faviconService: null, + get _faviconService() { + if (!this.__faviconService) + this.__faviconService = Cc["@mozilla.org/browser/favicon-service;1"]. + getService(Ci.nsIFaviconService); + + return this.__faviconService; + }, I don't think we have a favicon service yet do we? + for (var i = 0; i < feed.items.length; ++i) { I should mention this earlier. Sometimes you use for(var...) and sometime for(let...). "let" has advantages in tighter scoping rules in for loops, if blocks etc. I think you should consistently use or not use "let" here. + // XXXben - we need to compare this with the running instance executable + // just don't know how to do that via script... + // XXXmano TBD: can probably add this to nsIShellService Is there a bug filed? If so we should CC mcsmurf on this. +#ifdef XP_WIN +#expand if (fp.file.leafName != "__MOZ_APP_NAME__.exe") { +#else +#ifdef XP_MACOSX +#expand if (fp.file.leafName != "__MOZ_APP_DISPLAYNAME__.app") { +#else +#expand if (fp.file.leafName != "__MOZ_APP_NAME__-bin") { +#endif +#endif + case "chooseApplicationMenuItem": + /* Bug 351263: Make sure to not steal focus if the "Choose Bug 351263 fixed1.8.1? Do we still need a workaround? + case "bookmarks": { + var liveBookmarksMenuItem = this._document.getElementById("liveBookmarksMenuItem"); + if (liveBookmarksMenuItem) + this._safeDoCommand(liveBookmarksMenuItem); + break; + } As you point out elsewhere we don't support livemarks at the moment. Might as well remove all unnecessary references. +#ifdef MOZ_PLACES_BOOKMARKS + case "liveBookmarksMenuItem": + defaultHandler = "bookmarks"; + prefs.setCharPref(getPrefReaderForType(feedType), "bookmarks"); + break; +#endif or #ifdef consistently. + try { + this._defaultSystemReader = Cc["@mozilla.org/browser/shell-service;1"]. Several problems. "@mozilla.org/browser/shell-service;1" is the Firefox shell service. + getService(Ci.nsIShellService). + defaultFeedReader; And our shell service doesn't appear to support "defaultFeedReader" (Yes I know you have a try/catch block here). http://mxr.mozilla.org/comm-central/search?string=defaultFeedReader + var historySvc = Cc["@mozilla.org/browser/nav-history-service;1"]. + getService(Ci.nsINavHistoryService); Fortunately KaiRo just landed Places history. + var iconURL = makeURI(uri.prePath + "/favicon.ico"); We don't have a favicon service but perhaps we should check for a <link rel...> instead of/as well as. However I don't know how doable this is. + get stringBundle() { + var sb = Cc["@mozilla.org/intl/stringbundle;1"]. + getService(Ci.nsIStringBundleService). + createBundle(STRING_BUNDLE_URI); + delete WebContentConverterRegistrar.prototype.stringBundle; + return WebContentConverterRegistrar.prototype.stringBundle = sb; + }, Neil doesn't like smart GETTERs. Elsewhere you do this: + __bundle: null, + get _bundle() { + if (!this.__bundle) { + this.__bundle = Cc["@mozilla.org/intl/stringbundle;1"]. + getService(Ci.nsIStringBundleService). + createBundle(URI_BUNDLE); + } + return this.__bundle; + }, More later (if I remember)...

Comment on attachment 354609 [details] [diff] [review] Current WIPv1.1 Less of a review than I was planning, but these are comments I thought of/noticed while comparing these files to current Firefox (1.9.1) vers of the same files. Most of what Ratty agreed with I also agree here (didn't look too heavily into his comments). >+++ b/suite/browser/feeds/Makefile.in >+DIRS = public src I know firefox does this, but I hear Gecko People are planning to drop this style (the added make invocation does slow build down, especially on windows) and thinking about it I don't recall Neil liking the public/src indirection. So lets collapse those two folders and their Makefile's up one level. >diff --git a/suite/browser/feeds/src/FeedConverter.js b/suite/browser/feeds/src/FeedConverter.js >+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); >... >+#include GenericFactory.js If your including and using XPCOMUtils [good] no need (and in fact you shouldn't need) GenericFactory.js ... if you do, please fix that. > >diff --git a/suite/browser/feeds/src/FeedWriter.js b/suite/browser/feeds/src/FeedWriter.js >+ case "bookmarks": { >+ var liveBookmarksMenuItem = this._document.getElementById("liveBookmarksMenuItem"); >+ if (liveBookmarksMenuItem) >+ this._safeDoCommand(liveBookmarksMenuItem); >+ break; >+ } >+ case "messenger": Lets drop this doCommand on livemarks, and infact the whole case here, no need. >diff --git a/suite/browser/feeds/src/Makefile.in b/suite/browser/feeds/src/Makefile.in >+CPPSRCS = \ >+ nsFeedSniffer.cpp \ >+ nsAboutFeeds.cpp \ >+ $(NULL) Firefox's Makefile has a |LOCAL_INCLUDES = -I$(srcdir)/../../build| right about here, I did not investigate if we need something similar, or why its there... but you (or I) should investigate why that is before we pass off to Neil. (Err -- Looks like it was due to nsBrowserCompsCID.h being included by nsFeedSniffer -- you took care of that) >+include $(topsrcdir)/config/rules.mk >diff --git a/suite/browser/feeds/src/WebContentConverter.js b/suite/browser/feeds/src/WebContentConverter.js >+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); Nit: Trailing whitespace... >diff --git a/suite/browser/feeds/subscribe.js b/suite/browser/feeds/subscribe.js >+<!-- XXXasrail: change entity --> And Image I'd think... (Unless that IS the image we use in mailnews for feed accounts)

Comment on attachment 354609 [details] [diff] [review] Current WIPv1.1 Slight round 2 on this, for my next review I'll wait on v2 >diff --git a/suite/browser/linkToolbarOverlay.xul b/suite/browser/linkToolbarOverlay.xul >- <!-- XXXCallek oncommand temporary until feed Preview works --> >- <menupopup id="link-feed-popup" oncommand="event.stopPropagation(); return subscribeToFeed('feed:' + event.target.getAttribute('href');"/> >+ <menupopup id="link-feed-popup" oncommand="event.stopPropagation(); return subscribeToFeed(event.target.getAttribute('href'), event);"/> Given how generic the link Toolbar code is, if we can help it I'd rather drop the oncommand entirely, (We may have a _slight_ perf loss if we do that, due to X-Moz-Is-Feed not being automatically set, but IMO thats ok here, though Neil may have an alternate pref on this) >diff --git a/suite/browser/pageinfo/feeds.js b/suite/browser/pageinfo/feeds.js >-function onSubscribeFeed() >+function onSubscribeFeed(event) I'm not sure if passing the event along here is really worth it, we emulate an actual button not a UI image here... its more of a thought for UI tzar Neil though than me. [I wouldn't expect middle-click to open alternately here, or even work for that matter] >diff --git a/suite/common/utilityOverlay.js b/suite/common/utilityOverlay.js >-//XXXCallek only used until we implement Feed Preview > >-// uri is a string! >-function subscribeToFeed(uri) >+function subscribeToFeed(href, event) { Toolkit uses a |urlSecurityCheck| here, and I think it may be worth us to have it as well... (Yes we do check it in a few other places, but for API parity and security sanity its probably worth it. >diff --git a/suite/makefiles.sh b/suite/makefiles.sh >+ suite/browser/feeds/Makefile >+ suite/browser/feeds/public/Makefile >+ suite/browser/feeds/src/Makefile > suite/browser/public/Makefile > suite/browser/src/Makefile >+ suite/common/Makefile > suite/build/Makefile > suite/debugQA/Makefile > suite/debugQA/locales/Makefile If you do as I suggested with /feeds/... be sure to drop them from this file :-) >diff --git a/suite/themes/classic/jar.mn b/suite/themes/classic/jar.mn >+ skin/classic/navigator/icons/feedIcon.png (navigator/icons/feedIcon.png) >+ skin/classic/navigator/icons/page-livemarks.png (navigator/icons/page-livemarks.png) Where are these coming from, my file attached in this bug or what? if from in this bug, we may want to use updated images here rather than those (Firefox has had some icon refreshes since I attached those, iirc -- and we might not even want Firefox icons here).

Replying the non-codig topics. (In reply to comment #9) > >diff --git a/suite/browser/pageinfo/feeds.js b/suite/browser/pageinfo/feeds.js > >-function onSubscribeFeed() > >+function onSubscribeFeed(event) > > I'm not sure if passing the event along here is really worth it, we emulate an > actual button not a UI image here... its more of a thought for UI tzar Neil > though than me. [I wouldn't expect middle-click to open alternately here, or > even work for that matter] middle-click doesn't activate the button. But control+click works. (In reply to comment #8) > (From update of attachment 354609 [details] [diff] [review]) > >diff --git a/suite/browser/feeds/subscribe.js b/suite/browser/feeds/subscribe.js > >+<!-- XXXasrail: change entity --> > > And Image I'd think... (Unless that IS the image we use in mailnews for feed > accounts) > >diff --git a/suite/themes/classic/jar.mn b/suite/themes/classic/jar.mn > >+ skin/classic/navigator/icons/feedIcon.png (navigator/icons/feedIcon.png) > >+ skin/classic/navigator/icons/page-livemarks.png (navigator/icons/page-livemarks.png) > > Where are these coming from, my file attached in this bug or what? if from in > this bug, we may want to use updated images here rather than those (Firefox has > had some icon refreshes since I attached those, iirc -- and we might not even > want Firefox icons here). I used the ones attached to the bug. But we really should use the same ones we already use for RSS accounts and in the preferences. About the public/src style... I do prefer to wait a final decision. It's much easier to maintain the patches without moving the files and moving them back would be another headache. All of the idls on the codebase are inside "public" directories... I don't know whether Neil will wants this to change right now.

Note: the icon is now on the binary hg format. You can import the image as-is using hg. @ratty: If you can review some portion of this patch or at least nit me, I would thank you. > Fortunately KaiRo just landed Places history. Actually I removed the ifdefs Callek was using because places already had landed. > + /* Bug 351263: Make sure to not steal focus if the "Choose > Bug 351263 fixed1.8.1? Do we still need a workaround? That's actually a comment explaining what they (the FX people who made that code) did and they referenced the bug. That is not a workaround. > We don't have a favicon service but perhaps we should check for a > <link rel...>instead of/as well as. However I don't know how doable > this is. We have... it comes from toolkit and everything except the default favicon works fine. Well... and I can't complain since we don't actually have a default favicon. Our "default favicon" is defined on the CSS of the themes. If we ignore the default favicon (which comes from toolkit) and the functions that return it in case of error, that works fine for us. Of course I know we won't benefit from the cache because tabbrowser doesn't use it. But the pref-applications panel could benefit from this. > Is there a bug filed? If so we should CC mcsmurf on this. I don't know. I'll use if they become available. mcsmurf is aware of the service shell, but I don't think he is going to make new functions, but just port the ones FX provides. > Neil doesn't like smart GETTERs. Elsewhere you do this That's ported code. I don't know how doable or necessary is this. @callek, @ratty: > Some functions are duplicated in FeedConverter.js and FeedWriter.js. Perhaps a > shared FeedUtils component? I would feel more confortable leaving it as a followup. I'm using XPCOMUtils, even though there were things that weren't trivially handled by XPCOMUtils. Take a look on the solution found and see if that's acceptable. Neil said responding to middle clicks for the menu popups "sounds reasonable". I'll leave further questions for when he review this. @callek: I couldn't manage to make that urlsecurity check to actually "work". There is some issue with security and loading the nodePrincipal from the page. OBS: Still lacking the theme changes for modern... A "final" patch depends on bug 471346. Probably we'll have to change the shell service to the "shell feed service" once it lands.

@callek: using already existent 16px icon. Imported the 32px icon from gnomestripe. Addressing the other points not mentioned on the last comment. I'm still checking for solutions to the urlsecuritycheck issue.

(In reply to comment #11) > > We don't have a favicon service but perhaps we should check for a > > <link rel...>instead of/as well as. However I don't know how doable > > this is. > > We have... it comes from toolkit and everything except the default > favicon works fine. Well... and I can't complain since we don't > actually have a default favicon. http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/src/nsFaviconService.h#144 defines that it always returns the icon from toolkit - which at least is some icon we can use (if Modern doesn't have it, that's a bug in that theme and should be fixed) even if it's possibly not the icon we'd prefer. We can always check for that URL and use something different in SeaMonkey if we need to. > Of course I know we won't benefit from the cache because tabbrowser > doesn't use it. But the pref-applications panel could benefit from > this. I think the tabbrowser (and even bookmarks code) should actually use it, but that's a different bug (and should probably be filed as such).

(In reply to comment #13) > > We have... it comes from toolkit and everything except the default > > favicon works fine. Well... and I can't complain since we don't > > actually have a default favicon. > > http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/src/nsFaviconService.h#144 > defines that it always returns the icon from toolkit - which at least is some > icon we can use (if Modern doesn't have it, that's a bug in that theme and > should be fixed) even if it's possibly not the icon we'd prefer. We can always > check for that URL and use something different in SeaMonkey if we need to. The toolkit icon is always present and it actually works. What I meant is that the favicon service returns the icon from toolkit, which is different from the default favicon for Classic and Modern. The default favicon is defined on the CSS of the themes and the names of the files are different. Returning a default favicon consistent with the themes would require further changes. But the patch here doesn't expose the default favicon from toolkit, anyway. > > Of course I know we won't benefit from the cache because tabbrowser > > doesn't use it. But the pref-applications panel could benefit from > > this. > > I think the tabbrowser (and even bookmarks code) should actually use it, but > that's a different bug (and should probably be filed as such). Makes sense using that everywhere, because every clients would use the same cache.

(In reply to comment #14) > The toolkit icon is always present and it actually works. What I meant is that > the favicon service returns the icon from toolkit, which is different from the > default favicon for Classic and Modern. Right, and also right that we probably should deal with this where the actual default icon might appear somewhere. Still, I wouldn't worry too much about Modern in cases like that, as it can easily put an icon in the correct place once it implements mozapps/ (which is being worked on). But, as already said, we can also discuss the Classic case elsewhere.

> If you can review some portion of this patch or at least nit me, I would thank > you. I'd think you were sick and tired of me already. But OK, I'll get around to this RSN. >> Some functions are duplicated in FeedConverter.js and FeedWriter.js. Perhaps a >> shared FeedUtils component? > I would feel more confortable leaving it as a followup. Yes, a follow-up bug would be better. > using already existent 16px icon. Imported the 32px icon from gnomestripe. I think you mentioned some changes in the icon since the the current 16px icon was checked in. Perhaps we should get the high resolution original source and regenerate the 16px icon (in a follow-up bug, of course)

(In reply to comment #16) > > If you can review some portion of this patch or at least nit me, I would thank > > you. > I'd think you were sick and tired of me already. But OK, I'll get around to > this RSN. No, I really thank you. I'm sure Neil would nit me anyway. > > using already existent 16px icon. Imported the 32px icon from gnomestripe. > I think you mentioned some changes in the icon since the the current 16px icon > was checked in. Perhaps we should get the high resolution original source and > regenerate the 16px icon (in a follow-up bug, of course) I'm using the 16px icon which is already in SM. That icon is newer than the original and very much different. Both winstripe and gnomestripe are not much different. Our 16px icon is different from both, but after analyzing it looks like an older version of gnomestripe. The main difference is the border, but that's only 1px width. For a 16px icon, that's very hard to notice. Mainly when comparing a 16px with a 32px version (usually the 16px is simplified anyway). So, it's better to stick with this right now and change the 16px version and the image used on the menu popups in a follow-up bug. Take a look at the feed preview dialog with "News & Blogs" selected, where both icons appear. I think that's fine.

Note: I am only reviewing the .js and .xul files as I don't know how to review .cpp/.h/.idl/jar.mn/Makefile.in/makefiles.sh/packages/ +++ b/suite/browser/feeds/src/FeedConverter.js + // This means that content-type alone is not enough to determine whether + // or not a feed should be auto-handled. This means that for feeds we need Grammar nit. Two consecutive sentences starting with 'This means that" is sub-optimal. See my suggestions in Comment #7 + var wccr = + Components.classes["@mozilla.org/embeddor.implemented/web-content-handler-registrar;1"] Elsewhere the ported code puts these on one line. + } catch(ex) { /* fallback to preview mode */ } Yes this is an empty block but elsewhere with empty catch blocks the closing parenthesis is on the next line and aligned with the try block. +function build_component(component, ctor, properties) { + component.prototype = new ctor(); + for (var name in properties) { + component.prototype[name] = properties[name]; + } +} NOT a nit, just a FYI: this only works because the properties passed by all the callers don't have getters/setters/ + } catch(e) { + // If we couldn't use the shell service, fallback to using a + // nsIProcess instance + var p = Components.classes["@mozilla.org/process/util;1"] + .createInstance(Components.interfaces.nsIProcess); + p.init(clientApp); + p.run(false, [spec], 1); + } Is it possible to reach this code path on OSX? I remember reading that nsIProcess is broken on OSX and that args is ignored. Any OSX experts around? + const httpsChunk = "feed://https//"; + const httpChunk = "feed://http//"; + if (feedSpec.substr(0, httpsChunk.length) == httpsChunk) + feedSpec = "https://" + feedSpec.substr(httpsChunk.length); + else if (feedSpec.substr(0, httpChunk.length) == httpChunk) I wonder if using .match() here would be trying to be too clever. +let components = [FeedProtocolHandler, PodcastProtocolHandler, FeedResultService, + FeedConverter_feed, FeedConverter_audio_feed, + FeedConverter_video_feed]; +function NSGetModule(cm, file) { I think there should be a blank line somewhere in there. +++ b/suite/browser/feeds/src/FeedWriter.js + var prefs = Components.classes["@mozilla.org/preferences-service;1"] + .getService(Components.interfaces.nsIPrefBranch2); This is used several times. in FeedWriter.js. Perhaps we should cache this in a property. + var alwaysUse = false; + try { + var prefs = Components.classes["@mozilla.org/preferences-service;1"] + .getService(Components.interfaces.nsIPrefBranch); + if (prefs.getCharPref(getPrefActionForType(feedType)) != "ask") + alwaysUse = true; + } + catch(ex) { + } + var handler = "messenger"; + try { + handler = prefs.getCharPref(getPrefReaderForType(feedType)); + } + catch (ex) { + } FeedConverter.js has this useful helper function safeGetCharPref(). +++ b/suite/browser/feeds/src/WebContentConverter.js + var pb = Components.classes["@mozilla.org/preferences-service;1"] + .getService(Components.interfaces.nsIPrefBranch); Obsessive/Complusive nit. Everywhere else in the ported code |var prefs| is used,. +++ b/suite/browser/feeds/src/WebContentConverter.js +function WebContentConverter() { +} +WebContentConverter.prototype = { A blank line in between would look better (several instances in this file). + getAutoHandler: + function WCCR_getAutoHandler(contentType) { I don't like this style of splitting into two lines but I'll leave this up to you. + if (handler.uriTemplate == aURITemplate) { + handlers.removeElementAt(i); + var hs = Components.classes["@mozilla.org/uriloader/handler-service;1"] + .getService(Components.interfaces.nsIHandlerService); + hs.store(handlerInfo); + return; + } This is one place where you can take advantage of |let| scoping rules (let hs =...) + for (let i = 0; i < kids.length; i++) { + var match = /^(\d+)\.uri$/.exec(kids[i]); + if (!match) + continue; + else + nums.push(match[1]); + } How much of a speed up does this give us vs: if (match) nums.push(match[1]); With JIT turned on I'd think the difference would be down in the noise levels. + for (let i = 0; i < childPrefs.length; ++i) { + var type = childPrefs[i]; + var uri = autoBranch.getCharPref(type); + if (uri) { + var handler = this.getWebContentHandlerByURI(type, uri); + this._setAutoHandler(type, handler); + } + } Use |let|s inside for/while/ loops to take advantage of scoping rules. + getInterfaces: function WCCR_getInterfaces(countRef) { + var interfaces = [Components.interfaces.nsIWebContentConverterService, + Components.interfaces.nsIWebContentHandlerRegistrar, + Components.interfaces.nsIObserver, + Components.interfaces.nsIClassInfo, + Components.interfaces.nsIFactory, + Components.interfaces.nsISupports]; + countRef.value = interfaces.length; + return interfaces; + }, + getHelperForLanguage: function WCCR_getHelperForLanguage(language) { + return null; + }, Throwing in a few blank lines might make this more readable. I think my brain just melted so that's the end of my review.

(In reply to comment #18) > Note: I am only reviewing the .js and .xul files as I don't know how to review > .cpp/.h/.idl/jar.mn/Makefile.in/makefiles.sh/packages/ OK, thanks again. > +++ b/suite/browser/feeds/src/FeedConverter.js > > + // This means that content-type alone is not enough to determine whether > + // or not a feed should be auto-handled. This means that for feeds we need > > Grammar nit. Two consecutive sentences starting with 'This means that" is > sub-optimal. See my suggestions in Comment #7 Sorry, I forgot this. > + var wccr = > + > Components.classes["@mozilla.org/embeddor.implemented/web-content-handler-registrar;1"] > > Elsewhere the ported code puts these on one line. Missed this... > + } catch(ex) { /* fallback to preview mode */ } > > Yes this is an empty block but elsewhere with empty catch blocks the closing > parenthesis is on the next line and aligned with the try block. OK. > +function build_component(component, ctor, properties) { > + component.prototype = new ctor(); > + for (var name in properties) { > + component.prototype[name] = properties[name]; > + } > +} > > NOT a nit, just a FYI: this only works because the properties passed by all the > callers don't have getters/setters/ Is there a optimal solution? The old code registered everything manually and XPCOMUtils doesn't provide a clean way to do this. That was the easier way I found to register those without having to repeat code in some places. That is not intended to be a general solution, I just wanna it to work for my case. > + } catch(e) { > + // If we couldn't use the shell service, fallback to using a > + // nsIProcess instance > + var p = Components.classes["@mozilla.org/process/util;1"] > + .createInstance(Components.interfaces.nsIProcess); > + p.init(clientApp); > + p.run(false, [spec], 1); > + } > > Is it possible to reach this code path on OSX? I remember reading that > nsIProcess is broken on OSX and that args is ignored. Any OSX experts around? Usually the shell service should handle this (or feed shell service). At the time of the final patch, SM should have a feed shell service and on OSX that should be used. > + const httpsChunk = "feed://https//"; > + const httpChunk = "feed://http//"; > + if (feedSpec.substr(0, httpsChunk.length) == httpsChunk) > + feedSpec = "https://" + feedSpec.substr(httpsChunk.length); > + else if (feedSpec.substr(0, httpChunk.length) == httpChunk) > > I wonder if using .match() here would be trying to be too clever. Well... sure. > +let components = [FeedProtocolHandler, PodcastProtocolHandler, > FeedResultService, > + FeedConverter_feed, FeedConverter_audio_feed, > + FeedConverter_video_feed]; > +function NSGetModule(cm, file) { > > I think there should be a blank line somewhere in there. > > +++ b/suite/browser/feeds/src/FeedWriter.js > > + var prefs = Components.classes["@mozilla.org/preferences-service;1"] > + .getService(Components.interfaces.nsIPrefBranch2); > > This is used several times. in FeedWriter.js. Perhaps we should cache this in a > property. Sure. > + var alwaysUse = false; > + try { > + var prefs = Components.classes["@mozilla.org/preferences-service;1"] > + > .getService(Components.interfaces.nsIPrefBranch); > + if (prefs.getCharPref(getPrefActionForType(feedType)) != "ask") > + alwaysUse = true; > + } > + catch(ex) { > + } > > + var handler = "messenger"; > + try { > + handler = prefs.getCharPref(getPrefReaderForType(feedType)); > + } > + catch (ex) { > + } > > FeedConverter.js has this useful helper function safeGetCharPref(). So I would copy more a function a leave the module for a followup bug? > +++ b/suite/browser/feeds/src/WebContentConverter.js > > + var pb = Components.classes["@mozilla.org/preferences-service;1"] > + .getService(Components.interfaces.nsIPrefBranch); > > Obsessive/Complusive nit. Everywhere else in the ported code |var prefs| is > used,. OK. > +++ b/suite/browser/feeds/src/WebContentConverter.js > > +function WebContentConverter() { > +} > +WebContentConverter.prototype = { > > A blank line in between would look better (several instances in this file). > > + if (handler.uriTemplate == aURITemplate) { > + handlers.removeElementAt(i); > + var hs = > Components.classes["@mozilla.org/uriloader/handler-service;1"] > + > .getService(Components.interfaces.nsIHandlerService); > + hs.store(handlerInfo); > + return; > + } > > This is one place where you can take advantage of |let| scoping rules (let hs > =...) Oh... sure. > + for (let i = 0; i < kids.length; i++) { > + var match = /^(\d+)\.uri$/.exec(kids[i]); > + if (!match) > + continue; > + else > + nums.push(match[1]); > + } > > How much of a speed up does this give us vs: > > if (match) > nums.push(match[1]); > > With JIT turned on I'd think the difference would be down in the noise levels. I think the latter is even better. > + for (let i = 0; i < childPrefs.length; ++i) { > + var type = childPrefs[i]; > + var uri = autoBranch.getCharPref(type); > + if (uri) { > + var handler = this.getWebContentHandlerByURI(type, uri); > + this._setAutoHandler(type, handler); > + } > + } > > Use |let|s inside for/while/ loops to take advantage of scoping rules. OK. > + getInterfaces: function WCCR_getInterfaces(countRef) { > + var interfaces = [Components.interfaces.nsIWebContentConverterService, > + Components.interfaces.nsIWebContentHandlerRegistrar, > + Components.interfaces.nsIObserver, > + Components.interfaces.nsIClassInfo, > + Components.interfaces.nsIFactory, > + Components.interfaces.nsISupports]; > + countRef.value = interfaces.length; > + return interfaces; > + }, > + getHelperForLanguage: function WCCR_getHelperForLanguage(language) { > + return null; > + }, > > Throwing in a few blank lines might make this more readable. OK for the blank lines.

>> NOT a nit, just a FYI: this only works because the properties passed by all the >> callers don't have getters/setters/ > Is there a optimal solution? The old code registered everything manually and > XPCOMUtils doesn't provide a clean way to do this. That was the easier way I > found to register those without having to repeat code in some places. Um, I said that this is not a nit. If you control all the callers then this is definitely /not/ a problem. The general solution can be found here: <http://ejohn.org/blog/javascript-getters-and-setters/> but again this is superfluous for this particular case. >> FeedConverter.js has this useful helper function safeGetCharPref(). > So I would copy more a function a leave the module for a followup bug? Sure, the current code isn't broken so a follow-up bug seems reasonable.

> +++ b/suite/locales/en-US/chrome/browser/feeds/subscribe.properties > +chooseApplicationMenuItem=Choose Application... Very sorry. I forgot to mention that on trunk we are all now using the utf8 ellipse character instead of three dots.

changeset: 23541:ca9d3c35fe47 user/date: Phil Ringnalda <philringnalda@gmail.com> 2009-01-12 04:12:32 parent: 23540:4edbe170e6a1 Can not aquire, or even acquire, anything other than a CLOSED TREE tags: tip I would not have choosen the weekend before a freeze for so much CLOSED TREE === browser/components/feeds/src/FeedWriter.js === @@ -1265,7 +1265,7 @@ var selectedItem = this._getSelectedItemFromMenulist(handlersMenuList); // Show the file picker before subscribing if the - // choose application menuitem was choosen using the keyboard + // choose application menuitem was chosen using the keyboard if (selectedItem.id == "chooseApplicationMenuItem") { if (!this._chooseClientApp()) return;

@Ratty: Well... I've seen that caching the preferences service would be far better on the feedutils component, so I do prefer leaving this to a follow-up bug. @Neil: - Callek suggested merging the contents of src and public on the root folder, but I do prefer leaving this decision for you, since nowhere on suite we use this style. - I had to make navigator contentaccessible. The original code relies on that. I accept alternative solutions, but this patch requires changes to work if navigator is not contentaccessible. I'm aware communicator is contentaccessible as of now, but I don't know whether it will stay this way. - Ratty suggested to make a feedutils component for the functions shared across the files. I agree with him, but I would prefer leaving this to a follow-up. Otherwise I would ask to land the string changes earlier. - I'm using the favicon service. If that's OK, we could convert the other possible clients latter. There is some discussion about it on the previous comments. - The 32px feed icon is from gnomestripe and newer than the 16px one. There are a few differences - mainly the old one is more transparent. The final patch depends on the contract ID of the feed shell service and will depend on bug 471346.

Comment on attachment 358546 [details] [diff] [review] Almost final patch. The final one depends on feed shell service. warning: 65 lines add whitepace errors. >+++ b/suite/browser/feeds/Makefile.in Do not create this file. suite/browser/feeds isn't a good place to put these files. Either a) put them under suite/feeds or b) put them under suite/browser (WITHOUT feeds) Should you go with a) then add feeds/public and feeds/src to suite's Makefile. >+var ioSvc = Components.classes["@mozilla.org/network/io-service;1"] >+ .getService(Components.interfaces.nsIIOService); Is that safe at the top level of a component? >+ var shouldLog = false; >+ try { >+ shouldLog = prefs.getBoolPref("feeds.log"); >+ } >+ catch (ex) { >+ } >+ >+ if (shouldLog) >+ dump("*** Feeds: " + str + "\n"); try { if (prefs.getBoolPref("feeds.log")) dump("*** Feeds: " + str + "\n"); } catch (ex) { } >+ convert: function FC_convert(sourceStream, sourceType, destinationType, Personally I don't like these FC_ prefixes. >+ var units = ["bytes", "kilobyte", "megabyte", "gigabyte"]; *bytes everywhere please. >+ get scheme() { >+ return this._scheme; >+ }, Don't bother with a getter for a constant. Just set the scheme when building the component. >+#include ../../../../mozilla/toolkit/content/debug.js Left over? In which case, turn it into a non-preprocessed component. >+ Components.utils.evalInSandbox(codeStr, this._contentSandbox); What's all this sandbox stuff for? As far as I can see, you're not even trying to call content script. >+ _getSelectedItemFromMenulist: function FW__getSelectedItemFromList(aList) { >+ for (let node = aList.firstChild.firstChild; node; node = node.nextSibling) >+ if (node.localName == "menuitem" && node.getAttribute("selected") == "true") >+ return node; >+ >+ return null; return aList.getElementsByAttribute("selected", "true").item(0); >+ if (!dateObj.getTime()) if (isNAN(dateObj.getTime())) >+ "feedTitleText.style.marginRight = titleImageWidth + 'px';"; Is this correct for RTL locales, or do you need MozMarginEnd? >+ url.QueryInterface(Components.interfaces.nsIURL); >+ if (url == null || url.fileName.length == 0) url will never be null, although the QI could throw. Maybe you meant if ((url instanceof Components.interfaces.nsIURL) && url.fileName) return decodeURI(url.fileName); return aURL; >+ifndef MOZ_MEMORY >+USE_STATIC_LIBS = 1 >+endif Is this right? It wouldn't link for me. (Which meant that I couldn't test the patch...) >+ QueryInterface: function WCC_QueryInterface(iid) { You forgot to use generateQI in three places in this file. >+ delete WebContentConverterRegistrar.prototype.stringBundle; I prefer the __bundle version as used in FeedWriter. >+ var browserWindow = this._getBrowserWindowForContentWindow(aContentWindow); >+ var browserElement = this._getBrowserForContentWindow(browserWindow, aContentWindow); >+ var notificationBox = browserWindow.getBrowser().getNotificationBox(browserElement); Copy getNotificationBox from navigator.js instead. >+++ b/suite/browser/feeds/src/nsAboutFeeds.cpp This could probably be converted into JavaScript. See nsAboutAbout.js or nsAboutCertError.js for example. >+#define FEEDS_PAGE_URI "chrome://navigator/content/feeds/subscribe.xhtml" Possibly better in communicator as that is already contentaccessible. >+ if(w) { Nit: if (w) { >+function loadFeed(href, event) > { >+ openUILink(href, event, false, true); > } Might as well call openUILink directly? >+<!ENTITY feedPage.title >+ "Viewing Feed"> >+<!ENTITY feedSubscribeNow >+ "Subscribe Now"> >+<!ENTITY feedMessenger >+ "News &amp; Blogs"> One line per entity please!

FYI Bug 436659 (The description of Feed View is not displayed)

Hi, addressing last round of review comments, except for the Sandbox thing. On the comments from the ported code, there is reference to security, so I'm going to verify this before changing it. That makes some sense, since the scripts are run on the page with content access and chrome privileges. Even though no content script is called directly. I'm going to change that if that proves useless. I migrated subscribe.xhtml and friends to communicator. I'm asking for ui-review to the about:feeds pages. I've migrated the nsAboutFeeds.cpp to js. PS @ratty: I'll look into the solution for the application icon on FX, probably I'll use the same as the helper apps preference panel. But I would like to land this ASAP, since it contain string changes. So I would leave that for a follow-up bug.

Comment on attachment 368202 [details] [diff] [review] Another WIP. Still dependent on feed shell service. This is just a quick glance, but it's looking good! >-PARALLEL_DIRS = public src >+PARALLEL_DIRS = public \ >+ src \ >+ $(NULL) Don't need this change any more. >+ _ioSvc: Components.classes["@mozilla.org/network/io-service;1"] >+ .getService(Components.interfaces.nsIIOService), Actually that's still top-level. On the other hand, get _ioSvc() { return ... ; }, wouldn't be, but that gets the ioSvc each time, so perhaps function _FeedProtocolHandler() { this._ioSvc = ...; } would be best. >+ convert: function _convert(sourceStream, sourceType, destinationType, You removed the FC but not the _ :-( (be careful for method names beginning with one _ of course!) >+function FeedProtocolHandler() { >+ var ioSvc = Components.classes["@mozilla.org/network/io-service;1"] >+ .getService(Components.interfaces.nsIIOService); By the time we call FeedProtocolHandler() we should be able to use this._ioSvc >+ this._http = ioSvc.getProtocolHandler("http"); Although I wonder whether it would be better to add _http to _FeedProtocolHandler() so that PodcastProtocolHandler can use it >+ this.scheme = "feed"; >+} >+ >+build_component(FeedProtocolHandler, _FeedProtocolHandler, >+ {classID: Components.ID("{A95D7F48-11BE-4324-8872-D23BD79FE78B}"), >+ classDescription: "Feed Protocol Handler", >+ contractID: "@mozilla.org/network/protocol;1?name=feed" >+ }); I think it might be better to set the scheme here instead.

Comment on attachment 368202 [details] [diff] [review] Another WIP. Still dependent on feed shell service. > content/communicator/sidebar/sidebarOverlay.xul (sidebar/sidebarOverlay.xul) >+ content/communicator/feeds/subscribe.xhtml (feeds/subscribe.xhtml) >+ content/communicator/feeds/subscribe.js (feeds/subscribe.js) feeds belongs between directory and history (i.e. alphabetic order) >+ newURI: function _newURI(spec, originalCharset, baseURI) { >+ // See bug 408599 Eww, philor needs help :-( >+ try { >+ secman.checkLoadURIStrWithPrincipal(this._feedPrincipal, uri, flags); >+ // checkLoadURIStrWithPrincipal will throw if the link URI should not be >+ // loaded, either because our feedURI isn't allowed to load it or per >+ // the rules specified in |flags|, so we'll never "linkify" the link... >+ } >+ catch (e) { >+ // Not allowed to load this link because secman.checkLoadURIStr threw >+ return; >+ } >+ >+ this._contentSandbox.element = element; >+ this._contentSandbox.uri = uri; >+ var codeStr = "element.setAttribute('" + attribute + "', uri);"; >+ Components.utils.evalInSandbox(codeStr, this._contentSandbox); Nit: set the attribute in the try block, so you don't need the early return. >+ * Returns a date suitable for displaying in the feed preview. >+ * If the date cannot be parsed, the return value is "false". Weird. I guess that's what Firefox does? I'd prefer to return null. >+ url.QueryInterface(Components.interfaces.nsIURL); >+ if ((url instanceof Components.interfaces.nsIURL) && url.fileName) Whoops, not quite right, don't need that QI any more! >+ var roundme = function(n) { Unused? >+ var fph = this._ioSvc.getProtocolHandler("file") >+ .QueryInterface(Components.interfaces.nsIFileProtocolHandler); Nit: . needs to line up >+const NS_ERROR_MODULE_DOM = 2152923136; Does this make any more sense in hex? >+ throw("Permission denied to add " + aURIString + "as a protocol handler"); Nit: " as >+ _getBrowserWindowForContentWindow: function __getBrowserWindowForContentWindow(aContentWindow) { Unused? >+ _getBrowserForContentWindow: function __getBrowserForContentWindow(aBrowserWindow, aContentWindow) { Unused? >+ var kids = prefs.getBranch(PREF_CONTENTHANDLERS_BRANCH) >+ .getChildList("", {}); Nit: . needs to line up >+FindChar(char c, const char *begin, const char *end) [Is it possible to use memchr instead somehow?]

Comment on attachment 368202 [details] [diff] [review] Another WIP. Still dependent on feed shell service. >+ menuItem.setAttribute("hidden", true); Some places use the correct menuItem.hidden = true; some don't. >+*[hidden] { >+ display: none; >+} Unnecessary (both themes). >+ border: 1px solid THreeDShadow; Nit: typo (Three not THree) >+#alwaysUse { >+ padding: 5px; >+} Not sure what the point of this is (both themes). >+.link { >+ color: #0000FF; >+ text-decoration: underline; >+ cursor: pointer; >+} >+ >+.link:hover:active { >+ color: #FF0000; >+} These should use the same colours as the text-link class in global.css (which are different for Classic and Modern, of course). >+ color: ThreeDDarkShadow; In Modern, this should not use a system colour!

(In reply to comment #29) > (From update of attachment 368202 [details] [diff] [review]) > >+#alwaysUse { > >+ padding: 5px; > >+} > Not sure what the point of this is (both themes). The code sets the label on the checkbox, the padding makes the text readable (aiui).

(In reply to comment #30) > (In reply to comment #29) > > (From update of attachment 368202 [details] [diff] [review] [details]) > > >+#alwaysUse { > > >+ padding: 5px; > > >+} > > Not sure what the point of this is (both themes). > The code sets the label on the checkbox, the padding makes the text readable But checkboxes already have a margin, they don't need a padding as well...

Comment on attachment 368202 [details] [diff] [review] Another WIP. Still dependent on feed shell service. The Modern colour scheme needs tweaking, you can't see the focus on the feed preview subscribe controls.

Comment on attachment 368202 [details] [diff] [review] Another WIP. Still dependent on feed shell service. +#handlersMenuList > menupopup > menuitem { + -moz-padding-start: 23px; +} This is wrong for Classic; it should be 20px. Also for Modern I think it should be 4px. Actually, better still, remove all three rules for that menupopup.

Comment on attachment 368202 [details] [diff] [review] Another WIP. Still dependent on feed shell service. +build_component(FeedProtocolHandler, _FeedProtocolHandler, + {classID: Components.ID("{A95D7F48-11BE-4324-8872-D23BD79FE78B}"), + classDescription: "Feed Protocol Handler", + contractID: "@mozilla.org/network/protocol;1?name=feed" + }); + * Don't use if the constructor has getters or setters. .....................................^^^^^^^^^^^^^^^^^^ +function build_component(component, ctor, properties) { +_FeedProtocolHandler.prototype = { + get protocolFlags() { + return this._http.protocolFlags; + }, + + get defaultPort() { + return this._http.defaultPort; + },

According to Bug 436685 FeedConverter::canConvert is unused.

For mozilla-central builds (1.9.2) we probably need to add: + onBeforeDeleteURI: function() { }, to FeedWriter.prototype from Bug 487511.

(In reply to comment #28) > >+ var roundme = function(n) { > Unused? Yeah, removed. > >+const NS_ERROR_MODULE_DOM = 2152923136; > Does this make any more sense in hex? Using an hex constant now. > >+ _getBrowserWindowForContentWindow: function __getBrowserWindowForContentWindow(aContentWindow) { > Unused? > > >+ _getBrowserForContentWindow: function __getBrowserForContentWindow(aBrowserWindow, aContentWindow) { > Unused? Yeah, removed. > >+FindChar(char c, const char *begin, const char *end) > [Is it possible to use memchr instead somehow?] Using memchr now. (In reply to comment #29) > >+.link { > >+ color: #0000FF; > >+ text-decoration: underline; > >+ cursor: pointer; > >+} > >+ > >+.link:hover:active { > >+ color: #FF0000; > >+} > These should use the same colours as the text-link class in global.css (which > are different for Classic and Modern, of course). There was a mistake on the code, but for modern I think would make sense using the same color about:plugin, about:license and about:buildconfig use (imported from plugins.css). Removed the rules, since it will inherit from about:PreferenceStyleSheet. > >+ color: ThreeDDarkShadow; > In Modern, this should not use a system colour! Sure, I missed this reference. (In reply to comment #32) > (From update of attachment 368202 [details] [diff] [review]) > The Modern colour scheme needs tweaking, you can't see the focus on the feed > preview subscribe controls. Instead of changing the focus border color I've changed the background color. (In reply to comment #34) > (From update of attachment 368202 [details] [diff] [review]) > > +build_component(FeedProtocolHandler, _FeedProtocolHandler, > + {classID: > Components.ID("{A95D7F48-11BE-4324-8872-D23BD79FE78B}"), > + classDescription: "Feed Protocol Handler", > + contractID: "@mozilla.org/network/protocol;1?name=feed" > + }); > > + * Don't use if the constructor has getters or setters. > .....................................^^^^^^^^^^^^^^^^^^ > +function build_component(component, ctor, properties) { > > +_FeedProtocolHandler.prototype = { > + get protocolFlags() { > + return this._http.protocolFlags; > + }, > + > + get defaultPort() { > + return this._http.defaultPort; > + }, I should have done the right way instead of putting that comment. Done and comment removed. (In reply to comment #35) > According to Bug 436685 FeedConverter::canConvert is unused. Removed. (In reply to comment #36) > For mozilla-central builds (1.9.2) we probably need to add: > > + onBeforeDeleteURI: function() { }, > > to FeedWriter.prototype from Bug 487511. Leave to a followup bug. @all: I've removed the strict dependency from the feed shell service using an ifdef. We will benefit from that since it lands, but we can land this without feed shell service.

Comment on attachment 374983 [details] [diff] [review] Proposed patch >+/** >+ * Helper to register multiple components sharing the same prototype >+ * using XPCOMUtils. >+ */ >+function build_component(component, ctor, properties) { There seems to be a bug in this version, but I think the old version worked fine, because none of the properties tried to replace a getter or setter. >+function FeedWriter() { >+ this._ioSvc = Components.classes["@mozilla.org/network/io-service;1"] >+ .getService(Components.interfaces.nsIIOService); >+} >+ >+FeedWriter.prototype = { >+ _mimeSvc: Components.classes["@mozilla.org/mime;1"] >+ .getService(Components.interfaces.nsIMIMEService), I think this needs to go into the constructor too. >+#ifdef HAVE_SHELL_SERVICE None of our shell services(!) currently implement defaultFeedReader, so that this ifdef isn't that much help... except that it's effectively commenting the entire block out anyway, since we don't define HAVE_SHELL_SERVICE anywhere! >+ // nsIObserver >+ observe: function observe(subject, topic, data) { >+ // see init() >+ Nit: unnecessary blank line got added here >+const NS_ERROR_MODULE_DOM = 0x80530000; This is better, thanks... >+const NS_ERROR_DOM_SYNTAX_ERR = 0x80530012; ... but this is now wrong; the old version was correct. >+static const char* >+FindChar(char c, const char *begin, const char *end) >+{ >+ size_t n = end - begin; >+ void *match = memchr(begin, c, n); >+ >+ if(!match) >+ return nsnull; >+ return static_cast<char *>(match); >+} I'm surprised you got this to compile; by my reckoning C++ doesn't let you pass a const void* to memchr and return a void*. Also, I'd like to think that static_cast<char *> works on nsnull. And you only use n once. Which leaves very little code left: ;-) inline const char* FindChar(char c, const char *begin, const char *end) { return static_cast<const char *>(memchr(begin, c, end - begin)); } Haven't looked at the CSS changes yet.

(In reply to comment #38) > (From update of attachment 374983 [details] [diff] [review]) > >+/** > >+ * Helper to register multiple components sharing the same prototype > >+ * using XPCOMUtils. > >+ */ > >+function build_component(component, ctor, properties) { > There seems to be a bug in this version, but I think the old version worked > fine, because none of the properties tried to replace a getter or setter. Will check this. > >+function FeedWriter() { > >+ this._ioSvc = Components.classes["@mozilla.org/network/io-service;1"] > >+ .getService(Components.interfaces.nsIIOService); > >+} > >+ > >+FeedWriter.prototype = { > >+ _mimeSvc: Components.classes["@mozilla.org/mime;1"] > >+ .getService(Components.interfaces.nsIMIMEService), > I think this needs to go into the constructor too. OK. > >+#ifdef HAVE_SHELL_SERVICE > None of our shell services(!) currently implement defaultFeedReader, so that > this ifdef isn't that much help... except that it's effectively commenting the > entire block out anyway, since we don't define HAVE_SHELL_SERVICE anywhere! There is such ifdef used on pref-applications.js. I think bug 471346 should define this. Do you prefer to leave without that code block at all or leave without the ifdef? It raises a strict javascript error on the latter case. > >+ // nsIObserver > >+ observe: function observe(subject, topic, data) { > >+ // see init() > >+ > Nit: unnecessary blank line got added here > > >+const NS_ERROR_MODULE_DOM = 0x80530000; > This is better, thanks... > > >+const NS_ERROR_DOM_SYNTAX_ERR = 0x80530012; > ... but this is now wrong; the old version was correct. > > >+static const char* > >+FindChar(char c, const char *begin, const char *end) > >+{ > >+ size_t n = end - begin; > >+ void *match = memchr(begin, c, n); > >+ > >+ if(!match) > >+ return nsnull; > >+ return static_cast<char *>(match); > >+} > I'm surprised you got this to compile; by my reckoning C++ doesn't let you pass > a const void* to memchr and return a void*. Also, I'd like to think that > static_cast<char *> works on nsnull. And you only use n once. Which leaves very > little code left: ;-) > > inline const char* FindChar(char c, const char *begin, const char *end) > { > return static_cast<const char *>(memchr(begin, c, end - begin)); > } Hmm... nice.

Comment on attachment 374983 [details] [diff] [review] Proposed patch CSS looks good too, so r+sr=me with those nits fixed. Oh, and you apparently have 62 whitespace errors...

Leave the #ifdef in for now, we'll figure out what to do in whichever bug it is where we're upgrading the shell service.

(In reply to comment #39) > (In reply to comment #38) > > (From update of attachment 374983 [details] [diff] [review] [details]) > > >+/** > > >+ * Helper to register multiple components sharing the same prototype > > >+ * using XPCOMUtils. > > >+ */ > > >+function build_component(component, ctor, properties) { > > There seems to be a bug in this version, but I think the old version worked > > fine, because none of the properties tried to replace a getter or setter. > > Will check this. Well... The comment itself was wrong. The old way right. The issue was with the properties passed and makes no sense passing a getter or setter there anyway. I'm reverting to the old way with the comment removed. Nits addresed. I've removed all of the whitespace errors, including one from pref-applications.js that was between two lines touched by me (removed the existing whitespace error). Carrying r+rs=neil.

I'll file a bug to track the followup changes soon.

Pushed attachment #375137 [details] [diff] [review]: http://hg.mozilla.org/comm-central/rev/3c908fd10922

Between attachment 368202 [details] [diff] [review] (comment 26) and attachment 374983 [details] [diff] [review] (comment 37), a return statement was added to the first "if (handler)" block. That caused bug 612265, which Firefox doesn't have because it doesn't have that statement either. Unfortunately I cannot see any comments that requested or explain that change. :-(