508421 - move res/ stuff into toolkit.jar

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Comment 1

[(dormant account)]

Attachment: initial hack

This seems to work. For this I run
zip -o -r chrome/res.jar res/

I'm looking at js files too, those would be nice to have in there. After this patch, Fennec reads only 10 files through nsFileInputStream. 

25ms 1]nsFileInputStream /home/taras/builds/fennec.jarred/xulrunner/dist/bin/greprefs/xpinstall.js (2 reads,83 bytes, 0 seeks, 11857bytes/sec) 0ms
25ms 2]nsFileInputStream /home/taras/builds/fennec.jarred/xulrunner/dist/bin/greprefs/security-prefs.js (2 reads,3368 bytes, 0 seeks, 481142bytes/sec) 0ms
29ms 3]nsFileInputStream /home/taras/builds/fennec.jarred/xulrunner/dist/bin/greprefs/all.js (2 reads,68355 bytes, 0 seeks, 1265833bytes/sec) 3ms
29ms 4]nsFileInputStream /home/taras/builds/fennec.jarred/xulrunner/dist/bin/defaults/pref/xulrunner.js (2 reads,3816 bytes, 0 seeks, 545142bytes/sec) 0ms
30ms 5]nsFileInputStream /home/taras/builds/fennec.jarred/mobile/dist/bin/defaults/preferences/mobile.js (2 reads,12650 bytes, 0 seeks, 1054166bytes/sec) 0ms
30ms 6]nsFileInputStream /home/taras/builds/fennec.jarred/mobile/dist/bin/defaults/preferences/mobile-l10n.js (2 reads,209 bytes, 0 seeks, 52250bytes/sec) 0ms
123ms  8]nsFileInputStream /home/taras/.mozilla/fennec/br27ywik.default/prefs.js (2 reads,1802 bytes, 0 seeks, 901000bytes/sec) 0ms
299ms   9]nsFileInputStream /home/taras/.mozilla/fennec/br27ywik.default/localstore.rdf (1 reads,169 bytes, 0 seeks, 84500bytes/sec) 0ms
1955ms                   7]nsFileInputStream /home/taras/.mozilla/fennec/br27ywik.default/XPC.mfasl (54 reads,3082496 bytes, 14 seeks, 2520438bytes/sec) 1852ms
2027ms 10]nsFileInputStream /home/taras/.mozilla/fennec/br27ywik.default/XUL.mfasl (59 reads,738450 bytes, 6 seeks, 3516428bytes/sec) 1728ms

Comment 2

[(dormant account)]

Attachment: res jarring

Comment 3

[Benjamin Smedberg]

Why don't you just put them in toolkit.jar with the chrome?

Comment 4

[(dormant account)]

