Story - Change startup cache location for Metro Firefox

RESOLVED FIXED in Firefox 28

Status

defect
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

unspecified
Firefox 28
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [block28] feature=story c=tbd u=tbd p=1)

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

6 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
Summary: Change startup cache location for Metro Firefox → Story - Change startup cache location for Metro Firefox
Whiteboard: feature=story c=tbd u=tbd p=1
Whiteboard: feature=story c=tbd u=tbd p=1 → [block28] feature=story c=tbd u=tbd p=1
Assignee

Comment 1

6 years ago
Assignee: nobody → netzen
Attachment #824241 - Flags: review?(jmathies)
Assignee

Updated

6 years ago
Attachment #824241 - Attachment description: Patch v1 - Code to differentiate between metro front end and metro environment. rev1. → Patch 1 - Code to differentiate between metro front end and metro environment. rev1.
Assignee

Comment 2

6 years ago
If this gets feedack+ then I'll proceed to get a review for this small patch from bsmedberg.
Attachment #824243 - Flags: feedback?(jmathies)
Assignee

Updated

6 years ago
Blocks: metrov1it18
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Assignee

Comment 3

6 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.
Attachment #824241 - Flags: review?(jmathies)
Assignee

Updated

6 years ago
Attachment #824243 - Flags: feedback?(jmathies)
Priority: -- → P2
QA Contact: jbecerra
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.
Assignee

Comment 5

6 years ago
Attachment #824241 - Attachment is obsolete: true
Attachment #824329 - Flags: review?(jmathies)
Assignee

Comment 6

6 years ago
Attachment #824243 - Attachment is obsolete: true
Attachment #824332 - Flags: feedback?(jmathies)
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.
Assignee

Comment 8

6 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.
Assignee

Comment 9

6 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.
Assignee

Updated

6 years ago
Attachment #824817 - Attachment description: Alt implementation to replace patch 1 andd Patch 2 with less changes/risk but doesn't take into account -metrodesktop → Alt implementation to replace patch 1 and Patch 2 with less changes/risk but doesn't take into account -metrodesktop
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 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.
Attachment #824817 - Flags: feedback?(jmathies) → feedback+
Assignee

Updated

6 years ago
Attachment #824329 - Attachment is obsolete: true
Attachment #824329 - Flags: review?(jmathies)
Assignee

Updated

6 years ago
Attachment #824332 - Attachment is obsolete: true
Attachment #824332 - Flags: feedback?(jmathies)
Assignee

Comment 13

6 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?
Attachment #824817 - Flags: review?(taras.mozilla)
Assignee

Updated

6 years ago
Depends on: 932986
Assignee

Comment 14

6 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 :(
Attachment #824817 - Flags: review?(taras.mozilla) → review?(aklotz)
Attachment #824817 - Flags: review?(aklotz) → review+
Assignee

Updated

6 years ago
Whiteboard: [block28] feature=story c=tbd u=tbd p=1 → [block28][completed-oak] feature=story c=tbd u=tbd p=1
Assignee

Comment 16

6 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
Whiteboard: [block28][completed-oak] feature=story c=tbd u=tbd p=1 → [block28] feature=story c=tbd u=tbd p=1
Target Milestone: --- → Firefox 28
https://hg.mozilla.org/mozilla-central/rev/a63e406139b5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
See Also: → 932986
Assignee

Updated

6 years ago
No longer depends on: 932986
Could anyone please give guidelines that can help the QA in verifying this issue? Thanks!
Flags: needinfo?(netzen)
Assignee

Comment 19

6 years ago
Hard call, I think this is covered by testing of other bugs. It would be some obvious problems if anything was wrong.
Flags: needinfo?(netzen)
You need to log in before you can comment on or make changes to this bug.