Cleanup friends of LayoutObject

Cleanup friends of LayoutObject

The friends of LayoutObject only need to access the layout tree mutation
methods, but get access to all LayoutObject private members.

Add LayoutObject::setDangerousOneWayParent() to avoid friends.

Expose several private/protected members to public which were accessed
using friend relationship.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201265

[Xianzhu, 2015-08-21 19:52:04]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, leviw@chromium.org, pdr@chromium.org

[leviw_travelin_and_unemployed, 2015-08-21 21:10:42]

https://codereview.chromium.org/1306993002/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1306993002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.h:264: class DangerousLayoutTreeMutator {
I'd refactor this out of the other changes since it's the most bikesheddy.

[pdr., 2015-08-21 21:11:58]

pdr@chromium.org changed reviewers:
+ esprehn@chromium.org

[pdr., 2015-08-21 21:11:58]

LGTM, DangerousLayoutTreeMutator is a fun name.

+cc esprehn, could you please take a look too?

[pdr., 2015-08-21 21:14:20]

On 2015/08/21 at 21:10:42, leviw wrote:
>
https://codereview.chromium.org/1306993002/diff/40001/Source/core/layout/Layo...
> File Source/core/layout/LayoutObject.h (right):
> 
>
https://codereview.chromium.org/1306993002/diff/40001/Source/core/layout/Layo...
> Source/core/layout/LayoutObject.h:264: class DangerousLayoutTreeMutator {
> I'd refactor this out of the other changes since it's the most bikesheddy.

Ack, parallel reviews.

I'm okay with a followup too.

[leviw_travelin_and_unemployed, 2015-08-21 21:15:15]

On 2015/08/21 at 21:11:58, pdr wrote:
> LGTM, DangerousLayoutTreeMutator is a fun name.

I also like the name :)

> 
> +cc esprehn, could you please take a look too?

[esprehn, 2015-08-21 21:20:32]

Can we fix the bug and do the refactor separately? It's hard to review this
since it both refactors code for sanity but also fixes a paint bug.

https://codereview.chromium.org/1306993002/diff/40001/LayoutTests/fast/events...
File LayoutTests/fast/events/overflow-scroll-fake-mouse-move.html (right):

https://codereview.chromium.org/1306993002/diff/40001/LayoutTests/fast/events...
LayoutTests/fast/events/overflow-scroll-fake-mouse-move.html:26:
setTimeout(fail, 1000);
A 1 second timeout is really bad, what is this test doing? 200ms seems like it
should be enough..., and really the test should be waiting for a specific event
instead.

https://codereview.chromium.org/1306993002/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1306993002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutBlock.cpp:572:
DangerousLayoutTreeMutator(*nextSiblingOfPromotees).setPreviousSibling(lastPromotee);
Instead of having all methods I'd prefer it only had insertBefore() and
setParent() and we moved this tree mutation operation into the
LayoutObjectChildList or the DangerousLayoutTreeMutator.

setParent() should also ASSERT that nextSibling() and previousSibling() are
nullptr.

https://codereview.chromium.org/1306993002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutBlock.cpp:592:
DangerousLayoutTreeMutator(*child).removeFromLayoutFlowThread();
I think I would just make removeFromLayoutFlowThread public.

https://codereview.chromium.org/1306993002/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerReflectionInfo.cpp (right):

https://codereview.chromium.org/1306993002/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerReflectionInfo.cpp:66:
LayoutObject::DangerousLayoutTreeMutator(*m_reflection).setParent(m_box); // We
create a 1-way connection.
Please un-nest the classes.

https://codereview.chromium.org/1306993002/diff/40001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/1306993002/diff/40001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:416: FloatQuad
quadForFakeMouseMoveEvent =
FloatQuad(layer()->layoutObject()->previousPaintInvalidationRectIncludingCompositedScrolling(*paintInvalidationContainer));
Is this the bug fix? Can we fix it in a separate patch?

[Xianzhu, 2015-08-21 22:52:19]

The new patch is much simpler because we only need to expose setParent() which
is used by DeprecatedPaintLayerScrollableArea and
DeprecatedPaintLayerRelectionInfo only. Added setDangerousOneWayParent() for the
usage.

https://codereview.chromium.org/1306993002/diff/40001/LayoutTests/fast/events...
File LayoutTests/fast/events/overflow-scroll-fake-mouse-move.html (right):

https://codereview.chromium.org/1306993002/diff/40001/LayoutTests/fast/events...
LayoutTests/fast/events/overflow-scroll-fake-mouse-move.html:26:
setTimeout(fail, 1000);
On 2015/08/21 21:20:31, esprehn wrote:
> A 1 second timeout is really bad, what is this test doing? 200ms seems like it
> should be enough..., and really the test should be waiting for a specific
event
> instead.

