Last Comment Bug 648781 - Add support for Firefox's -search command line option
: Add support for Firefox's -search command line option
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: OS Integration (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.1final
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on:
Blocks: 505082
  Show dependency treegraph
 
Reported: 2011-04-09 13:19 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2011-04-11 09:42 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (4.67 KB, patch)
2011-04-09 14:13 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
patch v2 [Checkin: comment 8] (4.67 KB, patch)
2011-04-10 13:53 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Review

Description Jens Hatlak (:InvisibleSmiley) 2011-04-09 13:19:14 PDT
The FF implementation is in nsBrowserContentHandler.js, handleFlagWithParam("search" and function doSearch. SMOP with no l10n impact.
Comment 1 Jens Hatlak (:InvisibleSmiley) 2011-04-09 14:13:45 PDT
Created attachment 524877 [details] [diff] [review]
patch

I'm not sure about bsmedberg's comment, maybe you can enlighten me.

While I was at it I also added some more options to helpInfo.

I left out "? searchterm" for Windows Vista start menu integration (FF nsBrowserContentHandler.js line 555 and following) for now since FF uses preprocessing to limit it to Windows and that's so frowned upon here. [And since I didn't port it I didn't test it.]
Comment 2 neil@parkwaycc.co.uk 2011-04-09 15:42:43 PDT
(In reply to comment #1)
> I'm not sure about bsmedberg's comment, maybe you can enlighten me.
The -url parameter respects tabbed browsing preferences, but -search always opens a new window.

> I left out "? searchterm" for Windows Vista start menu integration (FF
> nsBrowserContentHandler.js line 555 and following) for now since FF uses
> preprocessing to limit it to Windows and that's so frowned upon here. [And
> since I didn't port it I didn't test it.]
Odd, I thought we'd already implemented something to support ? search term for Process Explorer integration. (But maybe that works slightly differently.)
Comment 3 Jens Hatlak (:InvisibleSmiley) 2011-04-10 00:02:34 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > I'm not sure about bsmedberg's comment, maybe you can enlighten me.
> The -url parameter respects tabbed browsing preferences, but -search always
> opens a new window.

Heh, I guess I wasn't clear enough. That part I understood, it was the last bit that I didn't get: "use handURIToExistingBrowser ... but need nsIBrowserDOMWindow extensions". In other words, if handURIToExistingBrowser does the right thing, what exactly is the reason it wasn't used?
Comment 4 neil@parkwaycc.co.uk 2011-04-10 06:54:59 PDT
(In reply to comment #3)
> In other words, if handURIToExistingBrowser
> does the right thing, what exactly is the reason it wasn't used?
I looked more closely and it doesn't support post data. (nsIBrowserDOMWindow doesn't support post data either, but then again the internal caller only ever asks for about:blank anyway and just loads what it wants in the new tab.)
Comment 5 neil@parkwaycc.co.uk 2011-04-10 07:03:27 PDT
Comment on attachment 524877 [details] [diff] [review]
patch

>+  // fill our nsISupportsArray with uri-as-wstring, null, null, postData
>+  var sa = Components.classes["@mozilla.org/supports-array;1"]
>+                     .createInstance(Components.interfaces.nsISupportsArray);
nsISupportsArray is deprecated. Please use nsIArray.

>+  var wuri = Components.classes["@mozilla.org/supports-string;1"]
>+                       .createInstance(Components.interfaces.nsISupportsString);
>+  wuri.data = submission.uri.spec;
openWindow (not to be confused with wwatch.openWindow) calls this argstring.
So maybe this should be called uristring?

>     if (cmdLine.handleFlag("preferences", false)) {
>       openPreferences();
>       cmdLine.preventDefault = true;
>     }
> 
>     if (cmdLine.handleFlag("silent", false))
>       cmdLine.preventDefault = true;
> 
>+    var searchParam = cmdLine.handleFlagWithParam("search", false);
>+    if (searchParam) {
>+      doSearch(searchParam, features);
>+      cmdLine.preventDefault = true;
>+    }
Nit: I think I'd prefer search before preferences (and in helpinfo obviously).
Comment 6 Jens Hatlak (:InvisibleSmiley) 2011-04-10 13:53:33 PDT
Created attachment 524990 [details] [diff] [review]
patch v2 [Checkin: comment 8]

(In reply to comment #5)
> nsISupportsArray is deprecated. Please use nsIArray.

ITYM nsIMutableArray. Some notes here:
0. I guess you assumed I knew. Someone less knowledgeable might have given up after searching around endlessly (the code in this phase isn't exactly easy to debug).
1. Your statement looks like copied from MDC, which isn't exactly precise either. Or rather *was*.
2. I have no idea whether the second parameter could be "true" as well. This back-end low-level stuff scares me. Bad enough that MDC expects everyone to know. I'm just glad I somehow made it through.
Comment 7 neil@parkwaycc.co.uk 2011-04-10 15:58:38 PDT
(In reply to comment #6)
> 0. I guess you assumed I knew. Someone less knowledgeable might have given up
> after searching around endlessly (the code in this phase isn't exactly easy to
> debug).
Yes, sorry I just think of nsIArray first. Besides, MDC normally seems to mention them both in the same breath.

> 1. Your statement looks like copied from MDC, which isn't exactly precise
> either.
Probably the XPCOM Array Guide.

> 2. I have no idea whether the second parameter could be "true" as well. This
> back-end low-level stuff scares me. Bad enough that MDC expects everyone to
> know. I'm just glad I somehow made it through.
The page on nsIMutableArray seems to be clear enough on this point.
Comment 8 Jens Hatlak (:InvisibleSmiley) 2011-04-11 09:42:37 PDT
Comment on attachment 524990 [details] [diff] [review]
patch v2 [Checkin: comment 8]

http://hg.mozilla.org/comm-central/rev/fca529126184

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