738500 - Implement ability to embed icons in Windows PE files from javascript

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1 - Initial implementation of icon embedding js module

As part of the installation process for natively-installed webapps, we need to embed the icon of the app in our Web App RunTime executable.  This was originally accomplished using the "redit" tool that is built as part of XULRunner, but it would be nice if our js code didn't have to invoke a separate executable as part of installing a webapp.  This bug tracks the progress toward a js-ctypes implementation of icon embedding.

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 608537 [details] [diff] [review]
Patch v1 - Initial implementation of icon embedding js module

bholley, could you take a look at the js-ctypes usage in this patch?  Based on the discussion in bug 738501, I have a sneaking suspicion that I'll soon be writing an XPCOM component for this, but maybe the situations are sufficiently different :)

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 608537 [details] [diff] [review]
Patch v1 - Initial implementation of icon embedding js module


>+XPCOMUtils.defineLazyGetter(this, "ctypes", function() {
>+  Cu.import("resource://gre/modules/ctypes.jsm");
>+  return ctypes;
>+});

What's the point of making this a lazy getter? ctypes is used in global scope, so the ctypes jsm will always need to be imported.


>+    try {
>+      let data = new CC("@mozilla.org/binaryinputstream;1",
>+                        "nsIBinaryInputStream",
>+                        "setInputStream")(iconFileStream)
>+                     .readByteArray(iconFileStream.available());
>+
>+      let cDataArrayType = BYTE.array(data.length);
>+      let cData = new cDataArrayType();
>+
>+      for(let curByte = 0; curByte < data.length; curByte++) {
>+        cData[curByte] = new BYTE(data[curByte]);

This is sort of inefficient. I'd suggest just creating the array and passing its address to readBytes.

Also, the |new BYTE(..)| is unnecessary.

>+      }
>+
>+      let iconHeader = ctypes.cast(cData.addressOfElement(0)
>+                                 , IconHeader.ptr).contents;

Funky comma placement. This happens a few places in this file (along with operator placement), and I don't think it conforms to the Mozilla style guide.

>+      let group = new groupArrayType();
>+      // The group struct has the same header as the .ico file.
>+      for(let curByte = 0; curByte < IconHeader.size; curByte++) {
>+        group[curByte] = new BYTE(data[curByte]);
>+      }

Again, this is unnecessarily slow, but maybe it doesn't matter (the faster approach would be to memcpy the two buffers). Also, no |new Byte|, here.

>+        let resourceID = ctypes.cast(new ctypes.uint64_t(imageIndex + 1), LPWSTR);

Wait, what? We're casting a 64-bit integer to a pointer? That seems wrong.

>+        for(let curByte = 0; curByte < IconResEntry.size; curByte++) {
>+          group[byteIndexRes+curByte] = new BYTE(data[byteIndexDir+curByte]);

Same thing here.

I think this kind of low-level buffer manipulation stuff should be done in C++. In fact, this should probably live in imagelib.

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1 - XPCOM service in xpcom/io

This patch moves the implementation of redit.exe out of xulrunner/tools/redit and into an XPCOM service at xpcom/io, with the contract id "@mozilla.org/file/windows_icon_embedding_service"

Benjamin; you're the module owner for xpcom.  Does that make you the right person to review this code? (and also, have I found the right place in the tree to put this code?)

You'll notice that the build system changes also include changes for the Windows Shortcut Creation service in bug 738501.  That is because these features are being developed together and will likely be submitted together as part of the patch in bug 725408.

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Tim Abraldes from comment #3)
> Created attachment 614044 [details] [diff] [review]
> Patch v1 - XPCOM service in xpcom/io
> 
> This patch moves the implementation of redit.exe out of
> xulrunner/tools/redit and into an XPCOM service at xpcom/io, with the
> contract id "@mozilla.org/file/windows_icon_embedding_service"

I should mention that this patch also includes changes to make the redit code work with unicode paths and adds some resource guarding helper classes.

Comment 5

[Dave Townsend [:mossop]]

I'd rather we didn't delete redit.exe as it is still a useful tool for people building xulrunner apps.

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Dave Townsend (:Mossop) from comment #5)
> I'd rather we didn't delete redit.exe as it is still a useful tool for
> people building xulrunner apps.

Simple enough to revert that change.  I was hoping someone would have an idea for keeping redit.exe alive while only having the code in one location.  Should I make the same changes to redit.cpp as I've made in the copy?

Comment 7

[Dave Townsend [:mossop]]

(In reply to Tim Abraldes from comment #6)
> (In reply to Dave Townsend (:Mossop) from comment #5)
> > I'd rather we didn't delete redit.exe as it is still a useful tool for
> > people building xulrunner apps.
> 
> Simple enough to revert that change.  I was hoping someone would have an
> idea for keeping redit.exe alive while only having the code in one location.
> Should I make the same changes to redit.cpp as I've made in the copy?

I guess you could compile the implementation in one place and then link against it from redit.exe and libxul, but I suspect it isn't worth the effort, I'm guessing there won't be a lot of changes to this code in the future. While you're here might as well update redit with the fixes you have though.

Comment 8

[Benjamin Smedberg]

It's not clear to me that this is a good idea. Don't we need this functionality from the stub itself? That is: when Firefox is updated and webapp launches itself and needs to copy the new version of itself over, doesn't it need to embed the icon again?

What's wrong with just using redit for this task in all cases?

Comment 9

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> It's not clear to me that this is a good idea.
> Don't we need this functionality from the stub itself? That is: when Firefox is updated and
> webapp launches itself and needs to copy the new version of itself over,
> doesn't it need to embed the icon again?

Yes, when the WebAppRT stub (webapprt.exe) copies over a new version of itself and launches it, it must first embed the app's icon in the new EXE file.

We need this functionality in 3 places:
  1) The current location (redit.exe)
  2) The WebAppRT stub (webapprt.exe - see bug 725408)
  3) The WebApp installation code (js code, part of Firefox - see bug 731541)
 
> What's wrong with just using redit for this task in all cases?

That's certainly an option, and is the first implementation we used.  Doing so means packaging redit.exe with Firefox installations and invoking it from locations 2 and 3 in the list above.

Comment 10

[Benjamin Smedberg]

I don't think that's a bad thing. It's kinda unix-y (have small programs which do specific tasks) but I tend to think it's the best solution here.

Comment 11

[Benjamin Smedberg]

Resolving, reopen if necessary.