Bug 32298 - Provide Default IWebUIDelegate and IWebFrameLoadDelegate Implementations
Summary: Provide Default IWebUIDelegate and IWebFrameLoadDelegate Implementations
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-08 17:32 PST by Brent Fulgham
Modified: 2010-06-10 20:05 PDT (History)
4 users (show)

See Also:


Attachments
Default implementation of delegates to reduce boilerplate requirements. (23.78 KB, patch)
2009-12-08 17:41 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
default implementation of delegates to reduce boilerplate requirements. (24.28 KB, patch)
2009-12-08 17:49 PST, Brent Fulgham
sam: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2009-12-08 17:32:12 PST
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.
Comment 1 Brent Fulgham 2009-12-08 17:41:21 PST
Created attachment 44501 [details]
Default implementation of delegates to reduce boilerplate requirements.
Comment 2 WebKit Review Bot 2009-12-08 17:41:39 PST
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
Comment 3 Brent Fulgham 2009-12-08 17:49:24 PST
Created attachment 44502 [details]
default implementation of delegates to reduce boilerplate requirements.

Corrected whitespace issues.
Comment 4 WebKit Review Bot 2009-12-08 17:52:07 PST
style-queue ran check-webkit-style on attachment 44502 [details] without any errors.
Comment 5 mitz 2009-12-08 18:39:47 PST
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?
Comment 6 Brent Fulgham 2009-12-08 20:22:19 PST
(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.
Comment 7 Eric Seidel (no email) 2009-12-22 16:43:13 PST
There was some recent discusion of removing the COM bindings, which would make this bug invalid.
Comment 8 Eric Seidel (no email) 2009-12-22 16:44:01 PST
Nevermind.  The discussion was about removing teh autogenerated COM DOM bindings, not the general COM API.  My bad.
Comment 9 Sam Weinig 2009-12-30 15:23:29 PST
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?
Comment 10 Adam Roben (:aroben) 2010-01-04 08:00:49 PST
(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.