Brian R. Bondy [:bbondy] Assignee
	Description • 11 years ago

For the shared profile work, we should not use the same startup cache.
Some chrome paths are re-used in Metro and Desktop even know they refer to different things.  Note: There is no problem with http cache since the user agents are the same and both browsers are indistinguishable to http servers.

I think the startup cache file, is where xul, css, script cache is stored:
http://dxr.mozilla.org/mozilla-central/source/startupcache/StartupCache.cpp#l89

I think the best way to do this is to add:
#ifdef MOX_METRO
static const char sMetroStartupCacheName[] = "metroStartupCache." SC_WORDSIZE "." SC_ENDIAN;
#endif

Then here:
http://dxr.mozilla.org/mozilla-central/source/startupcache/StartupCache.cpp#l204

You'd do this:

#ifdef MOX_METRO
if (IsRunningInWindowsMetro()) {
  rv = file->AppendNative(NS_LITERAL_CSTRING(sMetroStartupCacheName));
} else
#endif
{
  rv = file->AppendNative(NS_LITERAL_CSTRING(sStartupCacheName));
}

IsRunningInWindowsMetro is defined in (nsWindowsHelpers.h)


p=1

	Brian R. Bondy [:bbondy] Assignee
	Comment 2 • 11 years ago

If this gets feedack+ then I'll proceed to get a review for this small patch from bsmedberg.

	Brian R. Bondy [:bbondy] Assignee
	Comment 3 • 11 years ago

Comment on attachment 824241 [details] [diff] [review]
Patch 1 - Code to differentiate between metro front end and metro environment. rev1.

Missing a couple of forward declarations, will re-request later today.

	Jim Mathies [:jimm]
	Comment 4 • 11 years ago

Comment on attachment 824241 [details] [diff] [review]
Patch 1 - Code to differentiate between metro front end and metro environment. rev1.

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

::: toolkit/xre/nsEmbedFunctions.cpp
@@ +825,5 @@
> +                                     WindowsEnvironmentType_Desktop;
> +}
> +
> +void
> +XRE_SetIsUsingMetroFrontEnd(bool aIsMetroFrontEnd)

Don't you need to declare these in nsXULAppAPI.h? I don't see how XRE_SetIsUsingMetroFrontEnd is found in nsAppRunner.cpp.

	Jim Mathies [:jimm]
	Comment 7 • 11 years ago

Just out of curiosity, what breaks if we don't land this and someone runs metro in desktop mode using an existing firefox profile? This seems like quite a bit of work to support a non-supported testing environment.

	Brian R. Bondy [:bbondy] Assignee
	Comment 8 • 11 years ago

we'll you'd be using the wrong xul cache (when it's enabled, which it is not in debug mode from metro.js). So there could be a bunch of problems.   I'm fine with only doing the more simple path, but I think this way is more correct.

	Brian R. Bondy [:bbondy] Assignee
	Comment 9 • 11 years ago

The other more simple way discussed in channel is to check for XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Metro in patch 2 and drop patch 1. But the work is already done here and I think it's more correct, and we may need it in the future anyway.  

But if you'd rather minimize risk I'm also fine with doing that.

	Jim Mathies [:jimm]
	Comment 11 • 11 years ago

Poking through the code, it looks like there's a way to invalidate the cache via an observer or directly in the cpp. I wonder if we could get around the directory naming / environment detection stuff and instead detect the change in mode and reset the cache at an appropriate time? There's an observer that can trigger this ('startupcache-invalidate'). Maybe we could call that before shutdown when we switch modes?

For -metrodesktop, we could disable the cache completely based on XRE_WindowsEnvironement() and IsImmersiveBrowser() calls.

	Jim Mathies [:jimm]
	Comment 12 • 11 years ago

Comment on attachment 824817 [details] [diff] [review]
Alt implementation to replace patch 1 and Patch 2 with less changes/risk but doesn't take into account -metrodesktop

per irc discussion, we'll kick the metrodesktop issue out to another bug.

	Brian R. Bondy [:bbondy] Assignee
	Comment 13 • 11 years ago

Comment on attachment 824817 [details] [diff] [review]
Alt implementation to replace patch 1 and Patch 2 with less changes/risk but doesn't take into account -metrodesktop

Are you ok with this change?

	Brian R. Bondy [:bbondy] Assignee
	Comment 14 • 11 years ago

Comment on attachment 824817 [details] [diff] [review]
Alt implementation to replace patch 1 and Patch 2 with less changes/risk but doesn't take into account -metrodesktop

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

taras suggested you, sorry to throw another review your way :(

	Brian R. Bondy [:bbondy] Assignee
	Comment 15 • 11 years ago

https://hg.mozilla.org/projects/oak/rev/a384228760da

	Brian R. Bondy [:bbondy] Assignee
	Comment 16 • 11 years ago

Can safely land on m-c without any effects even before shared profile code lands.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a63e406139b5

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/a63e406139b5

	Manuela Muntean [Away]
	Comment 18 • 11 years ago

Could anyone please give guidelines that can help the QA in verifying this issue? Thanks!

	Brian R. Bondy [:bbondy] Assignee
	Comment 19 • 11 years ago

Hard call, I think this is covered by testing of other bugs. It would be some obvious problems if anything was wrong.