Port GetDefaultFeedReader to SeaMonkey shell service

RESOLVED FIXED

Status

SeaMonkey
OS Integration
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: mcsmurf, Assigned: mcsmurf)

Tracking

Trunk
x86
Windows XP
Dependency tree / graph
Bug Flags:
blocking-seamonkey2.0 -
blocking-seamonkey2.0.1 -
blocking-seamonkey2.0.3 -
blocking-seamonkey2.0.4 -
blocking-seamonkey2.0.5 -
blocking-seamonkey2.0.6 -
blocking-seamonkey2.0.7 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 10 obsolete attachments)

27.92 KB, patch
standard8
: review+
Callek
: review+
Ian Neal
: review+
Details | Diff | Splinter Review
26.49 KB, patch
Details | Diff | Splinter Review
9.73 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
3.12 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
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.
Blocks: 415372
(Assignee)

Comment 2

9 years ago
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.
Attachment #354629 - Flags: review?(robert.bugzilla)

Comment 3

9 years ago
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?
(Assignee)

Comment 5

9 years ago
Comment on attachment 354629 [details] [diff] [review]
Patch

*sigh* even more work :). But probably, yeah, this should be done.
Attachment #354629 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 6

9 years ago
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

9 years ago
Actually, IIRC, SeaMonkey doesn't have a Mac or Linux shellservice implementation yet.
(Assignee)

Comment 8

9 years ago
Created attachment 354850 [details] [diff] [review]
Patch, includes (dummy) shell service implementation for Linux and Mac
(Assignee)

Comment 9

9 years ago
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.
Attachment #354850 - Flags: review?(neil)
(Assignee)

Comment 10

9 years ago
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.
Attachment #354850 - Flags: review?(neil) → review?(robert.bugzilla)
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...
(Assignee)

Comment 13

9 years ago
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.
Attachment #354850 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 14

9 years ago
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.
Attachment #354629 - Attachment is obsolete: true
Attachment #354850 - Attachment is obsolete: true
Attachment #355261 - Flags: review?(robert.bugzilla)
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.
Attachment #355261 - Flags: review?(robert.bugzilla) → review-
(Assignee)

Updated

8 years ago
Component: General → OS Integration
QA Contact: general → os-integration
(Assignee)

Comment 17

8 years ago
Created attachment 364675 [details] [diff] [review]
Patch

Ok, this one uses native Windows API functions. Wrote a small helper function for reading.
Attachment #355261 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #364675 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

8 years ago
Attachment #364675 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 18

8 years ago
Comment on attachment 364675 [details] [diff] [review]
Patch

Actually I want to try another approach.
(Assignee)

Comment 19

8 years ago
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.
Attachment #364675 - Attachment is obsolete: true
Attachment #364719 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 20

8 years ago
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;
(Assignee)

Comment 21

8 years ago
Created attachment 369124 [details] [diff] [review]
New patch

nsIProcess changed, fixing this.
Attachment #364719 - Attachment is obsolete: true
Attachment #369124 - Flags: review?(robert.bugzilla)
Attachment #364719 - Flags: review?(robert.bugzilla)

Comment 22

8 years ago
In Bug 423672 The Firefox implementation of GetDefaultFeedReader() changed.
Depends on: 423672
(Assignee)

Comment 23

8 years ago
Created attachment 391129 [details] [diff] [review]
Patch

Updated patch with changes from Bug 423672.
Attachment #369124 - Attachment is obsolete: true
Attachment #391129 - Flags: review?(robert.bugzilla)
Attachment #369124 - Flags: review?(robert.bugzilla)
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.
Flags: blocking-seamonkey2?

Comment 25

8 years ago
> 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.

Comment 27

8 years ago
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?
Attachment #391129 - Flags: review?(bugzilla)
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
Attachment #391129 - Flags: review?(robert.bugzilla) → review+

Updated

8 years ago
Flags: blocking-seamonkey2.0? → blocking-seamonkey2.0+
Status: NEW → ASSIGNED
(Assignee)

Comment 29

8 years ago
Created attachment 403900 [details] [diff] [review]
New patch

Fixed bitrot and compile error.
Attachment #391129 - Attachment is obsolete: true
Attachment #403900 - Flags: ui-review?(bugzilla)
Attachment #403900 - Flags: review+
Attachment #391129 - Flags: review?(bugzilla)
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.
(Assignee)

Comment 31

8 years ago
Created attachment 404678 [details] [diff] [review]
Patch

Fixed the compile errors and the issues mentioned by Neil.
Attachment #403900 - Attachment is obsolete: true
Attachment #404678 - Flags: review?(bugzilla)
Attachment #403900 - Flags: ui-review?(bugzilla)
Attachment #404678 - Flags: review?(bugzilla) → review-
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

