[Regression] Fix Draggable is float:left and container is overflow:hidden

[Regression] Fix Draggable is float:left and container is overflow:hidden

The change from https://chromiumcodereview.appspot.com/23537039 is
reverted (the cause for the regression). This patch also fixes the original
issue 23537039 tried to address. If the target element was a descendant of an 
inline-block the PaintInfo::shouldPaintWithinRoot did an early return from the 
paint call on inlineFlowBox. We let the paint call go through by clearing the 
paintRoot, on the basis that the inlineFlowBoxes will be intersecting the paint 
rect.

BUG=341089, 101204
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176613

[spartha, 2014-05-12 10:12:12]

PTAL

[spartha, 2014-05-20 05:11:44]

On 2014/05/12 10:12:12, spartha wrote:
> PTAL

Need reviews please!!

[esprehn, 2014-05-20 05:56:15]

This doesn't look right, why only clear the first one you encounter? Also you
need a test that's not a manual test, it should be possible to his this generic
paint code without one.

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
File Source/core/rendering/InlineFlowBox.cpp (right):

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1151: bool paintingRootCleared =
!childInfo.paintingRoot;
Why do you only ever clear the first one? That doesnt' make sense.

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1154: if (curr->renderer().isText() ||
!curr->boxModelObject()->hasSelfPaintingLayer()) {
This needs a better test, you should be able to test this without dragging.

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1161: paintingRootCleared = true;
Nothing reads this, why assign it?

[esprehn, 2014-05-20 05:56:15]

This doesn't look right, why only clear the first one you encounter? Also you
need a test that's not a manual test, it should be possible to his this generic
paint code without one.

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
File Source/core/rendering/InlineFlowBox.cpp (right):

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1151: bool paintingRootCleared =
!childInfo.paintingRoot;
Why do you only ever clear the first one? That doesnt' make sense.

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1154: if (curr->renderer().isText() ||
!curr->boxModelObject()->hasSelfPaintingLayer()) {
This needs a better test, you should be able to test this without dragging.

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1161: paintingRootCleared = true;
Nothing reads this, why assign it?

[esprehn, 2014-05-20 05:56:42]

I think jchaffraix@ is your best bet for a reviewer of this stuff.

[spartha, 2014-05-20 09:24:10]

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
File Source/core/rendering/InlineFlowBox.cpp (right):

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1151: bool paintingRootCleared =
!childInfo.paintingRoot;
On 2014/05/20 05:56:15, esprehn wrote:
> Why do you only ever clear the first one? That doesnt' make sense.
I am looking to paint just the InlineBox that holds the paintingRoot and not the
rest. If the paintingRoot is not set, then the chidren are painted anyway.
Otherwise we check if the any of the lineboxes have the paintingRoot as its
child and paint it.

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1154: if (curr->renderer().isText() ||
!curr->boxModelObject()->hasSelfPaintingLayer()) {
On 2014/05/20 05:56:15, esprehn wrote:
> This needs a better test, you should be able to test this without dragging.
I agree, that this mode of testing is not effective. AFIK, drag feature is the
only one which sets the paintingRoot to draw an element sub-tree.

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1161: paintingRootCleared = true;
On 2014/05/20 05:56:15, esprehn wrote:
> Nothing reads this, why assign it?
paintingRootCleared is being evaluated @1157. I added the check to avoid
additional calls isDescendantOf().

[Julien - ping for review, 2014-05-22 09:26:03]

It seems to me that the whole logic of going to the enclosing layer is busted as
you end up enclosing a bigger subtree than what you need. This patch is just
working around that fact.

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
File Source/core/rendering/InlineFlowBox.cpp (right):

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1150:
childInfo.updatePaintingRootForChildren(&renderer());
You're setting childInfo.paintingRoot to a non-NULL renderer here so it seems
like |paintingRootCleared| will never be true.

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1157: if (!paintingRootCleared &&
childInfo.paintingRoot->isDescendantOf(&curr->renderer())) {
That potentially makes this loop go to the root all the time. It seems like a
bad trade-off for a rare case.

[spartha, 2014-05-22 12:24:15]

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
File Source/core/rendering/InlineFlowBox.cpp (right):

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1150:
childInfo.updatePaintingRootForChildren(&renderer());
On 2014/05/22 09:26:03, Julien Chaffraix - CET wrote:
> You're setting childInfo.paintingRoot to a non-NULL renderer here so it seems
> like |paintingRootCleared| will never be true.
updatePaintingRootForChildren does not set the paintingRoot. if the passed
renderer is same as the paintingRoot, it is cleared, otherwise untouched. So,
paintingRootCleared will be true in cases where renderer() is same as the
paintingRoot or if paintingRoot is already NULL.

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1157: if (!paintingRootCleared &&
childInfo.paintingRoot->isDescendantOf(&curr->renderer())) {
On 2014/05/22 09:26:03, Julien Chaffraix - CET wrote:
> That potentially makes this loop go to the root all the time. It seems like a
> bad trade-off for a rare case.
There are sufficient checks in place not to affect normal paint operations.
Firstly, paintingRoot is set only for the drag & drop feature (Correct me if I
am wrong). Secondly, RenderLineBoxList::paint filters off calls to
InflowBox::paint by checking against the dirtyRect. Would this still be a big
tradeoff?

[spartha, 2014-05-28 08:43:29]

Patch updated. PTAL

[eseidel, 2014-05-28 22:39:04]

jchaffraix, esprehn: ptal?

[Julien - ping for review, 2014-06-05 22:01:35]

LGTM, you've convinced me that it's not too terrible.

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
File Source/core/rendering/InlineFlowBox.cpp (right):

https://codereview.chromium.org/278293002/diff/90001/Source/core/rendering/In...
Source/core/rendering/InlineFlowBox.cpp:1150:
childInfo.updatePaintingRootForChildren(&renderer());
On 2014/05/22 12:24:15, spartha wrote:
> On 2014/05/22 09:26:03, Julien Chaffraix - CET wrote:
> > You're setting childInfo.paintingRoot to a non-NULL renderer here so it
seems
> > like |paintingRootCleared| will never be true.
> updatePaintingRootForChildren does not set the paintingRoot. if the passed
> renderer is same as the paintingRoot, it is cleared, otherwise untouched. So,
> paintingRootCleared will be true in cases where renderer() is same as the
> paintingRoot or if paintingRoot is already NULL.

OK, this function is confusingly named for sure :-/

https://codereview.chromium.org/278293002/diff/130001/Source/web/tests/WebFra...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/278293002/diff/130001/Source/web/tests/WebFra...
Source/web/tests/WebFrameTest.cpp:5471: // Sub Test 2
It's not usually a good practice to chain unit tests.

EXPECT_* functions don't stop the functions but there is potential for one of
the WebCore call in sub-test 1 to crash and make sub-test 2 not run.

Also this makes this test more fragile as it can break due to either sub-test.

I would just split it in 2 with a helper function to generate the expected
SkBitmap.

[spartha, 2014-06-09 09:50:57]

Thanks!

https://codereview.chromium.org/278293002/diff/130001/Source/web/tests/WebFra...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/278293002/diff/130001/Source/web/tests/WebFra...
Source/web/tests/WebFrameTest.cpp:5471: // Sub Test 2
On 2014/06/05 22:01:35, Julien Chaffraix - PST wrote:
> It's not usually a good practice to chain unit tests.
> 
> EXPECT_* functions don't stop the functions but there is potential for one of
> the WebCore call in sub-test 1 to crash and make sub-test 2 not run.
> 
> Also this makes this test more fragile as it can break due to either sub-test.
> 
> I would just split it in 2 with a helper function to generate the expected
> SkBitmap.

Done.

[spartha, 2014-06-09 09:51:50]

The CQ bit was checked by sudarshan.p@samsung.com

[commit-bot: I haz the power, 2014-06-09 09:52:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/278293002/170001

[spartha, 2014-06-09 10:01:45]

The CQ bit was unchecked by sudarshan.p@samsung.com

[spartha, 2014-06-09 10:02:38]

On 2014/06/09 10:01:45, spartha wrote:
> The CQ bit was unchecked by mailto:sudarshan.p@samsung.com

@esprehn, need an rslgtm for this patch. Thanks!

[spartha, 2014-06-13 10:13:58]

On 2014/06/09 10:02:38, spartha wrote:
> On 2014/06/09 10:01:45, spartha wrote:
> > The CQ bit was unchecked by mailto:sudarshan.p@samsung.com
> 
> @esprehn, need an rslgtm for this patch. Thanks!

Ping for rslgtm!!

[spartha, 2014-06-18 12:41:14]

On 2014/06/13 10:13:58, spartha wrote:
> On 2014/06/09 10:02:38, spartha wrote:
> > On 2014/06/09 10:01:45, spartha wrote:
> > > The CQ bit was unchecked by mailto:sudarshan.p@samsung.com
> > 
> > @esprehn, need an rslgtm for this patch. Thanks!
> 
> Ping for rslgtm!!

Need an rslgtm for this patch!!

[tkent, 2014-06-20 05:01:49]

web/ lgtm.

[tkent, 2014-06-20 05:01:52]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-20 05:02:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/278293002/170001

[commit-bot: I haz the power, 2014-06-20 05:06:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/8084)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[commit-bot: I haz the power, 2014-06-20 05:09:06]

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

[commit-bot: I haz the power, 2014-06-20 05:09:07]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[spartha, 2014-06-20 09:52:02]

The CQ bit was checked by sudarshan.p@samsung.com

[commit-bot: I haz the power, 2014-06-20 09:52:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/278293002/190001

[commit-bot: I haz the power, 2014-06-20 11:13:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-20 12:10:35]

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

[commit-bot: I haz the power, 2014-06-20 12:10:36]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[spartha, 2014-06-20 13:55:40]

The CQ bit was checked by sudarshan.p@samsung.com

[commit-bot: I haz the power, 2014-06-20 13:56:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sudarshan.p@samsung.com/278293002/190001

[commit-bot: I haz the power, 2014-06-20 14:31:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-20 14:49:13]

Change committed as 176613