This timeout happens only on failure. Previously we used this timeout on both
success and failure, now we finish the test directly on success.

It's also fine to remove the above statement, then we'll timeout by the layout
test runner on failure.

https://codereview.chromium.org/1306993002/diff/40001/Source/core/layout/Layo...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1306993002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutBlock.cpp:572:
DangerousLayoutTreeMutator(*nextSiblingOfPromotees).setPreviousSibling(lastPromotee);
On 2015/08/21 21:20:32, esprehn wrote:
> Instead of having all methods I'd prefer it only had insertBefore() and
> setParent() and we moved this tree mutation operation into the
> LayoutObjectChildList or the DangerousLayoutTreeMutator.
> 
> setParent() should also ASSERT that nextSibling() and previousSibling() are
> nullptr.

Thanks for the finding. Actually I didn't look into the real logic here.

Changed the whole function body to moveAllChildrenTo(parent(), nextSibling()).

https://codereview.chromium.org/1306993002/diff/40001/Source/core/layout/Layo...
Source/core/layout/LayoutBlock.cpp:592:
DangerousLayoutTreeMutator(*child).removeFromLayoutFlowThread();
On 2015/08/21 21:20:31, esprehn wrote:
> I think I would just make removeFromLayoutFlowThread public.

Done.

[mstensho (USE GERRIT), 2015-08-24 10:37:26]

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

[mstensho (USE GERRIT), 2015-08-24 10:37:27]

https://codereview.chromium.org/1306993002/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1306993002/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutBlock.cpp:558:
moveAllChildrenTo(toLayoutBoxModelObject(parent()), nextSibling());
Might as well get rid of promoteAllChildrenAndInsertAfter(), and call
moveAllChildrenTo() directly from the call site.

[Xianzhu, 2015-08-24 16:41:34]

https://codereview.chromium.org/1306993002/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1306993002/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutBlock.cpp:558:
moveAllChildrenTo(toLayoutBoxModelObject(parent()), nextSibling());
On 2015/08/24 10:37:27, mstensho wrote:
> Might as well get rid of promoteAllChildrenAndInsertAfter(), and call
> moveAllChildrenTo() directly from the call site.

Done.

[Xianzhu, 2015-08-25 22:45:08]

Any more comments?

[chrishtr, 2015-08-25 23:27:35]

https://codereview.chromium.org/1306993002/diff/80001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1306993002/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.h:1060: void removeFromLayoutFlowThread();
Document.

https://codereview.chromium.org/1306993002/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.h:1062: virtual void
updateAnonymousChildStyle(const LayoutObject& child, ComputedStyle& style) const
{ }
Document.

[Xianzhu, 2015-08-26 00:23:53]

https://codereview.chromium.org/1306993002/diff/80001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1306993002/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.h:1060: void removeFromLayoutFlowThread();
On 2015/08/25 23:27:35, chrishtr wrote:
> Document.

Done.

https://codereview.chromium.org/1306993002/diff/80001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.h:1062: virtual void
updateAnonymousChildStyle(const LayoutObject& child, ComputedStyle& style) const
{ }
On 2015/08/25 23:27:35, chrishtr wrote:
> Document.

Done.

[chrishtr, 2015-08-26 02:59:19]

lgtm

https://codereview.chromium.org/1306993002/diff/100001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1306993002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.h:264: void
setDangerousOneWayParent(LayoutObject* parent)
I don't think this needs to be inline.

[esprehn, 2015-08-26 07:43:47]

lgtm, beautiful. Land away after fixing christr@'s nit.

[Xianzhu, 2015-08-26 16:12:04]

https://codereview.chromium.org/1306993002/diff/100001/Source/core/layout/Lay...
File Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/1306993002/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutObject.h:264: void
setDangerousOneWayParent(LayoutObject* parent)
On 2015/08/26 02:59:19, chrishtr wrote:
> I don't think this needs to be inline.

Done.

[Xianzhu, 2015-08-26 16:12:08]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-08-26 16:12:08]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
chrishtr@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1306993002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-08-26 16:12:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306993002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306993002/120001

[commit-bot: I haz the power, 2015-08-26 16:37:13]

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

[commit-bot: I haz the power, 2015-08-26 16:37:14]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2015-08-26 16:43:56]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-08-26 16:44:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306993002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306993002/120001

[commit-bot: I haz the power, 2015-08-26 16:51:11]

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

[commit-bot: I haz the power, 2015-08-26 16:51:12]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Xianzhu, 2015-08-26 16:51:50]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-08-26 16:52:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306993002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306993002/120001

[commit-bot: I haz the power, 2015-08-26 17:06:16]

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

[commit-bot: I haz the power, 2015-08-26 17:06:17]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2015-08-26 23:11:22]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-08-26 23:11:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306993002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306993002/120001

[commit-bot: I haz the power, 2015-08-27 00:18:03]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201265