We need to port the GetDefaultFeedReader function from the FF shell service to the SeaMonkey shell service so the new feed preview UI and the Helper Applications pref pane can use it.

I removed the include of nsCOMPtr.h as it's already included in nsDirectoryServiceUtils.h for a long time now.

Comment on attachment 354629 [details] [diff] [review] Patch Rob, can you review this? This is just a 1:1 copy of the Firefox shell service code.

So, uh, what about nsGNOMEShellService and nsMacShellService?

Frank, as Philip points out it seems this should be implemented for Mac and Linux... any reason it shouldn't be at the same time?

Comment on attachment 354629 [details] [diff] [review] Patch *sigh* even more work :). But probably, yeah, this should be done.

Well, now I know a reason: I have no Linux or Mac build env here and try server only works for mozilla-central.

Actually, IIRC, SeaMonkey doesn't have a Mac or Linux shellservice implementation yet.

Comment on attachment 354850 [details] [diff] [review] Patch, includes (dummy) shell service implementation for Linux and Mac Ok, new patch, also ports OpenApplicationWithURI. This one creates a (mostly) dummy implementation for Unix and Mac as we have no shell service there yet and I do not even own a Mac box to do any further development there (I asked someone else to compile this patch and it did). I'm not sure if it is the best way to put the CID and CONTRACTID into nsIShellService.idl, I could also create a seperate header file for this if you think that would be better.

Comment on attachment 354850 [details] [diff] [review] Patch, includes (dummy) shell service implementation for Linux and Mac Rob: Let me know if you think you cannot review this patch (Mac and Linux part mostly copied from Firefox shell service code). See Comment 9 for a general comment on this patch.

Frank, I should be able to review this sometime over the weekend or next week.

Comment on attachment 354850 [details] [diff] [review] Patch, includes (dummy) shell service implementation for Linux and Mac >- shell/public \ >+ shell \ Will this change will break OS/2, BeOS, etc? >+#include "nsIShellService.h" Already included by the platform-specific header. >- { "SeaMonkey Windows Integration", >- NS_SUITEWININTEGRATION_CID, I think it's better to keep the OS-specific component name and CID. >+ NS_SUITEOSINTEGRATION_CONTRACTID, The problem here is that our OS hooks assume that if the contract exists then it works. If you really need the stuff to work on Mac/Linux then it might be better to have a (temporary?) second contract id just for feeds. >+nsresult >+nsGNOMEShellService::Init() >+{ >+ nsresult rv; >+ >+ // GConf and GnomeVFS _must_ be available, or we do not allow >+ // CreateInstance to succeed. >+ >+ nsCOMPtr<nsIGConfService> gconf = do_GetService(NS_GCONFSERVICE_CONTRACTID); >+ nsCOMPtr<nsIGnomeVFSService> vfs = >+ do_GetService(NS_GNOMEVFSSERVICE_CONTRACTID); Alternatively you'll need to rewrite our shell integration UI so that it can deal with a) no contract registered [OS/2] b) contract registered, but can't instantiate [Linux without GnomeVFS] c) contract registered, but methods are unimplemented [Mac, Linux with GnomeVFS]. >+ const nsCString spec(aURI); PromiseFlatCString >+ nsCOMPtr<nsIWindowsRegKey> regKey = >+ do_CreateInstance("@mozilla.org/windows-registry-key;1", &rv); Inconsistent with the rest of this file using native registry functions...

Comment on attachment 354850 [details] [diff] [review] Patch, includes (dummy) shell service implementation for Linux and Mac Valid concerns, need to create a new patch.

Uses nsIWindowsRegKey, now registers "@mozilla.org/suite/shell-feed-service;1" as CONTRACTID until all the shell service components support all functions from shell service properly (then the code can be switched to the normal contract id). The Makefile change should not break OS/2 or BeOS as no source file or lib gets registers for the final link step and this also works in the FF/TB Makefile code.

Comment on attachment 355261 [details] [diff] [review] New patch Frank, I've looked this over a couple of times and really don't like using nsIWindowsRegKey for this code. iirc nsIWindowsRegKey was introduced mainly for JavaScript callers and since this code gets called during startup I am against using it even if there is only a slight perf hit.

