Last Comment Bug 471346 - Port GetDefaultFeedReader to SeaMonkey shell service
: Port GetDefaultFeedReader to SeaMonkey shell service
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: OS Integration (show other bugs)
: Trunk
: x86 Windows XP
: -- normal with 1 vote (vote)
: ---
Assigned To: Frank Wein [:mcsmurf]
:
:
Mentors:
Depends on: 423672
Blocks: 415372
  Show dependency treegraph
 
Reported: 2008-12-28 13:15 PST by Frank Wein [:mcsmurf]
Modified: 2010-12-01 15:53 PST (History)
15 users (show)
kairo: blocking‑seamonkey2.0-
kairo: blocking‑seamonkey2.0.1-
kairo: blocking‑seamonkey2.0.3-
kairo: blocking‑seamonkey2.0.4-
kairo: blocking‑seamonkey2.0.5-
kairo: blocking‑seamonkey2.0.6-
kairo: blocking‑seamonkey2.0.7-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (3.51 KB, patch)
2008-12-28 13:52 PST, Frank Wein [:mcsmurf]
no flags Details | Diff | Splinter Review
Patch, includes (dummy) shell service implementation for Linux and Mac (26.08 KB, patch)
2008-12-30 11:31 PST, Frank Wein [:mcsmurf]
no flags Details | Diff | Splinter Review
New patch (30.89 KB, patch)
2009-01-03 20:40 PST, Frank Wein [:mcsmurf]
robert.strong.bugs: review-
Details | Diff | Splinter Review
Patch (27.90 KB, patch)
2009-02-28 08:03 PST, Frank Wein [:mcsmurf]
no flags Details | Diff | Splinter Review
Better patch (27.11 KB, patch)
2009-02-28 15:08 PST, Frank Wein [:mcsmurf]
no flags Details | Diff | Splinter Review
New patch (24.46 KB, patch)
2009-03-24 12:41 PDT, Frank Wein [:mcsmurf]
no flags Details | Diff | Splinter Review
Patch (24.53 KB, patch)
2009-07-28 11:16 PDT, Frank Wein [:mcsmurf]
robert.strong.bugs: review+
Details | Diff | Splinter Review
New patch (25.01 KB, patch)
2009-09-30 16:25 PDT, Frank Wein [:mcsmurf]
bugzilla: review+
Details | Diff | Splinter Review
Patch (24.91 KB, patch)
2009-10-05 13:42 PDT, Frank Wein [:mcsmurf]
standard8: review-
Details | Diff | Splinter Review
Patch (27.27 KB, patch)
2010-02-14 09:07 PST, Frank Wein [:mcsmurf]
standard8: review-
Details | Diff | Splinter Review
Patch (27.92 KB, patch)
2010-04-14 15:24 PDT, Frank Wein [:mcsmurf]
standard8: review+
bugspam.Callek: review+
iann_bugzilla: review+
Details | Diff | Splinter Review
patch for c-1.9.1 (26.49 KB, patch)
2010-06-15 19:33 PDT, Justin Wood (:Callek)
no flags Details | Diff | Splinter Review
Fix nits (9.73 KB, patch)
2010-06-19 14:35 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review
HAVE_SHELL_SERVICE (3.12 KB, patch)
2010-06-19 16:04 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review

Description Frank Wein [:mcsmurf] 2008-12-28 13:15:11 PST
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.
Comment 1 Frank Wein [:mcsmurf] 2008-12-28 13:52:50 PST
Created attachment 354629 [details] [diff] [review]
Patch

I removed the include of nsCOMPtr.h as it's already included in nsDirectoryServiceUtils.h for a long time now.
Comment 2 Frank Wein [:mcsmurf] 2008-12-28 14:09:39 PST
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.
Comment 3 Philip Chee 2008-12-28 17:12:31 PST
So, uh, what about nsGNOMEShellService and nsMacShellService?
Comment 4 Robert Strong [:rstrong] (use needinfo to contact me) 2008-12-28 18:04:19 PST
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 5 Frank Wein [:mcsmurf] 2008-12-28 18:05:42 PST
Comment on attachment 354629 [details] [diff] [review]
Patch

