We need to account for culled inline parents of the hit-tested nodes.(Reland)

Source/core/rendering/InlineFlowBox.cpp

 /*
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 1010 matching lines @@
 }
 
 bool InlineFlowBox::nodeAtPoint(const HitTestRequest& request, HitTestResult& result, const HitTestLocation& locationInContainer, const LayoutPoint& accumulatedOffset, LayoutUnit lineTop, LayoutUnit lineBottom)
 {
     LayoutRect overflowRect(visualOverflowRect(lineTop, lineBottom));
     flipForWritingMode(overflowRect);
     overflowRect.moveBy(accumulatedOffset);
     if (!locationInContainer.intersects(overflowRect))
         return false;
 
-    // Check children first.
-    // We need to account for culled inline parents of the hit-tested nodes, so that they may also get included in area-based hit-tests.
-    RenderObject* culledParent = 0;
-    for (InlineBox* curr = lastChild(); curr; curr = curr->prevOnLine()) {
-        if (curr->renderer().isText() || !curr->boxModelObject()->hasSelfPaintingLayer()) {
-            RenderObject* newParent = 0;
-            // Culled parents are only relevant for area-based hit-tests, so ignore it in point-based ones.
-            if (locationInContainer.isRectBasedTest()) {
-                newParent = curr->renderer().parent();
-                if (newParent == renderer())
-                    newParent = 0;
-            }
-            // Check the culled parent after all its children have been checked, to do this we wait until
-            // we are about to test an element with a different parent.
-            if (newParent != culledParent) {
-                if (!newParent || !newParent->isDescendantOf(culledParent)) {
-                    while (culledParent && culledParent != renderer() && culledParent != newParent) {
-                        if (culledParent->isRenderInline() && toRenderInline(culledParent)->hitTestCulledInline(request, result, locationInContainer, accumulatedOffset))
-                            return true;
-                        culledParent = culledParent->parent();
+    // Check all of children including culled inlines.

[pdr., 2014/12/05 21:21:01]

This may be fantastic code but I was unable to understand what it's doing in 20
minutes of studying it. To have a maintainable codebase we have to clean this up
so a reasonable person can understand it based on the code and comments alone.
This isn't necessarily your fault but we'll have to make it understandable to
get it through review.

I've added some questions below. I recommend we improve this top comment to
better describe what's going on.

In your suggested diagram, steps 2 and 3, why do we stop at text -> a -> span
instead of going up to the font?

[Miyoung Shin(g), 2014/12/06 07:22:11]

I agree with your opinion. I will add the comment about what we do in more
detail.
Because we should check first the children before parent.

+    // We need to account for culled inline parents of the hit-tested nodes,
+    // so that they may also get included in not only area-based hit-tests but also point-based ones.
+    // And we should consider of running the minimal loops for hit-test
+    InlineBox* currentBox = lastChild();

[pdr., 2014/12/05 21:21:01]

Why do we need to track the current box and current renderer?

[Miyoung Shin(g), 2014/12/06 07:22:11]

current box is for hit-testing inline boxes in inlineflowbox.
current renderer is for hit-testing culled parent of inline box.
But I will update the new patch because I should consider BIDI derection of
inline box.
And I will roll-back the main loop based on current box as it was.

+    RenderObject* currentRenderer = currentBox ? &currentBox->renderer() : 0;
+    while (currentRenderer && currentRenderer != renderer()) {
+        if (currentBox && currentBox->renderer() == currentRenderer) {
+            // We should check the previous line in current renderer until the next renderer comes
+            bool escapedRenderer = true;
+            while (escapedRenderer) {

[pdr., 2014/12/05 21:21:01]

What is this inner loop really doing?

Would a for loop be more understandable here?

[Miyoung Shin(g), 2014/12/06 07:22:11]

I will re-factor this and remove it.
If I remain the comment in more detail, the second loop is more understandable. 

+                if (currentBox->renderer().isText() || !currentBox->boxModelObject()->hasSelfPaintingLayer()) {

[pdr., 2014/12/05 21:21:01]

isText() and !hasSelfPaintingLayer() seem completely unrelated. What is this
conditional really checking?

[Miyoung Shin(g), 2014/12/06 07:22:11]

It was from origin code and I tried to keep them. I'm not that sure about them.

+                    if (currentBox->nodeAtPoint(request, result, locationInContainer, accumulatedOffset, lineTop, lineBottom)) {
+                        renderer().updateHitTestResult(result, locationInContainer.point() - toLayoutSize(accumulatedOffset));
+                        return true;
                     }
                 }
-                culledParent = newParent;
+
+                InlineBox* previousBox = currentBox;
+                currentBox = currentBox->prevOnLine();
+                if (!currentBox || currentBox->renderer() != previousBox->renderer())
+                    escapedRenderer = false;
             }
-            if (curr->nodeAtPoint(request, result, locationInContainer, accumulatedOffset, lineTop, lineBottom)) {
-                renderer().updateHitTestResult(result, locationInContainer.point() - toLayoutSize(accumulatedOffset));
+        } else if (!currentBox || !currentRenderer->isDescendantOf(&currentBox->renderer())) {
+            if (currentRenderer->isRenderInline() && !toRenderInline(currentRenderer)->alwaysCreateLineBoxes() && toRenderInline(currentRenderer)->hitTestCulledInline(request, result, locationInContainer, accumulatedOffset))
                 return true;
-            }
         }
-    }
-    // Check any culled ancestor of the final children tested.
-    while (culledParent && culledParent != renderer()) {
-        if (culledParent->isRenderInline() && toRenderInline(culledParent)->hitTestCulledInline(request, result, locationInContainer, accumulatedOffset))
-            return true;
-        culledParent = culledParent->parent();
+        currentRenderer = currentRenderer->previousInPreOrder();

[pdr., 2014/12/05 21:21:01]

previousInPreOrder could be rather expensive in this loop.

[Miyoung Shin(g), 2014/12/06 07:22:11]

I will re-factor this and remove it. the next patch will be more cheaper

     }
 
     // Now check ourselves. Pixel snap hit testing.
     LayoutRect frameRect = roundedFrameRect();
     LayoutUnit minX = frameRect.x();
     LayoutUnit minY = frameRect.y();
     LayoutUnit width = frameRect.width();
     LayoutUnit height = frameRect.height();
 
     // Constrain our hit testing to the line top and bottom if necessary.
@@ skipping 276 matching lines @@
         ASSERT(child->prevOnLine() == prev);
         prev = child;
     }
     ASSERT(prev == m_lastChild);
 #endif
 }
 
 #endif
 
 } // namespace blink