Bug 80751 – Windows IDL files should use generic pointers for Windows Handles

• Status: UNCONFIRMED
• Component: WebKit API

[Ashod Nakashian]

Due to the nature of Windows Handles which are opaque COM interfaces that use them force TLB files to use _RemotableHandle structures to wrap these platform-specific Handles. This is a nuance to deal with as they defeat the cross-language advantages of COM by forcing users to wrap around each interface member that has a _RemotableHandle.

The solution is simple: there are compile-time directives that replace _RemotableHandles with integral types. Once such a directive is used all Handles becomes integer types as wide as the target machine word.

[Ashod Nakashian]

Created attachment 131172 [details]
Patch

[Ashod Nakashian]

Added CC list based on previous commits.

[Patrick R. Gansterer]

Comment on attachment 131172 [details]
Patch

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

> Source/WebKit/win/ChangeLog:3
> +        Windows IDL files should use generic pointers for Windows Handles

can you explain the why in the changelog

> Source/WebKit/win/Interfaces/WebKit.idl:61
> +cpp_quote("#if 0")
> +typedef long HWND;
> +typedef long HDC;
> +typedef long HACCEL;
> +typedef long HMENU;
> +typedef long HICON;
> +typedef long HFONT;
> +typedef long HRGN;
> +cpp_quote("#endif")

can you explain this more detailed (in the ChangeLog)
Ususally HWND is defined as HANDLE, which is defined as void*. Why can't we do that here?

> Source/WebKit/win/WebKit.vcproj/Interfaces.vcproj:38
> +				PreprocessorDefinitions="_MIDL_DECLARE_WIREM_HANDLE"

is there no better place to set the PreporcessorDefinitions in one of the vsprops files?

[Ashod Nakashian]

(In reply to comment #3)
> (From update of attachment 131172 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131172&action=review
> 
> > Source/WebKit/win/ChangeLog:3
> > +        Windows IDL files should use generic pointers for Windows Handles
> 
> can you explain the why in the changelog
> 
> > Source/WebKit/win/Interfaces/WebKit.idl:61
> > +cpp_quote("#if 0")
> > +typedef long HWND;
> > +typedef long HDC;
> > +typedef long HACCEL;
> > +typedef long HMENU;
> > +typedef long HICON;
> > +typedef long HFONT;
> > +typedef long HRGN;
> > +cpp_quote("#endif")
> 
> can you explain this more detailed (in the ChangeLog)
> Ususally HWND is defined as HANDLE, which is defined as void*. Why can't we do that here?

HANDLE is defined as void*, but that's not a portable type. This change is meaningful for non-C languages such as VB and .Net. The one reference I found using this trick is http://miranda-gds-v2.googlecode.com/svn/trunk/common/GoogleDesktopDisplayAPI.idl. This change shouldn't negatively affect C/C++ but should help other languages.

Is there a way to test whether or not this will break some code in the wild? Does any of our EWS servers consume WebKit COM?

> 
> > Source/WebKit/win/WebKit.vcproj/Interfaces.vcproj:38
> > +				PreprocessorDefinitions="_MIDL_DECLARE_WIREM_HANDLE"
> 
> is there no better place to set the PreporcessorDefinitions in one of the vsprops files?

This should remain local to this project. If there is an appropriate prop file, I'll add it there, otherwise, vcproj is reasonable.

[Alexey Proskuryakov]

Comment on attachment 131172 [details]
Patch

Have you e-mailed webkit-dev about your desire to support non-C languages such as VB and .Net?

[Mark Rowe (bdash)]

Your email to webkit-dev suggests that this change would break binary compatibility with previous versions of WebKit.  Can you confirm that is the case?

[Build Bot]

Comment on attachment 131172 [details]
Patch

Attachment 131172 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11942271

[Ashod Nakashian]

(In reply to comment #6)
> Your email to webkit-dev suggests that this change would break binary compatibility with previous versions of WebKit.  Can you confirm that is the case?

That is the fear, yes. I can't confirm it without testing. But right now more interestingly the Win EWS shows errors. I don't know if it was offline or what, but for the longest time it didn't complain. Since I'm building with vs2010 I didn't get no build errors (yes, I've consumed the modified COM successfully and without hickups).

So first I have to resolve the build issues and, considering there isn't much feedback on (in)compatibility of my changes, I'll have to do testing and report my results to the community to decide whether or not this is an desired change.

[Mark Rowe (bdash)]

If this breaks binary compatibility then there's no way we can accept it.

[Ashod Nakashian]

(In reply to comment #9)
> If this breaks binary compatibility then there's no way we can accept it.

What are the known consumers of WebKit COM? What kind of effort is involved in changing binary compatibility?

[Steve Falkenburg]

Clients include Safari and iTunes on Windows. You can test binary compatibility by building the Apple Windows WebKit port and running it against installed Safari.

"Changing binary compatibility" is not something we do. We preserve binary compatibility for existing clients.

[Build Bot]

Comment on attachment 131172 [details]
Patch

Attachment 131172 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/41012

[Build Bot]

Comment on attachment 131172 [details]
Patch

Attachment 131172 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/74132

New failing tests:
css3/filters/custom/filter-fallback-to-software.html

[Build Bot]

Created attachment 197660 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Brent Fulgham]

Comment on attachment 131172 [details]
Patch

We cannot break binary compatibility.