Closed Bug 283834 Opened 20 years ago Closed 18 years ago

nsIXRemoteService code removed, resulting in _MOZILLA_* winprops not being created

Categories

(Core Graveyard :: X-remote, defect)

Product:
Core Graveyard
Component:
X-remote
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: lists-mozbugs, Assigned: blizzard)

Details

(Keywords: regression)

Attachments

(1 file)

restore browser window registration with XRemoteService
1.34 KB, patch
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050221 Firefox/1.0 (Ubuntu) (Ubuntu package 1.0+dfsg.1-6ubuntu1)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050221 Firefox/1.0 (Ubuntu) (Ubuntu package 1.0+dfsg.1-6ubuntu1)

revision 1.357 of mozilla/browser/base/content/browser.js removed "#ifdef
MOZ_ENABLE_XREMOTE" sections. As a result, the _MOZILLA_* winprops are not
created any more, and in certain circumstances (that happen to occur in my
environment) gnome-moz-remote needs those (in particular, _MOZILLA_VERSION) in
order to find the mozilla window.

Adding these lines back in (although i have to leave out the #ifdef's) gets
things working again. The important bit is
"remoteService.addBrowserInstance(window);", which is the call that results in
the winprops being added.

Reproducible: Always

Steps to Reproduce:
1. Run firefox from any point after the mentioned revision
2. Look at the list of window properties on the opened window

Actual Results:  
No _MOZILLA_* winprops were present

Expected Results:  
_MOZILLA_PROGRAM, _MOZILLA_USER and _MOZILLA_VERSION winprops should be there.
Assignee: firefox → blizzard
Component: General → X-remote
Product: Firefox → Core
QA Contact: general → blizzard
Version: unspecified → Trunk
Note a core issue... Over to firefox, nominating for blocking, since this breaks
integration with GNOME.  Dan, I believe this is you, right?  You certainly have
blame for removing these lines in rev 1.296.2.3.2.119	of browser.js on the
aviary branch.  ccing the reviewers for that checkin too.
Assignee: blizzard → bugs
Status: UNCONFIRMED → NEW
Component: X-remote → OS Integration
Ever confirmed: true
Flags: blocking-aviary1.1?
Keywords: aviary-landing, regression
Product: Core → Firefox
QA Contact: blizzard → os-integration
The xremote code sets up props on a special xremote server window. Why isn't
that window being used? I was hoping to remove the "addBrowserInstance()" method
altogether, in an upcoming xremote/command-line patch. Why do we need to fire
xremote commands at a particular window instead of the "server window"?
(In reply to comment #3)
> The xremote code sets up props on a special xremote server window. Why isn't
> that window being used? I was hoping to remove the "addBrowserInstance()" method
> altogether, in an upcoming xremote/command-line patch. Why do we need to fire
> xremote commands at a particular window instead of the "server window"?

On my system, whatever way firefox itself uses to try and find an existing
instance doesn't work properly:

jmb@artemis:~ $ firefox --remote "openurl(http://google.com)"
Error: No running window found

gnome-moz-remote used to work with 0.9.x, and does again with this code restored
into 1.0. i should point out that i'm not running gnome itself, just
gnome-moz-remote.
Component: OS Integration → Build Config
Version: Trunk → 1.0 Branch
Whoops. accidentally changed the component/version fields. reverting
Component: Build Config → OS Integration
Version: 1.0 Branch → Trunk
firefox -a firefox =remote "openurl(http://google.com)" should work, old-style
remote syntax doesn't quite work because we're no longer running as a single
process, so we need to specify which app we're looking for, fwiw.

Note that ./firefox "http://google.com" should work just fine for this case,
with the startup script improvements that landed post-0.9.x.
(In reply to comment #6)
> firefox -a firefox =remote "openurl(http://google.com)" should work

is that supposed to be an =, or should it be a - ?

either way, 

jmb@artemis:~ $ firefox -a firefox =remote "openurl(http://google.com)"
Usage: /usr/lib/mozilla-firefox/firefox-bin [ options ... ] [URL]

jmb@artemis:~ $ firefox -a firefox -remote "openurl(http://google.com)"
Error: No running window found


> Note that ./firefox "http://google.com" should work just fine for this case

how would the new-window option be specified to that (which is what i normally
use anyway)?

doesn't work anyway though. just brings up the profile manager.
In theory, new-window is handled by a user pref in the app in that case.  And it
should be a -, yes.
Could be that I removed too much when I rewrote the way external URLs were
handled for bug 172962. I'm surprised to hear in comment 4 that it's not
working; it seemed to be in testing at the time. I'm not going to be much help
at the moment, all unixless as I am.
(In reply to comment #9)

Could be that it's never been tested with non-conventional window managers. The
built-in -remote stuff never worked for me even back with 0.8/0.9.
What is unusual about your window manager? And why would the window manager make
any difference here at all?
Severity: normal → minor
(In reply to comment #11)
> What is unusual about your window manager? And why would the window manager make
> any difference here at all?

I use ion, which is a tabbed tiling window manager (although float workspaces
/are/ available if really needed; i only use them for gimp). it's much nicer
than anything else out their in my opinion.

As to why, i was going by this: http://modeemi.cs.tut.fi/~tuomov/ion/faq.html
(entry 3.4)
This might also be useful to read:
http://www.mail-archive.com/ion@list.rt.fm/msg01199.html
Severity: minor → normal
So summary, based on those documents:

1)  Our internal remote impl makes invalid assumptions about what the
    windowmanager is doing.
2)  People wrote tools to work around that, using 
3)  We went ahead and broke those tools....
(In reply to comment #13)
> So summary, based on those documents:
> 
> 1)  Our internal remote impl makes invalid assumptions about what the
>     windowmanager is doing.
> 2)  People wrote tools to work around that, using 
> 3)  We went ahead and broke those tools....

