Created attachment 608538[details][diff][review]
Patch v1 - Initial implementation of shortcut creation js module
As part of the installation process for natively-installed webapps, we need to be able to create Windows shortcuts (on the desktop and in the start menu). Windows only exposes shortcut creation/modification through the COM interface `IShellLink`. Using js-ctypes, we can create an instance of `IShellLink` to create/modify a shortcut, QueryInterface it to a `IPersistFile`, and save the icon to disk. This bug tracks progress toward a js-ctypes implementation of shortcut creation/modification.

I know close to nothing about windows platform stuff, so all I can really do here is review the js-ctypes usage.
But I'm sort of surprised that this works. JS-Ctypes doesn't support c++ calling conventions and linkage. Do Win32 API c++ objects set up their methods with c-style stdcall conventions? At the very least, this would preclude overloading. But I'm no expert in these things.
This begs the question though - why is this being done in js-ctypes? It would seem infinitely easier to do in C++. The js-ctypes approach is much more brittle - is there a good reason for it?

(In reply to Bobby Holley (:bholley) from comment #2)
> I know close to nothing about windows platform stuff, so all I can really do
> here is review the js-ctypes usage.
I'll ping someone in #windev to take a look at the windows platform API usage.
> But I'm sort of surprised that this works. JS-Ctypes doesn't support c++
> calling conventions and linkage. Do Win32 API c++ objects set up their
> methods with c-style stdcall conventions? At the very least, this would
> preclude overloading. But I'm no expert in these things.
IShellLinkW and IPersistFile are COM interfaces. The objects that implement them have a specific layout in memory and calling convention. The memory layout happens to be friendly to C++ (by design - https://blogs.msdn.com/b/oldnewthing/archive/2004/02/05/68017.aspx ) but the calling convention of all the functions is stdcall. Any language that can understand stdcall, make system calls (CoCreateInstance, etc), and understand the memory layout of COM objects is capable of using COM interfaces/objects (e.g. C, C++, python, VB, Delphi, js-ctypes).
> This begs the question though - why is this being done in js-ctypes? It
> would seem infinitely easier to do in C++. The js-ctypes approach is much
> more brittle - is there a good reason for it?
The consuming code for this functionality is in javascript. The two approaches that I can think of for writing this piece in C++ are 1) write an XPCOM component that is consumed from our js code or 2) compile the C++ code into a DLL that we subsequently access from js-ctypes. My understanding is that we're moving away from XPCOM components (preferring js modules when possible), so option 1) is not preferred. Given that the memory layout and calling convention of COM are well-defined and that COM interfaces (at least the ones provided by MSFT) are guaranteed not to change, is option 2) less brittle than the pure-js-ctypes approach?
This is being done in js-ctypes at least partly because I wanted to prove that it could be done; there's a rich array of functionality in Windows that is only available through COM (shortcut creation is one example, invoking a virus scan on a specified file is another). Thanks to js-ctypes and its ability to understand COM objects, that functionality is available to us in javascript (if we do a little work up front parsing the C interfaces for the COM objects we want to use). It seems silly to write a larger piece of functionality in C++ just because it wants to create a Desktop shortcut, and it seems equally silly to wrap MSFT COM in XPCOM if we have a more direct alternative. That being said, you're certainly the js-ctypes expert here so I'm not comfortable with this approach unless you are :)
(In reply to Dão Gottwald [:dao] from comment #3)
> What does "webapprt" mean?
"webapprt" stands for "Webapp Runtime" - The "webapprt" directory is being created (maybe under a different name, depending on what reviewers have to say) by the patches in bug 725408.
I don't think the "webapprt" directory is where the functionality described in this bug should live, but that's where I happen to have placed it during development (since I don't know where in the tree it should eventually go).

(In reply to Tim Abraldes from comment #4)
> IShellLinkW and IPersistFile are COM interfaces. The objects that implement
> them have a specific layout in memory and calling convention. The memory
> layout happens to be friendly to C++ (by design -
> https://blogs.msdn.com/b/oldnewthing/archive/2004/02/05/68017.aspx ) but the
> calling convention of all the functions is stdcall. Any language that can
> understand stdcall, make system calls (CoCreateInstance, etc), and
> understand the memory layout of COM objects is capable of using COM
> interfaces/objects (e.g. C, C++, python, VB, Delphi, js-ctypes).
Oh, interesting. Thanks for explaining. ;-)
> The consuming code for this functionality is in javascript. The two
> approaches that I can think of for writing this piece in C++ are 1) write an
> XPCOM component that is consumed from our js code or 2) compile the C++ code
> into a DLL that we subsequently access from js-ctypes. My understanding is
> that we're moving away from XPCOM components (preferring js modules when
> possible), so option 1) is not preferred.
That's not really the whole picture.
We're moving away from _extensions_ using XPCOM binary components, largely because we want them to only touch gecko internal interfaces from JS (so that they don't break every time we change the vtables). Historically, people have used binary components when they want to do native stuff, and then poked around at nsINode from C++ while they were at it.
To guard against this, we've been bumping kVersion every 6 weeks with each release, so that extension have to recompile every 6 weeks (which sucks). So we've been encouraging addons that just have to do a little bit of native stuff here and there to use js-ctypes to do it.
However, we've been specifically suggesting that people not expose use big complicated APIs directly with js-ctypes. Instead, we've been suggesting that people write a simple C shim around whatever native APIs they want to expose, so that they don't have to duplicate complicated interface declarations (more or less like what this patch does).
More to the point, none of this applies to code that lives in mozilla central (which is where I assume this code will live). That code gets rebuilt on every checkin (and is available to developers on mxr), so there's no worry of code getting out of sync when we change other things in gecko. I have not heard of anyone saying that we should be replacing binary XPCOM platform code with js-ctypes, and think it's a bad idea. We should use XPCOM/XPIDL/XPConnect for this kind of stuff.
> This is being done in js-ctypes at least partly because I wanted to prove
> that it could be done;
I agree that it's very cool! :-)
> It seems silly to write a larger piece of
> functionality in C++ just because it wants to create a Desktop shortcut
Agreed.
> and it seems equally silly to wrap MSFT COM in XPCOM if we have a more direct
> alternative.
I disagree with you here. The 'more direct alternative' involves duplicating the interface definitions from the Win32 headers into javascript. This is incredibly error-prone (which is what I really meant before when I said 'brittle'). And not really what js-ctypes is designed to do.
> That being said, you're certainly the js-ctypes expert here so
> I'm not comfortable with this approach unless you are :)
I think that this functionality should be exposed via an XPCOM service to avoid duplicating the declarations from the headers and scary vtable munging.