Comment on attachment 355261 [details] [diff] [review] New patch btw: I will likely change the Firefox GetDefaultFeedReader as time permits but it isn't all that important since it isn't in the startup code path.

Ok, this one uses native Windows API functions. Wrote a small helper function for reading.

Comment on attachment 364675 [details] [diff] [review] Patch Actually I want to try another approach.

Ok, compared to the previous patch this changes the code for calling the Windows API to something simpler.

Comment on attachment 364719 [details] [diff] [review] Better patch >+ PRBool exists; >+ rv = defaultReader->Exists(&exists); >+ NS_ENSURE_SUCCESS(rv, rv); >+ if (!exists) >+ return NS_ERROR_FAILURE; >+ >+ NS_ADDREF(*_retval = defaultReader); >+ return NS_OK; >+ } >+ Insert "::RegCloseKey(theKey);" here, forget that. >+ return NS_ERROR_FAILURE;

nsIProcess changed, fixing this.

In Bug 423672 The Firefox implementation of GetDefaultFeedReader() changed.

Updated patch with changes from Bug 423672.

Without the nsMacShellService::OpenApplicationWithURI impl, subscribing in external apps is just totally broken on OS X, since the fallback from not getting the shell service is to use nsIProcess.init(), at which point you hit bug 307463 and just sit there. Dunno how you're prioritizing Mac and feed bugs, but "Shiny new feature... just does nothing" seems like it might be a blocker.

> Dunno how you're prioritizing Mac and feed bugs Mac and feed bugs have a priority of 0 until someone buys mcsmurf a Mac (a small blue one will do).

He already wrote the patch: I drive enough that I wouldn't have nominated it if it was sitting in the "nobody has a clue how to even get started with this" state.

It surely doesn't look good. I wonder if someone else can review it. Mark, you have access to a Mac and might know some of thise code, would you be able to take over this review?

Comment on attachment 391129 [details] [diff] [review] Patch Just reviewing the Windows portion and suggest you get someone that is more familiar with Mac OS X, etc. for the rest. Looks fine though you might consider using PRUnichar instead. Also, I am not a Seamonkey peer so can't r+ to get this into comm-central. >diff --git a/suite/shell/src/nsWindowsShellService.cpp b/suite/shell/src/nsWindowsShellService.cpp >--- a/suite/shell/src/nsWindowsShellService.cpp >+++ b/suite/shell/src/nsWindowsShellService.cpp >... >+NS_IMETHODIMP >+nsWindowsShellService::GetDefaultFeedReader(nsILocalFile** _retval) >+{ >+ *_retval = nsnull; >+ >+ HKEY theKey; >+ nsresult rv = OpenKeyForReading(HKEY_CLASSES_ROOT, >+ L"feed\\shell\\open\\command", >+ &theKey); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ DWORD buf; >+ LONG res = ::RegQueryValueExW(theKey, NULL, NULL, NULL, NULL, &buf); >+ if (!REG_FAILED(res)) { Instead of having this large if statement just return NS_ERROR_FAILURE as you already do below when running into an error. Since you are modifying this file already perhaps you can also fix bug 360868 at the same time? r+ for the Windows portion

Fixed bitrot and compile error.

Comment on attachment 403900 [details] [diff] [review] New patch > security \ > shell/public \ > smile \ >+ shell/src \ Nit: belongs alphabetically just after shell/public >-ifeq ($(OS_ARCH),WINNT) >-PARALLEL_DIRS += shell/src > ifdef MOZ_INSTALLER > PARALLEL_DIRS += installer/windows > endif >-endif Doesn't the MOZ_INSTALLER ifdef still need to be inside the WINNT ifeq? > NS_SUITEWININTEGRATION_CID, > NS_SUITEWININTEGRATION_CONTRACTID, > nsWindowsShellServiceConstructor }, >+ { "SeaMonkey Windows Feed Integration", >+ NS_SUITEWINFEED_CID, >+ NS_SUITEWINFEED_CONTRACTID, >+ nsWindowsShellServiceConstructor }, Huh, I thought you needed to use the same CID, but if it works... >+REQUIRES += \ >+ mozgnome \ >+ thebes \ Really? (This is going to be fun with static builds again, I guess.) >+ const nsCString spec(aURI); Nit: PromiseFlatCString (twice; at least the Windows version has it right). >+ if (NS_SUCCEEDED(rv)) { >+ NS_ADDREF(*_retval = defaultReader); >+ rv = NS_OK; Fortunately, rv has already succeeded ;-) Nit: consider using forget or swap (multiple times). >+ if (!REG_FAILED(res)) { Turn this into an early return, like the other failure cases. >+ path.SetLength(buf/2 - 1); Nit: spaces around / >+ if (path.IsEmpty()) >+ return NS_ERROR_FAILURE; Nit: you didn't change path's emptiness since you called SetLength, so if a buf of 2 is invalid then return the error earlier.

