129411 - [BEOS]Need implementation of ArgvReceived and RefsReceived

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P4)

Description

[Paul]

We need to implement BApplication::ArgvReceived and BApplication::RefsReceived

ArgvReceived is called when mozilla called from the command line, with
arguments.  Since mozilla is set to B_SINGLE_LAUNCH, this can be used to
capture command line arguments if mozilla is already running (they are passed
to the running app)

RefsReceived is called when the user drops a file (or files) on your app's
icon, or double clicks a file that's handled by your app. The message can arrive
either at launch time, or while your app is already running.

(the second cannot be implemented, without Bug#128565)

Comment 1

[Paul]

Attachment: initial crack at ArgvReceived (doesn't work)

I was trying to get ArgvReceived implemented first, thinking it might be
easier.  I started (the commented out part) by looking at code in the windows
native app support, but it uses the embedding api, which isn't implemented
(fully) under BeOS.  I then tried calling one of the static functions in
nsAppRunner, but new windows were never openned.

Any ideas on what I can do to get this working, short of implementing the
embedding api?	I have a feeling I'm just over looking something, as
nsAppRunner is pretty ugly.

Comment 2

[Paul]

Attachment: cleaned up a bit, still not working

Ok, when I try to open the window, I get a couple of failed assertions.  Each
complaining that the nsIDOMElement does not exist.  I have a feeling it is
because a BApplication, which is where I am trying to run from, is in a
different thread, not controlled by mozilla.  If that is the case, I'm
wondering, how would I accomplish this?

Comment 3

[Sergei Dolgov]

Paul, i tried to do some experiments recently. Made code in nsWindow.cpp
in nsWindowBeOS::MessageReceived()
case for received references in our case is B_SIMPLE_DATA, not B_REFS_RECIEVED.
It catches drop on Mozilla screen perfectly, i prited out dropped file pathname,
but then we should now CallMethod() to redirect it somewhere. I tried to put
(Bmessage *)msg in info->argp[], bu had problems. Maybe i'll try to parse msg
immediatelly in MessageReceived and put entry_ref pointers in info->args[].
Though, still don't know what to do with those refs. Maybe any method from
nsFilePicker will be helpful.

Comment 4

[Paul]

Yes, I was looking at dropping html files/bookmarks on mozilla when I was doing the 
dnd code.  Still, ArgvReceived and RefsReceived NEED to be implemented in the BApplication
object.  Only one instance of the mozilla binary should ever be running at one time, so
that information in the settings directory does not become corrupted.  My initial comment
explains what these methods are for.  So, if we solve the problem here, we don't need to 
handle in the widget folder.

Comment 5

[timeless]

well um, in theory i should be allowed to run two mozillas with two different 
profiles, at least on most platforms.  If BeOS doesn't want to let me do that, 
i'll live...

anyway i'm very behind in bugmail i'll boot beos in a few days and look around 
stuff.

Comment 6

[Paul]

Changed to just XP apps, as this is Command Line AND Drag and Drop features.

Also, yes, there is the profile manager.  But, how would you suggest preventing
a user from running mozilla twice for the same profile?  How does Windows do it?
 Especially with the quick-launch feature?  *nix builds use the script, but,
that is not a perfect solution either.
To access the profile manager under BeOS, and most platforms IIRC, you must
specify proper command line options.  Also, the executable can be changed to
multi-launch from the filetypes option.  But, if you are not using different
profiles, I would think it would be safer to keep the app as single launch, to
help prevent corruption of the user's profile.

Either way, the RefsReceived need to be implemented in order to have Mozilla
open an HTML document/bookmark when double clicked in tracker.

Comment 7

[timeless]

the profilemanager startup stuff handles profile locking. don't worry about it 
:).

Comment 8

[Paul]

it does? even across multiple instances of the application?  i don't think so.

looking through the windows code, they won't open the profile manager if a window
is currently open.  To me, this means they don't want to profiles running at the same
time.  on the other hand, i'm not a windows programmer, so, i'm guessing as to what
that code is doing, but I think the DDE request would be from when someone is trying
to run mozilla a second time.

or, maybe I misunderstood you comment, and you were saying just don't worry about 
running mulitple profiles at once :)

