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

Source/core/rendering/InlineFlowBox.cpp

Index: Source/core/rendering/InlineFlowBox.cpp
diff --git a/Source/core/rendering/InlineFlowBox.cpp b/Source/core/rendering/InlineFlowBox.cpp
index 10918f9e4886f4037c2b3e46bb0a46be044f9f21..904bf68257e7f03ab6abe924a20ab6b7f99e4337 100644
--- a/Source/core/rendering/InlineFlowBox.cpp
+++ b/Source/core/rendering/InlineFlowBox.cpp
@@ -1028,41 +1028,34 @@ bool InlineFlowBox::nodeAtPoint(const HitTestRequest& request, HitTestResult& re
     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.