Remove clips where we can show that they are unnecessary.

Source/core/rendering/RenderOverflowTest.cpp

+/*
+ * Copyright (C) 2013 Google Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following disclaimer
+ * in the documentation and/or other materials provided with the
+ * distribution.
+ *     * Neither the name of Google Inc. nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "config.h"
+#include "core/rendering/RenderOverflow.h"
+
+#include "core/platform/graphics/LayoutRect.h"
+
+#include <gtest/gtest.h>
+
+using namespace WebCore;
+
+namespace WebCore {
+
+// FIXME: Move this somewhere more generic.
+void PrintTo(const LayoutRect& rect, ::std::ostream* os)
+{
+    *os << "LayoutRect("
+        << rect.x().toFloat() << ", "
+        << rect.y().toFloat() << ", "
+        << rect.width().toFloat() << ", "
+        << rect.height().toFloat() << ")";
+}
+
+} // namespace WebCore
+
+namespace {
+
+TEST(RenderOverflowTest, InitialOverflowRects)
+{
+    RenderOverflow overflow(LayoutRect(10, 10, 80, 80), LayoutRect(0, 0, 100, 100));
+    EXPECT_EQ(LayoutRect(10, 10, 80, 80), overflow.layoutOverflowRect());
+    EXPECT_EQ(LayoutRect(0, 0, 100, 100), overflow.visualOverflowRect());
+    EXPECT_TRUE(overflow.contentsVisualOverflowRect().isEmpty());
+}
+
+TEST(RenderOverflowTest, AddLayoutOverflow)
+{
+    RenderOverflow overflow(LayoutRect(10, 10, 80, 80), LayoutRect(0, 0, 100, 100));
+
+    overflow.addLayoutOverflow(LayoutRect(0, 10, 30, 10));
+    EXPECT_EQ(LayoutRect(0, 10, 90, 80), overflow.layoutOverflowRect());
+
+    overflow.addLayoutOverflow(LayoutRect(50, 50, 10, 20));
+    EXPECT_EQ(LayoutRect(0, 10, 90, 80), overflow.layoutOverflowRect());
+
+    // It's not clear whether any code relies on this behavior (it seems like this
+    // could probably just use LayoutRect::unite).

[Julien - ping for review, 2013/08/07 00:17:08]

No clue what this comment means.

[jbroman, 2013/08/08 14:35:39]

Removed. It didn't add much value anyhow.

+    overflow.addLayoutOverflow(LayoutRect(200, 200, 0, 0));

[Julien - ping for review, 2013/08/07 00:17:08]

This seems like a dumb case (and arguable in how we should handle it). Can we
either replace this test with something not 0 sized? If we really need to test
this case, it should be its own test and not mixed with other testing.

Your unit tests are also not very unitary: you test multiple operations and they
would be better off split into several more focused test case (including some
common set-up method as you repeat the overflow initialization 5 times). For
example, this test checks 3 cases:
- adding some "top" overflow should expand the layout overflow.
- adding some overflow contained in the existing one shouldn't modify the layout
overflow.
- adding a 0 sized rectangle expands the layout overflow.

On top of that, you check that none of the previous cases change the visual
overflow.

[jbroman, 2013/08/08 14:35:39]

I agree that it's a dumb case, and the previous comment was more or less a
(poorly phrased) comment on the same. I do think there's value in testing what
it does do, so that if we do change it in the future, we do so knowingly and
intentionally. Changing the test at the same time would be easy.
If you want unitary test cases, then unitary test cases shall you get.

+    EXPECT_EQ(LayoutRect(0, 10, 200, 190), overflow.layoutOverflowRect());
+
+    EXPECT_EQ(LayoutRect(0, 0, 100, 100), overflow.visualOverflowRect());
+    EXPECT_TRUE(overflow.contentsVisualOverflowRect().isEmpty());
+}
+
+TEST(RenderOverflowTest, AddVisualOverflow)
+{
+    RenderOverflow overflow(LayoutRect(10, 10, 80, 80), LayoutRect(0, 0, 100, 100));
+
+    overflow.addVisualOverflow(LayoutRect(-10, 0, 30, 30));
+    EXPECT_EQ(LayoutRect(-10, 0, 110, 100), overflow.visualOverflowRect());
+
+    overflow.addVisualOverflow(LayoutRect(50, 50, 10, 20));
+    EXPECT_EQ(LayoutRect(-10, 0, 110, 100), overflow.visualOverflowRect());
+
+    // It's not clear whether any code relies on this behavior (it seems like this
+    // could probably just use LayoutRect::unite).
+    overflow.addVisualOverflow(LayoutRect(200, 200, 0, 0));
+    EXPECT_EQ(LayoutRect(-10, 0, 210, 200), overflow.visualOverflowRect());
+
+    EXPECT_EQ(LayoutRect(10, 10, 80, 80), overflow.layoutOverflowRect());
+    EXPECT_TRUE(overflow.contentsVisualOverflowRect().isEmpty());
+}
+
+TEST(RenderOverflowTest, AddContentsVisualOverflow)
+{
+    RenderOverflow overflow(LayoutRect(10, 10, 80, 80), LayoutRect(0, 0, 100, 100));
+
+    overflow.addContentsVisualOverflow(LayoutRect(0, 0, 10, 10));
+    EXPECT_EQ(LayoutRect(0, 0, 10, 10), overflow.contentsVisualOverflowRect());
+
+    overflow.addContentsVisualOverflow(LayoutRect(80, 80, 10, 10));
+    EXPECT_EQ(LayoutRect(0, 0, 90, 90), overflow.contentsVisualOverflowRect());
+
+    // Empty contents visual overflow does not affect the overflow rect.
+    overflow.addContentsVisualOverflow(LayoutRect(200, 200, 0, 0));
+    EXPECT_EQ(LayoutRect(0, 0, 90, 90), overflow.contentsVisualOverflowRect());
+
+    EXPECT_EQ(LayoutRect(0, 0, 100, 100), overflow.visualOverflowRect());
+    EXPECT_EQ(LayoutRect(10, 10, 80, 80), overflow.layoutOverflowRect());
+}
+
+TEST(RenderOverflowTest, Move)
+{
+    RenderOverflow overflow(LayoutRect(10, 10, 80, 80), LayoutRect(0, 0, 100, 100));
+    overflow.addContentsVisualOverflow(LayoutRect(50, 20, 10, 100));
+
+    overflow.move(500, 0);
+    EXPECT_EQ(LayoutRect(510, 10, 80, 80), overflow.layoutOverflowRect());
+    EXPECT_EQ(LayoutRect(500, 0, 100, 100), overflow.visualOverflowRect());
+    EXPECT_EQ(LayoutRect(550, 20, 10, 100), overflow.contentsVisualOverflowRect());
+
+    overflow.move(-500, 500);
+    EXPECT_EQ(LayoutRect(10, 510, 80, 80), overflow.layoutOverflowRect());
+    EXPECT_EQ(LayoutRect(0, 500, 100, 100), overflow.visualOverflowRect());
+    EXPECT_EQ(LayoutRect(50, 520, 10, 100), overflow.contentsVisualOverflowRect());
+}
+
+} // namespace