Last Comment Bug 479600 - Support -new-tab and -new-window command line options
: Support -new-tab and -new-window command line options
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Startup & Profiles (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.0b1
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-21 06:45 PST by Jens Hatlak (:InvisibleSmiley)
Modified: 2009-03-12 14:21 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed patch (1.52 KB, patch)
2009-02-21 07:30 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v2 (with newline) (1.53 KB, patch)
2009-02-21 07:39 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v3 (1.48 KB, patch)
2009-03-06 14:36 PST, Jens Hatlak (:InvisibleSmiley)
neil: review+
neil: superreview+
Details | Diff | Splinter Review
patch v4, r+sr=neil (1.50 KB, patch)
2009-03-09 11:41 PDT, Jens Hatlak (:InvisibleSmiley)
jh: review+
jh: superreview+
Details | Diff | Splinter Review

Description Jens Hatlak (:InvisibleSmiley) 2009-02-21 06:45:41 PST
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>
Comment 1 Jens Hatlak (:InvisibleSmiley) 2009-02-21 07:30:34 PST
Created attachment 363478 [details] [diff] [review]
proposed patch
Comment 2 Jens Hatlak (:InvisibleSmiley) 2009-02-21 07:39:24 PST
Created attachment 363481 [details] [diff] [review]
patch v2 (with newline)

My bad, didn't see that with column width 80 in vim :-(
Comment 3 neil@parkwaycc.co.uk 2009-02-21 09:01:54 PST
Comment on attachment 363481 [details] [diff] [review]
patch v2 (with newline)

Hmm, we don't use while loops on our other params...
Comment 4 Jens Hatlak (:InvisibleSmiley) 2009-02-22 02:23:30 PST
(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.
Comment 5 neil@parkwaycc.co.uk 2009-02-22 12:30:34 PST
(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.
Comment 6 neil@parkwaycc.co.uk 2009-02-22 12:31:03 PST
Or indeed -browser, since -browser <URL> works the same way as -new-window.
Comment 7 Jens Hatlak (:InvisibleSmiley) 2009-02-22 14:31:20 PST
(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 8 neil@parkwaycc.co.uk 2009-03-05 15:50:56 PST
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.
Comment 9 Jens Hatlak (:InvisibleSmiley) 2009-03-06 14:36:10 PST
Created attachment 366007 [details] [diff] [review]
patch v3

(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.
Comment 10 neil@parkwaycc.co.uk 2009-03-09 07:28:44 PDT
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)
Comment 11 Jens Hatlak (:InvisibleSmiley) 2009-03-09 11:41:52 PDT
Created attachment 366343 [details] [diff] [review]
patch v4, r+sr=neil
Comment 12 Stefan [:stefanh] 2009-03-12 14:21:41 PDT
http://hg.mozilla.org/comm-central/rev/4d2686878445

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