Comment 9

[timeless]

there's new profile locking code which is much easier to see on linux, but on 
windows there's an env var to ignore the dde stuff. you can certainly run 
multiple mozillas on windows, i do.

Comment 10

[Sergei Dolgov]

btw, timeless,
i got crashreports from users who use multiple launch:
   frame         retaddr
fd001840   ecca6104  nsAppShell::Run(void) + 00000064
fd001868   ecc77ad0  nsAppShellService::Run(void) + 00000024
fd001878   80008bf6  nsCreateInstanceByContractID::operator()(nsID const &, void
**) const + 00008bf6
fd001938   80007f6d  nsCreateInstanceByContractID::operator()(nsID const &, void
**) const + 00007f6d
fd001964   80007ce5  nsCreateInstanceByContractID::operator()(nsID const &, void
**) const + 00007ce5  

This is especially frequent if you are closing one instance when launching other.

Comment 11

[Paul]

ok, so even if we allow multi-launch, by adding the "hack" that linux uses or
something else (though I really don't see any reason to have mulitple profiles
open at the same time), at least RefsReceived still needs to be implemented.

Has anyone even tried the patch?  The problem is that calling the
WindowWatcher's create window methods from within the BApplication object is NOT
creating a "full" browser window.  A window gets created, but without "guts",
i.e., nothing drawn in it.  

Comment 12

[Sergei Dolgov]

btw, bug 152156 depends, IMHO, on this bug.
It means, that file menu shouldn't wait for BFilePanel closing, but, instead,
launch this panel, release all, and then b_app or BWindow RefsReceived
(generated when file is choosen in File Panel) should call all these file open
procedures.

Comment 13

[Paul]

Setting the priority, so I can better organize the issues in bugzilla I need to
take care of

Comment 14

[tqh]

Attachment: working nsNativeAppSupportBeOS impl

Here is a working one. It implements nsNativeAppSupportBeOS so that it handles
args / refs received and quit requests.

Comment 15

[tqh]

Taking over bug.

Comment 16

[tqh]

Planning to add basic native app support first. See bug 281375.

Comment 17

[tqh]

Internal relaunching is a problem for doing argv properly under BeOS, so we're
dependent on bug 271613 to do it right. We want to avoid multiple launches of
Firefox (setting the 'SINGLE_LAUNCH' flag on the BeOS-app), which also makes it
possible for Firefox to get messages about double-clicked HTML-files.
Unfortunatly internal relaunching can't be done with that flag.

I've been waiting to see if I was gonna do it another way (basically duplicating
how BeOS does it), but now I hopefully don't need to.

Comment 18

[Sergei Dolgov]

Ok, now relaunching with single flag might be possible.
I investigated situation with patch https://bugzilla.mozilla.org/attachment.cgi?id=222725
applied.
Running firefox from one terminal, 
typing 
forefox YourURLHere.
SingleLaunch flag.

According to printouts in first terminal in XRE_main, it even don't try  to relaunch, at least LaunchChild isn't called.

But adding ArgsReceived in nsNativeAppSupportBeOS::BApplication clearly shows, that instance launched in first terminal gets arguments each time I'm trying to call FF in second.

In second terminal at same time it shows, that XRE_main is called with proper arguments, but immediately exits.

So, maybe I don't understand things well, but situation looks quite promising for attempt to implement our own "CmdLineHandler" for nsNativeAppSupportBeOS.

Comment 19

[tqh]

Be aware that the internal restarting is ONLY done when settings, profile or Firefox version is changed. So you should really delete or rename your current profile to get the LaunchChild() function to be triggered. Otherwise you are only testing under perfect conditions, not the worst.

Comment 20

[Sergei Dolgov]

Per comment 19:
I always testing for restart isses with deleted profile first.
All that process is very well visible with printf() statements in proper places.

Comment 21

[Sergei Dolgov]

Attachment: patch

Fully working implementation for toolkit-apps (FF etc).
Only issue is opening two windows when launching from file-click or "OpenWith".
After month of wasted time i found minimalistic solution for that.

Maybe in future we can resolve also this minor issue.
Main but very minimalistic changes are in toolkit/xre/nsNativeAppSupportBeOS.
Also there is little "temporary" change in widget's nsWindow, until we have native DnD support there.

Comment 22

[tqh]

I can't see a reason why you implement observer?

Comment 23

[Sergei Dolgov]

per comment 22:

For possible future use instead ::Enable, as i noticed in code.
At the moment Enable is at proper place to fullfil our needs, but no guarantee for future.

With observer we could get notification about readiness of services and UI from atbitrary place which we think is proper.

Maybe it even allows to resolve double-window issue at startup.

At all, i think we underuse observers and NS object proxies at all in our port.
Those fits quite well in BeOS multithreaded model.

But if you really dislike that Observer, i'll remove it.

Comment 24

[Sergei Dolgov]

Comment on attachment 225897 [details] [diff] [review]
patch

Looks like I found solution for double-window at start from click, but I still have question tqh - if we should remove Observer?

Comment 25

[tqh]

If you know that it will be used we can keep it, but otherwise it's just code bloat and a bit confusing for those who don't know why it's there.

Comment 26

[Sergei Dolgov]

Attachment: patch

r=?

(looks like fix for "two window" was occasion or illusion. So I'm over.
Submitting usable code to let some enthousiast to work on it.
Difference from previous version - Observer removed, two ifdef-ed functions added - GetMostRecentWindow() and ActivateWindow(). See comment in HandleCommandLine() function)

Comment 27

[tqh]

Shouldn't you move the if (!IsLaunching) out of for loop, and maybe do as ArgvReceived and return (fail early)? It's a bit unnecessary to process things if we arn't going to do anything.

Comment 28

[Sergei Dolgov]

agreed.
maybe I will do:
if (!IsLaunching())
 for().

