Closed Bug 750911 Opened 8 years ago Closed 8 years ago

Review and land metro specific toolkit/xre changes

Categories

(Toolkit :: Startup and Profile System, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(2 files, 5 obsolete files)

No description provided.
Summary: Review and land Fx metro specific XRE changes → Review and land metro specific toolkit/xre changes
Attached patch xre win env setting v.1 (obsolete) — Splinter Review
Attached patch metro app runner v.1 (obsolete) — Splinter Review
Duplicate of this bug: 745820
Attached patch xre win env setting v.1 (obsolete) — Splinter Review
adds an xre entry point for setting or querying the current windows environment xul lib is using.
Attachment #622706 - Attachment is obsolete: true
Attachment #622724 - Flags: review?(benjamin)
Comment on attachment 622710 [details] [diff] [review]
metro app runner v.1

metro xre_main entry points currently in use on elm.
Attachment #622710 - Flags: review?(netzen)
Comment on attachment 622710 [details] [diff] [review]
metro app runner v.1

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

::: toolkit/xre/nsAppRunner.cpp
@@ +3940,5 @@
> +{
> +  nsresult rv;
> +
> +  bool exit = false;
> +  if (xreMainPtr->XRE_mainStartup(&exit) != 0 || !exit)

Shouldn't this be  || exit?
Attachment #622710 - Flags: review?(netzen)
Attachment #622710 - Attachment is obsolete: true
Attached patch metro app runner v.2 (obsolete) — Splinter Review
fix for bad exit flag check.
Attachment #622744 - Flags: review?(netzen)
Attachment #622744 - Flags: review?(netzen) → review+
Attached patch xre win env setting v.1 (obsolete) — Splinter Review
minor update - make sWindowsEnvironmentType static.
Attachment #622724 - Attachment is obsolete: true
Attachment #622724 - Flags: review?(benjamin)
Attachment #624027 - Flags: review?(benjamin)
Comment on attachment 624027 [details] [diff] [review]
xre win env setting v.1

Who calls this API? Why wouldn't we just want this to be a flag to XRE_main, so that there isn't a chance of resetting it to the wrong value?
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Comment on attachment 624027 [details] [diff] [review]
> xre win env setting v.1
> 
> Who calls this API? Why wouldn't we just want this to be a flag to XRE_main,
> so that there isn't a chance of resetting it to the wrong value?

The metro browserapp currently calls it. Widget also calls the get function to check the value. 

By flag to XRE_main, do you mean a command line flag? I'm fine with that, although I like the api better. :) Anybody who goes against the comments and tries to reset it mid-run will soon find out that does not work. I'd also like to keep the (mostly) internal get function rather than exposing a global variable.
If the big issue is post main changes to the value, I can put together a patch that adds a flag that's set after main is called. We could then assert if someone tries to change the value after things get going.
No, not a commandline flag, a flag parameter, e.g.

#define XRE_FLAG_USE_METRO 0x1

143 XRE_API(int,
144         XRE_main, (int argc, char* argv[], const nsXREAppData* sAppData, PRUint32 flags))

Then you can have a getter if other code needs it, but you don't need a setter. I just don't think that a setter function makes any sense in context.
Comment on attachment 622744 [details] [diff] [review]
metro app runner v.2

Moving this patch to bug 750901 since it depends on XRE_MetroCoreApplicationRun().
Attachment #622744 - Attachment is obsolete: true
Assignee: nobody → jmathies
Attachment #624027 - Attachment is obsolete: true
Attachment #624027 - Flags: review?(benjamin)
Attachment #626816 - Flags: review?(benjamin)
Attached patch cc changes v.1Splinter Review
Attachment #626816 - Flags: review?(benjamin) → review+
Attachment #626817 - Flags: review?(bugspam.Callek)
Attachment #626817 - Flags: review?(bugspam.Callek) → review+
https://hg.mozilla.org/mozilla-central/rev/0d1dcdf58ffb
https://hg.mozilla.org/comm-central/rev/8f91e6d380a6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.