Bug 111882 – [Chromium] Switch ContextMenu implementation to CROSS_PLATFORM_CONTEXT_MENU

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Jesus Sanchez-Palencia]

Chromium could easily switch to Cross platform context menu implementation and reduce Context Menu related code significantly.

[Jesus Sanchez-Palencia]

Created attachment 192985 [details]
Patch

[Jesus Sanchez-Palencia]

Created attachment 193177 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 193177 [details]
Patch

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

What about making a ContextMenu(Item)?Common.cpp that have empty implementations for the ports that do not make use of the platform hooks for context menu?

This would prevent us for having stub-only file for both EFL and Chromium here. I also think those should not be marked as NotImplemented, since it is simply a hook that will not be used.

> Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:189
> -        return 0;
> +        return defaultMenu;

This looks like a change from previous behavior of this function, is it intended?

> Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:374
> -    return 0;
> +    return defaultMenu;

Same as above.

[Jesus Sanchez-Palencia]

(In reply to comment #3)
> (From update of attachment 193177 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193177&action=review
> 
> What about making a ContextMenu(Item)?Common.cpp that have empty implementations for the ports that do not make use of the platform hooks for context menu?
> 
> This would prevent us for having stub-only file for both EFL and Chromium here. I also think those should not be marked as NotImplemented, since it is simply a hook that will not be used.

Yes sure, but there are a lot of places in WebCore where we could use stub (common) .cpp files but we just don't, so that is why I haven't added one. I will do that.


> 
> > Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:189
> > -        return 0;
> > +        return defaultMenu;
> 
> This looks like a change from previous behavior of this function, is it intended?
> 
> > Source/WebKit/chromium/src/ContextMenuClientImpl.cpp:374
> > -    return 0;
> > +    return defaultMenu;
> 
> Same as above.

At a first glance I thought that this wouldn't change anything since Chromium will use customizeMenu() to display the ContextMenu anyway, but now I believe that ContextMenuController should know about this null ptr just in case. I will have a look and fix that if needed.

[Adam Barth]

Comment on attachment 193177 [details]
Patch

This looks great!

[Adam Barth]

> I will have a look and fix that if needed.

Ok.  Sounds like we should wait for that before landing this patch.

[Jesus Sanchez-Palencia]

(In reply to comment #6)
> > I will have a look and fix that if needed.
> 
> Ok.  Sounds like we should wait for that before landing this patch.

I had a better look at ContextMenuController and I believe the patch is correct the way it is. This doesn't change any behavior. The nullptr was used there to set m_contextMenu->setPlatformDescription, which in Chromium was an empty implementation. ContextMenuController always rely on having a valid m_contextMenu, and the valid checks are actually related to its items vector size.

Returning the same defaultMenu here is only a way to say that there was no need to customize the menu.


(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 193177 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=193177&action=review
> > 
> > What about making a ContextMenu(Item)?Common.cpp that have empty implementations for the ports that do not make use of the platform hooks for context menu?
> > 
> > This would prevent us for having stub-only file for both EFL and Chromium here. I also think those should not be marked as NotImplemented, since it is simply a hook that will not be used.
> 
> Yes sure, but there are a lot of places in WebCore where we could use stub (common) .cpp files but we just don't, so that is why I haven't added one. I will do that.

I will leave this for another patch, after I'm done with the other ports.


Thanks for having a looking at this patch!

[Jesus Sanchez-Palencia]

Created attachment 196351 [details]
Patch