Bug 70491 - Add mechanisms to notify WebCore of showing and dismissing context menu
Summary: Add mechanisms to notify WebCore of showing and dismissing context menu
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 69137
  Show dependency treegraph
 
Reported: 2011-10-20 04:21 PDT by chandra shekar vallala
Modified: 2013-02-03 14:13 PST (History)
11 users (show)

See Also:


Attachments
patch for WebCore changes. (1.92 KB, patch)
2011-10-21 07:34 PDT, chandra shekar vallala
rniwa: review-
Details | Formatted Diff | Diff
updated patch with chromium + gtk port changes. (7.61 KB, patch)
2011-10-25 07:46 PDT, chandra shekar vallala
rniwa: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chandra shekar vallala 2011-10-20 04:21:36 PDT
Port specific changes for bug https://bugs.webkit.org/show_bug.cgi?id=69137
Comment 1 Ryosuke Niwa 2011-10-20 10:52:40 PDT
Sorry I was probably not clear. What I meant is that you need to file blockers and fix them first, not the other way around.
Comment 2 chandra shekar vallala 2011-10-21 07:34:42 PDT
Created attachment 111964 [details]
patch for WebCore changes.

Updated the WebCore changes ..
Comment 3 Ryosuke Niwa 2011-10-21 11:18:30 PDT
Comment on attachment 111964 [details]
patch for WebCore changes.

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

> Source/WebCore/page/EventHandler.cpp:2264
> +    m_frame->selection()->setCaretBlinkingSuspended(true);

This patch shouldn't be adding a bug fix. Instead, you should add an empty stub functions that are called by each port.
Comment 4 chandra shekar vallala 2011-10-25 07:46:49 PDT
Created attachment 112334 [details]
updated patch with chromium + gtk port changes.

Patch includes changes for chromium and gtk port changes to notify EventHandler about context menu show & hide actions.
I don't have the infrastructure at my side to make the changes for other ports, so it would be great if someone can add the patches for those ports in the bug.
Comment 5 WebKit Review Bot 2011-10-25 07:48:25 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 6 Ryosuke Niwa 2011-10-25 07:58:05 PDT
Comment on attachment 112334 [details]
updated patch with chromium + gtk port changes.

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

I'd say r- due to various nits. How about win, mac, qt, etc...?

> Source/WebCore/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description and blank lines before and after.

> Source/WebCore/ChangeLog:9
> +        No new tests. (OOPS!)

Please remove this line or explain why this change can't be tested.

> Source/WebKit/chromium/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).

Again this line should appear above the description and a blank line.

> Source/WebKit/chromium/public/WebView.h:328
> +    virtual void didCloseContextMenu() { }

Shouldn't this be a pure virtual function?

> Source/WebKit/gtk/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).

Ditto.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:320
> +    HitTestResult result = controller->hitTestResult();
> +
> +    Node* node = result.innerNonSharedNode();
> +    if (!node)
> +        return;

Why do we need to hit-test?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:321
> +    sleep(1000);

Where did this sleep(1000); come from?
Comment 7 Ryosuke Niwa 2011-10-25 08:05:25 PDT
Comment on attachment 112334 [details]
updated patch with chromium + gtk port changes.

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

Also, I'm wondering

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:320
>> +        return;
> 
> Why do we need to hit-test?

Ah, now I see why you need a hit testing here to access frame. I would move this code into ContextMenuController if I were you.
Comment 8 Chang Shu 2011-10-25 10:17:21 PDT
Comment on attachment 112334 [details]
updated patch with chromium + gtk port changes.

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

> Source/WebCore/page/EventHandler.cpp:2269
> +void EventHandler::willShowContextMenu()
> +{
> +}
> +
> +void EventHandler::respondToContextMenuDismissal()
> +{
> +}
> +

What do these blank functions do?
Comment 9 Chang Shu 2011-10-25 10:23:54 PDT
(In reply to comment #8)
> (From update of attachment 112334 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112334&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:2269
> > +void EventHandler::willShowContextMenu()
> > +{
> > +}
> > +
> > +void EventHandler::respondToContextMenuDismissal()
> > +{
> > +}
> > +
> 
> What do these blank functions do?

Ryosuke told me they will be implemented in a follow-up patch. So I think it's good to put notImplemented() there.