Summary: | Remove unnecessary ASSERT from XMLDocumentParser::notifyFinished() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gabor Rapcsanyi <rgabor> | ||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED CONFIGURATION CHANGED | ||||||
Severity: | Normal | CC: | ahmad.saleem792, ap, beidson, bfulgham, cdumez, darin, koivisto, rniwa | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Gabor Rapcsanyi
2015-04-02 06:42:31 PDT
Created attachment 249976 [details]
proposed patch
Comment on attachment 249976 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=249976&action=review > Source/WebCore/ChangeLog:9 > + Remove the ASSERT(m_pendingScript->accessCount() > 0) because if the MemoryCache is disabled > + then we never icrease the accessCount. Is it unnecessary, or incorrect? (In reply to comment #2) > Comment on attachment 249976 [details] > proposed patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249976&action=review > > > Source/WebCore/ChangeLog:9 > > + Remove the ASSERT(m_pendingScript->accessCount() > 0) because if the MemoryCache is disabled > > + then we never icrease the accessCount. > > Is it unnecessary, or incorrect? Well, maybe incorrect is a better word for it. I'm not sure whether we should change the assert or just remove it. Comment on attachment 249976 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=249976&action=review Couldn't we have a layout test for this by exposing MemoryCache::setDisabled() via Internals? > Source/WebCore/xml/parser/XMLDocumentParser.cpp:-226 > - ASSERT(m_pendingScript->accessCount() > 0); Maybe ASSERT(m_pendingScript->accessCount() > 0 || MemoryCache::singleton().disabled()); then? Comment on attachment 249976 [details]
proposed patch
This patch has been pending review since 2015 with no recent activity.
It seems unlikely that it would even still apply to trunk in its current form.
Clearing from the review queue.
Feel free to update and resubmit if the patch is still relevant.
>> ap@webkit.org - is this required? https://github.com/WebKit/WebKit/blob/1ffe018f5d5cd17e88f1a34ee7a62d47290ed07d/Source/WebCore/xml/parser/XMLDocumentParser.cpp#L227 Seems like m_pendingScript is now changed to nullptr and it does not asset. Thanks! There is no such assertion in XMLDocumentParser::notifyFinished any more. |