In PageClickTracker, only store last clicked node if it's significant.

In PageClickTracker:
As soon as we start to handle an event, filter against the type of
nodes we care about (only text fields), rather than waiting to filter
in DidHandleMouseEvent().

Otherwise, PageClickTracker could hold a reference to the node until the
next mouse event, potentially keeping the node alive for longer than
necessary.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86759

[dpanpong, 2011-05-24 01:21:40]

Hi Jay,

Would you be able to take a look at this?

Thanks!

http://codereview.chromium.org/7064012/diff/1/chrome/renderer/page_click_trac...
File chrome/renderer/page_click_tracker.cc (right):

http://codereview.chromium.org/7064012/diff/1/chrome/renderer/page_click_trac...
chrome/renderer/page_click_tracker.cc:34: if (!node.isElementNode())
The logic in this function moved from DidHandleMouseEvent().  I changed the
take-by-reference variables to simple variables, because toConst() returns a
temporary, and I became a bit concerned about handling a reference to a
temporary.

[Jay Civelli, 2011-05-24 04:47:25]

This looks good, just a suggestion below.

http://codereview.chromium.org/7064012/diff/1/chrome/renderer/page_click_trac...
File chrome/renderer/page_click_tracker.cc (right):

http://codereview.chromium.org/7064012/diff/1/chrome/renderer/page_click_trac...
chrome/renderer/page_click_tracker.cc:32: // Returns whether |node| is a text
input element.
I wonder if we need this method. May be GetTextWebInputElement could return an
empty WebInputElement if the node is not a text webinputelement?

[dpanpong, 2011-05-24 17:10:26]

Thanks for the review, Jay!

If it looks good, could you send it to the try server for me?

Thanks,
Dave

http://codereview.chromium.org/7064012/diff/1/chrome/renderer/page_click_trac...
File chrome/renderer/page_click_tracker.cc (right):

http://codereview.chromium.org/7064012/diff/1/chrome/renderer/page_click_trac...
chrome/renderer/page_click_tracker.cc:32: // Returns whether |node| is a text
input element.
On 2011/05/24 04:47:25, Jay Civelli wrote:
> I wonder if we need this method. May be GetTextWebInputElement could return an
> empty WebInputElement if the node is not a text webinputelement?

I like that much better.  Done.

[commit-bot: I haz the power, 2011-05-24 23:27:16]

No LGTM from valid reviewers yet.

[Jay Civelli, 2011-05-24 23:27:43]

LGTM

[commit-bot: I haz the power, 2011-05-25 01:06:46]

Try job failure for 7064012-4001 on mac:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[dpanpong, 2011-05-25 17:09:49]

On 2011/05/25 01:06:46, commit-bot wrote:
> Try job failure for 7064012-4001 on mac:
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
The job failure appears to be unrelated to this change.

It looks to me like the first few changes below might be the culprit:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/cocoa/bookm...

[commit-bot: I haz the power, 2011-05-26 01:09:49]

Change committed as 86759