Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(427)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1195473005: Let PageClickTracker use onMouseDown (Closed)

Created:
5 years, 6 months ago by Yufeng Shen (Slow to review)
Modified:
5 years, 6 months ago
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, browser-components-watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org, Evan Stade, Rick Byers, esprehn
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Let PageClickTracker use onMouseDown PageClickTracker listens on mouse press and gesture tap event to get the information of the last clicked node. Currently it does an extra call of hit test on the mouse/tap event to get the click node after the event has been processed in blink. This is wasteful since the event processing part has already done the hit test and known the clicked node. So lets get rid of this extra call of hit testing. In blink at the end of mouse press/tap event processing, the clicked node information is passed out to chrome clients through WebWidgetClient::onMouseDown(const WebNode&) and PageClickTracker can just listen to that. BUG=498150 Committed: https://crrev.com/e8d28bafe7b5aff68cbb1d6b4fa8745bc3ea430c Cr-Commit-Position: refs/heads/master@{#335210}

Patch Set 1 #

Patch Set 2 : add test for right click #

Patch Set 3 : change name onCLickInput to onMouseDown #

Total comments: 2

Patch Set 4 : address comments #

Total comments: 4

Patch Set 5 : address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+58 lines, -45 lines) Patch
M chrome/renderer/autofill/page_click_tracker_browsertest.cc View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download
M components/autofill/content/renderer/page_click_tracker.h View 1 2 3 4 2 chunks +2 lines, -4 lines 0 comments Download
M components/autofill/content/renderer/page_click_tracker.cc View 1 2 3 2 chunks +5 lines, -29 lines 0 comments Download
M content/public/renderer/render_view_observer.h View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M content/public/test/render_view_test.h View 1 1 chunk +8 lines, -0 lines 0 comments Download
M content/public/test/render_view_test.cc View 1 1 chunk +24 lines, -0 lines 0 comments Download
M content/renderer/render_view_impl.h View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M content/renderer/render_view_impl.cc View 1 2 3 2 chunks +5 lines, -4 lines 0 comments Download
M content/renderer/render_widget.h View 1 2 1 chunk +0 lines, -4 lines 0 comments Download
M content/renderer/render_widget.cc View 1 2 1 chunk +0 lines, -2 lines 0 comments Download

Messages

Total messages: 18 (5 generated)
Yufeng Shen (Slow to review)
5 years, 6 months ago (2015-06-17 19:48:24 UTC) #2
Yufeng Shen (Slow to review)
On 2015/06/17 19:48:24, Yufeng Shen wrote: blink side change is here https://codereview.chromium.org/1176423008/
5 years, 6 months ago (2015-06-17 19:52:54 UTC) #3
please use gerrit instead
Can we keep the test in page_click_tracker_browsertest.cc to verify that right mouse button does not ...
5 years, 6 months ago (2015-06-17 20:10:09 UTC) #4
Yufeng Shen (Slow to review)
On 2015/06/17 20:10:09, Rouslan wrote: > Can we keep the test in page_click_tracker_browsertest.cc to verify ...
5 years, 6 months ago (2015-06-17 20:40:19 UTC) #5
please use gerrit instead
page_click_tracker lgtm
5 years, 6 months ago (2015-06-17 21:57:37 UTC) #6
Yufeng Shen (Slow to review)
OWNERS added: +sievers@ for content/* isherman@ for */autofill/*
5 years, 6 months ago (2015-06-18 19:21:55 UTC) #9
no sievers
content lgtm w/nit https://codereview.chromium.org/1195473005/diff/40001/content/public/renderer/render_view_observer.h File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/1195473005/diff/40001/content/public/renderer/render_view_observer.h#newcode85 content/public/renderer/render_view_observer.h:85: virtual void onMouseDown(const blink::WebNode& mouse_down_node) {} ...
5 years, 6 months ago (2015-06-18 20:36:15 UTC) #10
Yufeng Shen (Slow to review)
https://codereview.chromium.org/1195473005/diff/40001/content/public/renderer/render_view_observer.h File content/public/renderer/render_view_observer.h (right): https://codereview.chromium.org/1195473005/diff/40001/content/public/renderer/render_view_observer.h#newcode85 content/public/renderer/render_view_observer.h:85: virtual void onMouseDown(const blink::WebNode& mouse_down_node) {} On 2015/06/18 20:36:15, ...
5 years, 6 months ago (2015-06-18 20:49:39 UTC) #11
Ilya Sherman
LGTM. Thanks for the cleanup! https://codereview.chromium.org/1195473005/diff/60001/chrome/renderer/autofill/page_click_tracker_browsertest.cc File chrome/renderer/autofill/page_click_tracker_browsertest.cc (right): https://codereview.chromium.org/1195473005/diff/60001/chrome/renderer/autofill/page_click_tracker_browsertest.cc#newcode124 chrome/renderer/autofill/page_click_tracker_browsertest.cc:124: EXPECT_FALSE(text_ == test_listener_.form_control_element_clicked_); nit: ...
5 years, 6 months ago (2015-06-18 21:58:16 UTC) #12
Yufeng Shen (Slow to review)
https://codereview.chromium.org/1195473005/diff/60001/chrome/renderer/autofill/page_click_tracker_browsertest.cc File chrome/renderer/autofill/page_click_tracker_browsertest.cc (right): https://codereview.chromium.org/1195473005/diff/60001/chrome/renderer/autofill/page_click_tracker_browsertest.cc#newcode124 chrome/renderer/autofill/page_click_tracker_browsertest.cc:124: EXPECT_FALSE(text_ == test_listener_.form_control_element_clicked_); On 2015/06/18 21:58:16, Ilya Sherman wrote: ...
5 years, 6 months ago (2015-06-19 00:08:32 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1195473005/80001
5 years, 6 months ago (2015-06-19 04:00:14 UTC) #16
commit-bot: I haz the power
Committed patchset #5 (id:80001)
5 years, 6 months ago (2015-06-19 04:52:14 UTC) #17
commit-bot: I haz the power
5 years, 6 months ago (2015-06-19 04:53:43 UTC) #18
Message was sent while issue was closed. 
Patchset 5 (id:??) landed as
https://crrev.com/e8d28bafe7b5aff68cbb1d6b4fa8745bc3ea430c
Cr-Commit-Position: refs/heads/master@{#335210}

Powered by Google App Engine
This is Rietveld 408576698