Closed Bug 333169 Opened 18 years ago Closed 18 years ago

Enable nsBookmarksService to open a window when not using xpfe's commandline service.

Categories

(SeaMonkey :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Assigned: standard8)

References

()

Details

Attachments

(2 files, 2 obsolete files)

Currently when nsBookmarksService wishes to open a window, it asks the command line service for the chrome url for the browser task and then opens a window with the chrome url.

We don't have this facility in toolkit and there are not many places we need to use it.

However, I'm stuck as to the best way to go, so suggestions are welcome.
Attached patch Temporary patch (obsolete) — Splinter Review
I believe this bug is the only thing blocking us from getting a compiled build when we get part 5 of bug 330053 completed. Therefore I'm submitting this as a temporary patch so we can get something building so that its easier for >1 developers to do work on SeaMonkey as an XUL app.

We can then work on a proper patch before our first release.
Attachment #217609 - Flags: superreview?(neil)
Attachment #217609 - Flags: review?(neil)
Comment on attachment 217609 [details] [diff] [review]
Temporary patch

This looks like a bad workaround to me, but it may be acceptable to get at least a somewhat working build of "suiterunner" now.

Actually, I even wonder if a "proper fix" in the current world might be to "hardcode" how to get a SeaMonkey browser window there, as I wildly guess noone outside of suite might use that code, right?
Comment on attachment 217609 [details] [diff] [review]
Temporary patch

I think we should just inline the code from http://lxr.mozilla.org/seamonkey/source/xpfe/browser/src/nsBrowserInstance.cpp#647 (and do the xremote service to if we're still using it)
Attachment #217609 - Flags: superreview?(neil)
Attachment #217609 - Flags: superreview-
Attachment #217609 - Flags: review?(neil)
Attachment #217609 - Flags: review-
Attached patch Patch v2 (obsolete) — Splinter Review
Revised patch addressing Neil's comments.
Attachment #217609 - Attachment is obsolete: true
Attachment #217668 - Flags: superreview?(neil)
Attachment #217668 - Flags: review?(neil)
You almost certainly want nsIURILoader.loadURI
(In reply to comment #5)
>You almost certainly want nsIURILoader.loadURI
You'd think so, but it can't open a content window without an opener.
Comment on attachment 217668 [details] [diff] [review]
Patch v2

Your new code should work both with and without MOZ_XUL_APP, no need to ifdef it.

>+              if (NS_SUCCEEDED(rv) && chromeUrl.IsEmpty())
>+              {
>+                rv = NS_ERROR_FAILURE;
>+              }
>+            }
>+            if (NS_FAILED(rv))
Rather than all these tests I think you might as well test for chromeUrl.IsEmpty() instead.

>             wwatch->OpenWindow(0, chromeUrl, "_blank", "chrome,dialog=no,all", 
>                                suppArray, getter_AddRefs(newWindow));
>                     }
The indentation in this whole section of code is screwy. Would you mind reindenting it to the 4 spaces that the rest of the file seems to use?
Attachment #217668 - Flags: superreview?(neil)
Attachment #217668 - Flags: superreview-
Attachment #217668 - Flags: review?(neil)
Attachment #217668 - Flags: review-
Revised patch as per Neil's comments.
Assignee: nobody → bugzilla
Attachment #217668 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #217914 - Flags: superreview?(neil)
Attachment #217914 - Flags: review?(neil)
Normal version of the -w.
Comment on attachment 217914 [details] [diff] [review]
Patch v3 (diff -w)

>+                        nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
>+                        if (NS_SUCCEEDED(rv))
>+                        {
>+                            prefs->GetCharPref("browser.chromeURL", getter_Copies(chromeUrl));
>+                            if (chromeUrl.IsEmpty())
>+                            {
>+                                chromeUrl.AssignLiteral("chrome://navigator/content/navigator.xul");
>+                            }
>+                            wwatch->OpenWindow(0, chromeUrl, "_blank",
>+                                               "chrome,dialog=no,all",
>+                                               suppArray,
>+                                               getter_AddRefs(newWindow));
>+                        }
I think you've got your nesting wrong here; only the GetCharPref needs to be inside the first if. Also when outdenting the wwatch lines you should find you can tack suppArray onto the end of the previous line. r+sr=me with those fixed.
Attachment #217914 - Flags: superreview?(neil)
Attachment #217914 - Flags: superreview+
Attachment #217914 - Flags: review?(neil)
Attachment #217914 - Flags: review+
This is now checked in with Neil's nits addressed.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: helpwanted
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: