Clear highlight when the highlighted node gets removed from the document

Clear highlight when the highlighted node gets removed from the document

Some web pages use a mask element to intercept tap gesture and then hide
the element immediately. The orphaned highlight can be very annoying.
For example, pages that show a popup often use a full screen transparent
element to intercept events, and will hide the popup if the user attempt
to tap on the main document. If we don't remove highlight from the hidden
element, the whole screen will be covered by a highlight.

This patch clears highlight from a layer when the tap node is no longer valid.

TEST=webkit_unit_tests:LinkHighlightTest.resetDuringNodeRemoval
BUG=232926
R=jamesr@chromium.org

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

[trchen, 2013-05-03 00:04:55]

Hello James,
Do you have time to take a look at this patch please? This bug targets M27
stable. Thanks!

[jamesr, 2013-05-03 00:12:11]

Woah, this seems dangerous.  The link highlight stuff really shouldn't have to
be tied into core WebCore Node lifetime this tightly.

If the node was removed, it should invalidate the composited layer it was
rendered into which is also the LinkHighlight's parent layer.  That should call
LinkHighlight::invalidate() which should in turn make sure that on the next
layout we recompute the highlight path.  We should notice the Node is gone at
that point.  Why isn't that working as intended?

[trchen, 2013-05-03 02:16:42]

Good point. Here comes with a much simpler version. PTAL! :)

On 2013/05/03 00:12:11, jamesr wrote:
> Woah, this seems dangerous.  The link highlight stuff really shouldn't have to
> be tied into core WebCore Node lifetime this tightly.
> 
> If the node was removed, it should invalidate the composited layer it was
> rendered into which is also the LinkHighlight's parent layer.  That should
call
> LinkHighlight::invalidate() which should in turn make sure that on the next
> layout we recompute the highlight path.  We should notice the Node is gone at
> that point.  Why isn't that working as intended?

[wjmaclean, 2013-05-03 12:48:46]

On 2013/05/03 00:12:11, jamesr wrote:
> Woah, this seems dangerous.  The link highlight stuff really shouldn't have to
> be tied into core WebCore Node lifetime this tightly.
> 
> If the node was removed, it should invalidate the composited layer it was
> rendered into which is also the LinkHighlight's parent layer.  That should
call
> LinkHighlight::invalidate() which should in turn make sure that on the next
> layout we recompute the highlight path.  We should notice the Node is gone at
> that point.  Why isn't that working as intended?

LinkHighlight holds a refptr to the node, so we need a test to determine if the
node has been removed, perhaps checking the node's ownerDocument() or
renderer().

This test can happen in LinkHighlight::invalidate()  or updateGeometry() then we
can release the node and the other resources held by the highlight.

[trchen, 2013-05-03 21:10:27]

On 2013/05/03 12:48:46, wjmaclean wrote:
> On 2013/05/03 00:12:11, jamesr wrote:
> > Woah, this seems dangerous.  The link highlight stuff really shouldn't have
to
> > be tied into core WebCore Node lifetime this tightly.
> > 
> > If the node was removed, it should invalidate the composited layer it was
> > rendered into which is also the LinkHighlight's parent layer.  That should
> call
> > LinkHighlight::invalidate() which should in turn make sure that on the next
> > layout we recompute the highlight path.  We should notice the Node is gone
at
> > that point.  Why isn't that working as intended?
> 
> LinkHighlight holds a refptr to the node, so we need a test to determine if
the
> node has been removed, perhaps checking the node's ownerDocument() or
> renderer().
> 
> This test can happen in LinkHighlight::invalidate()  or updateGeometry() then
we
> can release the node and the other resources held by the highlight.

Yep, it makes sense to release the node too. A new patch has been uploaded.

[trchen, 2013-05-06 21:04:48]

Hello (both) James, does the patch look good to you? Thanks!

On 2013/05/03 00:12:11, jamesr wrote:
> Woah, this seems dangerous.  The link highlight stuff really shouldn't have to
> be tied into core WebCore Node lifetime this tightly.
> 
> If the node was removed, it should invalidate the composited layer it was
> rendered into which is also the LinkHighlight's parent layer.  That should
call
> LinkHighlight::invalidate() which should in turn make sure that on the next
> layout we recompute the highlight path.  We should notice the Node is gone at
> that point.  Why isn't that working as intended?

It was a bug. The code early exited without clearing existing highlight path and
layers. The new patch removes the highlight pointer from the layer.

[jamesr, 2013-05-06 21:06:51]

lgtm for this James. Adding the other James to the reviewer list

https://codereview.chromium.org/14582012/diff/9001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/LinkHighlight.h (right):

https://codereview.chromium.org/14582012/diff/9001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/LinkHighlight.h:73: WebCore::GraphicsLayerChromium*
currentGraphicsLayer() const { return m_currentGraphicsLayer; }
could you suffix the function name with 'ForTesting'? then you don't need the
command and it's clear in all code that this is meant for testing

[wjmaclean, 2013-05-06 21:43:01]

lgtm

[commit-bot: I haz the power, 2013-05-06 21:58:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/14582012/16002

[commit-bot: I haz the power, 2013-05-06 22:35:30]

Change committed as 149814