But I need to figure out what and where to perform new and delete for mArgv.
Seems unclear for me now:(

Do you have other notices?

Comment 29

[Sergei Dolgov]

Just recalled that there might be multiple files clicked at once at start or "Opened With"!

So replaced stored args with stored BMessage, as I did for SeaMonkey once.

Works ok with multiple files at start.

Submitting new patch.

Comment 30

[Sergei Dolgov]

Attachment: patch

patch with stored BMessage

Comment 31

[tqh]

This looks quite nice, unless you suspect several messages may be received while IsLaunching.

Comment 32

[Sergei Dolgov]

As we do it for RefsReceived only, i doubt someone acts so fast that can trigger several such events inside some microseconds.

That trick targets only situation - refs submitted by tracker at start.
This is only know (and handled by Be API) message before real be_app start.

But if you mean another thing, messages which user can trigger on in addition to startup links before Enable()
 was called, it means, before any mozilla window appeared, I don't know what to do with such extremism:(

Comment 33

[tqh]

That's my thoughts as well, just wanted to check.

Comment 34

[tqh]

Comment on attachment 225927 [details] [diff] [review]
patch

r=[e-mail]

Comment 35

[Sergei Dolgov]

Comment on attachment 225927 [details] [diff] [review]
patch

rubberstamp request for BeOS-only code in xre/nsNativeAppSupportBeOS.cpp

Comment 36

[Sergei Dolgov]

per comment 31: and comment 32:

You were right. We can create such situation. Select multiple html files in Tracker and pus ENTER - and it will generate sequence of RefsReceived instead one with multiple refs. While OpenWith for multiple files generates one message. At least my Tracker does it that way.

And it appeared to be safe and even working properly with current FF/toolkit patch - it opens all files in multiple windows.

But when i tried to do same in SeaMonkey/xpfe - it crashed at second RefsReceived. So I implemented there more sofisticated logic - introduced bool appEnabled in BeApp which is false at startup and goes true with similar to FF/toolit implementation message (only suitable place to generate such message appeared to be HideSplashScreen - tried several places about 6 hours!

until appEnabled != true I collect all RefsReceived messages into nsVoidArray.
and when appEnabled turns to be true, I ivoking whole bunch if stored messages in for() loop. Works nice - will submit patch soon.

P.S. Looks like some things in toolkit are done in more proper way:)

Comment 37

[Sergei Dolgov]

Attachment: patch

Final patch for SeaMonkey/xpfe.
Looks like all is working.
r=?

Comment 38

[Sergei Dolgov]

Comment on attachment 226031 [details] [diff] [review]
patch

r=?

Comment 39

[tqh]

I may be blind, but where is mMessage declared?

Comment 40

[tqh]

Comment on attachment 226031 [details] [diff] [review]
patch

r=[e-mail]

Comment 41

[Sergei Dolgov]

Checking in mozilla/xpfe/bootstrap/nsNativeAppSupportBeOS.cpp;
/cvsroot/mozilla/xpfe/bootstrap/nsNativeAppSupportBeOS.cpp,v  <--  nsNativeAppSupportBeOS.cpp
new revision: 1.14; previous revision: 1.13
done

Comment 42

[Sergei Dolgov]

Checking in mozilla/widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.126; previous revision: 1.125
done 

Comment 43

[Benjamin Smedberg]

Comment on attachment 225927 [details] [diff] [review]
patch

There are IMO too many #if 0 and commented out blocks in this patch. In particular in ::HandleCommandLine the section with NS_NewNativeLocalFile should be removed, since it's commented out. The #if 0 "try to use OpenURI" should also be removed. The GetMostRecentWindow() function should also be removed.

Comment 44

[Sergei Dolgov]

Checking in mozilla/toolkit/xre/nsNativeAppSupportBeOS.cpp;
/cvsroot/mozilla/toolkit/xre/nsNativeAppSupportBeOS.cpp,v  <--  nsNativeAppSupportBeOS.cpp
new revision: 1.3; previous revision: 1.2
done

Comment 45

[Sergei Dolgov]

Attachment: patch

NS_CreateSplashScreen exists already in nsApprunner.

Comment 46

[Sergei Dolgov]

Checking in mozilla/xpfe/bootstrap/nsNativeAppSupportBeOS.cpp;
/cvsroot/mozilla/xpfe/bootstrap/nsNativeAppSupportBeOS.cpp,v  <--  nsNativeAppSupportBeOS.cpp
new revision: 1.15; previous revision: 1.14
done  

Hope bustage is fixed with that.

Too bad that mmadia's hardware died:(

Comment 47

[Doug Shelton]

Should this also be fixed on the 1.8 branch for Firefox 2.0?

Comment 48

[Sergei Dolgov]

Would be nice for something called "release". But this depends on "proper restart" bug, so that should be landed first.

Comment 49

[Sergei Dolgov]

Comment on attachment 226031 [details] [diff] [review]
patch

asking approval for 1.8.1 branch. BeOS-only change

Comment 50

[Sergei Dolgov]

Comment on attachment 225927 [details] [diff] [review]
patch

asking approval for 1.8.1
BeOS-only change

Comment 51

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 225927 [details] [diff] [review]
patch

a=drivers

Comment 52

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 226031 [details] [diff] [review]
patch

a=drivers

Comment 53

[Sergei Dolgov]

Checking in mozilla/widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.91.4.21; previous revision: 1.91.4.20
done
Checking in mozilla/xpfe/bootstrap/nsNativeAppSupportBeOS.cpp;
/cvsroot/mozilla/xpfe/bootstrap/nsNativeAppSupportBeOS.cpp,v  <--  nsNativeAppSupportBeOS.cpp
new revision: 1.12.8.1; previous revision: 1.12
done

Comment 54

[Sergei Dolgov]

Checking in mozilla/toolkit/xre/nsNativeAppSupportBeOS.cpp;
/cvsroot/mozilla/toolkit/xre/nsNativeAppSupportBeOS.cpp,v  <--  nsNativeAppSupportBeOS.cpp
new revision: 1.1.8.2; previous revision: 1.1.8.1
done