Closed Bug 508421 Opened 10 years ago Closed 10 years ago

move res/ stuff into toolkit.jar

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 5 obsolete files)

No description provided.
Attached patch initial hack (obsolete) — Splinter Review
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
Assignee: nobody → tglek
Attached patch res jarring (obsolete) — Splinter Review
Why don't you just put them in toolkit.jar with the chrome?
(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.
Attached patch nsResProtocolHandler (obsolete) — Splinter Review
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?
Attachment #392611 - Attachment is obsolete: true
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?
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.
Attached patch nsResProtocolHandler change (obsolete) — Splinter Review
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])
Attachment #394898 - Attachment is obsolete: true
Attachment #394969 - Flags: review?(benjamin)
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
Attachment #394969 - Flags: review?(benjamin) → review-
(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.
Yes, change the URI callers are using.
Attached patch resource://gre-resources support (obsolete) — Splinter Review
This way of doing this will make /res jarring much easier to land. Here is the protocol piece.
Attachment #394969 - Attachment is obsolete: true
Attachment #396557 - Flags: review?(benjamin)
Summary: move res/ stuff into res.jar → move res/ stuff into toolkit.jar
Sorry, forgot the NS_NAMED_LITERAL_CSTRING part in prior patch
Attachment #396557 - Attachment is obsolete: true
Attachment #396816 - Flags: review?(benjamin)
Attachment #396557 - Flags: review?(benjamin)
charset properties
Attachment #392644 - Attachment is obsolete: true
Attachment #396817 - Flags: review?(benjamin)
Comment on attachment 396816 [details] [diff] [review]
resource://gre-resources support
[Checkin: See comment 19]

Needs a test.
Attachment #396816 - Flags: review?(benjamin) → review-
Attachment #396817 - Flags: review?(benjamin) → review+
(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?
netwerk/test/unit

I think you should land the patch and the test and the charsetalias patch at once.
can i have s/r-/r+/ on the gre-resources now?
Attachment #396858 - Flags: review?(benjamin)
Attachment #396858 - Flags: review?(benjamin) → review+
Attachment #396816 - Flags: review- → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/a0c23bfa5219
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
OS: Linux → All
Hardware: x86 → All
Blocks: 509755
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.
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.
Blocks: 513027
Attachment #397059 - Flags: review?(ted.mielczarek)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
(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 on attachment 397059 [details] [diff] [review]
removed files entry + osx installer
[Checkin: Comment 29]

Thanks!
Attachment #397059 - Flags: review?(ted.mielczarek) → review+
is this bug ready to be closed? what else needs to be done here?
(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.
Keywords: checkin-needed
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.
Fixed and pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/dd26d29368ae
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
(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.
Blocks: 514803
With Mardak's help I managed to reproduce this on windows, investigating.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I think you want to work in bug 514803 and leave this fixed, no?
(In reply to comment #32)
> I think you want to work in bug 514803 and leave this fixed, no?

ok, fix looks easy.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Attachment #397059 - Attachment description: removed files entry + osx installer → removed files entry + osx installer [Checkin: Comment 29]
Attachment #396858 - Attachment description: testcase → testcase [Checkin: Comment 19]
Attachment #396817 - Attachment description: charset property reading via jar → charset property reading via jar [Checkin: Comment 19]
Attachment #396816 - Attachment description: resource://gre-resources support → resource://gre-resources support [Checkin: See comment 19]
Attachment #396817 - Attachment description: charset property reading via jar [Checkin: Comment 19] → charset property reading via jar [Checkin: See comment 19]
Attachment #396858 - Attachment description: testcase [Checkin: Comment 19] → testcase [Checkin: See comment 19]
Flags: in-testsuite+
Target Milestone: --- → mozilla1.9.3a1
(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...)
You need to log in before you can comment on or make changes to this bug.