931846 - Story - Change startup cache location for Metro Firefox

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[Brian R. Bondy [:bbondy]]

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

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch 1 - Code to differentiate between metro front end and metro environment. rev1.

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 - Determine which StartupCache file to use based on front end. rev1

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

Comment 3

[Brian R. Bondy [:bbondy]]

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.

Comment 4

[Jim Mathies [:jimm]]

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.

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch 1 - Code to differentiate between metro front end and metro environment. rev2.

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch 2 - Determine which StartupCache file to use based on front end. rev2

Comment 7

[Jim Mathies [:jimm]]

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.

Comment 8

[Brian R. Bondy [:bbondy]]

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.

Comment 9

[Brian R. Bondy [:bbondy]]

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.

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Alt implementation to replace patch 1 and Patch 2 with less changes/risk but doesn't take into account -metrodesktop

Comment 11

[Jim Mathies [:jimm]]

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.

Comment 12

[Jim Mathies [:jimm]]

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.

Comment 13

[Brian R. Bondy [:bbondy]]

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?

Comment 14

[Brian R. Bondy [:bbondy]]

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

Comment 15

[Brian R. Bondy [:bbondy]]

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

Comment 16

[Brian R. Bondy [:bbondy]]

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

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 18

[Manuela Muntean [Away]]

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

Comment 19

[Brian R. Bondy [:bbondy]]

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