Closed
Bug 590242
Opened 15 years ago
Closed 15 years ago
Do not open omnijar 3x
Categories
(Core :: Networking: JAR, defect)
Tracking
()
RESOLVED
FIXED
mozilla2.0b7
People
(Reporter: taras.mozilla, Assigned: taras.mozilla)
References
Details
Attachments
(1 file, 4 obsolete files)
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)
Assignee | ||
Updated•15 years ago
|
Attachment #468738 -
Attachment is obsolete: true
Attachment #468738 -
Flags: review?(mwu)
Comment 2•15 years ago
|
||
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+
Assignee | ||
Comment 3•15 years ago
|
||
(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?
Assignee | ||
Updated•15 years ago
|
Attachment #468740 -
Flags: superreview?(benjamin)
Comment 4•15 years ago
|
||
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+
Assignee | ||
Comment 5•15 years ago
|
||
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?
Assignee: nobody → tglek
Assignee | ||
Comment 6•15 years ago
|
||
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 7•15 years ago
|
||
Comment on attachment 471638 [details] [diff] [review]
reuse omnijar + review comments
a=beltzner
Attachment #471638 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 8•15 years ago
|
||
Contrary to what the check-in message says, this bug is not a final blocker.
Comment 9•15 years ago
|
||
The patch is bitrotted.
Assignee | ||
Comment 10•15 years ago
|
||
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?
Comment 11•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Updated•15 years ago
|
Attachment #473723 -
Flags: approval2.0?
You need to log in
before you can comment on or make changes to this bug.
Description
•