I recently explored the IWebUIDelegate interface to customize the context menu in WebKit. In Cocoa, using the WebUIDelegate is very convenient -- I simply provide an implementation for the one or two methods I wish to customize, and leave the others alone. I was proudly showing a coworker how cool and simple it was going to be to customize the context menu in our Windows build by implementing a delegate class to control this, when to my horror I realized that IWebUIDelegate (which is the equivalent object on the Windows side) is implemented as a set of pure virtual methods, all of which must be stubbed out to successfully compile. To add insult to injury, all of these interface classes also require each concrete implementation to implement a stub QueryInterface, AddRef, and RemoveRef method. This seems likely to result in a huge amount of wasted boilerplate code, where a Windows developer must create a page of stubbed implementations just to turn off (or otherwise modify) the context menu. This same anti-pattern can be found in the other I...Delegate objects as well (which also require the same boilerplate QueryInterface, AddRef, RemoveRef, etc.) For my own purposes, I created a concrete class "WebUIDelegate" that stubs all methods as E_NOTIMPL and provides the boilerplate QueryInterface, AddRef, etc.. I subclass from *this* class as my delegate, which allows me to simply write one method that does the menu customization. Attached is a proposed implementation for a default IWebUIDelegate and IWebFrameLoadDelegate for consideration. Note: These DefaultWeb[...]Delegate implementations will not be available via DCOM. However, that's not a critical loss of functionality since you can still write your own IWebUIDelegate from scratch for those cases.
Created attachment 44501 [details] Default implementation of delegates to reduce boilerplate requirements.
Attachment 44501 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Last 5120 characters of output: : Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:137: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:144: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:151: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:158: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:165: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:172: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:179: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:186: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:194: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:203: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:212: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:219: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:227: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:236: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:245: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:253: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:261: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:269: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:277: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:287: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:295: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:301: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:309: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:316: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:323: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:330: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:337: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:345: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:352: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:360: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:366: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:382: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:388: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:394: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:401: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:408: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:415: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:422: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:430: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:440: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:447: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:454: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:462: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:468: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:475: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:482: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:491: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:506: Extra space after ( in function call [whitespace/parens] [4] WebKit/win/DefaultWebUIDelegate.h:515: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 89
Created attachment 44502 [details] default implementation of delegates to reduce boilerplate requirements. Corrected whitespace issues.
style-queue ran check-webkit-style on attachment 44502 [details] without any errors.
While the interfaces don’t change too often, these files put an extra burden on anyone modifying them. Have you thought about generating these files? Is there a reason why that wouldn’t work?
(In reply to comment #5) > While the interfaces don’t change too often, these files put an extra burden on > anyone modifying them. Have you thought about generating these files? Is there > a reason why that wouldn’t work? That's a good idea. (To answer the original question, no I had not thought about that!) The bulk of the file consists of stuff that was originally generated from the IDL file; perhaps this could be partitioned off in some fashion for just the handful of delegate classes that would benefit from this treatment.
There was some recent discusion of removing the COM bindings, which would make this bug invalid.
Nevermind. The discussion was about removing teh autogenerated COM DOM bindings, not the general COM API. My bad.
Comment on attachment 44502 [details] default implementation of delegates to reduce boilerplate requirements. > Index: WebKit/win/WebFrame.cpp > =================================================================== > --- WebKit/win/WebFrame.cpp (revision 51877) > +++ WebKit/win/WebFrame.cpp (working copy) > @@ -986,7 +986,14 @@ HRESULT STDMETHODCALLTYPE WebFrame::sele > > HRESULT STDMETHODCALLTYPE WebFrame::selectAll() > { > - return E_NOTIMPL; > + Frame* coreFrame = core(this); > + if (!coreFrame) > + return E_FAIL; > + > + if (!coreFrame->editor()->command("SelectAll").execute()) > + return E_FAIL; > + > + return S_OK; > } > > HRESULT STDMETHODCALLTYPE WebFrame::deselectAll() I don't believe this was intended to be added. Other comments. Does everything really need to inline? Should these be inheriting from the private interfaces as well?
(In reply to comment #9) > (From update of attachment 44502 [details]) > Does everything really need to inline? The definitions won't really be inlined since they're all for virtual functions. But I do think it's good to keep everything in the header. That will make it so we don't have to export (and then link against) symbols for these classes.