Fixed the compile errors and the issues mentioned by Neil.

Comment on attachment 404678 [details] [diff] [review] Patch >+NS_IMETHODIMP >+nsMacShellService::OpenApplicationWithURI(nsILocalFile* aApplication, const nsACString& aURI) ... >+ const nsCString& spec = PromiseFlatString(aURI); Should be PromiseFlatCString otherwise the patch doesn't compile. With the patch applied, If I visit http://planet.mozilla.org/atom.xml with Thunderbird or Mail.app set as my default feed reader, then they aren't shown as default or even listed. In Firefox they are at least shown (I didn't want to restart FF at the time to check default reader). I did notice the following on the text console around the time I opened the atom feed: bit length overflow code 2 bits 6->7 code 6 bits 6->7 If I try and subscribe to a feed, within SeaMonkey it works, if I aim for an external app then I get: WARNING: NS_ENSURE_TRUE(uri) failed: file /Users/moztest/comm/main/src/mozilla/caps/src/nsScriptSecurityManager.cpp, line 162 JavaScript error: , line 0: Permission denied for <moz-safe-about:feeds> to call method UnnamedClass.toString on <>. JavaScript error: , line 0: uncaught exception: unknown (can't convert to string)

We agreed to let this slip for 2.0 proper, but as it can be somewhat annoying to a certain part of our users, we really should push for this to go 2.0.1 at least.

