750911 - Review and land metro specific toolkit/xre changes

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Comment 1

[Jim Mathies [:jimm]]

Attachment: xre win env setting v.1

Comment 2

[Jim Mathies [:jimm]]

Attachment: metro app runner v.1

Comment 4

[Jim Mathies [:jimm]]

Attachment: xre win env setting v.1

adds an xre entry point for setting or querying the current windows environment xul lib is using.

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 622710 [details] [diff] [review]
metro app runner v.1

metro xre_main entry points currently in use on elm.

Comment 6

[Brian R. Bondy [:bbondy]]

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?

Comment 7

[Jim Mathies [:jimm]]

Attachment: metro app runner v.2

fix for bad exit flag check.

Comment 8

[Jim Mathies [:jimm]]

Attachment: xre win env setting v.1

minor update - make sWindowsEnvironmentType static.

Comment 9

[Benjamin Smedberg]

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?

Comment 10

[Jim Mathies [:jimm]]

(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.

Comment 11

[Jim Mathies [:jimm]]

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.

Comment 12

[Benjamin Smedberg]

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 13

[Jim Mathies [:jimm]]

Comment on attachment 622744 [details] [diff] [review]
metro app runner v.2

Moving this patch to bug 750901 since it depends on XRE_MetroCoreApplicationRun().

Comment 14

[Jim Mathies [:jimm]]

Attachment: xre win env setting v.2

Comment 15

[Jim Mathies [:jimm]]

Attachment: cc changes v.1

Comment 16

[Jim Mathies [:jimm]]

https://hg.mozilla.org/mozilla-central/rev/0d1dcdf58ffb
https://hg.mozilla.org/comm-central/rev/8f91e6d380a6