Bug 60563 – [Chromium] Click event is not fired for a menulist <select>

• Status: RESOLVED
• Resolution: WONTFIX
• Component: UI Events

[Naoki Takano]

Popup window consumes the mouse up event and the event doesn't reach to the <select> node.
That is the reason why the even is not fired.

Actually, not only click but also mouseup isn't fired.

The same problem is in WebKit2 here
https://bugs.webkit.org/show_bug.cgi?id=57904

That solution was to send mouse event directly (e.g. using PostMessage() WIn32 API) but for Chromium, I'd rather like to use call API directly to the target <select> node.

[Naoki Takano]

Created attachment 93065 [details]
Try to forward mouse button event if it is select popup element.

The current my wekit-patch command is broken, so I upload my patch manually.

Please review it.

[Naoki Takano]

The bug is related to
http://code.google.com/p/chromium/issues/detail?id=54442
http://code.google.com/p/chromium/issues/detail?id=79815

Please review.

[Dimitri Glazkov (Google)]

Comment on attachment 93065 [details]
Try to forward mouse button event if it is select popup element.

This seems like the wrong approach. From the perspective of WebCore, there shouldn't be a difference in how ports dispatch events.

[Naoki Takano]

Ok,

So could you tell me what approach we need?
Do we need to send mouse event directly?

Thanks,

