Closed Bug 479600 Opened 11 years ago Closed 11 years ago

Support -new-tab and -new-window command line options

Categories

(SeaMonkey :: Startup & Profiles, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0b1

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

Details

Attachments

(1 file, 3 obsolete files)

SeaMonkey already supports this another way, using -remote "openURL(http://some.url, new-tab)" (also working on Windows for some time) but it is not very discoverable and probably hard to remember for the average user.

Looking at the FF code it seems all it takes is the few lines starting here:
<http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#419>
Should be easy to port if we have similar (same?) functions to open tabs and windows--at least the -remote code looks similar to me:
<http://mxr.mozilla.org/comm-central/source/suite/browser/nsBrowserContentHandler.js#276>
Attached patch proposed patch (obsolete) — Splinter Review
Assignee: nobody → jh
Status: NEW → ASSIGNED
Attachment #363478 - Flags: superreview?(neil)
Attachment #363478 - Flags: review?(neil)
Attached patch patch v2 (with newline) (obsolete) — Splinter Review
My bad, didn't see that with column width 80 in vim :-(
Attachment #363478 - Attachment is obsolete: true
Attachment #363481 - Flags: superreview?(neil)
Attachment #363481 - Flags: review?(neil)
Attachment #363478 - Flags: superreview?(neil)
Attachment #363478 - Flags: review?(neil)
Comment on attachment 363481 [details] [diff] [review]
patch v2 (with newline)

Hmm, we don't use while loops on our other params...
(In reply to comment #3)
> (From update of attachment 363481 [details] [diff] [review])
> Hmm, we don't use while loops on our other params...

Yes but we don't handle similar command line options yet. Without the loop this won't work as expected:
seamonkey.exe -new-tab mozilla.org -new-tab planet.mozilla.org -new-window mozillazine.org -new-window seamonkey-project.org

I'm only thinking whether uriparam should be renamed to uriParam for consistency with the other cases. Please let me know what you think.
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 363481 [details] [diff] [review])
> > Hmm, we don't use while loops on our other params...
> Yes but we don't handle similar command line options yet.
I would count -remote and -url as similar, and we don't use while loops there.
Or indeed -browser, since -browser <URL> works the same way as -new-window.
(In reply to comment #5)
> > > Hmm, we don't use while loops on our other params...
> > Yes but we don't handle similar command line options yet.
> I would count -remote and -url as similar, and we don't use while 
> loops there.

Granted, but maybe that is only for historic reasons. I don't think it should prevent us from implementing the new options with more flexibility (I hope the example I provided shows that this can be handy) and compatible with FF.

(In reply to comment #6)
> Or indeed -browser, since -browser <URL> works the same way as -new-window.

Almost. FF doesn't accept an URL there and for us it's optional, unlike with the proposed -new-window behavior.
Comment on attachment 363481 [details] [diff] [review]
patch v2 (with newline)

>+    var uriparam;
"param" on its own would be fine too.

>+        if (!shouldLoadURI(uri))
>+          continue;
Everywhere else we still call preventDefault if this fails.

>+        openWindow(null, getBrowserURL(), features, uri.spec);
Or consider using handURIToExistingBrowser instead.
Attached patch patch v3 (obsolete) — Splinter Review
(In reply to comment #8)
> (From update of attachment 363481 [details] [diff] [review])
> >+    var uriparam;
> "param" on its own would be fine too.

I tried to be consistent (browserParam, urlParam, chromeParam) but since this one is used for two cases I'm also fine with a more generic name.

> >+        if (!shouldLoadURI(uri))
> >+          continue;
> Everywhere else we still call preventDefault if this fails.
> 
> >+        openWindow(null, getBrowserURL(), features, uri.spec);
> Or consider using handURIToExistingBrowser instead.

Actually you're right, handURIToExistingBrowser() also does the check and allows to request opening new windows so I switched to that. I also corrected a mistake introduced by the previous patches where I passed cmdLine to handURIToExistingBrowser() in the new-tab case which is correct for FF but not for us--we have to pass in the features variable. I further checked that the -width and -height parameters are actually reflected in the features variable but somehow they seem to have no effect (neither in SM nor FF), at least here on Windows. I lost track in nsWindowWatcher.cpp but anyway that's another issue.
Attachment #363481 - Attachment is obsolete: true
Attachment #366007 - Flags: superreview?(neil)
Attachment #366007 - Flags: review?(neil)
Attachment #363481 - Flags: superreview?(neil)
Attachment #363481 - Flags: review?(neil)
Attachment #366007 - Flags: superreview?(neil)
Attachment #366007 - Flags: superreview+
Attachment #366007 - Flags: review?(neil)
Attachment #366007 - Flags: review+
Comment on attachment 366007 [details] [diff] [review]
patch v3

>+      while (param = cmdLine.handleFlagWithParam("new-window", false)) {
Isn't this a strict JS warning? Please put while ((param = ...) != null)
Attachment #366007 - Attachment is obsolete: true
Attachment #366343 - Flags: superreview+
Attachment #366343 - Flags: review+
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/4d2686878445
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0b1
You need to log in before you can comment on or make changes to this bug.