Bug 80751 - Windows IDL files should use generic pointers for Windows Handles
Summary: Windows IDL files should use generic pointers for Windows Handles
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 80749
  Show dependency treegraph
 
Reported: 2012-03-10 01:49 PST by Ashod Nakashian
Modified: 2013-10-30 10:54 PDT (History)
10 users (show)

See Also:


Attachments
Patch (2.45 KB, patch)
2012-03-10 06:57 PST, Ashod Nakashian
bfulgham: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (1.00 MB, application/zip)
2013-04-11 13:13 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ashod Nakashian 2012-03-10 01:49:54 PST
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.
Comment 1 Ashod Nakashian 2012-03-10 06:57:38 PST
Created attachment 131172 [details]
Patch
Comment 2 Ashod Nakashian 2012-03-10 08:45:47 PST
Added CC list based on previous commits.
Comment 3 Patrick R. Gansterer 2012-03-11 09:16:32 PDT
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?
Comment 4 Ashod Nakashian 2012-03-11 10:05:45 PDT
(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.
Comment 5 Alexey Proskuryakov 2012-03-11 21:23:31 PDT
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?
Comment 6 Mark Rowe (bdash) 2012-03-12 15:21:29 PDT
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?
Comment 7 Build Bot 2012-03-12 19:10:43 PDT
Comment on attachment 131172 [details]
Patch

Attachment 131172 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11942271
Comment 8 Ashod Nakashian 2012-03-13 00:58:57 PDT
(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.
Comment 9 Mark Rowe (bdash) 2012-03-13 01:30:30 PDT
If this breaks binary compatibility then there's no way we can accept it.
Comment 10 Ashod Nakashian 2012-03-13 04:36:06 PDT
(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?
Comment 11 Steve Falkenburg 2012-03-13 09:51:23 PDT
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.
Comment 12 Build Bot 2013-04-10 19:31:35 PDT
Comment on attachment 131172 [details]
Patch

Attachment 131172 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/41012
Comment 13 Build Bot 2013-04-11 13:13:25 PDT
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
Comment 14 Build Bot 2013-04-11 13:13:28 PDT
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
Comment 15 Brent Fulgham 2013-10-30 10:54:57 PDT
Comment on attachment 131172 [details]
Patch

We cannot break binary compatibility.