(In reply to comment #3)
> Why don't you just put them in toolkit.jar with the chrome?

I will, just called it res.jar for initial testing.

Comment 5

[(dormant account)]

Attachment: nsResProtocolHandler

Benjamin, I need help with this part. Code works fine as is, but it wont work with a flat/symlink chrome. I think I have to daisy-chain nsResProtocolHandler on top of nsChromeProtocolHandler.cpp by having a .manifest that registers .res stuff. Is that right?

Comment 6

[(dormant account)]

Pike says I shouldn't mix jar and chome protocol handlers, instead go by directories..ie sort of like my existing patch. The only thing i need in that case is a define to see when a non-jar chrome-packing mode is specified. Is there one? if not how do i make one?

Comment 7

[Axel Hecht]

Note, I'm mostly concerned of mixing the permissions stuff we have around chrome:// with that around resource://. That probably should work fine, but keeping those two in separate files in the backend might defend us in a bit more depth.

Not sure if https://developer.mozilla.org/en/Chrome_Registration#resource would help us with having a flag on jar'ed or not, we could probably add the gre resource protocol to toolkit.manifest and switch there with a flag? Just rambling.

Comment 8

[(dormant account)]

Attachment: nsResProtocolHandler change

Asking for review on this as it works as well I need it to work. Once this is approved, I'll do another patch for moving res/ into toolkit.jar(more stuff like attachment 392644 [details] [diff] [review])

Comment 9

[Benjamin Smedberg]

Comment on attachment 394969 [details] [diff] [review]
nsResProtocolHandler change

>diff --git a/intl/uconv/src/nsGREResProperties.cpp b/intl/uconv/src/nsGREResProperties.cpp

> nsGREResProperties::nsGREResProperties(const nsACString& aFile)
> {
>-  nsCOMPtr<nsIFile> file;
>-  nsresult rv = NS_GetSpecialDirectory(NS_GRE_DIR, getter_AddRefs(file));
>+  nsCOMPtr<nsIInputStream> inStr;
>+  nsresult rv;

It's common practice to make `nsresult rv;` the first declaration of the function

>+  if (NS_FAILED(rv)) return;

Please put returns on the following line here and below

>+  nsCOMPtr<nsIChannel> channel;
>+  rv = NS_NewChannel(getter_AddRefs(channel), uri);

for efficiency pass the ioservice that you already have into this function

>diff --git a/netwerk/protocol/res/src/nsResProtocolHandler.cpp b/netwerk/protocol/res/src/nsResProtocolHandler.cpp

>-    rv = AddSpecialDir(NS_GRE_DIR, NS_LITERAL_CSTRING("gre"));
>+    const nsDependentCString strGRE_DIR = NS_LITERAL_CSTRING("gre");
>+    rv = AddSpecialDir(NS_GRE_DIR, strGRE_DIR);

NS_NAMED_LITERAL_CSTRING

>+    // make resource://gre/res/ point to a jar
>+    nsCOMPtr<nsIURI> greURI;
>+    GetSubstitution(strGRE_DIR, getter_AddRefs(greURI));
>+#ifdef MOZ_CHROME_FILE_FORMAT_JAR
>+    rv = mIOService->NewURI(NS_LITERAL_CSTRING("jar:chrome/toolkit.jar!/"), nsnull, greURI,
>+                            getter_AddRefs(mResURI));
>+#else
>+    rv = mIOService->NewURI(NS_LITERAL_CSTRING("chrome/toolkit/"), nsnull, greURI,
>+                            getter_AddRefs(mResURI));
>+#endif

1) Making resource://gre/res go somewhere different from resource://gre doesn't map with the normal behavior of the resource protocol handler. Instead, I think you should just create a new mapping resource://gre-resources/

2) We should not expose the entire contents of toolkit.jar to script. Instead, package up the resources in a subdirectory of toolkit.jar, e.g. toolkit.jar!/resources

Comment 10

[(dormant account)]

(In reply to comment #9)

> you should just create a new mapping resource://gre-resources/

So change all existing users the uses resource://gre/res to above?

> 
> 2) We should not expose the entire contents of toolkit.jar to script. Instead,
> package up the resources in a subdirectory of toolkit.jar, e.g.
> toolkit.jar!/resources

Indeed.

Comment 11

[Benjamin Smedberg]

Yes, change the URI callers are using.

Comment 12

[(dormant account)]

Attachment: resource://gre-resources support

This way of doing this will make /res jarring much easier to land. Here is the protocol piece.

Comment 13

[(dormant account)]

Attachment: resource://gre-resources support [Checkin: See comment 19]

Sorry, forgot the NS_NAMED_LITERAL_CSTRING part in prior patch

Comment 14

[(dormant account)]

Attachment: charset property reading via jar [Checkin: See comment 19]

charset properties

Comment 15

[Benjamin Smedberg]

Comment on attachment 396816 [details] [diff] [review]
resource://gre-resources support
[Checkin: See comment 19]

Needs a test.

Comment 16

[(dormant account)]

