753123 - Improve redit.exe to handle unicode file paths

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

If we're going to distribute redit.exe with Firefox and use it to embed icons, it has to work with unicode file paths.

Comment 1

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

Attachment: Patch v1 - Build redit as unicode app. Use unicode system calls for filepath manipulation and resource updating. Use RAII classes.

In addition to dealing with unicode filepaths, this patch adds some Mozilla RAII classes to redit which means that redit now links to our glue libraries.  If this feels like overkill, I can revert that part of the patch.

Mossop: Are you the right person to review this patch?  If not, I can get someone from #windev to do the review.

Comment 2

[Dave Townsend [:mossop]]

(In reply to Tim Abraldes from comment #1)
> Created attachment 622592 [details] [diff] [review]
> Patch v1 - Build redit as unicode app.  Use unicode system calls for
> filepath manipulation and resource updating.  Use RAII classes.
> 
> In addition to dealing with unicode filepaths, this patch adds some Mozilla
> RAII classes to redit which means that redit now links to our glue
> libraries.  If this feels like overkill, I can revert that part of the patch.
> 
> Mossop: Are you the right person to review this patch?  If not, I can get
> someone from #windev to do the review.

Would be better to get someone who actually knows the win APIs, maybe jimm?

Comment 3

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

Comment on attachment 622592 [details] [diff] [review]
Patch v1 - Build redit as unicode app.  Use unicode system calls for filepath manipulation and resource updating.  Use RAII classes.

Chatted with jimm in #windev, he'll take over the review

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 622592 [details] [diff] [review]
Patch v1 - Build redit as unicode app.  Use unicode system calls for filepath manipulation and resource updating.  Use RAII classes.

Review of attachment 622592 [details] [diff] [review]:
-----------------------------------------------------------------

couple minor touchups needed.

::: xulrunner/tools/redit/Makefile.in
@@ +25,5 @@
> +					 -DXPCOM_GLUE \
> +					 $(NULL)
> +
> +LIBS = \
> +  $(XPCOM_STANDALONE_STATICRUNTIME_GLUE_LDOPTS) \

Note, either make this bug dependent on bug 732124 or update before you land!

::: xulrunner/tools/redit/redit.cpp
@@ +113,1 @@
>    if (file == -1) {

according to msdn, "A nonzero return value indicates an error" for _wsopen_s so this needs an update as well.

Also nit - '( &file'

@@ +128,4 @@
>  
>    // Open the target library for updating
> +  ScopedResourceUpdate updateRes(BeginUpdateResourceW(argv[1], FALSE));
> +  if (NULL == updateRes) {

Can this really ever be NULL? I think you want to check empty() here.

Comment 5

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

Attachment: Patch v2 - Remove dependency on 732124. Check return value of _wsopen_s.

I've pushed this to try:
  https://tbpl.mozilla.org/?tree=Try&rev=ccab7b5f0c6b


(In reply to Jim Mathies [:jimm] from comment #4)
> Comment on attachment 622592 [details] [diff] [review]
> Patch v1 - Build redit as unicode app.  Use unicode system calls for
> filepath manipulation and resource updating.  Use RAII classes.
> 
> Review of attachment 622592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> couple minor touchups needed.
> 
> ::: xulrunner/tools/redit/Makefile.in
> @@ +25,5 @@
> > +					 -DXPCOM_GLUE \
> > +					 $(NULL)
> > +
> > +LIBS = \
> > +  $(XPCOM_STANDALONE_STATICRUNTIME_GLUE_LDOPTS) \
> 
> Note, either make this bug dependent on bug 732124 or update before you land!

I've changed this to '$(XPCOM_STANDALONE_GLUE_LDOPTS)' but I was hoping bug 732124 would land soon :)

> ::: xulrunner/tools/redit/redit.cpp
> @@ +113,1 @@
> >    if (file == -1) {
> 
> according to msdn, "A nonzero return value indicates an error" for _wsopen_s
> so this needs an update as well.

The MSDN article also says "In the case of an error, -1 will be returned through pfh (unless pfh is a null pointer)."  I've updated the patch to check for either/both.

> Also nit - '( &file'

Done. I've removed the space between the parenthesis and the rest of the line.

> @@ +128,4 @@
> >  
> >    // Open the target library for updating
> > +  ScopedResourceUpdate updateRes(BeginUpdateResourceW(argv[1], FALSE));
> > +  if (NULL == updateRes) {
> 
> Can this really ever be NULL? I think you want to check empty() here.

(NULL == updateRes) will  be true if BeginUpdateResourceW fails (ScopedResourceUpdate implicitly casts to a HANDLE).  I've verified this by making an executable file readonly and attempting to embed an icon in it.  We could equivalently check (ScopedResourceUpdate::empty() == updateRes).

Comment 6

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

Comment on attachment 622921 [details] [diff] [review]
Patch v2 - Remove dependency on 732124. Check return value of _wsopen_s.

https://hg.mozilla.org/integration/mozilla-inbound/rev/984d745ceda7

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/984d745ceda7