(In reply to comment #3)
> (From update of attachment 93065 [details])
> This seems like the wrong approach. From the perspective of WebCore, there shouldn't be a difference in how ports dispatch events.

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> Ok,
> 
> So could you tell me what approach we need?
> Do we need to send mouse event directly?

I am not sure. Can we forward the mouse up from the popup? This way we don't need to fire simulated clicks.

[Naoki Takano]

Actually, WebKit2 has the same problem, see
https://bugs.webkit.org/show_bug.cgi?id=57904

Their solution is to send native mouse event to the browser.

But I don't like this way;-(

What do you think?

(In reply to comment #5)
> (In reply to comment #4)
> > Ok,
> > 
> > So could you tell me what approach we need?
> > Do we need to send mouse event directly?
> 
> I am not sure. Can we forward the mouse up from the popup? This way we don't need to fire simulated clicks.

[Dimitri Glazkov (Google)]

(In reply to comment #6)
> Actually, WebKit2 has the same problem, see
> https://bugs.webkit.org/show_bug.cgi?id=57904
> 
> Their solution is to send native mouse event to the browser.
> 
> But I don't like this way;-(
> 
> What do you think?

Can you explain why you don't like it? Forwarding a PlatformMouseEvent from the popup seems like the right thing to do.

> 
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Ok,
> > > 
> > > So could you tell me what approach we need?
> > > Do we need to send mouse event directly?
> > 
> > I am not sure. Can we forward the mouse up from the popup? This way we don't need to fire simulated clicks.

[Naoki Takano]

My intention is not that I don't like to use Windows or Mac specific calls. As you see, they use ::PostMessage(), right?

But if we can use PlatformMouseEvent, it might not use Native calls.

I'll try it.

(In reply to comment #7)
> (In reply to comment #6)
> > Actually, WebKit2 has the same problem, see
> > https://bugs.webkit.org/show_bug.cgi?id=57904
> > 
> > Their solution is to send native mouse event to the browser.
> > 
> > But I don't like this way;-(
> > 
> > What do you think?
> 
> Can you explain why you don't like it? Forwarding a PlatformMouseEvent from the popup seems like the right thing to do.
> 
> > 
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > Ok,
> > > > 
> > > > So could you tell me what approach we need?
> > > > Do we need to send mouse event directly?
> > > 
> > > I am not sure. Can we forward the mouse up from the popup? This way we don't need to fire simulated clicks.

[Naoki Takano]

Hmmm... There is the function, Node::dispatchMouseEvent(), so maybe I can use the function to forward the mouse up event to the node.

Thanks,

(In reply to comment #8)
> My intention is not that I don't like to use Windows or Mac specific calls. As you see, they use ::PostMessage(), right?
> 
> But if we can use PlatformMouseEvent, it might not use Native calls.
> 
> I'll try it.
> 
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Actually, WebKit2 has the same problem, see
> > > https://bugs.webkit.org/show_bug.cgi?id=57904
> > > 
> > > Their solution is to send native mouse event to the browser.
> > > 
> > > But I don't like this way;-(
> > > 
> > > What do you think?
> > 
> > Can you explain why you don't like it? Forwarding a PlatformMouseEvent from the popup seems like the right thing to do.
> > 
> > > 
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > Ok,
> > > > > 
> > > > > So could you tell me what approach we need?
> > > > > Do we need to send mouse event directly?
> > > > 
> > > > I am not sure. Can we forward the mouse up from the popup? This way we don't need to fire simulated clicks.

[Naoki Takano]

Created attachment 93236 [details]
Use disptachMouseEvent version

Please review again.

I believe this version is much better.

Thanks,

[Dimitri Glazkov (Google)]

Comment on attachment 93236 [details]
Use disptachMouseEvent version

View in context: https://bugs.webkit.org/attachment.cgi?id=93236&action=review

I think you're almost there. For the most natural way, try handing this off to eventHandler::handleMouseRelease in AutoFillPopuMenuClient?

> Source/WebCore/rendering/RenderMenuList.cpp:314
> +    node()->dispatchMouseEvent(event, eventNames().mouseupEvent);
> +    node()->dispatchMouseEvent(event, eventNames().clickEvent);

This logic is probably a bit more complicated than this. When you mouse down on a menu, then move the mouse off the menu, should the click still be dispatched? That's not how normal DOM works. See http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/page/EventHandler.cpp&l=1666&exact_package=chromium. Perhaps you should just hand this to EventHandler?

> Source/WebCore/rendering/RenderTextControlSingleLine.h:118
> +    virtual void dispatchMouseUpAndClick(const PlatformMouseEvent&) { }

This is definitely wrong. We shouldn't dispatch events from RenderObjects.

[Naoki Takano]

Thank you for your review.

(In reply to comment #11)
> (From update of attachment 93236 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93236&action=review
> 
> I think you're almost there. For the most natural way, try handing this off to eventHandler::handleMouseRelease in AutoFillPopuMenuClient?

I want to make sure what you are saying.

Why do we need to forward the event to eventHandler::handleMouseRelease in AutoFillPopuMenuClient? I guess this is only for select tag event handler and we should not trigger the event in autofill popup.

> 
> > Source/WebCore/rendering/RenderMenuList.cpp:314
> > +    node()->dispatchMouseEvent(event, eventNames().mouseupEvent);
> > +    node()->dispatchMouseEvent(event, eventNames().clickEvent);
> 
> This logic is probably a bit more complicated than this. When you mouse down on a menu, then move the mouse off the menu, should the click still be dispatched? That's not how normal DOM works. See http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/page/EventHandler.cpp&l=1666&exact_package=chromium. Perhaps you should just hand this to EventHandler?

I see, I try it.

> 
> > Source/WebCore/rendering/RenderTextControlSingleLine.h:118
> > +    virtual void dispatchMouseUpAndClick(const PlatformMouseEvent&) { }
> 
> This is definitely wrong. We shouldn't dispatch events from RenderObjects.
As you see, I don't do anything for the object. dispatchMouseUpAndClick() is pure virtual function, so we need to implement something here.

Or don't we declare the function as pure virtual function?

Thanks

[Dimitri Glazkov (Google)]

Comment on attachment 93236 [details]
Use disptachMouseEvent version

View in context: https://bugs.webkit.org/attachment.cgi?id=93236&action=review

>>> Source/WebCore/rendering/RenderTextControlSingleLine.h:118
>>> +    virtual void dispatchMouseUpAndClick(const PlatformMouseEvent&) { }
>> 
>> This is definitely wrong. We shouldn't dispatch events from RenderObjects.
> 
> As you see, I don't do anything for the object. dispatchMouseUpAndClick() is pure virtual function, so we need to implement something here.
> 
> Or don't we declare the function as pure virtual function?
> 
> Thanks

Dispatching DOM events from a RenderObject is just a perverse concept :) RenderTrees and DOM trees are two separate things.

[Naoki Takano]

Ok, I got your point.

But from the event handled class(here PopupListBox), we only can use PopupMenuClient class as a channel to forward the event.

That's why I added the function, dispatchMouseUpAndClick() in PopupMenuClient class.

So is it Ok to change the name to forwardMouseUpAndClick()?

Or do you mean to drag other information like event handler into PopupListBox to forward the event?

If you have any advice or comment, I would appreciate.

Thanks,


(In reply to comment #13)
> (From update of attachment 93236 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93236&action=review
> 
> >>> Source/WebCore/rendering/RenderTextControlSingleLine.h:118
> >>> +    virtual void dispatchMouseUpAndClick(const PlatformMouseEvent&) { }
> >> 
> >> This is definitely wrong. We shouldn't dispatch events from RenderObjects.

[Naoki Takano]

Created attachment 93407 [details]
EventHandler version

According to your suggestion, I changed to use EventHandler to forward the event.

Actually, just forwarding doesn't work well because of coordinate issue.

As you know, user click the item in select option window, the mouse point is out of bound from the actual select widget.
The event hander isn't fired correctly.

So, inspired by WebKit2 solution, I stored the mouse button down position first, and use the location when the up.

I left dispatchMouseUpAndClick() in AutoFillPopupMenuClient.h, because I didn't get your answer.

Please review again.

Thanks,

[Dimitri Glazkov (Google)]

(In reply to comment #15)
> Created an attachment (id=93407) [details]
> EventHandler version
> 
> According to your suggestion, I changed to use EventHandler to forward the event.
> 
> Actually, just forwarding doesn't work well because of coordinate issue.
> 
> As you know, user click the item in select option window, the mouse point is out of bound from the actual select widget.
> The event hander isn't fired correctly.
> 
> So, inspired by WebKit2 solution, I stored the mouse button down position first, and use the location when the up.
> 
> I left dispatchMouseUpAndClick() in AutoFillPopupMenuClient.h, because I didn't get your answer.
> 
> Please review again.
> 
> Thanks,

This patch looks even worse than the others :(

I am sorry that this is taking so many roundtrips. I don't know the code around popups well enough to offer the solution directly, but I can see a problem with existing code. In short, it's:

Don't reach into WebCore to make stuff in WebKit layer work. 

The long version: See how the patch in bug 57904 doesn't make any changes in WebCore code?  That's because they respect the layering boundaries and realize that even though it is more work to fix this particular issue, the WebCore code should work the same way for _all_ ports. What you're proposing is hooks in WebCore that are only relevant for Chromium. I assert that this is unnecessary.

I actually rather like the solution in bug 57904 and find it very rational. Co-opting the mousedown event seems reasonable and logical from the perspective of WebCore.

Hope this helps.

[Dimitri Glazkov (Google)]

Also, the manual test is using alerts, which is a bit disruptive for the testers. How about something like this:

http://jsfiddle.net/FXBgG/

[Naoki Takano]

Thank you for your review so many times!!
I appreciate your review even if so many round trips;-)
Now I understand more clearly what you are talking about.

AFAIK, in chromium, we use PopupMenuChromium.cpp to handle events.
But it doesn't know the parent window. That's why I forward the event to WebCore.

On the other hand, PopupMenuWin.cpp and WebKit2 have there parent window handles.
That's why they can simulate the mouse click natively.

So I guess, if we cannot touch WebCore part, we should also have to have parent handle in PopupMenuChromium.cpp. Of course the handle should be Chromium specific handle, not native HWND or GTK handles.

But still I'm still not sure how we can simulate mouse event between Chromium and WebKit.


>Kent-san,

Do you have any idea about that, if you know?

Thanks,



(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=93407) [details] [details]
> > EventHandler version
> > 
> > According to your suggestion, I changed to use EventHandler to forward the event.
> > 
> > Actually, just forwarding doesn't work well because of coordinate issue.
> > 
> > As you know, user click the item in select option window, the mouse point is out of bound from the actual select widget.
> > The event hander isn't fired correctly.
> > 
> > So, inspired by WebKit2 solution, I stored the mouse button down position first, and use the location when the up.
> > 
> > I left dispatchMouseUpAndClick() in AutoFillPopupMenuClient.h, because I didn't get your answer.
> > 
> > Please review again.
> > 
> > Thanks,
> 
> This patch looks even worse than the others :(
> 
> I am sorry that this is taking so many roundtrips. I don't know the code around popups well enough to offer the solution directly, but I can see a problem with existing code. In short, it's:
> 
> Don't reach into WebCore to make stuff in WebKit layer work. 
> 
> The long version: See how the patch in bug 57904 doesn't make any changes in WebCore code?  That's because they respect the layering boundaries and realize that even though it is more work to fix this particular issue, the WebCore code should work the same way for _all_ ports. What you're proposing is hooks in WebCore that are only relevant for Chromium. I assert that this is unnecessary.
> 
> I actually rather like the solution in bug 57904 and find it very rational. Co-opting the mousedown event seems reasonable and logical from the perspective of WebCore.
> 
> Hope this helps.

[Naoki Takano]

After looking around the source code, I guess render_view in Chromium code is better choice to forward mouse up event.

But I'm stuck I cannot forward the event well.

Dimitri,
Are you familiar with render_view message routing in Chromium?

Or do you know who is the right person to ask?

Thanks,

(In reply to comment #18)
> Thank you for your review so many times!!
> I appreciate your review even if so many round trips;-)
> Now I understand more clearly what you are talking about.
> 
> AFAIK, in chromium, we use PopupMenuChromium.cpp to handle events.
> But it doesn't know the parent window. That's why I forward the event to WebCore.
> 
> On the other hand, PopupMenuWin.cpp and WebKit2 have there parent window handles.
> That's why they can simulate the mouse click natively.
> 
> So I guess, if we cannot touch WebCore part, we should also have to have parent handle in PopupMenuChromium.cpp. Of course the handle should be Chromium specific handle, not native HWND or GTK handles.
> 
> But still I'm still not sure how we can simulate mouse event between Chromium and WebKit.
> 
> 
> >Kent-san,
> 
> Do you have any idea about that, if you know?
> 
> Thanks,
> 
> 
> 
> (In reply to comment #16)
> > (In reply to comment #15)
> > > Created an attachment (id=93407) [details] [details] [details]
> > > EventHandler version
> > > 
> > > According to your suggestion, I changed to use EventHandler to forward the event.
> > > 
> > > Actually, just forwarding doesn't work well because of coordinate issue.
> > > 
> > > As you know, user click the item in select option window, the mouse point is out of bound from the actual select widget.
> > > The event hander isn't fired correctly.
> > > 
> > > So, inspired by WebKit2 solution, I stored the mouse button down position first, and use the location when the up.
> > > 
> > > I left dispatchMouseUpAndClick() in AutoFillPopupMenuClient.h, because I didn't get your answer.
> > > 
> > > Please review again.
> > > 
> > > Thanks,
> > 
> > This patch looks even worse than the others :(
> > 
> > I am sorry that this is taking so many roundtrips. I don't know the code around popups well enough to offer the solution directly, but I can see a problem with existing code. In short, it's:
> > 
> > Don't reach into WebCore to make stuff in WebKit layer work. 
> > 
> > The long version: See how the patch in bug 57904 doesn't make any changes in WebCore code?  That's because they respect the layering boundaries and realize that even though it is more work to fix this particular issue, the WebCore code should work the same way for _all_ ports. What you're proposing is hooks in WebCore that are only relevant for Chromium. I assert that this is unnecessary.
> > 
> > I actually rather like the solution in bug 57904 and find it very rational. Co-opting the mousedown event seems reasonable and logical from the perspective of WebCore.
> > 
> > Hope this helps.

[Jay Civelli]

From what I understand, the external popup is consuming the mouse up and down events so they dont have a chance to be sent to the renderer.


I think one way to do it would be to:
- On the Chrome side, change the ViewMsg_SelectPopupMenuItem message (see content/common/view_messages.h) to take an extra parameter which is the click coordinates of the location clicked (probably translated to the page coordinates) when the selection occurred (could be -1, -1 to mean no click triggered the selection). That message is sent by the browser when a popup menu item is clicked from chrome/browser/tab_contents/popup_menu_helper_mac.mm, that would also have to be changed to include that click info. The render view would then pass that click along to the WebKit side.
- On the WebKit Chromium API side the select popup interfaces would have to be modified to take in that new parameter, and the WebViewImpl or ExternalPopupMenu would create and send a click event to the select.

Now that I think of it, another way that would probably be much easier woud be to forward the mouse up event from the external popup menu to the the render widget host view (after translating the coordinates as necessary). That would probably happen in  chrome/browser/tab_contents/popup_menu_helper_mac.mm. That solution is probably better and would only affect Chromium code.

[Naoki Takano]

Jay,

Thank you for your good suggestion!!

I'll try to do it.

For me, Linux is dev environment, so I'll compare between Mac and Linux code.

Thanks,

(In reply to comment #20)
> From what I understand, the external popup is consuming the mouse up and down events so they dont have a chance to be sent to the renderer.
> 
> 
> I think one way to do it would be to:
> - On the Chrome side, change the ViewMsg_SelectPopupMenuItem message (see content/common/view_messages.h) to take an extra parameter which is the click coordinates of the location clicked (probably translated to the page coordinates) when the selection occurred (could be -1, -1 to mean no click triggered the selection). That message is sent by the browser when a popup menu item is clicked from chrome/browser/tab_contents/popup_menu_helper_mac.mm, that would also have to be changed to include that click info. The render view would then pass that click along to the WebKit side.
> - On the WebKit Chromium API side the select popup interfaces would have to be modified to take in that new parameter, and the WebViewImpl or ExternalPopupMenu would create and send a click event to the select.
> 
> Now that I think of it, another way that would probably be much easier woud be to forward the mouse up event from the external popup menu to the the render widget host view (after translating the coordinates as necessary). That would probably happen in  chrome/browser/tab_contents/popup_menu_helper_mac.mm. That solution is probably better and would only affect Chromium code.

[Naoki Takano]

Jay,

After just glance in Chromium source code, Mac is really special and it might not be fitting to Linux and Win.

Because your suggesting way uses external_popup_menu_, and which is called from  RenderView::OnSelectPopupMenuItem(). But external_popup_menu_ looks only used for Mac.

I look into it more, but what do you think of Mac and Win?

Thanks,

(In reply to comment #21)
> Jay,
> 
> Thank you for your good suggestion!!
> 
> I'll try to do it.
> 
> For me, Linux is dev environment, so I'll compare between Mac and Linux code.
> 
> Thanks,
> 
> (In reply to comment #20)
> > From what I understand, the external popup is consuming the mouse up and down events so they dont have a chance to be sent to the renderer.
> > 
> > 
> > I think one way to do it would be to:
> > - On the Chrome side, change the ViewMsg_SelectPopupMenuItem message (see content/common/view_messages.h) to take an extra parameter which is the click coordinates of the location clicked (probably translated to the page coordinates) when the selection occurred (could be -1, -1 to mean no click triggered the selection). That message is sent by the browser when a popup menu item is clicked from chrome/browser/tab_contents/popup_menu_helper_mac.mm, that would also have to be changed to include that click info. The render view would then pass that click along to the WebKit side.
> > - On the WebKit Chromium API side the select popup interfaces would have to be modified to take in that new parameter, and the WebViewImpl or ExternalPopupMenu would create and send a click event to the select.
> > 
> > Now that I think of it, another way that would probably be much easier woud be to forward the mouse up event from the external popup menu to the the render widget host view (after translating the coordinates as necessary). That would probably happen in  chrome/browser/tab_contents/popup_menu_helper_mac.mm. That solution is probably better and would only affect Chromium code.

[Jay Civelli]

Naoki,
You are right, my bad. I mistakingly thought this was a Mac only problem.
I'll look into it a bit more and will have more suggestions soon.

Jay

(In reply to comment #22)
> Jay,
> 
> After just glance in Chromium source code, Mac is really special and it might not be fitting to Linux and Win.
> 
> Because your suggesting way uses external_popup_menu_, and which is called from  RenderView::OnSelectPopupMenuItem(). But external_popup_menu_ looks only used for Mac.
> 
> I look into it more, but what do you think of Mac and Win?
> 
> Thanks,
> 
> (In reply to comment #21)
> > Jay,
> > 
> > Thank you for your good suggestion!!
> > 
> > I'll try to do it.
> > 
> > For me, Linux is dev environment, so I'll compare between Mac and Linux code.
> > 
> > Thanks,
> > 
> > (In reply to comment #20)
> > > From what I understand, the external popup is consuming the mouse up and down events so they dont have a chance to be sent to the renderer.
> > > 
> > > 
> > > I think one way to do it would be to:
> > > - On the Chrome side, change the ViewMsg_SelectPopupMenuItem message (see content/common/view_messages.h) to take an extra parameter which is the click coordinates of the location clicked (probably translated to the page coordinates) when the selection occurred (could be -1, -1 to mean no click triggered the selection). That message is sent by the browser when a popup menu item is clicked from chrome/browser/tab_contents/popup_menu_helper_mac.mm, that would also have to be changed to include that click info. The render view would then pass that click along to the WebKit side.
> > > - On the WebKit Chromium API side the select popup interfaces would have to be modified to take in that new parameter, and the WebViewImpl or ExternalPopupMenu would create and send a click event to the select.
> > > 
> > > Now that I think of it, another way that would probably be much easier woud be to forward the mouse up event from the external popup menu to the the render widget host view (after translating the coordinates as necessary). That would probably happen in  chrome/browser/tab_contents/popup_menu_helper_mac.mm. That solution is probably better and would only affect Chromium code.

[Naoki Takano]

Hi, Jay.

Do you have any update?

[Naoki Takano]

Jay,

Do you have any update since the last discussion?

Thanks,

[Naoki Takano]

Jay,

Stil, I'm thinking how to deal with this problem.

And my current idea is to forward the mouse down event in WebPopupMenuImpl::popupClosed().
In the function, we can get PopupContainer from m_widget, and get ChromeClientImpl::webView().

We can get WebViewClient from webView()::client(). Once we can get WebViewClient which derives RenderView, we can forward mouse click events to RenderView.

The tricky point is to get PopupContainer from m_widget with static_cast. But if we don't change anything in WebCore source code, I guess that is the only way.

What do you think?

Thanks,

(In reply to comment #25)
> Jay,
> 
> Do you have any update since the last discussion?
> 
> Thanks,

[Naoki Takano]

Jay looks pretty busy.

Is there anybody who are familiar with this area?

This bug need knowledge about Chromium, popup window and <select> form.

Thanks,

(In reply to comment #26)
> Jay,
> 
> Stil, I'm thinking how to deal with this problem.
> 
> And my current idea is to forward the mouse down event in WebPopupMenuImpl::popupClosed().
> In the function, we can get PopupContainer from m_widget, and get ChromeClientImpl::webView().
> 
> We can get WebViewClient from webView()::client(). Once we can get WebViewClient which derives RenderView, we can forward mouse click events to RenderView.
> 
> The tricky point is to get PopupContainer from m_widget with static_cast. But if we don't change anything in WebCore source code, I guess that is the only way.
> 
> What do you think?
> 
> Thanks,
> 
> (In reply to comment #25)
> > Jay,
> > 
> > Do you have any update since the last discussion?
> > 
> > Thanks,

[Jay Civelli]

(In reply to comment #26)
> Jay,
> 
> Stil, I'm thinking how to deal with this problem.
> 
> And my current idea is to forward the mouse down event in WebPopupMenuImpl::popupClosed().
> In the function, we can get PopupContainer from m_widget, and get ChromeClientImpl::webView().
> 
> We can get WebViewClient from webView()::client(). Once we can get WebViewClient which derives RenderView, we can forward mouse click events to RenderView.
> 
> The tricky point is to get PopupContainer from m_widget with static_cast. But if we don't change anything in WebCore source code, I guess that is the only way.
> 
> What do you think?

Sorry for the delay.
Wouldn't WebPopupMenuImpl::popupClosed() be called even when the mouse is not involved (popup closed by keyboard action)?
I am looking into it to see if I have other suggestions.

[Naoki Takano]

Yes, I forgot to tell you one more thing.

So, to make sure the even is accepted or cancel, I add one more function in PopupContainer, like bool clickIsAccepted() something like that.

And only if the return value is true, we send the event.

What do you think?

(In reply to comment #28)
> (In reply to comment #26)
> > Jay,
> > 
> > Stil, I'm thinking how to deal with this problem.
> > 
> > And my current idea is to forward the mouse down event in WebPopupMenuImpl::popupClosed().
> > In the function, we can get PopupContainer from m_widget, and get ChromeClientImpl::webView().
> > 
> > We can get WebViewClient from webView()::client(). Once we can get WebViewClient which derives RenderView, we can forward mouse click events to RenderView.
> > 
> > The tricky point is to get PopupContainer from m_widget with static_cast. But if we don't change anything in WebCore source code, I guess that is the only way.
> > 
> > What do you think?
> 
> Sorry for the delay.
> Wouldn't WebPopupMenuImpl::popupClosed() be called even when the mouse is not involved (popup closed by keyboard action)?
> I am looking into it to see if I have other suggestions.

[Jay Civelli]

I kind of dislike having to keep a state (whether mouse was clicked or not).
Could we may be add a pointer to a PlatformEvent as an optional param in PopupMenuClient::valueChanged()?
If there the RenderMenuList would dispatch it to the frame event handler, if NULL nothing would happen.

That way PopupMenuChromium.cpp could decide whether the event needs to be forwarded or not.

Let me know what you think.

Jay

[Naoki Takano]

Hmmm...

But, as you know, PopupMenuClient::valueChanged() is in WebCore domain. So we have to change the logic only for Chromium in WebCore, right?

Dimitri already said,
> This seems like the wrong approach. From the perspective of WebCore, there shouldn't be a difference in how ports dispatch events.

Or does he mean different thing? Did I misunderstand?

Thanks,


(In reply to comment #30)
> I kind of dislike having to keep a state (whether mouse was clicked or not).
> Could we may be add a pointer to a PlatformEvent as an optional param in PopupMenuClient::valueChanged()?
> If there the RenderMenuList would dispatch it to the frame event handler, if NULL nothing would happen.
> 
> That way PopupMenuChromium.cpp could decide whether the event needs to be forwarded or not.
> 
> Let me know what you think.
> 
> Jay

[Jay Civelli]

Here is another suggestion that might be interesting.
The problem when you wat to forward the mouse event from PopupMenuChromium.cpp is that you need access to the node you want to forward this event to.
Since the popup menu is created from  PopupMenuChromium::show() at which point we have access to the Frame (through the FrameView).
We could associate the Frame with the PopupContainer.
So then in PopupListBox::handleMouseReleaseEvent() you could forward the mouse event to the focused node (assuming this is the right one).
That way you would not have to change the PopupMenuClient API.

Let me know what you think.

[Naoki Takano]

Created attachment 94743 [details]
Use focused node version.

Just with only focused node, I cant forward mouse release event!!

[Naoki Takano]

Comment on attachment 94743 [details]
Use focused node version.

Jay

Thank you for your great suggestion!!

Now I can implement the simplest way to forward the event.

[Naoki Takano]

Dimitri or Jay,

Could you review my patch?

Thanks,

(In reply to comment #34)
> (From update of attachment 94743 [details])
> Jay
> 
> Thank you for your great suggestion!!
> 
> Now I can implement the simplest way to forward the event.

[Dimitri Glazkov (Google)]

Comment on attachment 94743 [details]
Use focused node version.

Is this the right patch?

[Naoki Takano]

Sorry, my webkit-patch script didn't work, and I did wrong thing manually;-(

I'll upload it again.

Sorry for bothering you again...

(In reply to comment #36)
> (From update of attachment 94743 [details])
> Is this the right patch?

[Naoki Takano]

Created attachment 94898 [details]
Use focused node version.

Please review.

[Jay Civelli]

Comment on attachment 94898 [details]
Use focused node version.

View in context: https://bugs.webkit.org/attachment.cgi?id=94898&action=review

> Source/WebCore/manual-tests/select-element-mouse-event.html:1
> + <!DOCTYPE html>

I wonder if it would be possible to add a test case for this in Source/WebKit/chromium/tests/PopupMenuTest.cpp?
The test framework let's you load HTML in a page, you could then explicitly open the popup and send a fake mouse event to it and ensure everything works as expected.
(and you could test with and without a focused node).

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:414
> +    listBox()->m_focusedNode = m_frameView->frame()->document()->focusedNode();

It would be a good idea to set m_focusedNode to 0 when we hide the popup, so we don't keep a pointer to a potentially deleted object.

[Dimitri Glazkov (Google)]

Comment on attachment 94898 [details]
Use focused node version.

View in context: https://bugs.webkit.org/attachment.cgi?id=94898&action=review

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:193
> +    void acceptIndex(int index, const PlatformMouseEvent* event = 0);

this should just be const PlatformMouseEvent&.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:283
> +    Node* m_focusedNode;

This node could be removed from the tree while the popup is still open. I think you need a RefPtr here.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1104
> +            m_focusedNode->dispatchMouseEvent(*event, eventNames().mouseupEvent);

What's the re-entrance opportunity here? If up event removes the corresponding select element, are we still safe to continue this execution?

[Naoki Takano]

Dimitri and Jay,

Thank you for review.

About test, I'll try it. But I've heard a couple of times, we don't have any good way to handle autofill test.

(In reply to comment #40)
> (From update of attachment 94898 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94898&action=review
> 
> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:193
> > +    void acceptIndex(int index, const PlatformMouseEvent* event = 0);
> 
> this should just be const PlatformMouseEvent&.

But acceptIndex() is called by other functions where we don't have event. That's why I declared where with default value.

> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:283
> > +    Node* m_focusedNode;
> 
> This node could be removed from the tree while the popup is still open. I think you need a RefPtr here.

Ok


> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1104
> > +            m_focusedNode->dispatchMouseEvent(*event, eventNames().mouseupEvent);
> 
> What's the re-entrance opportunity here? If up event removes the corresponding select element, are we still safe to continue this execution?
In this case, it should be fine if we use RefPtr.

But to clarify it, do we have to move this dispatch call to upper function PopupListBox::handleMouseReleaseEvent() from here?

If so, acceptIndex() don't have to take event anymore either. But acceptIndex() have to return boolean to check if we should forward event or not.

Thanks,

[Naoki Takano]

Created attachment 95112 [details]
Change to use RefPtr.

Please review.

1. I cannot write automation test, because PopupMenuTest cannot load test html file and cannot test event handler.
2. Change to use RefPtr.
3. Clear m_focusedNode after the event forward.
4. Check select popup, because we don't want to fire when autofill.

[Dimitri Glazkov (Google)]

Comment on attachment 95112 [details]
Change to use RefPtr.

View in context: https://bugs.webkit.org/attachment.cgi?id=95112&action=review

ok with nits, if Jay confirms this can't be tested using unit testing framework.

> Source/WebCore/ChangeLog:5
> +        [Chromium]Click event is not fired for a menulist <select>

Need a space between [Chromium] and Click.

> Source/WebCore/ChangeLog:8
> +        Manual test with select-element-mouse-event.html.

Manual test: select-element-mouse-event.html

> Source/WebCore/manual-tests/select-element-mouse-event.html:19
> +  alert('down');

Please don't use alerts. Log into the page itself. That's a lot less intrusive.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:684
> +    // Clear m_fcosuedNode here, because we cannot clear in hidePopup() which is called before dispatchMouseEvent() is called.

typo: m_focusedNode.

[Jay Civelli]

Comment on attachment 95112 [details]
Change to use RefPtr.

View in context: https://bugs.webkit.org/attachment.cgi?id=95112&action=review

> Source/WebCore/manual-tests/select-element-mouse-event.html:12
> +<select id=s size=1>

I can think of several other important test cases:
- have disabled items in the select and try to click it
- have a onchange event handler that removes the select node from the DOM (we had crashes in the past with such scenarios)
- have a onclick event handler that removes the select node from the DOM
- use the keyboard to make the selection and ensures the right events are sent (change but no click)

Regarding making unit-tests. 
You can load actual content in a WebView from a test. See for example WebFrameTest and WebPageSerializerTest.
These tests use mocking of URLs, but in your case you could probably just get away with a data URL for the main frame content.
I cannot guarantee this is going to work, but I think it would be a good thing to try as having unit-tests for this would be really nice.

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:685
> +    m_focusedNode = 0;

In the case where acceptIndex returned false, don't we want to keep the m_focusedNode? (the popup might not have been hidden)

> Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1111
> +    return true;

Don't you want to return false in that case?

[Naoki Takano]

Jay,

Thank you for your detail suggestion for the test.

Now I can find how I write unit test.

But this looks like only for Win, and my dev environment is Linux;-(

Actually I have Win7 VMWare environment, so I try to implement it on it. It is really slow though;-(

If I have chance, I want to port this Popup window unit test for Linux. But this is another topic...

Thanks,

(In reply to comment #44)
> (From update of attachment 95112 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95112&action=review
> 
> > Source/WebCore/manual-tests/select-element-mouse-event.html:12
> > +<select id=s size=1>
> 
> I can think of several other important test cases:
> - have disabled items in the select and try to click it
> - have a onchange event handler that removes the select node from the DOM (we had crashes in the past with such scenarios)
> - have a onclick event handler that removes the select node from the DOM
> - use the keyboard to make the selection and ensures the right events are sent (change but no click)
> 
> Regarding making unit-tests. 
> You can load actual content in a WebView from a test. See for example WebFrameTest and WebPageSerializerTest.
> These tests use mocking of URLs, but in your case you could probably just get away with a data URL for the main frame content.
> I cannot guarantee this is going to work, but I think it would be a good thing to try as having unit-tests for this would be really nice.
> 
> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:685
> > +    m_focusedNode = 0;
> 
> In the case where acceptIndex returned false, don't we want to keep the m_focusedNode? (the popup might not have been hidden)
> 
> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1111
> > +    return true;
> 
> Don't you want to return false in that case?

[Naoki Takano]

Jay,

Thank you for your detail suggestion for the test.

Now I can find how I write unit test.

But this looks like only for Win, and my dev environment is Linux;-(

Actually I have Win7 VMWare environment, so I try to implement it on it. It is really slow though;-(

If I have chance, I want to port this Popup window unit test for Linux. But this is another topic...

Thanks,

(In reply to comment #44)
> (From update of attachment 95112 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95112&action=review
> 
> > Source/WebCore/manual-tests/select-element-mouse-event.html:12
> > +<select id=s size=1>
> 
> I can think of several other important test cases:
> - have disabled items in the select and try to click it
> - have a onchange event handler that removes the select node from the DOM (we had crashes in the past with such scenarios)
> - have a onclick event handler that removes the select node from the DOM
> - use the keyboard to make the selection and ensures the right events are sent (change but no click)
> 
> Regarding making unit-tests. 
> You can load actual content in a WebView from a test. See for example WebFrameTest and WebPageSerializerTest.
> These tests use mocking of URLs, but in your case you could probably just get away with a data URL for the main frame content.
> I cannot guarantee this is going to work, but I think it would be a good thing to try as having unit-tests for this would be really nice.
> 
> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:685
> > +    m_focusedNode = 0;
> 
> In the case where acceptIndex returned false, don't we want to keep the m_focusedNode? (the popup might not have been hidden)
> 
> > Source/WebCore/platform/chromium/PopupMenuChromium.cpp:1111
> > +    return true;
> 
> Don't you want to return false in that case?

[Naoki Takano]

Created attachment 95389 [details]
Implement unit test

Jay,

I implemented unit test.

Please review again.

[Naoki Takano]

Jay,

Could you review?

(In reply to comment #47)
> Created an attachment (id=95389) [details]
> Implement unit test
> 
> Jay,
> 
> I implemented unit test.
> 
> Please review again.

[Jay Civelli]

Comment on attachment 95389 [details]
Implement unit test

View in context: https://bugs.webkit.org/attachment.cgi?id=95389&action=review

> Source/WebKit/chromium/tests/PopupMenuTest.cpp:255
> +        filePath += "/Source/WebKit/chromium/tests/data/";

Could you move the test files to a subdirectory (ex: data/popup)?
This folder is only going to get more crowded, so it's good to keep it organized.

> Source/WebKit/chromium/tests/PopupMenuTest.cpp:424
> +    // mousedown evnet is held by select node, and we don't simulate the event for the node.

Typo: evnet

> Source/WebKit/chromium/tests/PopupMenuTest.cpp:473
> +    // Make sure no crash, even if select node is moreved on 'change' event handler.

Typo: moreved -> removed

> Source/WebKit/chromium/tests/PopupMenuTest.cpp:495
> +    // Make sure no crash, even if select node is moreved on 'click' event handler.

Same typo as above.

[Jay Civelli]

Great tests! Thanks for writing them!
Some minor comments, otherwise this looks good to me.

[Naoki Takano]

Created attachment 95533 [details]
Fix some nits.

Please review again.

[WebKit Commit Bot]

Comment on attachment 95533 [details]
Fix some nits.

Rejecting attachment 95533 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'land-a..." exit_code: 2

Last 500 characters of output:
ce/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp
	M	Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h
r87822 = f5ebb50a8f945b5c8570319b6a74a5d16f3bd332 (refs/remotes/trunk)
	M	Source/WebKit/gtk/webkit/webkitwebframe.cpp
	M	Source/WebKit/gtk/webkit/webkitwebframe.h
	M	Source/WebKit/gtk/ChangeLog
r87823 = 774fb7c2f75f2e5c7f97deebeee7bf94fd600f63 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.

Full output: http://queues.webkit.org/results/8752875

[Naoki Takano]

Oops,

I forgot to include ChangeLog;-(

I will include it into the patch tonight...

(In reply to comment #52)
> (From update of attachment 95533 [details])
> Rejecting attachment 95533 [details] from commit-queue.
> 
> Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'land-a..." exit_code: 2
> 
> Last 500 characters of output:
> ce/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp
>     M    Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h
> r87822 = f5ebb50a8f945b5c8570319b6a74a5d16f3bd332 (refs/remotes/trunk)
>     M    Source/WebKit/gtk/webkit/webkitwebframe.cpp
>     M    Source/WebKit/gtk/webkit/webkitwebframe.h
>     M    Source/WebKit/gtk/ChangeLog
> r87823 = 774fb7c2f75f2e5c7f97deebeee7bf94fd600f63 (refs/remotes/trunk)
> First, rewinding head to replay your work on top of it...
> Fast-forwarded master to refs/remotes/trunk.
> 
> Full output: http://queues.webkit.org/results/8752875

[Naoki Takano]

Created attachment 95695 [details]
Added ChangeLog

Please commit again.

I just added ChangeLog.

[WebKit Commit Bot]

Comment on attachment 95695 [details]
Added ChangeLog

Clearing flags on attachment: 95695

Committed r87920: <http://trac.webkit.org/changeset/87920>

[Adam Barth]

I had to roll out this patch because it does not compile:

http://build.chromium.org/p/chromium/builders/Win%20Builder%20%28dbg%29%28shared%29/builds/8325/steps/compile/logs/stdio


24>.\tests\PopupMenuTest.cpp(254) :error C2440: 'initializing' : cannot convert from 'WebKit::WebCString' to 'std::basic_string<_Elem,_Traits,_Ax>'
24>        with
24>        [
24>            _Elem=char,
24>            _Traits=std::char_traits<char>,
24>            _Ax=std::allocator<char>
24>        ]
24>        No constructor could take the source type, or constructor overload resolution was ambiguous
24>.\tests\PopupMenuTest.cpp(258) :error C2665: 'WebKit::WebString::fromUTF8' : none of the 2 overloads could convert all the argument types
24>        public\WebString.h(87): could be 'WebKit::WebString WebKit::WebString::fromUTF8(const char *)'
24>        while trying to match the argument list '(std::string)'
24>.\tests\PopupMenuTest.cpp(258) :error C3861: 'GURL': identifier not found
24>.\tests\PopupMenuTest.cpp(270) :error C3861: 'GURL': identifier not found
24>.\tests\PopupMenuTest.cpp(426) :error C2440: '<function-style-cast>' : cannot convert from 'WebKit::WebCString' to 'std::string'
24>        No constructor could take the source type, or constructor overload resolution was ambiguous
24>.\tests\PopupMenuTest.cpp(426) :error C2228: left of '.c_str' must have class/struct/union
24>.\tests\PopupMenuTest.cpp(426) :error C2512: 'testing::AssertionResult' : no appropriate default constructor available
24>.\tests\PopupMenuTest.cpp(438) :error C2440: '<function-style-cast>' : cannot convert from 'WebKit::WebCString' to 'std::string'
24>        No constructor could take the source type, or constructor overload resolution was ambiguous
24>.\tests\PopupMenuTest.cpp(438) :error C2228: left of '.c_str' must have class/struct/union
24>.\tests\PopupMenuTest.cpp(438) :error C2512: 'testing::AssertionResult' : no appropriate default constructor available
24>.\tests\PopupMenuTest.cpp(447) :error C2440: '<function-style-cast>' : cannot convert from 'WebKit::WebCString' to 'std::string'
24>        No constructor could take the source type, or constructor overload resolution was ambiguous
24>.\tests\PopupMenuTest.cpp(447) :error C2228: left of '.c_str' must have class/struct/union
24>.\tests\PopupMenuTest.cpp(447) :error C2512: 'testing::AssertionResult' : no appropriate default constructor available
24>.\tests\PopupMenuTest.cpp(468) :error C2440: '<function-style-cast>' : cannot convert from 'WebKit::WebCString' to 'std::string'
24>        No constructor could take the source type, or constructor overload resolution was ambiguous
24>.\tests\PopupMenuTest.cpp(468) :error C2228: left of '.c_str' must have class/struct/union
24>.\tests\PopupMenuTest.cpp(468) :error C2512: 'testing::AssertionResult' : no appropriate default constructor available
24>.\tests\PopupMenuTest.cpp(490) :error C2440: '<function-style-cast>' : cannot convert from 'WebKit::WebCString' to 'std::string'
24>        No constructor could take the source type, or constructor overload resolution was ambiguous
24>.\tests\PopupMenuTest.cpp(490) :error C2228: left of '.c_str' must have class/struct/union
24>.\tests\PopupMenuTest.cpp(490) :error C2512: 'testing::AssertionResult' : no appropriate default constructor available
24>.\tests\PopupMenuTest.cpp(512) :error C2440: '<function-style-cast>' : cannot convert from 'WebKit::WebCString' to 'std::string'
24>        No constructor could take the source type, or constructor overload resolution was ambiguous
24>.\tests\PopupMenuTest.cpp(512) :error C2228: left of '.c_str' must have class/struct/union
24>.\tests\PopupMenuTest.cpp(512) :error C2512: 'testing::AssertionResult' : no appropriate default constructor available

[Naoki Takano]

Hmmm...

Very strange, I thought PopupMenuTest.cpp is only for chromium, so I wonder why WebKit win build bot try to build...

Do you have any suggestions?

(In reply to comment #56)
> I had to roll out this patch because it does not compile:
> 
> http://build.chromium.org/p/chromium/builders/Win%20Builder%20%28dbg%29%28shared%29/builds/8325/steps/compile/logs/stdio
> 
> 
> 24>.\tests\PopupMenuTest.cpp(254) :error C2440: 'initializing' : cannot convert from 'WebKit::WebCString' to 'std::basic_string<_Elem,_Traits,_Ax>'
> 24>        with
> 24>        [
> 24>            _Elem=char,
> 24>            _Traits=std::char_traits<char>,
> 24>            _Ax=std::allocator<char>
> 24>        ]
> 24>        No constructor could take the source type, or constructor overload resolution was ambiguous
> 24>.\tests\PopupMenuTest.cpp(258) :error C2665: 'WebKit::WebString::fromUTF8' : none of the 2 overloads could convert all the argument types
> 24>        public\WebString.h(87): could be 'WebKit::WebString WebKit::WebString::fromUTF8(const char *)'
> 24>        while trying to match the argument list '(std::string)'
> 24>.\tests\PopupMenuTest.cpp(258) :error C3861: 'GURL': identifier not found
> 24>.\tests\PopupMenuTest.cpp(270) :error C3861: 'GURL': identifier not found
> 24>.\tests\PopupMenuTest.cpp(426) :error C2440: '<function-style-cast>' : cannot convert from 'WebKit::WebCString' to 'std::string'
> 24>        No constructor could take the source type, or constructor overload resolution was ambiguous
> 24>.\tests\PopupMenuTest.cpp(426) :error C2228: left of '.c_str' must have class/struct/union
> 24>.\tests\PopupMenuTest.cpp(426) :error C2512: 'testing::AssertionResult' : no appropriate default constructor available
> 24>.\tests\PopupMenuTest.cpp(438) :error C2440: '<function-style-cast>' : cannot convert from 'WebKit::WebCString' to 'std::string'
> 24>        No constructor could take the source type, or constructor overload resolution was ambiguous
> 24>.\tests\PopupMenuTest.cpp(438) :error C2228: left of '.c_str' must have class/struct/union
> 24>.\tests\PopupMenuTest.cpp(438) :error C2512: 'testing::AssertionResult' : no appropriate default constructor available
> 24>.\tests\PopupMenuTest.cpp(447) :error C2440: '<function-style-cast>' : cannot convert from 'WebKit::WebCString' to 'std::string'
> 24>        No constructor could take the source type, or constructor overload resolution was ambiguous
> 24>.\tests\PopupMenuTest.cpp(447) :error C2228: left of '.c_str' must have class/struct/union
> 24>.\tests\PopupMenuTest.cpp(447) :error C2512: 'testing::AssertionResult' : no appropriate default constructor available
> 24>.\tests\PopupMenuTest.cpp(468) :error C2440: '<function-style-cast>' : cannot convert from 'WebKit::WebCString' to 'std::string'
> 24>        No constructor could take the source type, or constructor overload resolution was ambiguous
> 24>.\tests\PopupMenuTest.cpp(468) :error C2228: left of '.c_str' must have class/struct/union
> 24>.\tests\PopupMenuTest.cpp(468) :error C2512: 'testing::AssertionResult' : no appropriate default constructor available
> 24>.\tests\PopupMenuTest.cpp(490) :error C2440: '<function-style-cast>' : cannot convert from 'WebKit::WebCString' to 'std::string'
> 24>        No constructor could take the source type, or constructor overload resolution was ambiguous
> 24>.\tests\PopupMenuTest.cpp(490) :error C2228: left of '.c_str' must have class/struct/union
> 24>.\tests\PopupMenuTest.cpp(490) :error C2512: 'testing::AssertionResult' : no appropriate default constructor available
> 24>.\tests\PopupMenuTest.cpp(512) :error C2440: '<function-style-cast>' : cannot convert from 'WebKit::WebCString' to 'std::string'
> 24>        No constructor could take the source type, or constructor overload resolution was ambiguous
> 24>.\tests\PopupMenuTest.cpp(512) :error C2228: left of '.c_str' must have class/struct/union
> 24>.\tests\PopupMenuTest.cpp(512) :error C2512: 'testing::AssertionResult' : no appropriate default constructor available

[Naoki Takano]

Created attachment 95858 [details]
Add WEBKIT_IMPLEMENTATION change

Jay,

Still I'm not sure this correct is fix, but I believe WEBKIT_IMPLEMENTATION causes the build error.

Do you know why?

I added #define WEBKIT_IMPLEMENTATION 0, but it seems we have better way...

Thanks,

[Naoki Takano]

Jay,

Could you give me your thought?

(In reply to comment #58)
> Created an attachment (id=95858) [details]
> Add WEBKIT_IMPLEMENTATION change
> 
> Jay,
> 
> Still I'm not sure this correct is fix, but I believe WEBKIT_IMPLEMENTATION causes the build error.
> 
> Do you know why?
> 
> I added #define WEBKIT_IMPLEMENTATION 0, but it seems we have better way...
> 
> Thanks,

[Kent Tamura]

Comment on attachment 95858 [details]
Add WEBKIT_IMPLEMENTATION change

View in context: https://bugs.webkit.org/attachment.cgi?id=95858&action=review

> Source/WebKit/chromium/tests/PopupMenuTest.cpp:33
> +#if WEBKIT_IMPLEMENTATION
> +#define WEBKIT_IMPLEMENTATION 0
> +#endif

Don't do this.

> Source/WebKit/chromium/tests/PopupMenuTest.cpp:38
> +#include <webkit/support/webkit_support.h>

Because webkit_support dependency is added, we can't build PopupMenuTest.cpp with Chromium-win (shared) configuration.
You need to add PopupMenuTest.cpp to the sources! section of https://trac.webkit.org/browser/trunk/Source/WebKit/chromium/WebKit.gyp?rev=87948#L642

[Naoki Takano]

Created attachment 96177 [details]
Add PopupMenuTest.cpp into WebKit.gyp

Kent-san,

Please review again.

[WebKit Commit Bot]

Comment on attachment 96177 [details]
Add PopupMenuTest.cpp into WebKit.gyp

Clearing flags on attachment: 96177

Committed r88232: <http://trac.webkit.org/changeset/88232>

[Dmitry Lomov]

(In reply to comment #62)
> (From update of attachment 96177 [details])
> Clearing flags on attachment: 96177
> 
> Committed r88232: <http://trac.webkit.org/changeset/88232>

This changeset introduced a crash in crhromium: http://code.google.com/p/chromium/issues/detail?id=88260