Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2290523004:
Move onMouseDown from WebWidgetClient to WebViewClient. (Closed)
DescriptionMove onMouseDown from WebWidgetClient to WebViewClient.
This is part of a refactor to split WebView/WebWidget.
BUG=583347
Committed: https://crrev.com/e7bd6c5ed1ac6f9bcbdf9b88f9f640c8d384b8f3
Cr-Commit-Position: refs/heads/master@{#418131}
Patch Set 1 #Patch Set 2 : adding todo #
Messages
Total messages: 23 (14 generated)
Description was changed from ========== WIP ========== to ========== Move onMouseDown from WebWidgetClient to WebViewClient. This is part of a refactor to split WebView/WebWidget. BUG=583347 ==========
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lfg@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng, please take a look.
Can you give me some background on why this should be part of view, not widget? At first glance, it seems widget-related, since it's input?
On 2016/08/31 17:58:54, dcheng wrote: > Can you give me some background on why this should be part of view, not widget? > At first glance, it seems widget-related, since it's input? I agree that onMouseDown seems like a widget function, however, we also pass a WebNode to the client, which I don't think belongs to RenderWidget, since it has no knowledge of the DOM. Overall, this is only used to implement PageClickTracker, and the overall plumbing of this data seems poorly designed in my opinion.
Meh. LGTM, I guess. We should probably add a TODO: it looks like this is exposed in RenderViewObserver, which we'd like to get rid of anyway... Hopefully once onion soup is a bit further along, we can figure out a better way to plumb this for autofill, since it's literally the only RenderViewObserver that cares about this.
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
On 2016/09/01 16:50:58, dcheng wrote: > Meh. LGTM, I guess. We should probably add a TODO: it looks like this is exposed > in RenderViewObserver, which we'd like to get rid of anyway... Done, please take another look.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/31 17:58:54, dcheng wrote: > Can you give me some background on why this should be part of view, not widget? > At first glance, it seems widget-related, since it's input? This is more like a (slightly hacky) way for components to register event listeners on a node, which isn't really something a widget needs to be a part of. It doesn't matter that much since as you said RenderViewObserver will go away anyway, I'm just commenting that the change itself is sound.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lfg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2290523004/#ps20001 (title: "adding todo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Move onMouseDown from WebWidgetClient to WebViewClient. This is part of a refactor to split WebView/WebWidget. BUG=583347 ========== to ========== Move onMouseDown from WebWidgetClient to WebViewClient. This is part of a refactor to split WebView/WebWidget. BUG=583347 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Move onMouseDown from WebWidgetClient to WebViewClient. This is part of a refactor to split WebView/WebWidget. BUG=583347 ========== to ========== Move onMouseDown from WebWidgetClient to WebViewClient. This is part of a refactor to split WebView/WebWidget. BUG=583347 Committed: https://crrev.com/e7bd6c5ed1ac6f9bcbdf9b88f9f640c8d384b8f3 Cr-Commit-Position: refs/heads/master@{#418131} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e7bd6c5ed1ac6f9bcbdf9b88f9f640c8d384b8f3 Cr-Commit-Position: refs/heads/master@{#418131} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
