Closed Bug 590242 Opened 10 years ago Closed 10 years ago

Do not open omnijar 3x

Categories

(Core :: Networking: JAR, defect)

Product:
Core
Component:
Networking: JAR
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b7

People

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

References

Details

Attachments

(1 file, 4 obsolete files)

unbitrotted
6.49 KB, patch
taras.mozilla
: review+
taras.mozilla
: superreview+
Details | Diff | Splinter Review 
Currently we open the omnijar 3 times due to 3 different ways to access it in the code.
Attachment #468738 - Flags: review?(mwu)
Attached patch reuse omnijar (obsolete) — — Splinter Review 
no with qrefed changes
Attachment #468740 - Flags: review?(mwu)
Attachment #468738 - Attachment is obsolete: true
Attachment #468738 - Flags: review?(mwu)
Comment on attachment 468740 [details] [diff] [review]
reuse omnijar

I'd like to have the code use nsIZipReader consistently throughout one day and have this automatically handled by the zip cache. But, this will do just fine till we get there. (if we can get there..)

Just one thing - nsAutoPtr<nsZipArchive> seems a little sketchy when it points to the omnijar allocated nsZipArchive. Maybe we can special case it to not free the omnijar nsZipArchive?
Attachment #468740 - Flags: review?(mwu) → review+
(In reply to comment #2)
> Comment on attachment 468740 [details] [diff] [review]
> reuse omnijar
> 
> I'd like to have the code use nsIZipReader consistently throughout one day and
> have this automatically handled by the zip cache. But, this will do just fine
> till we get there. (if we can get there..)

Yeah, that'd be nicer, but seems like a pain atm.

> 
> Just one thing - nsAutoPtr<nsZipArchive> seems a little sketchy when it points
> to the omnijar allocated nsZipArchive. Maybe we can special case it to not free
> the omnijar nsZipArchive?

I did. The destructor calls close() which in turn calls .forget() on the autoptr if it's the omnijar. Is that not sufficient?
Attachment #468740 - Flags: superreview?(benjamin)
Comment on attachment 468740 [details] [diff] [review]
reuse omnijar

Please don't use namespace mozilla, use specific symbols such as mozilla::OmnijarReader and mozilla::OmnijarPath.
Attachment #468740 - Flags: superreview?(benjamin) → superreview+
Attached patch reuse omnijar + review comments (obsolete) — — Splinter Review 
Requesting approval, because opening omnijar 3x is morally wrong.
Attachment #468740 - Attachment is obsolete: true
Attachment #470486 - Flags: superreview+
Attachment #470486 - Flags: review+
Attachment #470486 - Flags: approval2.0?
Attached patch reuse omnijar + review comments (obsolete) — — Splinter Review 
Missed a line of code that broke opening of non-omnijars.
Attachment #470486 - Attachment is obsolete: true
Attachment #471638 - Flags: superreview+
Attachment #471638 - Flags: review+
Attachment #471638 - Flags: approval2.0?
Attachment #470486 - Flags: approval2.0?
Comment on attachment 471638 [details] [diff] [review]
reuse omnijar + review comments

a=beltzner
Attachment #471638 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
Contrary to what the check-in message says, this bug is not a final blocker.
The patch is bitrotted.
Attached patch unbitrotted — — Splinter Review 
This should carryover approval from earlier patch.
Attachment #471638 - Attachment is obsolete: true
Attachment #473723 - Flags: superreview+
Attachment #473723 - Flags: review+
Attachment #473723 - Flags: approval2.0?
Pushed http://hg.mozilla.org/mozilla-central/rev/6b5f65982976
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Attachment #473723 - Flags: approval2.0?
You need to log in before you can comment on or make changes to this bug.