*sigh* even more work :). But probably, yeah, this should be done.
Comment 6 Frank Wein [:mcsmurf] 2008-12-29 06:10:59 PST
Well, now I know a reason: I have no Linux or Mac build env here and try server only works for mozilla-central.
Comment 7 Robert Kaiser 2008-12-29 06:14:24 PST
Actually, IIRC, SeaMonkey doesn't have a Mac or Linux shellservice implementation yet.
Comment 8 Frank Wein [:mcsmurf] 2008-12-30 11:31:53 PST
Created attachment 354850 [details] [diff] [review]
Patch, includes (dummy) shell service implementation for Linux and Mac
Comment 9 Frank Wein [:mcsmurf] 2008-12-30 13:16:33 PST
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 10 Frank Wein [:mcsmurf] 2008-12-30 13:21:15 PST
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.
Comment 11 Robert Strong [:rstrong] (use needinfo to contact me) 2008-12-31 12:08:56 PST
Frank, I should be able to review this sometime over the weekend or next week.
Comment 12 neil@parkwaycc.co.uk 2009-01-01 09:15:41 PST
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 13 Frank Wein [:mcsmurf] 2009-01-01 11:33:28 PST
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.
Comment 14 Frank Wein [:mcsmurf] 2009-01-03 20:40:08 PST
Created attachment 355261 [details] [diff] [review]
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 15 Robert Strong [:rstrong] (use needinfo to contact me) 2009-02-01 17:12:16 PST
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 16 Robert Strong [:rstrong] (use needinfo to contact me) 2009-02-01 17:13:26 PST
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.
Comment 17 Frank Wein [:mcsmurf] 2009-02-28 08:03:06 PST
Created attachment 364675 [details] [diff] [review]
Patch

Ok, this one uses native Windows API functions. Wrote a small helper function for reading.
Comment 18 Frank Wein [:mcsmurf] 2009-02-28 10:04:35 PST
Comment on attachment 364675 [details] [diff] [review]
Patch

Actually I want to try another approach.
Comment 19 Frank Wein [:mcsmurf] 2009-02-28 15:08:43 PST
Created attachment 364719 [details] [diff] [review]
Better patch

Ok, compared to the previous patch this changes the code for calling the Windows API to something simpler.
Comment 20 Frank Wein [:mcsmurf] 2009-02-28 15:10:56 PST
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;
Comment 21 Frank Wein [:mcsmurf] 2009-03-24 12:41:01 PDT
Created attachment 369124 [details] [diff] [review]
New patch

nsIProcess changed, fixing this.
Comment 22 Philip Chee 2009-04-06 11:11:40 PDT
In Bug 423672 The Firefox implementation of GetDefaultFeedReader() changed.
Comment 23 Frank Wein [:mcsmurf] 2009-07-28 11:16:27 PDT
Created attachment 391129 [details] [diff] [review]
Patch

Updated patch with changes from Bug 423672.
Comment 24 Phil Ringnalda (:philor) 2009-08-22 17:44:46 PDT
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.
Comment 25 Philip Chee 2009-08-22 18:20:56 PDT
> 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).
Comment 26 Phil Ringnalda (:philor) 2009-08-22 18:29:06 PDT
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.
Comment 27 Robert Kaiser 2009-08-23 08:08:12 PDT
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 28 Robert Strong [:rstrong] (use needinfo to contact me) 2009-09-02 13:04:58 PDT
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
Comment 29 Frank Wein [:mcsmurf] 2009-09-30 16:25:15 PDT
Created attachment 403900 [details] [diff] [review]
New patch

Fixed bitrot and compile error.
Comment 30 neil@parkwaycc.co.uk 2009-10-04 06:37:37 PDT
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.
Comment 31 Frank Wein [:mcsmurf] 2009-10-05 13:42:22 PDT
Created attachment 404678 [details] [diff] [review]
Patch