8 years ago
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.
Flags: blocking-seamonkey2.0.1+
Flags: blocking-seamonkey2.0-
Flags: blocking-seamonkey2.0+
(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

Comment 36

8 years ago
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?
Flags: blocking-seamonkey2.0.2+
Flags: blocking-seamonkey2.0.1-
Flags: blocking-seamonkey2.0.1+

Comment 37

7 years ago
Hate to do this again, but forwarding to yet another update. Could we please get progress here?
Flags: blocking-seamonkey2.0.4+
Flags: blocking-seamonkey2.0.3-
Flags: blocking-seamonkey2.0.3+
(Assignee)

Comment 38

7 years ago
Created attachment 426903 [details] [diff] [review]
Patch

Now compiles on all three platforms, Mac was only tested as static build (try server)
Attachment #404678 - Attachment is obsolete: true

Comment 39

7 years ago
Did you intend to request review on that patch?
(Assignee)

Updated

7 years ago
Attachment #426903 - Flags: review?(bugzilla)
Attachment #426903 - Flags: review?(bugzilla) → review-
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

7 years ago
Here we go again, pushing to yet another release.
Flags: blocking-seamonkey2.0.5+
Flags: blocking-seamonkey2.0.4-
Flags: blocking-seamonkey2.0.4+
(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.
(Assignee)

Comment 45

7 years ago
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

7 years ago
(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?
(Assignee)

Comment 47

7 years ago
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.
(Assignee)

Comment 48

7 years ago
(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.
(Assignee)

Comment 49

7 years ago
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.
Attachment #426903 - Attachment is obsolete: true
Attachment #439110 - Flags: review?(bugzilla)
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.
Attachment #439110 - Flags: review?(bugzilla) → review+

Comment 51

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

Updated

7 years ago
Flags: blocking-seamonkey2.0.6+
Flags: blocking-seamonkey2.0.5-
Flags: blocking-seamonkey2.0.5+
(Assignee)

Comment 52

7 years ago
Comment on attachment 439110 [details] [diff] [review]
Patch

suite/shell/src/nsWindows* are the Windows bits that need review
Attachment #439110 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 53

7 years ago
Comment on attachment 439110 [details] [diff] [review]
Patch

suite/shell/src/nsGNOME* are the Linux bits
Attachment #439110 - Flags: review?(iann_bugzilla)
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.
Attachment #439110 - Flags: review?(bugspam.Callek)
Attachment #439110 - Flags: review+
Attachment #439110 - Flags: feedback?(bugspam.Callek)

Comment 55

7 years ago
Comment on attachment 439110 [details] [diff] [review]
Patch

r=me
Attachment #439110 - Flags: review?(iann_bugzilla) → review+
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

7 years ago
(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...
(Assignee)

Comment 59

7 years ago
Pushed to comm-central: http://hg.mozilla.org/comm-central/rev/141ea4d58ba8
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
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)

Comment 62

7 years ago
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".
(Assignee)

Updated

7 years ago
Attachment #439110 - Flags: approval-seamonkey2.0.6?

Updated

7 years ago
Attachment #439110 - Flags: approval-seamonkey2.0.6? → approval-seamonkey2.0.6+
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 ....?
Attachment #451451 - Flags: review?(mnyromyr)
Comment on attachment 451451 [details] [diff] [review]
patch for c-1.9.1

err, request from correct person.
Attachment #451451 - Flags: review?(mnyromyr) → review?(bugzilla)
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.
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
Attachment #452492 - Flags: review?(iann_bugzilla)
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.]
Attachment #452501 - Flags: review?(iann_bugzilla)
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.)

Updated

7 years ago
Attachment #452492 - Flags: review?(iann_bugzilla) → review+

Comment 69

7 years ago
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.
Attachment #452501 - Flags: review?(iann_bugzilla) → review+
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

7 years ago
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)
(Assignee)

Comment 73

7 years ago
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?
(Assignee)

Comment 74

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

Comment 75

7 years ago
This didn't land for 2.0.6, so pushing off one more time. Hope 2.0.7 will really be it.
Flags: blocking-seamonkey2.0.7?
Flags: blocking-seamonkey2.0.6-
Flags: blocking-seamonkey2.0.6+

Comment 76

7 years ago
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.
Attachment #439110 - Flags: feedback?(bugspam.Callek)
Attachment #439110 - Flags: approval-seamonkey2.0.6+

Comment 77

7 years ago
Let's not block any more on this.
Flags: blocking-seamonkey2.0.7? → blocking-seamonkey2.0.7-

Comment 78

7 years ago
(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.

Comment 80

7 years ago
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.

Updated

7 years ago
Attachment #451451 - Flags: review?(bugzilla)
You need to log in before you can comment on or make changes to this bug.