Clean up WebCore based on Coverty Prevent static analysis results See attached patch.
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