Break XRE_main up into parts that can be called individually

RESOLVED FIXED in mozilla14

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
mozilla14
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Assignee

Description

7 years ago
Win8 metro's app model is a little different from conventional desktop apps - the thread that enters XRE_main is not the thread the UI is rendered on. We need to be able to shunt off the main thread early and drop it into a main metro entry point, then call back using a ui worker thread to finish up threading & xpcom initialization. This patch makes that possible.
Assignee

Comment 1

7 years ago
Posted patch patch v.1 (obsolete) — Splinter Review
Assignee: nobody → jmathies
There's no need to prefix the XREMain methods with XRE_, is there?
Assignee

Comment 3

7 years ago
(In reply to Ms2ger from comment #2)
> There's no need to prefix the XREMain methods with XRE_, is there?

Not really, but it does make searching for these methods easier. XRE_main was a really big function.
OS: Windows 7 → Windows 8 Metro
Assignee

Comment 4

7 years ago
Posted patch patch v.2 (obsolete) — Splinter Review
- Catch some missed early exits for command line dump commands.
Attachment #607086 - Attachment is obsolete: true
Assignee

Comment 5

7 years ago
Posted patch patch v.3 (obsolete) — Splinter Review
- fix up NS_TIME_FUNCTION diagnostic code so that it builds.
Attachment #607089 - Attachment is obsolete: true
Assignee

Comment 6

7 years ago
Posted patch patch v.4 (obsolete) — Splinter Review
- linux / mac bustage fixes.

I ran this through try a few times, didn't see any issues.
Attachment #607097 - Attachment is obsolete: true
Assignee

Comment 7

7 years ago
Comment on attachment 607735 [details] [diff] [review]
patch v.4

Dave, since this is a pretty big change in the xre code I figured I'd ask you for r?. Note the patch looks pretty big but really 90% of this is just code reformatting. Comment 1 explains a bit about why these changes were needed. 

Generally what I've done is break up XRE_main into a set of methods in a simple class names XREMain. In win8 metro, we'll need to to call the init parts and then call back so that a different thread is considered the main thread. Here's that code if your curious:

https://hg.mozilla.org/projects/elm/rev/ff3c7db1fce0
Attachment #607735 - Flags: review?(dtownsend+bugmail)
Assignee

Comment 8

7 years ago
Posted patch patch with -w (obsolete) — Splinter Review
Comment on attachment 607735 [details] [diff] [review]
patch v.4

Review of attachment 607735 [details] [diff] [review]:
-----------------------------------------------------------------

I want to look over this a second time to make sure I didn't miss anything but otherwise this is looking pretty good.

::: toolkit/xre/nsAppRunner.cpp
@@ +3097,5 @@
>  
>    // Handle -no-remote and -new-instance command line arguments. Setup
>    // the environment to better accommodate other components and various
>    // restart scenarios.
> +  if (CheckArg("no-remote", true) == ARG_BAD) {

You need to still store this in ar for the else-if below

@@ +3104,5 @@
>    } else if (ar == ARG_FOUND) {
>      SaveToEnv("MOZ_NO_REMOTE=1");
>    }
> +
> +  if (CheckArg("new-instance", true) == ARG_BAD) {

Ditto

@@ +3178,5 @@
> +  bool canRun = false;
> +  rv = mNativeApp->Start(&canRun);
> +  if (NS_FAILED(rv) || !canRun) {
> +    return 1;
> +  }

Why move this stuff earlier?

@@ +3180,5 @@
> +  if (NS_FAILED(rv) || !canRun) {
> +    return 1;
> +  }
> +
> +  rv = mDirProvider.Initialize(mAppData->directory, mAppData->xreDirectory);

This seems to happen twice now
Attachment #607735 - Flags: review?(dtownsend+bugmail) → review-
Assignee

Updated

7 years ago
Blocks: 737975
Assignee

Comment 10

7 years ago
Posted patch patch v.5 -w (obsolete) — Splinter Review
> I want to look over this a second time to make sure I didn't miss anything
> but otherwise this is looking pretty good.

Ok, posting a new rev now.

> 
> ::: toolkit/xre/nsAppRunner.cpp
> @@ +3097,5 @@
> >  
> >    // Handle -no-remote and -new-instance command line arguments. Setup
> >    // the environment to better accommodate other components and various
> >    // restart scenarios.
> > +  if (CheckArg("no-remote", true) == ARG_BAD) {
> 
> You need to still store this in ar for the else-if below
> 
> @@ +3104,5 @@
> >    } else if (ar == ARG_FOUND) {
> >      SaveToEnv("MOZ_NO_REMOTE=1");
> >    }
> > +
> > +  if (CheckArg("new-instance", true) == ARG_BAD) {
> 
> Ditto

fixed.

> 
> @@ +3178,5 @@
> > +  bool canRun = false;
> > +  rv = mNativeApp->Start(&canRun);
> > +  if (NS_FAILED(rv) || !canRun) {
> > +    return 1;
> > +  }
> 
> Why move this stuff earlier?

Debugging mistake on my part, was experimenting with this in mainInit vs. mainStartup and didn't put it back in the right place. :/ Updated.

> 
> @@ +3180,5 @@
> > +  if (NS_FAILED(rv) || !canRun) {
> > +    return 1;
> > +  }
> > +
> > +  rv = mDirProvider.Initialize(mAppData->directory, mAppData->xreDirectory);
> 
> This seems to happen twice now

Removed the incorrectly placed chuck of code.

I'll fire off a new set of try builds as well with these changes.
Attachment #607735 - Attachment is obsolete: true
Attachment #608020 - Attachment is obsolete: true
Attachment #608060 - Flags: review?(dtownsend+bugmail)
Assignee

Comment 11

7 years ago
Posted patch patch v.5 -wSplinter Review
sorry, missed the -w diff option.
Attachment #608060 - Attachment is obsolete: true
Attachment #608060 - Flags: review?(dtownsend+bugmail)
Attachment #608061 - Flags: review?(dtownsend+bugmail)
Assignee

Comment 12

7 years ago
(In reply to Jim Mathies [:jimm] from comment #11)
> Created attachment 608061 [details] [diff] [review]
> patch v.5 -w
> 
> sorry, missed the -w diff option.

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=e1005c7e270e

These changes passed try.
Attachment #608061 - Flags: review?(dtownsend+bugmail) → review+
https://hg.mozilla.org/mozilla-central/rev/0089fd403e76
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Assignee

Updated

7 years ago
No longer blocks: 737975
Assignee

Updated

7 years ago
Blocks: elm-merge
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.