The default bug view has changed. See this FAQ.

Improve redit.exe to handle unicode file paths

RESOLVED FIXED in Firefox 15

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: TimAbraldes, Assigned: TimAbraldes)

Tracking

Trunk
Firefox 15
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

If we're going to distribute redit.exe with Firefox and use it to embed icons, it has to work with unicode file paths.
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.
Attachment #622592 - Flags: review?(dtownsend+bugmail)
(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 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
Attachment #622592 - Flags: review?(dtownsend+bugmail) → review?(jmathies)

Comment 4

5 years ago
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.
Attachment #622592 - Flags: review?(jmathies) → review-
Created attachment 622921 [details] [diff] [review]
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).
Attachment #622592 - Attachment is obsolete: true
Attachment #622921 - Flags: review?(jmathies)

Updated

5 years ago
Attachment #622921 - Flags: review?(jmathies) → review+
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
Attachment #622921 - Flags: checkin+
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 15
https://hg.mozilla.org/mozilla-central/rev/984d745ceda7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
No longer blocks: 745988

Updated

5 years ago
Blocks: 745988
Depends on: 767802
No longer depends on: 767802
You need to log in before you can comment on or make changes to this bug.