Bug 35900

Summary: [Chromium] Need to send mouse events to plugin when it has mouse capture
Product: WebKit Reporter: John Abd-El-Malek <jam>
Component: PlatformAssignee: John Abd-El-Malek <jam>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dglazkov, fishd, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed patch
none
Added missing file
none
Updated patch fishd: review+, fishd: commit-queue-

Description John Abd-El-Malek 2010-03-08 19:25:27 PST
This would allow a plugin to implement scrollbars in its region, for example.
Comment 1 John Abd-El-Malek 2010-03-08 19:32:24 PST
Created attachment 50270 [details]
Proposed patch

I don't like how I can't set m_haveMouseCapture to false in mouseCaptureLost(), any suggestions?
Comment 2 WebKit Review Bot 2010-03-08 20:33:24 PST
Attachment 50270 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/453001
Comment 3 John Abd-El-Malek 2010-03-08 20:38:32 PST
Created attachment 50276 [details]
Added missing file
Comment 4 Darin Fisher (:fishd, Google) 2010-03-08 22:58:01 PST
What if WebWidget::mouseCaptureLost() is called?  See the callsite in chrome/renderer/render_widget.cc.
Comment 5 John Abd-El-Malek 2010-03-09 14:03:29 PST
Right, that's my worry, that the window loses mouse capture without a right mouse button up event.  Is it possible though?
Comment 6 John Abd-El-Malek 2010-03-09 14:27:37 PST
Created attachment 50350 [details]
Updated patch

Per Darin's advice, reversed the order that the browser sends the events (http://codereview.chromium.org/743003).  This makes this change cleaner.
Comment 7 Darin Fisher (:fishd, Google) 2010-03-09 16:12:30 PST
Comment on attachment 50350 [details]
Updated patch

> Index: WebKit/chromium/src/WebViewImpl.cpp
...
> +    if (m_haveMouseCapture && WebInputEvent::isMouseEventType(inputEvent.type)) {
> +        Node* node = focusedWebCoreNode();
> +        if (node && node->renderer() && node->renderer()->isEmbeddedObject()) {
> +          AtomicString eventType;
> +            switch (inputEvent.type) {

nit: indentation of AtomicString is off.


>  void WebViewImpl::mouseCaptureLost()
>  {
> +    m_haveMouseCapture = false;
>  }

We may eventually wish to pass this event along to the plugin.

Are there any compatibility concerns with enabling this patch for ordinary plugins?
Comment 8 John Abd-El-Malek 2010-03-09 17:39:17 PST
My hunch is that there shouldn't be any issue with passing this to NPAPI plugins.  Basic testing with Flash hasn't shown any problems, but pushing this out will tell us conclusively.
Comment 9 John Abd-El-Malek 2010-03-09 17:45:22 PST
Committed r55758
Comment 10 Darin Fisher (:fishd, Google) 2010-03-09 21:08:19 PST
(In reply to comment #8)
> My hunch is that there shouldn't be any issue with passing this to NPAPI
> plugins.  Basic testing with Flash hasn't shown any problems, but pushing this
> out will tell us conclusively.

OK, sounds good.