Make the -remote option return an error code

RESOLVED FIXED in Firefox 39

Status

defect
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla39
All
Linux
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

4 years ago
No description provided.

Comment 1

4 years ago
I'm blissfully unaware of deprecation procedures within Firefox, but I would rather see a deprecation warning (in increasing intensity?) before we hit regression again with version 37.
So:
- deprecation warning on console (/log) in 37 (annoy downstream devs)
- deprecation popup in 38 (annoy users)
- error in 39
Assignee

Comment 2

4 years ago
Having -remote return an error code would be the equivalent of the current behavior of -remote when Firefox is not already running. If scripts or whatever currently using -remote are not handling this case, then they are already not working as users would expect.

Comment 3

4 years ago
Let me gently suggest again that there is now a decade's worth of code which uses this command line API.  Some of it is free/libre software -- some is not.  Recently a the sigvec system call was removed from GNU libc: decades after it had ceased to be the "proper" way of handling signals.  

Is this removal really necessary?  The command line is the API to instantiating browser pages for a body of code, some of which doesn't have source available.

The volume of code involved in the cleanup is incredibly tiny compared to the browser as a whole.  I understand the desire for clean interfaces, but one must weigh this against damage to a community which relies on the older unclean interfaces when the clean ones weren't available.  

If this were a relied upon kernel ABI, what do you think Linus would say?
Assignee

Comment 4

4 years ago
Assignee: nobody → mh+mozilla
Attachment #8575142 - Flags: review?(benjamin)

Comment 5

4 years ago
Again, as Don said, why is there this level of urgency in removing a command line option that has been used for many years in scripts?  The fact that there is another way to do everything that -remote does suggests that there isn't a lot of code or complication involved, and indeed, it wouldn't be hard to implement a wrapper script that interprets the -remote option and issues the underlying command with the appropriate arguments.

The fact that it wouldn't be hard to implement this kind of wrapper script doesn't mean that it wouldn't be an enormous nuisance for everyone who uses it (including end users who don't know that they do).

Please just leave this well-established command line API in place.

Comment 6

4 years ago
Comment on attachment 8575142 [details] [diff] [review]
Make the -remote option return an error code

Won't this affect both the initial-commandline and remoted-commandline processing? e.g. won't this trigger the codepath at http://hg.mozilla.org/mozilla-central/annotate/fd8e079d6335/toolkit/components/remote/nsXRemoteService.cpp#l298

Does this patch rely on the bug/feature that http://hg.mozilla.org/mozilla-central/annotate/fd8e079d6335/toolkit/xre/nsAppRunner.cpp#l1668 never actually checks the xremote response string?
Flags: needinfo?(mh+mozilla)
Assignee

Comment 7

4 years ago
As indicated by bug dependencies, this patch relies on bug 1141439 being fixed, so it's the opposite: it relies on checking the xremote response string.
Flags: needinfo?(mh+mozilla)

Comment 8

4 years ago
Won't this cause -remote usage to return a nonzero error code in every case, whether there is an existing process or not?

I think we need to document the expected return code behavior somewhere in the code.

Updated

4 years ago
Attachment #8575142 - Flags: review?(benjamin) → review-
Assignee

Comment 9

4 years ago
Note the comment was in the commit message, but maybe you missed it. It's better in the code anyways.
Attachment #8575142 - Attachment is obsolete: true
Attachment #8578359 - Flags: review?(benjamin)
Comment on attachment 8578359 [details] [diff] [review]
Make the -remote option return an error code

I still don't understand. Doesn't this have the effect of returning a nonzero exit code in *every* case, both when an instance was already running and when it was not?
Flags: needinfo?(mh+mozilla)
Assignee

Comment 11

4 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> Comment on attachment 8578359 [details] [diff] [review]
> Make the -remote option return an error code
> 
> I still don't understand. Doesn't this have the effect of returning a
> nonzero exit code in *every* case, both when an instance was already running
> and when it was not?

Yes, and that's the point I'm trying to make in the comment. It's probably not as clear as I'd hope.

So the thing is that if you're currently using -remote, you have to do something like this:
  if ! firefox -remote "openurl($url)"; then
    firefox $url
  fi
or
  if firefox -remote "ping()"; then
    firefox -remote "openurl($url)"
  else
    firefox $url
  fi

If you omit the firefox $url case, and firefox is not already running, your script does nothing.

In both cases, making -remote always fail make you go through the fallback for when firefox is not already running.
Flags: needinfo?(mh+mozilla)

Comment 12

4 years ago
Why is there such a pressing need to deprecate -remote?  It doesn't sound like there's an urgent need to remove the code, since there's already an equivalent way to do everything that -remote does; it feels like cleanup for the sake of cleanup.

Not all scripts and such do the ping().  Maybe that's incorrect, but it works whenever firefox is running, and a lot of people just leave it running all the time, so it works just fine in practice.  End users can't fix all of the changes necessarily; they may be baked into something that they can't change.  I have the workaround (for emacs) of using xdg-open, but that only exists with some desktop environments (it doesn't work for people using small, fast window managers who don't want a full-blown desktop environment).

Please reconsider this change.  It's going to break a number of users for no clearly stated gain.
Comment on attachment 8578359 [details] [diff] [review]
Make the -remote option return an error code

Oh, I missed that you didn't put back the nsBrowserContentHandler.js bits which actually handled "-remote" args. Is that just because that code is complex? I guess this is ok, although I'd also be fine with putting that stuff back. Then -remote work work in every case (even if Firefox wasn't previously running).
Attachment #8578359 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/c091c6c2861f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39

Comment 16

4 years ago
I wish someone would answer the very reasonable question being asked by Don, Robert, and others in #1080319, namely why on earth it is deemed necessary to remove the -remote option in the first place, given that doing so will break a whole plethora of third party software, and that keeping it implemented (albeit deprecated) seems to have near zero cost. 

Why do you insist on breaking backwards compatibility by removing a commandline switch which has existed for a very long time and which a lot of things depend on? 

By all means guide new software to use the newer commandline semantics by marking -remote as deprecated in the documentation and showing how to translate to the newer equivalents, but I really do not understand this zeal for getting rid of -remote within a short space of time. What exactly is the problem you're trying to solve here?

Updated

2 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.