(In reply to comment #15)
> (From update of attachment 396816 [details] [diff] [review])
> Needs a test.

actually i was thinking of doing the test after I land the subsequent patches.

ie I can actually try reading charsetalias.data once this lands. Also where would the testcase go?

Comment 17

[Benjamin Smedberg]

netwerk/test/unit

I think you should land the patch and the test and the charsetalias patch at once.

Comment 18

[(dormant account)]

Attachment: testcase [Checkin: See comment 19]

can i have s/r-/r+/ on the gre-resources now?

Comment 19

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/a0c23bfa5219

Comment 20

[(not currently active) Ted Mielczarek]

Two things to note:
1) I added a mac packaging file: http://mxr.mozilla.org/mozilla-central/source/browser/installer/osx/packages-static so you should add/remove files there in the future (until I merge the three packaging files)
2) If you remove files, you should add them to removed-files.in: http://mxr.mozilla.org/mozilla-central/source/browser/installer/removed-files.in so they get removed by the updater in the future. For these particular files it's unlikely to harm anything if you don't remove them.

Comment 21

[Robert Kaiser]

And, as the package files are application-specific, it's good to file a followup bug for Thunderbird and SeaMonkey to update theirs as well. This can be done in "MailNews Core - Build Config" if it's easy and we can just do a patch for all comm-central-based applications at once. At least having the bug filed would be helpful though.

Comment 22

[(dormant account)]

Attachment: removed files entry + osx installer [Checkin: Comment 29]

Comment 23

[tabmix.onemen]

since yesterday textbox with type="number" throw exception from this line in numberbox.xml "numberbox" binding constructor:

>  var dsymbol = (Number(5.4)).toLocaleString().match(/\D/); 

i'v narrow the regression time to this bug

Comment 24

[(dormant account)]

(In reply to comment #23)
> since yesterday textbox with type="number" throw exception from this line in
> numberbox.xml "numberbox" binding constructor:
> 
> >  var dsymbol = (Number(5.4)).toLocaleString().match(/\D/); 
> 
> i'v narrow the regression time to this bug

I can't reproduce that here.

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 397059 [details] [diff] [review]
removed files entry + osx installer
[Checkin: Comment 29]

Thanks!

Comment 26

[Dietrich Ayala (:dietrich)]

is this bug ready to be closed? what else needs to be done here?

Comment 27

[(dormant account)]

(In reply to comment #26)
> is this bug ready to be closed? what else needs to be done here?

Needs checking in of the patch that ted just reviewed.

Comment 28

[(not currently active) Ted Mielczarek]

I probably broke the hell out of your patch with my landing of bug 511642. I'll fix your patch and land it for you.

Comment 29

[(not currently active) Ted Mielczarek]

Fixed and pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/dd26d29368ae

Comment 30

[Ed Lee :Mardak]

(In reply to comment #24)
> > >  var dsymbol = (Number(5.4)).toLocaleString().match(/\D/); 
> > i'v narrow the regression time to this bug
> I can't reproduce that here.
Try on Windows and change the locale to Russian.

The regression range for bug 514803 has been reduced to:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=72d25f49cbcb&tochange=4ee238eba089

This seems like the only candidate and comment #23 points at this as well.

Comment 31

[(dormant account)]

With Mardak's help I managed to reproduce this on windows, investigating.

Comment 32

[(not currently active) Ted Mielczarek]

I think you want to work in bug 514803 and leave this fixed, no?

Comment 33

[(dormant account)]

(In reply to comment #32)
> I think you want to work in bug 514803 and leave this fixed, no?

ok, fix looks easy.

Comment 34

[Serge Gautherie (:sgautherie)]

(In reply to comment #21)
> And, as the package files are application-specific, it's good to file a
> followup bug for Thunderbird and SeaMonkey to update theirs as well. This can
> be done in "MailNews Core - Build Config" if it's easy and we can just do a

I filed bug 521382.

> patch for all comm-central-based applications at once. At least having the bug
> filed would be helpful though.

(Yes, that would be very appreciated...)