Search correctly for inline ancestors without a sibling when dirtying lineboxes

third_party/WebKit/Source/core/layout/line/LineBoxList.cpp

 /*
  * Copyright (C) 2008 Apple 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:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 271 matching lines @@
       container.Parent().DirtyLinesFromChangedChild(container);
       // Mark the container to avoid dirtying the same lines again across
       // multiple destroy() calls of the same subtree.
       container.SetAncestorLineBoxDirty();
     }
     return;
   }
 
   // Try to figure out which line box we belong in. First try to find a previous
   // line box by examining our siblings. If we are a float inside an inline then
-  // check the siblings of our inline parent. If we didn't find a line box, then
-  // use our parent's first line box.
+  // check our nearest inline ancestor with siblings. If we didn't find a line
+  // box, then use our parent's first line box.
   RootInlineBox* box = nullptr;
   LineLayoutItem curr = child.PreviousSibling();
-  if (child.IsFloating() && !curr) {
-    LineLayoutInline outer_inline;
-    for (LineLayoutItem parent = child.Parent();
-         parent && parent.IsLayoutInline() && !parent.PreviousSibling();
-         parent = parent.Parent())
-      outer_inline = LineLayoutInline(parent);
+  LineLayoutItem parent = child.Parent();

[mstensho (USE GERRIT), 2017/04/19 11:37:24]

|parent| is only needed if child.IsFloating(), so I think it would be better to
move it inside some scope (not that it's so much cheaper, but it makes it easier
to read). Something like this:

if (child.IsFloating() && !curr)  {
  LineLayoutItem parent = child.Parent();
  if (parent && parent.IsLayoutInline()) {
     outer_inline = parent;
     while ...
     if (outer_inline) ...
  }
}

... or just omit the declaration of |parent| completely. Writing
"child.Parent()" a couple of times extra doesn't sound so harmful. Do we really
need the parent NULL-check, BTW? I think I'd like the program to just go ahead
and die of the child doesn't have a parent. :)

+  if (child.IsFloating() && !curr && parent && parent.IsLayoutInline()) {
+    LineLayoutItem outer_inline = parent;
+    while (outer_inline && outer_inline.Parent().IsLayoutInline() &&
+           !outer_inline.PreviousSibling())

[mstensho (USE GERRIT), 2017/04/19 11:37:24]

Could you check !outer_inline.PreviousSibling() BEFORE
outer_inline.Parent().IsLayoutInline()? I had a hard time reading this, and
besides it doesn't matter what the parent is if outer_inline has siblings.

+      outer_inline = outer_inline.Parent();
     if (outer_inline)
       curr = outer_inline.PreviousSibling();
   }
 
   for (; curr; curr = curr.PreviousSibling()) {
     if (curr.IsFloatingOrOutOfFlowPositioned())
       continue;
 
     if (curr.IsAtomicInlineLevel()) {
       InlineBox* wrapper = LineLayoutBox(curr).InlineBoxWrapper();
@@ skipping 64 matching lines @@
     DCHECK_EQ(child->prevLineBox(), prev);
     prev = child;
   }
   DCHECK_EQ(prev, m_lastLineBox);
 #endif
 }
 
 #endif
 
 }  // namespace blink