Created attachment 613336[details][diff][review]
XPCOM Service Patch v1 - WIP, working implementation
Just wanted to post the WIP patch. I've been able to successfully create shortcuts from JS while testing this patch.
Some remaining code issues:
CoInitialize/CoUninitialize should be called before using COM and after we're done using COM, respectively
It doesn't make sense to create shortcuts with no target, so if the shortcut file doesn't exist then `setShortcut` should check whether it was passed a target file and error out if it was not
Some remaining organizational issues:
I put the shortcut service in xpcom/io; not sure if there are strong opinions about better locations for it
"@mozilla.org/file/windows_shortcut_service;1" is the current contract ID, but I'm open to better ones

Created attachment 614047[details][diff][review]
Patch v2 - Reviewable patch
This patch implements Windows shortcut creation as an XPCOM service in xpcom/io with the contract ID "@mozilla.org/file/windows_shortcut_service".
You'll notice that the build system changes also include changes for the Windows Shortcut Creation service in bug 738500. That is because these features are being developed together and will likely be submitted together as part of the patch in bug 725408.
Benjamin; same questions as here - [https://bugzilla.mozilla.org/show_bug.cgi?id=738500#c3]. Namely, are you the right person to review this code? and have I placed this code in the right place in the tree?
Also, the build system changes in this patch include changes for the Windows icon embedding service in bug 738500. That is because these features are being developed together and will likely be submitted together as part of the patch in bug 725408.

Comment on attachment 614047[details][diff][review]
Patch v2 - Reviewable patch
It seems to me that it would be a simpler API to just have nsILocalFileWin.setShortcut, and avoid the new service and its associated gunk. Is there a reason we can't just do that? re-request review on this patch if necessary, but I'd like that approach better.

Created attachment 614891[details][diff][review]
Patch v1 - First iteration of adding SetShortcut function to nsILocalFileWin
(In reply to Benjamin Smedberg [:bsmedberg] from comment #9)
> [...]
>
> It seems to me that it would be a simpler API to just have
> nsILocalFileWin.setShortcut [...]
Done. Benjamin: I believe you're still the right person from whom to request review?

Comment on attachment 615999[details][diff][review]
Patch v2 - Adds tests, documents behavior in IDL, fixes nits.
[Approval Request Comment]
This is part of the webapps work which is targeted for Firefox 14. Without this patch, the webapp installer will be unable to create desktop and start menu shortcuts.
Risk for Fennec: none, the code here affects only nsILocalFileWin/nsLocalFileWin

Comment on attachment 615999[details][diff][review]
Patch v2 - Adds tests, documents behavior in IDL, fixes nits.
Shouldn't you use a manifest annotation to avoid running the test on non-Windows, rather than baking that into the test itself?