Fixed the compile errors and the issues mentioned by Neil.
Comment 32 Mark Banner (:standard8, limited time in Dec) 2009-10-06 04:24:04 PDT
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)
Comment 33 Robert Kaiser 2009-10-13 06:37:23 PDT
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.
Comment 34 neil@parkwaycc.co.uk 2009-10-14 04:36:48 PDT
(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 35 neil@parkwaycc.co.uk 2009-10-18 15:59:15 PDT
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
Comment 36 Robert Kaiser 2009-12-09 07:26:06 PST
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?
Comment 37 Robert Kaiser 2010-02-05 14:35:18 PST
Hate to do this again, but forwarding to yet another update. Could we please get progress here?
Comment 38 Frank Wein [:mcsmurf] 2010-02-14 09:07:55 PST
Created attachment 426903 [details] [diff] [review]
Patch

Now compiles on all three platforms, Mac was only tested as static build (try server)
Comment 39 Robert Kaiser 2010-02-14 10:38:10 PST
Did you intend to request review on that patch?
Comment 40 Mark Banner (:standard8, limited time in Dec) 2010-03-15 03:06:02 PDT
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.
Comment 41 Robert Kaiser 2010-03-17 12:16:43 PDT
Here we go again, pushing to yet another release.
Comment 42 neil@parkwaycc.co.uk 2010-03-23 11:02:42 PDT
(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 %* ;-)
Comment 43 Mark Banner (:standard8, limited time in Dec) 2010-03-23 11:20:17 PDT
(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.
Comment 44 neil@parkwaycc.co.uk 2010-03-23 18:00:57 PDT
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.
Comment 45 Frank Wein [:mcsmurf] 2010-03-29 15:18:49 PDT
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.
Comment 46 Robert Kaiser 2010-03-29 18:04:45 PDT
(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?
Comment 47 Frank Wein [:mcsmurf] 2010-03-30 00:31:42 PDT
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.
Comment 48 Frank Wein [:mcsmurf] 2010-04-07 14:27:11 PDT
(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.
Comment 49 Frank Wein [:mcsmurf] 2010-04-14 15:24:11 PDT
Created attachment 439110 [details] [diff] [review]
Patch

Ok, this one should work now, stefanh tested it. The change to FeedConverter.js was what was missing before.
Comment 50 Mark Banner (:standard8, limited time in Dec) 2010-04-15 04:33:11 PDT
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.
Comment 51 Robert Kaiser 2010-05-04 06:11:34 PDT
What more do we need here?
I would love to see this finally land in our trees...
Comment 52 Frank Wein [:mcsmurf] 2010-05-05 14:35:28 PDT
Comment on attachment 439110 [details] [diff] [review]
Patch

suite/shell/src/nsWindows* are the Windows bits that need review
Comment 53 Frank Wein [:mcsmurf] 2010-05-05 14:37:28 PDT
Comment on attachment 439110 [details] [diff] [review]
Patch

suite/shell/src/nsGNOME* are the Linux bits
Comment 54 Justin Wood (:Callek) 2010-05-10 22:47:44 PDT
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 55 Ian Neal 2010-05-24 11:46:55 PDT
Comment on attachment 439110 [details] [diff] [review]
Patch

r=me
Comment 56 neil@parkwaycc.co.uk 2010-06-02 09:54:13 PDT
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...]
Comment 57 Robert Kaiser 2010-06-02 12:09:03 PDT
(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.
Comment 58 neil@parkwaycc.co.uk 2010-06-02 12:13:24 PDT
(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...
Comment 59 Frank Wein [:mcsmurf] 2010-06-04 14:52:40 PDT
Pushed to comm-central: http://hg.mozilla.org/comm-central/rev/141ea4d58ba8
Comment 60 neil@parkwaycc.co.uk 2010-06-04 16:47:03 PDT
Bah, I forgot to warn you about trailing whitespace on four lines...
Comment 61 Jens Hatlak (:InvisibleSmiley) 2010-06-05 02:10:17 PDT
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)
Comment 62 Robert Kaiser 2010-06-05 09:21:37 PDT
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".
Comment 63 Justin Wood (:Callek) 2010-06-15 19:33:58 PDT
Created attachment 451451 [details] [diff] [review]
patch for c-1.9.1

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 64 Justin Wood (:Callek) 2010-06-15 19:34:28 PDT
Comment on attachment 451451 [details] [diff] [review]
patch for c-1.9.1

err, request from correct person.
Comment 65 neil@parkwaycc.co.uk 2010-06-18 17:15:00 PDT
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.
Comment 66 neil@parkwaycc.co.uk 2010-06-19 14:35:00 PDT
Created attachment 452492 [details] [diff] [review]
Fix nits

* Slight improvement to ifdefs in nsSuiteModule
* Centralise strings in nsShellService.h copied from Firefox
* Improve CID usage and naming
Comment 67 neil@parkwaycc.co.uk 2010-06-19 16:04:55 PDT
Created attachment 452501 [details] [diff] [review]
HAVE_SHELL_SERVICE

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 68 Jens Hatlak (:InvisibleSmiley) 2010-06-19 16:20:00 PDT
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 69 Ian Neal 2010-06-20 11:06:56 PDT
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 70 neil@parkwaycc.co.uk 2010-06-20 15:54:59 PDT
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.
Comment 71 Philip Chee 2010-06-20 22:12:15 PDT
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?
Comment 72 Justin Wood (:Callek) 2010-06-21 20:34:26 PDT
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 73 Frank Wein [:mcsmurf] 2010-06-22 15:06:49 PDT
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?
Comment 74 Frank Wein [:mcsmurf] 2010-06-22 22:32:26 PDT
Sorry, actually the ifndef MOZILLA_1_9_1_BRANCH needs to stay. A updated patch would still be good.
Comment 75 Robert Kaiser 2010-07-01 12:51:31 PDT
This didn't land for 2.0.6, so pushing off one more time. Hope 2.0.7 will really be it.
Comment 76 Robert Kaiser 2010-07-01 12:53:18 PDT
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.
Comment 77 Robert Kaiser 2010-08-25 06:37:34 PDT
Let's not block any more on this.
Comment 78 erpman1 2010-09-01 10:35:56 PDT
(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?
Comment 79 Justin Wood (:Callek) 2010-10-03 11:34:44 PDT
(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.
Comment 80 Robert Kaiser 2010-11-30 11:23:30 PST
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.

Note You need to log in before you can comment on or make changes to this bug.