Last Comment Bug 753123 - Improve redit.exe to handle unicode file paths
: Improve redit.exe to handle unicode file paths
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: Firefox 15
Assigned To: Tim Abraldes [:TimAbraldes] [:tabraldes]
:
Mentors:
Depends on:
Blocks: 745984 745988
  Show dependency treegraph
 
Reported: 2012-05-08 14:47 PDT by Tim Abraldes [:TimAbraldes] [:tabraldes]
Modified: 2012-06-26 18:19 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 - Build redit as unicode app. Use unicode system calls for filepath manipulation and resource updating. Use RAII classes. (10.51 KB, patch)
2012-05-09 17:34 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
jmathies: review-
Details | Diff | Splinter Review
Patch v2 - Remove dependency on 732124. Check return value of _wsopen_s. (10.52 KB, patch)
2012-05-10 14:24 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
jmathies: review+
timabraldes: checkin+
Details | Diff | Splinter Review

Description Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-05-08 14:47:55 PDT
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] 2012-05-09 17:34:59 PDT
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.
Comment 2 Dave Townsend [:mossop] 2012-05-09 17:42:58 PDT
(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] 2012-05-09 18:15:24 PDT
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] 2012-05-10 08:55:58 PDT
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] 2012-05-10 14:24:37 PDT
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).
Comment 6 Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-05-14 16:59:04 PDT
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] 2012-05-15 06:32:55 PDT
https://hg.mozilla.org/mozilla-central/rev/984d745ceda7

Note You need to log in before you can comment on or make changes to this bug.