Chromium could easily switch to Cross platform context menu implementation and reduce Context Menu related code significantly.
Created attachment 192985 [details] Patch
Created attachment 193177 [details] Patch
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.
(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.
Comment on attachment 193177 [details] Patch This looks great!
> I will have a look and fix that if needed. Ok. Sounds like we should wait for that before landing this patch.
(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!
Created attachment 196351 [details] Patch