Issue 696703005: Remove RenderSelectionInfo

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+45 lines, -113 lines)			Patch
M	LayoutTests/TestExpectations	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderBlock.h	View	1 2 3	2 chunks	+2 lines, -4 lines	0 comments	Download
M	Source/core/rendering/RenderBlock.cpp	View	1 2 3	1 chunk	+8 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderListMarker.cpp	View	1	1 chunk	+3 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderObject.h	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/rendering/RenderObject.cpp	View	1 2 3	2 chunks	+2 lines, -5 lines	0 comments	Download
M	Source/core/rendering/RenderReplaced.cpp	View	1	1 chunk	+3 lines, -0 lines	0 comments	Download
D	Source/core/rendering/RenderSelectionInfo.h	View		1 chunk	+0 lines, -89 lines	0 comments	Download
M	Source/core/rendering/RenderText.cpp	View	1 2 3	1 chunk	+3 lines, -0 lines	0 comments	Download
M	Source/core/rendering/RenderView.cpp	View	1 2 3 4 5	3 chunks	+21 lines, -14 lines	0 comments	Download

Messages

Total messages: 14 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Julien - ping for review

This is the last nail in RenderSelectionInfo's coffin. The failing test is a progression AFAICT ...

6 years, 1 month ago (2014-11-12 23:35:49 UTC) #2

leviw_travelin_and_unemployed

This LGTM. Do we have tests for layer squashing and selection invalidation? It'd be nice ...

6 years, 1 month ago (2014-11-12 23:41:43 UTC) #4

chrishtr

On 2014/11/12 at 23:41:43, leviw wrote: > This LGTM. Do we have tests for layer ...

6 years, 1 month ago (2014-11-13 00:20:29 UTC) #5

chrishtr

https://codereview.chromium.org/696703005/diff/60001/Source/core/rendering/RenderReplaced.cpp File Source/core/rendering/RenderReplaced.cpp (right): https://codereview.chromium.org/696703005/diff/60001/Source/core/rendering/RenderReplaced.cpp#newcode440 Source/core/rendering/RenderReplaced.cpp:440: // FIXME: groupedMapping() leaks the squashing abstraction. On 2014/11/12 ...

6 years, 1 month ago (2014-11-13 00:22:36 UTC) #6

Julien - ping for review

On 2014/11/13 at 00:20:29, chrishtr wrote: > On 2014/11/12 at 23:41:43, leviw wrote: > > ...

6 years, 1 month ago (2014-11-13 01:09:33 UTC) #7

On 2014/11/13 at 00:20:29, chrishtr wrote:
> On 2014/11/12 at 23:41:43, leviw wrote:
> > This LGTM. Do we have tests for layer squashing and selection invalidation?
It'd be nice since there's a lot of re-shuffling of that code in this patch.
> 
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Julien, could you double check that this test exercises this code? I'm pretty
sure it does.

The test covers 2 of the 3 functions that checks for groupedMapping (the test
has no list marker so it cannot validate this part of the change). I don't think
the last function is covered by any test.

https://codereview.chromium.org/696703005/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/696703005/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:3011: const RenderBlock* containingBlock
= this->containingBlock();
On 2014/11/12 23:41:42, leviw wrote:
> Nit: Is this-> needed?

It should be: the variable has the same name as the function so C++ (not sure
about C++11) required the this-> to disambiguate the 2.

https://codereview.chromium.org/696703005/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderReplaced.cpp (right):

https://codereview.chromium.org/696703005/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderReplaced.cpp:440: // FIXME: groupedMapping() leaks
the squashing abstraction.
On 2014/11/13 at 00:22:36, chrishtr wrote:
> On 2014/11/12 at 23:41:43, leviw wrote:
> > *sigh*... this continually makes me sad.
> 
> I think you should be able to just call
RenderLayer::mapRectToPaintInvalidationBacking and not leak groupedMapping().
> 
> I meant to try this in the old selection code and never got around to it.

This change is a refactoring so it should just move code around, not fix some
existing bug. I am happy to follow-up on that!

chrishtr

On 2014/11/13 at 01:09:33, jchaffraix wrote: > On 2014/11/13 at 00:20:29, chrishtr wrote: > > ...

6 years, 1 month ago (2014-11-13 01:32:47 UTC) #8

On 2014/11/13 at 01:09:33, jchaffraix wrote:
> On 2014/11/13 at 00:20:29, chrishtr wrote:
> > On 2014/11/12 at 23:41:43, leviw wrote:
> > > This LGTM. Do we have tests for layer squashing and selection
invalidation? It'd be nice since there's a lot of re-shuffling of that code in
this patch.
> > 
> > 
> >
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Julien, could you double check that this test exercises this code? I'm
pretty sure it does.
> 
> The test covers 2 of the 3 functions that checks for groupedMapping (the test
has no list marker so it cannot validate this part of the change). I don't think
the last function is covered by any test.
> 
>
https://codereview.chromium.org/696703005/diff/60001/Source/core/rendering/Re...
> File Source/core/rendering/RenderObject.cpp (right):
> 
>
https://codereview.chromium.org/696703005/diff/60001/Source/core/rendering/Re...
> Source/core/rendering/RenderObject.cpp:3011: const RenderBlock*
containingBlock = this->containingBlock();
> On 2014/11/12 23:41:42, leviw wrote:
> > Nit: Is this-> needed?
> 
> It should be: the variable has the same name as the function so C++ (not sure
about C++11) required the this-> to disambiguate the 2.
> 
>
https://codereview.chromium.org/696703005/diff/60001/Source/core/rendering/Re...
> File Source/core/rendering/RenderReplaced.cpp (right):
> 
>
https://codereview.chromium.org/696703005/diff/60001/Source/core/rendering/Re...
> Source/core/rendering/RenderReplaced.cpp:440: // FIXME: groupedMapping() leaks
the squashing abstraction.
> On 2014/11/13 at 00:22:36, chrishtr wrote:
> > On 2014/11/12 at 23:41:43, leviw wrote:
> > > *sigh*... this continually makes me sad.
> > 
> > I think you should be able to just call
RenderLayer::mapRectToPaintInvalidationBacking and not leak groupedMapping().
> > 
> > I meant to try this in the old selection code and never got around to it.
> 
> This change is a refactoring so it should just move code around, not fix some
existing bug. I am happy to follow-up on that!

+1. Was mentioning for a followup. Thanks for doing that!

kouhei (in TOK)

https://codereview.chromium.org/696703005/diff/60001/Source/core/rendering/RenderObject.cpp File Source/core/rendering/RenderObject.cpp (right): https://codereview.chromium.org/696703005/diff/60001/Source/core/rendering/RenderObject.cpp#newcode3011 Source/core/rendering/RenderObject.cpp:3011: const RenderBlock* containingBlock = this->containingBlock(); On 2014/11/13 01:09:32, Julien ...

6 years, 1 month ago (2014-11-13 01:33:38 UTC) #9

Julien - ping for review

6 years, 1 month ago (2014-11-13 17:03:42 UTC) #10

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/696703005/100001

6 years, 1 month ago (2014-11-13 17:13:20 UTC) #12

commit-bot: I haz the power

Committed patchset #6 (id:100001) as 185307

6 years, 1 month ago (2014-11-13 18:24:34 UTC) #13

kouhei (in TOK)

6 years, 1 month ago (2014-11-14 03:11:57 UTC) #14

Message was sent while issue was closed.

https://codereview.chromium.org/696703005/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/696703005/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:3011: const RenderBlock* containingBlock
= this->containingBlock();
On 2014/11/13 17:03:42, Julien Chaffraix - PST wrote:
> On 2014/11/13 01:33:37, kouhei wrote:
> > On 2014/11/13 01:09:32, Julien Chaffraix - PST wrote:
> > > On 2014/11/12 23:41:42, leviw wrote:
> > > > Nit: Is this-> needed?
> > > 
> > > It should be: the variable has the same name as the function so C++ (not
> sure
> > > about C++11) required the this-> to disambiguate the 2.
> > 
> > I think this code is a bit tricky.
> > Would you rename the variable to something else so we can avoid "this->"?
> 
> I don't agree. The code is super straight-forward. The variable name is
explicit
> and is just a cache of the function's return value (which is why it mirrors
the
> function name). Finally this is also a fairly common pattern in the code.

Got it. lgtm

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages