Summary: | Clean up WebCore based on Coverty Prevent static analysis results | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | cmarrin, hyatt, playmobil, simon.fraser | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2009-03-18 16:39:02 PDT
There is no attached patch. Yup. Patch had an issue, will be attached soon... Created attachment 28756 [details]
First pass cleanup with changelog, all tests pass
WebCore/ChangeLog | 107 +++++++++++++++++
WebCore/css/CSSFontSelector.cpp | 12 ++-
WebCore/css/CSSStyleSelector.cpp | 6 +
WebCore/css/CSSStyleSheet.cpp | 7 +-
WebCore/css/SVGCSSStyleSelector.cpp | 121 +++++---------------
WebCore/dom/ContainerNode.cpp | 14 +-
WebCore/dom/Document.cpp | 2 -
WebCore/editing/ApplyStyleCommand.cpp | 10 +-
.../editing/InsertParagraphSeparatorCommand.cpp | 3 +-
WebCore/editing/markup.cpp | 3 +-
WebCore/html/CanvasStyle.cpp | 13 ++-
WebCore/loader/TextDocument.cpp | 13 ++-
WebCore/page/FocusController.cpp | 6 +-
WebCore/page/animation/AnimationBase.h | 5 +-
WebCore/page/animation/AnimationController.cpp | 1 +
WebCore/page/animation/ImplicitAnimation.cpp | 3 +-
WebCore/page/animation/ImplicitAnimation.h | 4 +-
WebCore/page/animation/KeyframeAnimation.cpp | 2 +-
WebCore/page/animation/KeyframeAnimation.h | 4 +-
WebCore/platform/chromium/PasteboardChromium.cpp | 19 ++--
WebCore/platform/graphics/GlyphPageTreeNode.cpp | 1 +
.../transforms/PerspectiveTransformOperation.cpp | 5 +-
.../transforms/RotateTransformOperation.cpp | 8 +-
WebCore/rendering/RenderLayer.cpp | 12 +-
WebCore/rendering/RenderTableSection.cpp | 8 +-
WebCore/rendering/SVGRenderSupport.cpp | 2 +-
WebCore/rendering/bidi.cpp | 3 +-
27 files changed, 226 insertions(+), 168 deletions(-)
Please split this up. Many of these changes are completely unrelated. Comment on attachment 28756 [details]
First pass cleanup with changelog, all tests pass
r- based on Marks comment.
Comment on attachment 28756 [details]
First pass cleanup with changelog, all tests pass
I've split this into a bunch of patches. Attaching...
Created attachment 28899 [details]
Coverty fixes in Animations/Transforms
WebCore/ChangeLog | 29 ++++++++++++++++++++
WebCore/page/animation/AnimationBase.h | 5 +--
WebCore/page/animation/AnimationController.cpp | 1 +
WebCore/page/animation/ImplicitAnimation.cpp | 3 +-
WebCore/page/animation/ImplicitAnimation.h | 4 +-
WebCore/page/animation/KeyframeAnimation.cpp | 2 +-
WebCore/page/animation/KeyframeAnimation.h | 4 +-
.../transforms/PerspectiveTransformOperation.cpp | 5 +---
.../transforms/RotateTransformOperation.cpp | 8 +----
9 files changed, 41 insertions(+), 20 deletions(-)
Created attachment 28900 [details]
Font fallback cleanup and added ASSERT for GlyphPageTreeNode
WebCore/ChangeLog | 17 +++++++++++++++++
WebCore/css/CSSFontSelector.cpp | 12 ++++++++----
WebCore/platform/graphics/GlyphPageTreeNode.cpp | 1 +
3 files changed, 26 insertions(+), 4 deletions(-)
Created attachment 28901 [details]
CSS dead code removal and cleanup from Coverty errors
WebCore/ChangeLog | 24 +++++++
WebCore/css/CSSStyleSelector.cpp | 6 ++
WebCore/css/CSSStyleSheet.cpp | 7 +-
WebCore/css/SVGCSSStyleSelector.cpp | 121 +++++++++--------------------------
4 files changed, 63 insertions(+), 95 deletions(-)
Created attachment 28902 [details]
Style cleanup and dead code removal in dom, editing
WebCore/ChangeLog | 31 ++++++++++++++++++++
WebCore/dom/ContainerNode.cpp | 14 ++++----
WebCore/dom/Document.cpp | 2 -
WebCore/editing/ApplyStyleCommand.cpp | 10 +++---
.../editing/InsertParagraphSeparatorCommand.cpp | 3 +-
WebCore/editing/markup.cpp | 3 +-
6 files changed, 45 insertions(+), 18 deletions(-)
Created attachment 28903 [details]
Fix case where lBreak.obj->isBR() when lBreak.obj was NULL
WebCore/ChangeLog | 15 +++++++++++++++
WebCore/rendering/bidi.cpp | 3 +--
2 files changed, 16 insertions(+), 2 deletions(-)
Created attachment 28904 [details]
Make TextTokenizer ASSERT that the buffer was freed
WebCore/ChangeLog | 13 +++++++++++++
WebCore/loader/TextDocument.cpp | 13 +++++++++++--
2 files changed, 24 insertions(+), 2 deletions(-)
Created attachment 28905 [details]
Move ASSERT(foo) to before where foo-> is used
WebCore/ChangeLog | 15 +++++++++++++++
WebCore/platform/chromium/PasteboardChromium.cpp | 19 +++++++++----------
WebCore/rendering/SVGRenderSupport.cpp | 2 +-
3 files changed, 25 insertions(+), 11 deletions(-)
Created attachment 28906 [details]
Remove dead code and style cleanup
WebCore/ChangeLog | 23 +++++++++++++++++++++++
WebCore/html/CanvasStyle.cpp | 13 +++++++++++--
WebCore/page/FocusController.cpp | 6 +++---
WebCore/rendering/RenderLayer.cpp | 12 ++++++------
WebCore/rendering/RenderTableSection.cpp | 8 +++-----
5 files changed, 46 insertions(+), 16 deletions(-)
Comment on attachment 28899 [details]
Coverty fixes in Animations/Transforms
r=me
Comment on attachment 28906 [details]
Remove dead code and style cleanup
r=me
Comment on attachment 28901 [details] CSS dead code removal and cleanup from Coverty errors > + * css/CSSStyleSheet.cpp: > + (WebCore::CSSStyleSheet::styleSheetChanged): > + Removing dead code, "root" can never be null here. Can static_cast<CSSStyleSheet*>(root)->doc() ever be null? Comment on attachment 28903 [details] Fix case where lBreak.obj->isBR() when lBreak.obj was NULL > + Fix case where lBreak.obj->isBR() when lBreak.obj was NULL > + https://bugs.webkit.org/show_bug.cgi?id=24684 > + > + I don't have a test case where we were taking the wrong path here > + However, adding an ASSERT(lBreak.obj) before this usage caused > + multiple bidi tests to fail, so lBreak.obj can be null here. Tabs in ChangeLog. > - if (lBreak == resolver.position() && !lBreak.obj->isBR()) { > + if (lBreak == resolver.position() && (!lBreak.obj || !lBreak.obj->isBR())) { Seems harmless to add this, even though it may be impossible in practice. r=me Comment on attachment 28905 [details]
Move ASSERT(foo) to before where foo-> is used
I understand the desire to move asserts before the thing asserted are used, but I don't like the idea of moving all the declarations to the top of the function. Could you make a more limited version that doesn't move the definitions of renderer, cachedImage, and image up to the top of the function?
In either case, r=me
(In reply to comment #17) > (From update of attachment 28901 [details] [review]) > > + * css/CSSStyleSheet.cpp: > > + (WebCore::CSSStyleSheet::styleSheetChanged): > > + Removing dead code, "root" can never be null here. > > Can static_cast<CSSStyleSheet*>(root)->doc() ever be null? I'm not sure. It appears it can be, not necessarily in this case. But I'll update the patch anyway. Created attachment 28933 [details]
CSS dead code removal and cleanup from Coverty errors
WebCore/ChangeLog | 24 +++++++
WebCore/css/CSSStyleSelector.cpp | 6 ++
WebCore/css/CSSStyleSheet.cpp | 2 +-
WebCore/css/SVGCSSStyleSelector.cpp | 121 +++++++++--------------------------
4 files changed, 61 insertions(+), 92 deletions(-)
Comment on attachment 28901 [details]
CSS dead code removal and cleanup from Coverty errors
Now null-checking doc() again, per darin's suggestion.
http://trac.webkit.org/changeset/41983 http://trac.webkit.org/changeset/41984 http://trac.webkit.org/changeset/41985 http://trac.webkit.org/changeset/41986 http://trac.webkit.org/changeset/41988 http://trac.webkit.org/changeset/41989 http://trac.webkit.org/changeset/41990 http://trac.webkit.org/changeset/41991 |