(In reply to comment #32) > bit length overflow > code 2 bits 6->7 > code 6 bits 6->7 That's zlib's trees.c

Comment on attachment 404678 [details] [diff] [review] Patch Just so that you know, this doesn't compile on Linux either ;-) > EXTRA_DSO_LDOPTS += $(LIBXUL_DIST)/lib/$(LIB_PREFIX)thebes.$(LIB_SUFFIX) Not your fault, but this line is wrong, it should be EXTRA_DSO_LIBS += thebes Even then it wouldn't link; it complained about the vtable being missing? >+ const nsCString& spec = PromisteFlatString(aURI); Typo: PromiseFlatCString

Moving blocking flag to next update, we can actually ship a security update as long as it doesn't regress major stuff compared to the previous version. Still, can we please get some progress here? Frank?

Hate to do this again, but forwarding to yet another update. Could we please get progress here?

Now compiles on all three platforms, Mac was only tested as static build (try server)

Did you intend to request review on that patch?

Comment on attachment 426903 [details] [diff] [review] Patch It compiles, but I'm still getting: JavaScript error: , line 0: Permission denied for <moz-safe-about:feeds> to call method UnnamedClass.toString on <>. JavaScript error: , line 0: uncaught exception: unknown (can't convert to string) when clicking on the subscribe button with an external app selected (I'm only testing Mac). Given the nature of the error and the fact the code looks like the same as FF, I very much doubt this is an effect of your c++ changes but something in js somewhere which means I'll be surprised if this is cross-platform.

Here we go again, pushing to yet another release.

(In reply to comment #40) > (From update of attachment 426903 [details] [diff] [review]) > It compiles, but I'm still getting: > > JavaScript error: , line 0: Permission denied for <moz-safe-about:feeds> to > call method UnnamedClass.toString on <>. > JavaScript error: , line 0: uncaught exception: unknown (can't convert to > string) Hmm... I don't suppose this exception is catchable by Venkman, is it? > when clicking on the subscribe button with an external app selected (I'm only > testing Mac). Seems to work fine on Windows with my test app which is @start echo %* ;-)

(In reply to comment #42) > (In reply to comment #40) > > (From update of attachment 426903 [details] [diff] [review]) > > It compiles, but I'm still getting: > > > > JavaScript error: , line 0: Permission denied for <moz-safe-about:feeds> to > > call method UnnamedClass.toString on <>. > > JavaScript error: , line 0: uncaught exception: unknown (can't convert to > > string) > Hmm... I don't suppose this exception is catchable by Venkman, is it? Venkman is pretty unusable on trunk - it tends to abort at frequent times. Dunno if a release build would help.

You could try breakpointing a debug build here where the exception gets thrown: > http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#854 Then maybe the C++ and/or JS stacks would shed some light on the problem.

Mnyromyr tested this yesterday and got this exception when looking at this with Venkman: Exception ``[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProcess.init]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: file:///Users/karsten/Projekte/Mozilla/obj/sr/mozilla/dist/SeaMonkeyDebug.app/Contents/MacOS/components/FeedConverter.js :: addToClientReader :: line 467" data: no]'' thrown from function addToClientReader() in <file:/Users/karsten/Projekte/Mozilla/obj/sr/mozilla/dist/SeaMonkeyDebug.app/Contents/MacOS/components/FeedConverter.js> line 467. init(...) uses a variable as argument that get its value via getComplexValue from a pref that does not exist as far as I see. Maybe this is part of the problem.

(In reply to comment #45) > init(...) uses a variable as argument that get its value via getComplexValue > from a pref that does not exist as far as I see. Maybe this is part of the > problem. Is this a pref that _should_ exist, i.e. that we should set in browser-prefs.js or something like that?

Hrm, after looking at the code again, I think I need to take a closer look at the code that sets this pref. I think we do set set this pref when selecting an client application for feed reading (because otherwise another code path would be used), so maybe something is wrong in that part of the code.

(In reply to comment #45) > Exception ``[Exception... "Component returned failure code: 0x80004005 > (NS_ERROR_FAILURE) [nsIProcess.init]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" > location: "JS frame :: > file:///Users/karsten/Projekte/Mozilla/obj/sr/mozilla/dist/SeaMonkeyDebug.app/Contents/MacOS/components/FeedConverter.js > :: addToClientReader :: line 467" data: no]'' thrown from function > addToClientReader() in > <file:/Users/karsten/Projekte/Mozilla/obj/sr/mozilla/dist/SeaMonkeyDebug.app/Contents/MacOS/components/FeedConverter.js> > line 467. This error is caused by Bug 307463. So I also need to change FeedConverter.js code to support the new shell-feed-service CID (it currently checks for shell-service CID only) so that it does not use the nsIProcess fallback.

Ok, this one should work now, stefanh tested it. The change to FeedConverter.js was what was missing before.

Comment on attachment 439110 [details] [diff] [review] Patch I've only really looked and tested the build config and Mac changes. r=Standard8 for those.

What more do we need here? I would love to see this finally land in our trees...

Comment on attachment 439110 [details] [diff] [review] Patch suite/shell/src/nsWindows* are the Windows bits that need review

Comment on attachment 439110 [details] [diff] [review] Patch suite/shell/src/nsGNOME* are the Linux bits

Comment on attachment 439110 [details] [diff] [review] Patch >+++ b/suite/Makefile.in >+++ b/suite/app/Makefile.in Good. >+++ b/suite/build/Makefile.in Why the location of this block changing? >--- a/suite/build/nsSuiteModule.cpp >+++ b/suite/build/nsSuiteModule.cpp >-#endif // Windows >+#elif defined(XP_MACOSX) >+#if !defined(BUILD_STATIC_SHELL) >+NS_GENERIC_FACTORY_CONSTRUCTOR(nsMacShellService) >+#endif >+#elif defined(MOZ_WIDGET_GTK2) >+#if !defined(BUILD_STATIC_SHELL) >+NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsGNOMEShellService, Init) >+#endif >+#endif nit: break the shell service stuff for windows out of the win ifdef, and do the OS elif's inside a seperate if !defined(BUILD_STATIC_SHELL), rather than doing the STATIC_SHELL check for each os seperately. >+#elif defined(XP_MACOSX) && !defined(BUILD_STATIC_SHELL) >+ { "SeaMonkey Mac Feed Integration", >+ NS_SUITEMACFEED_CID, >+ NS_SUITEMACFEED_CONTRACTID, >+ nsMacShellServiceConstructor }, >+#elif defined(MOZ_WIDGET_GTK2) && !defined(BUILD_STATIC_SHELL) >+ { "SeaMonkey Linux Feed Integration", >+ NS_SUITEGNOMEFEED_CID, >+ NS_SUITEGNOMEFEED_CONTRACTID, >+ nsGNOMEShellServiceConstructor }, >+#endif same nit here. >+++ b/suite/shell/src/Makefile.in >+REQUIRES += \ >+ mozgnome \ >+ thebes \ Just curious, why does the gtk shell service require thebes? I checked everything except the linux and OSX bits in shell/src. and have not yet tested (or thoroughly checked) the win/ bits. But my notes above were mostly with my build-config hat on. Setting feedback? to myself to remind me to test and thoroughly check the win/ stuff if this doesn't land, or no-one beats me to it.

Comment on attachment 439110 [details] [diff] [review] Patch r=me

Comment on attachment 439110 [details] [diff] [review] Patch >+ readonly attribute nsILocalFile defaultFeedReader; [Where does this get used?] >+ // GConf and GnomeVFS _must_ be available, or we do not allow >+ // CreateInstance to succeed. [We do all these checks, but the one method we support doesn't use them...]

(In reply to comment #56) > >+ // GConf and GnomeVFS _must_ be available, or we do not allow > >+ // CreateInstance to succeed. > [We do all these checks, but the one method we support doesn't use them...] I think it's still a good idea to have them in place as it eases us implementing the other functions once this has landed.

(In reply to comment #56) >(From update of attachment 439110 [details] [diff] [review]) >>+ readonly attribute nsILocalFile defaultFeedReader; >[Where does this get used?] Ah, I see now, the code thinks it's already there...

Pushed to comm-central: http://hg.mozilla.org/comm-central/rev/141ea4d58ba8

Bah, I forgot to warn you about trailing whitespace on four lines...

I presume we don't have a bug for activating the "Feeds" checkbox in the Default Client ("Use SeaMonkey as the default client for:") dialog yet? (This is because nsWindowsShellService::IsDefaultClient returns TRUE for RSS) (xref TB bug 445823)

If this works on trunk well enough, can it land on branch as well (i.e. will you ask for approval)? We have set this a needed/soft-blocking on branch so we should still let it in despite the branch being "stable".

I know I mentioned on wiki that this was to land by my hand tonight, but it appears something might be wrong here; on branch part of what changed here is inside a ifndef MOZILLA_1_9_1 ....?

Comment on attachment 451451 [details] [diff] [review] patch for c-1.9.1 err, request from correct person.

Comment on attachment 439110 [details] [diff] [review] Patch > #if !defined(BUILD_STATIC_SHELL) > #include "nsWindowsShellService.h" > #endif >+#elif defined(XP_MACOSX) >+#include "nsMacShellService.h" >+#elif defined(MOZ_WIDGET_GTK2) >+#include "nsGNOMEShellService.h" > #endif This includes the Mac/GNOME headers unnecessarily in static builds. The BUILD_STATIC_SHELL define should wrap the whole block. >+#elif defined(XP_MACOSX) && !defined(BUILD_STATIC_SHELL) OK, so this works slightly better, I guess. >+ >+ Nit: doubled blank line. In fact, neither blank line is necessary. >+#define NS_SUITEGNOMEFEED_CONTRACTID "@mozilla.org/suite/shell-feed-service;1" We really should have this string in a central header file, it's not GNOME (or Mac or Windows) specific. >+#define NS_SUITEWINFEED_CID \ >+{0xd5cbb2a1, 0xaa33, 0x478e, {0xa1, 0x2a, 0xd4, 0xb2, 0x2b, 0x4f, 0x19, 0xd8}} This is just wrong; it's the same object, so it needs to use the same CID.

* Slight improvement to ifdefs in nsSuiteModule * Centralise strings in nsShellService.h copied from Firefox * Improve CID usage and naming

Is it me or are we not actually using HAVE_SHELL_SERVICE correctly? We might as well remove it. We're also not using shell-feed-service... [Anyone else, feel free to review, comment, etc.]

Comment on attachment 452501 [details] [diff] [review] HAVE_SHELL_SERVICE >+var shellSvc = "@mozilla.org/suite/shell-feed-service;1" in Components.classes ? >+ Components.classes["@mozilla.org/suite/shell-feed-service;1"] >+ .getService(Components.interfaces.nsIShellService) : >+ null; Looks odd. Why not: var shellSvc = null; if ("@mozilla.org/suite/shell-feed-service;1" in Components.classes) shellSvc = Components.classes["@mozilla.org/suite/shell-feed-service;1"] .getService(Components.interfaces.nsIShellService); (I don't know enough about the actual changes to comment on them.)

Comment on attachment 452501 [details] [diff] [review] HAVE_SHELL_SERVICE Doing the shellSvc the alternative way does look more readable, but I'm not going to fall out over it.

Comment on attachment 452492 [details] [diff] [review] Fix nits Pushed changeset 231e6783e915 to comm-central. (In reply to comment #68) > (From update of attachment 452501 [details] [diff] [review]) > Why not: > > var shellSvc = null; > if ("@mozilla.org/suite/shell-feed-service;1" in Components.classes) > shellSvc = Components.classes["@mozilla.org/suite/shell-feed-service;1"] > .getService(Components.interfaces.nsIShellService); Pushed changeset 240d34f4920e to comm-central with this change.

http://mxr.mozilla.org/comm-central/source/suite/feeds/src/FeedWriter.js#749 749 // XXXben - we need to compare this with the running instance executable 750 // just don't know how to do that via script... 751 // XXXmano TBD: can probably add this to nsIShellService 752 #ifdef XP_WIN 753 #expand if (fp.file.leafName != "__MOZ_APP_NAME__.exe") { .... If we add this to nsIShellService we could remove the remaining preprocessor usages in these two files right?

mcsmurf, ping re: 1.9.1 patch (I don't think I'll iterate, but I just need to be sure things _are_ right)

Comment on attachment 451451 [details] [diff] [review] patch for c-1.9.1 >diff --git a/suite/app/Makefile.in b/suite/app/Makefile.in >--- a/suite/app/Makefile.in >+++ b/suite/app/Makefile.in >@@ -156,7 +156,7 @@ > ifdef BUILD_STATIC_LIBS > > ifndef MOZILLA_1_9_1_BRANCH >-ifeq ($(OS_ARCH),WINNT) >+ifneq (,$(filter windows cocoa gtk2, $(MOZ_WIDGET_TOOLKIT))) > LIBS += ../shell/src/$(LIB_PREFIX)shellservice_s.$(LIB_SUFFIX) > endif > endif You need to remove this ifndef MOZILLA_1_9_1_BRANCH. Can you fix this and re-diff so that it will include the changes Neil made?

Sorry, actually the ifndef MOZILLA_1_9_1_BRANCH needs to stay. A updated patch would still be good.

This didn't land for 2.0.6, so pushing off one more time. Hope 2.0.7 will really be it.

Comment on attachment 439110 [details] [diff] [review] Patch This landed, so probably doesn't need feedback any more, and it couldn't make it for 2.0.6, so cancelling that Flag as well. Please request 2.0.7 approval once a known-to-apply 1.9.1 patch is here.

Let's not block any more on this.

(In reply to comment #77) > Let's not block any more on this. meaning what? will the fix be implemented in the next release of Seamonkey 2.0? what is going on here?

(In reply to comment #78) > (In reply to comment #77) > > Let's not block any more on this. > > meaning what? > > will the fix be implemented in the next release of Seamonkey 2.0? what is > going on here? erpman1, if you want to help get this into SeaMonkey 2.0 you could (and happily could) work on updating my patch per mcsmurf's comments, and get it reviewed.

Callek, does it make sense to have an open review in this bug? If not, please cancel it to get it off queries and queues.