Sounds about right, yep :)
> 2)  People wrote tools to work around that, using 

I meant "using the window properties our XRemote service sets on windows".
Can we just fix the way xremote client looks up windows?

http://lxr.mozilla.org/mozilla/source/widget/src/xremoteclient/XRemoteClient.cpp#402

Uses XQueryTree, which is apparently wrong for this window manager. What would
the correct code look like? We want to check on all toplevel applications windows.

Patches welcome for the window-lookup issue; I do not want to futz with
re-adding addBrowserInstance().
Severity: normal → minor
Then we should clearly relnote that all methods of doing remoting except for our
built-in xremote are not supported and that we have broken them all.
OK. "The only official/supported methods of mozilla remoting are the mozilla
command line and xremote client built from the tree. If those are broken, please
help fix them, instead of introducing external remoting architectures."

This seems like a silly discussion to me. Let's just fix it and move on with our
lives.
Fine.  Over to xremote core, since we've decided to blame them for firefox UI
changes.  Re-raising severity; this is a major issue for users who have to deal
with this "we'll just break your being able to use the browser" crap.
Assignee: bugs → blizzard
Severity: minor → major
Component: OS Integration → X-remote
Product: Firefox → Core
QA Contact: os-integration → blizzard
There's no reason to break backwards compatibility.  There are lots and lots of
client implementations out there.  We can't update all of them.  We need to fix
this in our server code (assuming it's broken and not just our busted client)
not make everyone use our client "interface" on the command line.
I think we should work to ensure that our command line flags provide what all
consumers need.  That said, I agree that we cannot afford to make a backwards
incompatible change to the XRemote protocol.
The problem here is that this client depends on the window properties our
xremote service sets.  Since windows are no longer being passed to the xremote
service, the service can no longer set said properties...
Then someone needs to replace that call that allowed the properties to be set on
the window.
Yep, that's what I suggested too (see comment 2).  That suggestion was received
with some hostility (comment 3, comment 16, comment 18).
Yes, what I'm saying is I don't want xremote properties on arbitrary displayed
windows, just the hidden server window. Otherwise xulrunner is going to be stuck
with all kinds of unpleasant browser-specific code.
Frankly, I think any window added to windowwatcher should have the XRemote
properties set on it accordingly...
How about we fix the regression by restoring this part which it would seem I
removed erroneously, then file a separate bug for fixing whatever is suboptimal
about it?
(In reply to comment #27)
> Created an attachment (id=175965) [edit]
> restore browser window registration with XRemoteService
> 
> How about we fix the regression by restoring this part which it would seem I
> removed erroneously, then file a separate bug for fixing whatever is suboptimal
> about it?

Verified that this is enough to get things working for me again.
Flags: blocking-aviary1.1? → blocking-aviary1.1+
Flags: blocking-aviary1.1+ → blocking-aviary1.1-
Flags: blocking1.9?
We've release two milestones like this, and I really don't think we want to go this route anyway. WONTFIX.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → WONTFIX
So have we fixed the original issue with our xremote not working for the reporter?  Or filed a bug on it?

Chris, got any folks interested in actually making xremote work well?  ;)
Flags: blocking1.9?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: