XULRunner stub doesn't properly pass '-foreground' option, app restarts behind other applications

VERIFIED FIXED in mozilla1.9.1b3

Status

VERIFIED FIXED
12 years ago
3 years ago

People

(Reporter: bent.mozilla, Assigned: matt)

Tracking

({verified1.9.1})

Trunk
mozilla1.9.1b3
All
Mac OS X
verified1.9.1

Details

Attachments

(2 attachments, 1 obsolete attachment)

After an app-initiated restart the apppops up behind other applications. After digging around a little I figured out that it was because no one was passing the -foreground command line option to XRE_main.
This bug is filed against Mac OS X. Shouldn't this be all or is it really Mac OS X only? I ask because of your bug 399580 comment 1.
(Assignee)

Updated

11 years ago
Blocks: 357052
(Assignee)

Comment 2

11 years ago
Created attachment 287030 [details] [diff] [review]
Patch to add -foreground when restarting.

SetupMacCommandLine(gRestartArgc, gRestartArgv) is not called in the normal startup case, so -foreground is never added.  Even if it were called, LaunchChild resets gRestartArgc/v for app initiated restarts.

I think LaunchChildMac is the appropriate place to add the param, as it seems like the kind of thing you'd want to do immediately before restarting.

Thoughts?
Assignee: nobody → matt
Status: NEW → ASSIGNED
(Assignee)

Comment 3

11 years ago
Comment on attachment 287030 [details] [diff] [review]
Patch to add -foreground when restarting.

Hi Mark, do you mind reviewing this tiny patch?  I'm told you're the best person to ask.

Thanks.
Attachment #287030 - Flags: review?(mark)

Comment 4

10 years ago
Created attachment 345568 [details] [diff] [review]
mattc's patch updated for tip

This is just mattc's patch updated for tip... no functional code changes, it just needed to be updated since MacLaunchHelper.m moved to MacLaunchHelper.mm
Attachment #345568 - Flags: review?(benjamin)

Comment 5

10 years ago
Comment on attachment 287030 [details] [diff] [review]
Patch to add -foreground when restarting.

This is not xulrunner-specific code. I'd like rob to look at this.
Attachment #287030 - Flags: review?(mark) → review?(robert.bugzilla)

Updated

10 years ago
Attachment #345568 - Flags: review?(benjamin) → review?(robert.bugzilla)
Attachment #287030 - Flags: review?(robert.bugzilla)
Comment on attachment 345568 [details] [diff] [review]
mattc's patch updated for tip

I've had next to no exposure to this code. Since it is Mac specific could you take this review Dave?.
Attachment #345568 - Flags: review?(robert.bugzilla) → review?(dtownsend)
Attachment #345568 - Flags: review?(dtownsend) → review-
Comment on attachment 345568 [details] [diff] [review]
mattc's patch updated for tip

Unless I'm missing something I think we are better off calling SetupMacCommandLine after clearing the arguments for the restart. Otherwise we have -foreground handling in two parts of the code that ends up adding multiple -foreground flags.

Comment 8

10 years ago
Created attachment 348109 [details] [diff] [review]
Totally different patch to do what (I think) Mossop suggested

I think this implements what Mossop suggested.
Attachment #345568 - Attachment is obsolete: true
Attachment #348109 - Flags: review?(dtownsend)
Attachment #348109 - Flags: review?(dtownsend)
Attachment #348109 - Flags: review+
Attachment #348109 - Flags: approval1.9.1?
Comment on attachment 348109 [details] [diff] [review]
Totally different patch to do what (I think) Mossop suggested

Yep that works.

We should take this for 1.9.1, safe simple fix.
Comment on attachment 348109 [details] [diff] [review]
Totally different patch to do what (I think) Mossop suggested

a191=beltzner
Attachment #348109 - Flags: approval1.9.1? → approval1.9.1+
Should have landed this before the branch, I'll try to remember when the tree reopens
Keywords: checkin-needed
Landed in mozilla-central: http://hg.mozilla.org/mozilla-central/rev/5567bece7eed
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Dave, lets add the checkin-needed keyword meanwhile so it doesn't get lost.
Keywords: checkin-needed
Hardware: Macintosh → All
Version: unspecified → Trunk
(In reply to comment #13)
> Dave, lets add the checkin-needed keyword meanwhile so it doesn't get lost.

It needs to bake on trunk before we can look to land on the branch
Keywords: checkin-needed
Starting my todays debug build (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20081206 Minefield/3.2a1pre) shows following in the Error Console. Should this also be covered by this bug or is it another issue because it doesn't happen while restarting the browser?

Error: Warning: unrecognized command line flag -foreground

Source File: file:///Volumes/Daten/mozilla/source/obj/browser-i386-apple-darwin9.5.0/dist/MinefieldDebug.app/Contents/MacOS/components/nsBrowserContentHandler.js
Line: 713
(In reply to comment #15)
> Starting my todays debug build (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5;
> en-US; rv:1.9.2a1pre) Gecko/20081206 Minefield/3.2a1pre) shows following in the
> Error Console. Should this also be covered by this bug or is it another issue
> because it doesn't happen while restarting the browser?
> 
> Error: Warning: unrecognized command line flag -foreground
> 
> Source File:
> file:///Volumes/Daten/mozilla/source/obj/browser-i386-apple-darwin9.5.0/dist/MinefieldDebug.app/Contents/MacOS/components/nsBrowserContentHandler.js
> Line: 713

This is bug 369147
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090413 Minefield/3.6a1pre ID:20090413031052

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090413 Minefield/3.6a1pre ID:20090413031052
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
ack, here's the build ID for Shiretoko verification: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090413 Shiretoko/3.5b4pre ID:20090413031313
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.