Remove RenderObject::isMarquee()

Remove RenderObject::isMarquee()

Now RenderMarquee is gone, why don't remove this one?

R=haraken@chromium.org, arg@chromium.org
BUG=341031

[mstensho (USE GERRIT), 2014-10-06 20:00:34]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2014-10-06 20:00:35]

lgtm with nits.

Also, there's a bunch of marquee stuff in rendering/style/ (and -internal-
prefixed marquee CSS properties) that can probably be removed. That's for a
separate CL, though.

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (left):

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:2041: // Marquees in WinIE are like a
mixture of blocks and inline-blocks.  They size as though they're blocks,
This comment about marquees is no longer valid.

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:3800: // Floating objects don't shrink. 
Objects that don't avoid floats don't shrink.  Marquees don't shrink.
Can you remove the comment about marquees, please?

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:2043: if (isFloating() ||
(isInlineBlockOrInlineTable()))
Extraneous set of parentheses.

[Mike West, 2014-10-06 20:04:00]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-10-06 20:04:01]

core/ LGTM % nits.

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:3380: // the normal flow.  We make an
exception for marquees, since their baselines are meaningless
Nit: the marquee comment here should also be removed.

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:3384: bool ignoreBaseline = (layer() &&
layer()->scrollableArea() && ((direction == HorizontalLine ?
(layer()->scrollableArea()->verticalScrollbar() ||
layer()->scrollableArea()->scrollYOffset())
Nit: This is hard to parse. Perhaps you could move the ? clause to another line?
As in:

bool ignoreBaseline = condition
    ? true stuff goes here
    : false stuff goes here;

[Hajime Morrita, 2014-10-06 20:22:10]

Thanks for the review folks! Landing.

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:3380: // the normal flow.  We make an
exception for marquees, since their baselines are meaningless
On 2014/10/06 20:04:01, Mike West wrote:
> Nit: the marquee comment here should also be removed.

Done.

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:3384: bool ignoreBaseline = (layer() &&
layer()->scrollableArea() && ((direction == HorizontalLine ?
(layer()->scrollableArea()->verticalScrollbar() ||
layer()->scrollableArea()->scrollYOffset())
On 2014/10/06 20:04:01, Mike West wrote:
> Nit: This is hard to parse. Perhaps you could move the ? clause to another
line?
> As in:
> 
> bool ignoreBaseline = condition
>     ? true stuff goes here
>     : false stuff goes here;

Done.

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (left):

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:2041: // Marquees in WinIE are like a
mixture of blocks and inline-blocks.  They size as though they're blocks,
On 2014/10/06 20:00:34, mstensho wrote:
> This comment about marquees is no longer valid.

Done.

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:3800: // Floating objects don't shrink. 
Objects that don't avoid floats don't shrink.  Marquees don't shrink.
On 2014/10/06 20:00:34, mstensho wrote:
> Can you remove the comment about marquees, please?

Done.

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/629143005/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBox.cpp:2043: if (isFloating() ||
(isInlineBlockOrInlineTable()))
On 2014/10/06 20:00:34, mstensho wrote:
> Extraneous set of parentheses.

Done.

[Hajime Morrita, 2014-10-06 20:24:47]

On 2014/10/06 20:00:35, mstensho wrote:
> lgtm with nits.
> 
> Also, there's a bunch of marquee stuff in rendering/style/ (and -internal-
> prefixed marquee CSS properties) that can probably be removed. That's for a
> separate CL, though.
> 
Right. will do in separate CL.

[haraken, 2014-10-06 23:12:06]

LGTM

Thanks for the clean-up!

[mstensho (USE GERRIT), 2014-10-13 09:53:19]

commit?

[haraken, 2014-10-13 14:56:09]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-10-13 14:56:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/629143005/1

[commit-bot: I haz the power, 2014-10-13 14:57:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-13 14:57:22]

Failed to apply patch for Source/core/rendering/RenderLayerScrollableArea.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/RenderLayerScrollableArea.cpp
  Hunk #1 succeeded at 350 with fuzz 2 (offset 6 lines).
  Hunk #2 succeeded at 392 (offset 6 lines).
  Hunk #3 FAILED at 574.
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/rendering/RenderLayerScrollableArea.cpp.rej

Patch:       Source/core/rendering/RenderLayerScrollableArea.cpp
Index: Source/core/rendering/RenderLayerScrollableArea.cpp
diff --git a/Source/core/rendering/RenderLayerScrollableArea.cpp
b/Source/core/rendering/RenderLayerScrollableArea.cpp
index
806fdaf31e92388deb1fa0b23ddf41b13abfe004..e6efd178d18fc200563ef4135e762b5317a58379
100644
--- a/Source/core/rendering/RenderLayerScrollableArea.cpp
+++ b/Source/core/rendering/RenderLayerScrollableArea.cpp
@@ -344,11 +344,9 @@ int
RenderLayerScrollableArea::scrollSize(ScrollbarOrientation orientation) cons
 
 void RenderLayerScrollableArea::setScrollOffset(const IntPoint&
newScrollOffset)
 {
-    if (!box().isMarquee()) {
-        // Ensure that the dimensions will be computed if they need to be (for
overflow:hidden blocks).
-        if (m_scrollDimensionsDirty)
-            computeScrollDimensions();
-    }
+    // Ensure that the dimensions will be computed if they need to be (for
overflow:hidden blocks).
+    if (m_scrollDimensionsDirty)
+        computeScrollDimensions();
 
     if (scrollOffset() == toIntSize(newScrollOffset))
         return;
@@ -388,7 +386,7 @@ void RenderLayerScrollableArea::setScrollOffset(const
IntPoint& newScrollOffset)
 
     bool requiresPaintInvalidation = true;
 
-    if (!box().isMarquee() && box().view()->compositor()->inCompositingMode())
{
+    if (box().view()->compositor()->inCompositingMode()) {
         // Hits in virtual/gpu/fast/canvas/canvas-scroll-path-into-view.html.
         DisableCompositingQueryAsserts disabler;
         bool onlyScrolledCompositedLayers = scrollsOverflow()
@@ -576,13 +574,11 @@ void RenderLayerScrollableArea::updateAfterLayout()
 
     computeScrollDimensions();
 
-    if (!box().isMarquee()) {
-        // Layout may cause us to be at an invalid scroll position. In this
case we need
-        // to pull our scroll offsets back to the max (or push them up to the
min).
-        IntSize clampedScrollOffset =
clampScrollOffset(adjustedScrollOffset());
-        if (clampedScrollOffset != adjustedScrollOffset())
-            scrollToOffset(clampedScrollOffset);
-    }
+    // Layout may cause us to be at an invalid scroll position. In this case we
need
+    // to pull our scroll offsets back to the max (or push them up to the min).
+    IntSize clampedScrollOffset = clampScrollOffset(adjustedScrollOffset());
+    if (clampedScrollOffset != adjustedScrollOffset())
+        scrollToOffset(clampedScrollOffset);
 
     if (originalScrollOffset != adjustedScrollOffset())
         scrollToOffsetWithoutAnimation(-scrollOrigin() +
adjustedScrollOffset());

[Timothy Loh, 2014-10-20 01:51:26]

timloh@chromium.org changed reviewers:
+ timloh@chromium.org

[Timothy Loh, 2014-10-20 01:51:26]

I've uploaded an updated version of this at
https://codereview.chromium.org/669553002/

[Hajime Morrita, 2014-10-20 17:06:00]

Oops. Sorry for overlooking this and thank you for taking care of it!