250392 - Support "UniformResourceLocatorW" and "FileGroupDescriptorW" clipboard formats for Internet Shortcut

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/20040702 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/20040702 Firefox/0.8.0+

If the title of document has non-navtive code character,
the internet shortcut caption is broken.
The reason is "FileGroupDescriptorW" is not implemented.

Reproducible: Always
Steps to Reproduce:
1. See http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2316&action=view
2. Drag the icon that is in location bar.
3. Drop to desktop.

Actual Results:  
The Internet Shortcut caption is broken.

Expected Results:  
The Internet Shortcut caption is not broken.

This problem is fixed by only to support "FileGroupDescriptorW".
However, other applications may want Unicode URI when "FileGroupDescriptorW"
data is dropped.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch rv.1

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 152612 [details] [diff] [review]
patch rv.1

Sorry this patch is buggy.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase(very long title)

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch rv.1.1(work in progress)

Comment 5

[Jungshik Shin]

Would the latest patch work on Win 9x/ME without MSLU as it is?

Comment 6

[Brodie]

The current behaviour is actually according to specifications. We remove all
characters which can't be represented in the Ansi codepage. This behaviour is
correct on Windows 9x/ME. On NT we want to supply the unicode version.

In general the best solution for the clipboard is: 
1. always put both ANSI and Unicode versions into the clipboard so the client
can choose the best one for them (thus supporting both Win9x, Win9x+MSLU, and NT). 
2. Always try to extract Unicode first and if not available then ANSI from the
clipboard.

I think it would be better to explicitly name everything as either Ansi or
Unicode versions, rather than blank and W forms. So it would be better to change
CFSTR_FILEDESCRIPTOR to CFSTR_FILEDESCRIPTORA, fileDescriptorFlavor to
fileDescriptorFlavorA, UniformResourceLocator to UniformResourceLocatorA, etc.

It would be better to create a new CreateFilenameFromText function and overload
it for the Unicode version. 

What's the point to moving the following block?

#include <ole2.h>
#ifndef __MINGW32__
#include <urlmon.h>
#endif
#include <shlobj.h>

Why don't you move the code following
  // one file in the file block
into the if statement earlier in the function.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> So it would be better to change
> CFSTR_FILEDESCRIPTOR to CFSTR_FILEDESCRIPTORA, fileDescriptorFlavor to
> fileDescriptorFlavorA, UniformResourceLocator to UniformResourceLocatorA, etc.

O.K.
I will change it.

> What's the point to moving the following block?
The new constants are blocking to build.
Because I re-define the constants in 'DataObj.h'.
I don't know that the 'shlobj.h' having these new constants after what version.

> Why don't you move the code following
>   // one file in the file block
> into the if statement earlier in the function.

Thanks.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase(very long title)

Comment 9

[Brodie]

See also the testcases at bug 103468 which includes amongst others a testcase
for long titles.

> If the title of document has non-navtive code character,
> the internet shortcut caption is broken.

By broken you mean that it removes the characters which do not fit in the
current ANSI code page right? 

Just to clarify that I understand what you are doing...
1. advertise on the drag/drop (IDataObject) that we support both A and W
versions of file descriptors and urls
2. if W version is requested then supply the Unicode version of the shortcut
name (filename sanitized)
3. if A version is requested then supply the ANSI codepage version of the
shortcut name (filename sanitized)

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

fmm...

IE6 set to...
FileDescriptor  .. 258bytes + NULL1byte (exclude extension)
FileDescriptorW .. 254caracters + NULL 1caracter (include extension)

In "A", this behavior is similar current Mozilla behavior,
and it cannot drop to any path.
In "W", this behavior is mysterious.
It can drop to 'c:\' etc. But it cannot drop to desktop.

I propose that the FileDescriptor's maxlength is 'MAX_PATH - Length(DesktopFolder)'.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Brodie:

> By broken you mean that it removes the characters which do not fit in the
> current ANSI code page right? 

Yes.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

>> Why don't you move the code following
>>   // one file in the file block
>> into the if statement earlier in the function.
> 
> Thanks.

I mistook.
What is problem?

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch rv.1.1

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

When I get the desktop path, Bug 239279 block it.
I will re-propose the issue after Bug 239279.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

>> By broken you mean that it removes the characters which do not fit in the
>> current ANSI code page right? 
> Yes.

Sorry. it is wrong.
Un-fit character is not removed, the caracter is replaced to '?'.
Explorer removed '?' in the filename.

Comment 16

[Brodie]

Attachment: use default character

Re: comment #15 
This problem can and should be fixed by the attached patch. Please incorporate
these changes into your bugfix.

Re: comment #10 and comment #14
I have no idea what you are talking about. We don't set any path. Only the
filename is being returned.

Re: comment #12
The lines following the comment "// one file in the file block" can be moved to
earlier in the function where you actually set the fileGroupDescA and
fileGroupDescW pointers. This avoids the extra if() statement.

I'd still prefer to see all |fileDescriptorFlavor| turned into
|fileDescriptorFlavorA|, ditto for |fileDescriptorFlavor|, and
|UniformResourceLocator|. 

There are more differences in CreateFilenameFromText than there are
similarities. Therefore, rename it to CreateFilenameFromTextA and create a new
function CreateFilenameFromTextW for the widechar version. The same for
ExtractUniformResourceLocator.

> memcpy(fileName, NS_LITERAL_STRING("Untitled.URL").get(),
> NS_LITERAL_STRING("Untitled.URL").Length() + sizeof(PRUnichar));

This is better as:
  wcscpy((wchar_t*)fileName, L"Untitled.URL");

I'm assuming that Windows-only means we can directly use L"". If not then:
  wcscpy((wchar_t*)fileName, NS_LITERAL_STRING("Untitled.URL").get());

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> I have no idea what you are talking about. We don't set any path. Only the
> filename is being returned.

If 259bytes or 259chars filename is always faild to make the Internet Shortcut.
Because path string cannot be inserted.

I think that if the FileDescriptor's Filename is short for creating the Internet
Shortcut on desktop folder, users will not fail to create the Internet Shortcut.
# the desktop folder path is long.
So, 'MAX_PATH - Length(DesktopFolder)' is set to the parameter that name is
|aFilenameLen| on |CreateFilenameFromText| function.

I re-think it.
To get desctop folder function is necessary to use A/W API.
So, Bug 239279 is not related this issue.

Please wait. I will create proposal patch.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch rv1.2

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: proposal patch

This patch is based rv1.2.
This patch can create the shortcut of the very long title's page.
But the file cannot delete from explorer(explorer's bug?).

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: proposal patch

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I want to use attachment 152696 [details] [diff] [review].
But I cannot test on Win9x.
Please test on Win9x.

test build:
http://www.d-toybox.com/mozilla/testbuilds/bug3884.zip

test1
See http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2316&action=view
and drop to desktop.
Is the file name "abc(CJK character)abc".
# This test may be impossible if you don't have Japanese font.

test2
See http://bugzilla.mozilla.org/attachment.cgi?id=152656&action=view
and drop to desktop.
Is the file created fine?
And can delete it?
# If the file cannot be deleted by Explorer,
# execute 'delete *.url' from command prompt.

Comment 22

[Ere Maijala (slow)]

According to MSDN, while MAX_PATH is 260, the maximum length of a single
component (filename) in a path is 255 characters. I believe that could be the
reason why a 259 character filename would fail.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> the maximum length of a single
> component (filename) in a path is 255 characters.

Really? I don't know the spec.
I will limit to the length of the filename in next patch.

I think that if light users create the Internet Shortcut,
they drop to desktop folder.
Therefor the filename should be shrink to 'MAX_PATH - Desktop folder length'.

Is there people having objection to this?

Comment 24

[Ere Maijala (slow)]

I don't know if that's necessary at least on UNICODE enabled systems. See 
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/fileio/base/naming_a_file.asp
for more information.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch rv.1.3

This patch is fixing following problems.
1. Support "UniformResourceLocatorW" and "FileGroupDescriptorW".
2. Always convert to '_' on "UniformResourceLocator" if the character is not in
native charset.
3. Max length of "FileGroupDescriptor" and "FileGroupDescriptorW" are shrinked
to the filename that can be created to Desktop folder.

Comment 26

[Ere Maijala (slow)]

You can't know that the shortcut is being dropped to the desktop, right? It
could be any other folder too. Checking the length of the desktop path doesn't
seem right to me.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> the shortcut is being dropped to the desktop, right?

Yes.

> Checking the length of the desktop path doesn't
> seem right to me.

I don't have better idea.
Current and IE cannot create shortcut in any path when the title element's
content is very long. I think this behavior is no good.

The reason of choosing desktop path is:

1. I think that the light users drop to desktop or desktop's subfolder.
 If the target is desktop's subfolder, creating the shortcut is faild.
 But it will be possible that the shortcut drop to the desktop first,
 and rename and move to subfolder.

2. The desktop folder is long. So, some folders will be able to be dropped.

I have another idea, the filename shrink to fixed length; e.x.,
128bytes/characters, 64bytes/characters...

Comment 28

[Brodie]

I object to using the desktop path length as a limit. We don't know where the
user will drop the file. I would support an arbitrary fixed length however. e.g.
128 is fine with me. This gives 64 characters for MBCS filenames in a double
byte characterset (e.g. Japanese), 128 chars for western and Unicode. 

Note that nothing we can do will remove the problem altogether. There will
always be a filename and drop location with the combination puts the path length
over the limit. The entire idea of this filename length limit would be to reduce
the occasions that this happens. It should already be rare, this would make it
more rare.

Note: if MS used Unicode long-path escaping internally in the shell (e.g.
"\\?\c:\...") we wouldn't have to do this, however one of the comments in the
article that ere linked to suggests that they don't always:

"Note that the shell and the file system may have different requirements. It may
be possible to create a path with the API that the shell UI cannot handle."

Comment 29

[Brodie]

> CreateFilenameFromTextW:
>  if (aText.Length() + wcslen(aExtension) + 1 > aFilenameLen)
>     aText.Truncate(aFilenameLen - wcslen(aExtension) - 1);

Just calculate the length of aExtension once and store it. The comparison should
be >= aFilenameLen because you also need room for the \0 character.

> InitializeDesktopFolderPathLength()

Would you summarize what the behaviours are on each platform? It seems from your
comments that:

95: unknown
98: create ok, delete NOT ok
ME: unknown
NT4: unknown
2K: create ok, delete NOT ok
XP: create ok, delete ok
2K3: unknown

create ok == drag and drop url shortcut with title > 250 bytes to desktop.
delete ok == able to delete that shortcut using shell

Is this correct?

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you Brodie.
I will create new patch that filename limit length is  128bytes/characters.

> Is this correct?

Yes. You are right.
I think that the explorer's bug is fixed on WinMe or WinXP.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

>> CreateFilenameFromTextW:
>>  if (aText.Length() + wcslen(aExtension) + 1 > aFilenameLen)
>>     aText.Truncate(aFilenameLen - wcslen(aExtension) - 1);
> 
> The comparison should be >= aFilenameLen because you also need room
> for the \0 character.

What?

aText.Length() ....... exclude NULL character
wcslen(aExtension) ... exclude NULL character
1 .................... NULL character
i.e., aText.Length() + wcslen(aExtension) + 1 ... include NULL character

aFilenameLen ......... include NULL character

I think the comparison is '> aFilenameLen'.
Is this wrong?

Comment 32

[Brodie]

My mistake - I missed the +1. 

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch rv1.4(final?)

Comment 34

[Brodie]

>  // It is not using MAX_PATH. See Bug 250392.
>  const int filenameMaxLength = 128 + 1;

Instead of declaring this twice just make it a global constant. In the comment
describe the reasoning a little more. e.g.

// deliberately not using MAX_PATH. This is because on platforms < XP
// a file created with a long filename may be mishandled by the shell
// resulting in it not being able to be deleted or moved. 
// See bug 250392 for more details.

Comment 35

[Brodie]

A few more nits...

> static CLIPFORMAT UniformResourceLocator = ...

It would be better to also name UniformResourceLocator as
uniformResourceLocatorA (lowercase U + suffix A) to match the other naming
schemes, e.g. fileDescriptorFlavorA, fileDescriptorFlavorW

> GetFileDescriptor ( ... STGMEDIUM& aSTG, PRBool IsUnicode )

Use aIsUnicode to match existing argument naming scheme.

> -  HGLOBAL fileGroupDescHandle =
::GlobalAlloc(GMEM_ZEROINIT|GMEM_SHARE,sizeof(FILEGROUPDESCRIPTOR));
> +  HGLOBAL fileGroupDescHandle;
> +  fileGroupDescHandle =
::GlobalAlloc(GMEM_ZEROINIT|GMEM_SHARE,sizeof(FILEGROUPDESCRIPTORA));

Any reason to split the single line to two? 

> +        !CreateFilenameFromTextA(untitled, ".URL",
fileGroupDescA->fgd[0].cFileName, filenameMaxLength)) {
> +        strcpy(fileGroupDescA->fgd[0].cFileName, "Untitled.URL");

Indent the "!CreateFilenameFromTextA(" a little more than the strcpy so that it
is easier to read. Ditto for the similar line "!CreateFilenameFromTextW(" in the
next function.

> GetUniformResourceLocator

Again change IsUnicode to aIsUnicode to match existing style.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch rv1.4.1(final?)

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 152908 [details] [diff] [review]
patch rv1.4.1(final?)

Sorry my mistake.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch rv.1.4.1

Comment 39

[Brodie]

> MAX_FILEDESCRIPTOR
This is only used in the .cpp file, so probably best to define it there. Name it
NS_MAX_FILEDESCRIPTOR to indicate that it is our own define and not from Windows
headers.

> * CFSTR_SHELLURL is deprecated and non-existing Unicode version.
> * Therefor we are using CFSTR_INETURL instead of CFSTR_SHELLURL.
"Therefor" -> "Therefore"
"and non-existing Unicode version." -> "and doesn't have a Unicode version."

I have just noticed that the error handling for GlobalAlloc/Lock in
ExtractUniformResourceLocatorA/W is not as robust as in
GetFileDescriptorInternetShortcutA/W. Do you want to re-implement the error
handling of ExtractUniformResourceLocatorA/W to handle failure of
GlobalAlloc/Lock in the same way as ExtractUniformResourceLocatorA/W?

Comment #35:
IsUnicode -> aIsUnicode

Comment #35:
UniformResourceLocatorA -> uniformResourceLocatorA (lowercase 'u')

Comment #35:
    if (!GetLocalizedString(NS_LITERAL_STRING("noPageTitle").get(), untitled) ||
fallback
        !CreateFilenameFromTextA(untitled, ".URL",
fileGroupDescA->fgd[0].cFileName, MAX_FILEDESCRIPTOR)) {
        strcpy(fileGroupDescA->fgd[0].cFileName, "Untitled.URL");
    }

update the indenting so the 'if' tests are easier to see.
"!CreateFilenameFromTextA" should be indented more than strcpy. Same for
"!CreateFilenameFromTextW" and wcscpy.

    if (!GetLocalizedString(NS_LITERAL_STRING("noPageTitle").get(), untitled) ||
fallback
            !CreateFilenameFromTextA(untitled, ".URL",
fileGroupDescA->fgd[0].cFileName, MAX_FILEDESCRIPTOR)) {
        strcpy(fileGroupDescA->fgd[0].cFileName, "Untitled.URL");
    }


Otherwise looks good.

Comment 40

[Ere Maijala (slow)]

Comment on attachment 152909 [details] [diff] [review]
patch rv.1.4.1

Delegating review request to Brodie.

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry I missed comment 35.

> Any reason to split the single line to two?

It is my mistake. Because in first beta patch don't separate to A/W any functions.

I create the new patch now, please wait.

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch rv.1.5

Comment 43

[Brodie]

Comment on attachment 152921 [details] [diff] [review]
patch rv.1.5

Would you please move the comment "// one file in the file block" in the
functions GetFileDescriptorInternetShortcutA/W back to above the
"fileGroupDescA/W->cItems = 1;" lines where I think it used to be.

I'm getting warning messages for macro redefinition of the CFSTR_INETURLA/W
macros when building nsDragService.cpp. This is because nsDataObj.h is being
included before system headers. Would you please move those declarations into
nsDataObj.cpp instead.

Wrap the following if() test from GetFileDescriptor in braces just to ensure we
don't have side effects and make it clearer. e.g.

  if ( IsInternetShortcut() ) {
    if ( aIsUnicode )
      res = GetFileDescriptorInternetShortcutW ( aFE, aSTG );
    else
      res = GetFileDescriptorInternetShortcutA ( aFE, aSTG );
  }
  else

The following lines in GetFileDescriptorInternetShortcutA/W are a bit long to
read easily. Would you please rewrap them similar to make them easier to read,
e.g.

  if (!CreateFilenameFromTextW(title, NS_LITERAL_STRING(".URL").get(), 
	    fileGroupDescW->fgd[0].cFileName, NS_MAX_FILEDESCRIPTOR)) {
    nsXPIDLString untitled;
    if (!GetLocalizedString(NS_LITERAL_STRING("noPageTitle").get(), untitled)
||
	!CreateFilenameFromTextW(untitled, NS_LITERAL_STRING(".URL").get(), 
		fileGroupDescW->fgd[0].cFileName, NS_MAX_FILEDESCRIPTOR)) {
      wcscpy(fileGroupDescW->fgd[0].cFileName,
NS_LITERAL_STRING("Untitled.URL").get());
    }
  }


In GetFileDescriptorInternetShortcutA/W:
>  fmetc.cfFormat = RegisterClipboardFormat ( CFSTR_FILEDESCRIPTOR );

This should be CFSTR_FILEDESCRIPTORA/W. Please use a static the same as GetData
does.

The last one is the worst. It still works even with that mismatch (excellent to
see Arabic text drag and drop to the desktop on XP!), but fix that and it's all
done.

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Wrap the following if() test from GetFileDescriptor in braces just to ensure we
> don't have side effects and make it clearer.

Sorry. I cannot understand this paragraph.
Do you said to insert '{' and '}'?

Comment 45

[Brodie]

Sorry I wasn't clear. Yes, I think it is clearer if you add braces around the
inner if/else test.

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> In GetFileDescriptorInternetShortcutA/W:
>>  fmetc.cfFormat = RegisterClipboardFormat ( CFSTR_FILEDESCRIPTOR );
> 
> This should be CFSTR_FILEDESCRIPTORA/W. Please use a static the same as GetData
> does.
> 
> The last one is the worst. It still works even with that mismatch (excellent to
> see Arabic text drag and drop to the desktop on XP!), but fix that and it's all
> done.

Wait.
|fmetc| is not used.
It can be removed.

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch rv.1.6

Comment 48

[Brodie]

Comment on attachment 152936 [details] [diff] [review]
patch rv.1.6

r=brofield

otsukaresama desu. 

This is great. Thanks for your patience and for fixing this Masayuki. It's
great to be able to drag and drop Unicode to the desktop (e.g. Arabic to
Japanese OS).

I assume that you have tested this on a non-Unicode Windows (95, 98 or ME) as
well? I only have XP to test on.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> otsukaresama desu. 

Thank you. I was surprised.
I didn't think that you know Japanese greeting.

> I assume that you have tested this on a non-Unicode Windows (95, 98 or ME) as
> well? I only have XP to test on.

In bugizlla-jp, patch rv.1.3 is tested on Win98.
# Explorer of Win98 is ignore "W" formats.
And I don't change the logic after the test.
Furthermore, I tested always with my tester application and I checked that the
"A" formats doesn't have any problem.

Comment 50

[Ere Maijala (slow)]

Fix checked in for Masayuki.

Comment 51

[David G King]

This seems to have broken Win32/MinGW/cygwin builds due to FILEGROUPDESCRIPTORW
and LPFILEGROUPDESCRIPTORW being undefined. Reopening...

Comment 52

[Brodie]

Would someone please back this out until Masayuki can have a look at it and fix 
it. I am away for the next 5 days and can't do anything to help. Is it only 
MinGW and cygwin that are busted on Windows? It built fine for me and Masayuki 
with VC++.

Comment 53

[Brodie]

This is the problem on creature. The _T() macro isn't defined. 
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1089968220.29127.gz#err0

Remove the _T() from these definitions so they are plain strings.

nsDataObj.h
 58               #ifndef CFSTR_INETURLA
 59               #define CFSTR_INETURLA _T("UniformResourceLocator")
 60               #endif
 61               #ifndef CFSTR_INETURLW
 62               #define CFSTR_INETURLW _T("UniformResourceLocatorW")
 63               #endif

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Umm...

I think that |FILEGROUPDESCRIPTORW| and |LPFILEGROUPDESCRIPTORW| are should be
in libs on MinGW...
It is very serious problem for Internationarization. 

Brodie:
Is the patch rv.1.6 backed out?
Should I create new patch that is for All? or for MinGW?

Comment 55

[Brodie]

Attachment: Fix bustage on creature

Checked in fix for _T() bustage on creature. I don't know anything about the
MinGW/cygwin bustage. I can't find a tinderbox for that.

cvs commit -m "Fix bustage caused by checkin for bug 250392 - Support
"UniformResourceLocatorW"..." nsDataObj.h (in directory
C:\dev\mozilla\widget\src\windows\)
Checking in nsDataObj.h;
/cvsroot/mozilla/widget/src/windows/nsDataObj.h,v  <--	nsDataObj.h
new revision: 1.23; previous revision: 1.22
done

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Brodie:
Thank you for the patch.

Should I create the patch for definition of |FILEGROUPDESCRIPTORW| and
|LPFILEGROUPDESCRIPTORW|?

Comment 57

[Brodie]

MinGW doesn't define the W versions of FILEDESCRIPTOR and FILEGROUPDESCRIPTOR 
and mis-defines the A versions. I've lodged a bug for w32api at:
http://sourceforge.net/tracker/index.php?
func=detail&aid=992313&group_id=2435&atid=102435

Masayuki:
Would you please create a patch for the MinGW build. You will need to ifdef for 
MinGW. Define the W versions of the required structures, and define the 
FILEGROUPDESCRIPTORA to be FILEGROUPDESCRIPTOR. Ditto for FILEDESCRIPTORA.

Comment 58

[Brodie]

It looks like creature is building again. I'm out of here for the next 5 days.

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch for MinGW

This patch is for MinGW(non-tested).
But this is MinGW's bug.
I object that this patch checks in Mozilla...

Comment 60

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch for MinGW

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Please test on MinGW with attachment 153407 [details] [diff] [review].

Comment 62

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch for MinGW(Better version?)

Comment 63

[David G King]

Attachment 153407 [details] [diff] has fixed my MinGW/cygwin build of Mozilla. I have yet to test
Firefox and Thunderbird.

After that, I'll test Attachment 153409 [details] [diff].

Comment 64

[David G King]

Hmmm, after looking at Attachment 153409 [details] [diff], I don't think I need to test it much.
Also, I prefer Attachment 153409 [details] [diff] as it would prevent any bustage caused by
redefining when the problem is fixed. Also, I like the comments that refer back
to the cause of the problem.

Comment 65

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 153409 [details] [diff] [review]
patch for MinGW(Better version?)

Thank you David.

Ere:
I think this patch will be backed out after fixed on MinGW.
Do you think it?

Comment 66

[David G King]

Attachment 153407 [details] [diff] allows be to build Mozilla, Firefox, Thunderbird and Sunbird
under Win32/MinGW/cygwin.

Attachment 153409 [details] [diff] (my preferred patch) works fine on Mozilla. I didn't check the
other 3 due to the minimal changes btw patches.

Comment 67

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

O.K. Thank you David!
It is enough.

Comment 68

[Ere Maijala (slow)]

Comment on attachment 153409 [details] [diff] [review]
patch for MinGW(Better version?)

Eventually it can be removed, but probably not anytime soon. I think it needs
to be there until most people have upgraded to a fixed MingW, and it doesn't do
any real harm. r=ere

Comment 69

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 153409 [details] [diff] [review]
patch for MinGW(Better version?)

blizzard:
This patch comes from VC7's shlobj.h.( I edited indent and '#ifndef')
This patch may have licence problem...

Comment 70

[Christopher Blizzard (:blizzard)]

Comment on attachment 153409 [details] [diff] [review]
patch for MinGW(Better version?)

You need to resolve the licensing issues first.  Talk to gerv.

Comment 71

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Gerv:

This bug is resoluved the first problem.
But the patch made compile error for MinGW.
The reason is that MinGW's library doesn't define FILEDESCRIPTORW struct and
FILEGROUPDESCRIPTORW struct.
So it is MinGW's bug.
http://sourceforge.net/tracker/index.php?func=detail&aid=992313&group_id=2435&atid=102435

But Mozilla can not be built by MinGW. It is no good.
Therefore we need to define these structs.
But the 'W' struct is not written in MSDN. That exists VC's shlobj.h only.
So the structs definision may have licence problem of VC.

Please tell me your opinion.

Comment 72

[Gervase Markham [:gerv]]

It depends under what license you are allowed to use that header file in your
products. I suspect copying code out of it is not allowed - but I can't tell
until I've seen the license which covers it. I don't have a copy of Visual Studio.

However, if it's purely a copyright issue, then what you are not allowed to do
is copy and paste code out of that file. If you or someone else were to figure
out what the structure would need to be (by examining code which used it, for
example) and reconstructed the structure definition, there'd certainly be no
problem. That would be a "clean room implementation".

Gerv

Comment 73

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Gerv:

Thank you for the reply.

> If you or someone else were to figure
> out what the structure would need to be (by examining code which used it, for
> example) and reconstructed the structure definition, there'd certainly be no
> problem. That would be a "clean room implementation".

Sorry I cannot understand well.

Do you said that I understand the structs(by knowledge) and I will rewrite the
definision from MSDN has no problem?
So I should read the MSDN document, I should rewrite the definition from
non-Unicode version struct.(the difference between ANSI version and Unicode
version is always to change 'char' to 'WCHAR'.) It is "clean room
implementation", right? 

Comment 74

[Brodie]

Masayuki:
Why don't you take the public definition of the TCHAR structure from MSDN and 
rewrite it for the W version. You can then submit that code to the w32api 
project as a potential fix as well as include it in our source. As long as it 
is not a copy/paste from MS header files, but is a rewrite from public 
documents (MSDN) we are okay (right?).

Comment 75

[Brodie]

(In reply to comment #59)
> But this is MinGW's bug.
> I object that this patch checks in Mozilla...

Would it be valid to create a patch for the MinGW component w32api (see comment 
#71) and then say that a requirement for building Mozilla with MinGW is that 
this patch is applied first? It would solve this problem now without tainting 
the Mozilla code with unnecessary defines, plus give MinGW a fix for future use.

Comment 76

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Brodie:

In another way, these formats are supported when only these struct defined only...

Comment 77

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch for MinGW

I rewrite it.
But the patch is same as attachment 153409 [details] [diff] [review].

Comment 78

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch for MinGW

Sorry my mistake.

Comment 79

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch for MinGW

Comment 80

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Gerv:

Is this "clean room implementation"?
http://bugzilla.mozilla.org/attachment.cgi?oldid=153409&action=interdiff&newid=153911&headers=1

Comment 81

[Gervase Markham [:gerv]]

"Clean room" work is usually done by someone with no initial knowledge of the
code they are trying to reproduce... I can't tell if the patch you reference is
a clean room patch - it depends how you wrote it!

But yes, if you write it from public MSDN documentation, then that's reverse
engineering for the purposes of interoperability, and I don't think anyone could
complain about that.

Gerv

Comment 82

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Gerv:

I rewrite it from MSDN.
But I cannot prove it. And I know the struct from ShlObj.h before reading MSDN.

I think the patch is clean. But anybody may say "the patch is not clean."...

Do you accept the patch?

Or should I rewrite negative patch that is kill the this clipboard format when
the struct is not defined?

Comment 83

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops...

the structs is 'struct', is not 'macro'.
the patch has compile error with VC.

Umm... 

Comment 84

[Gervase Markham [:gerv]]

> Do you accept the patch?

It's the module owner's call, I'd say.

Gerv

Comment 85

[Simon Montagu :smontagu]

It seems OK to me (as joint module owner). I would suggest that you add a
comment referencing the public MSDN URL that the patch is derived from.

Comment 86

[Simon Montagu :smontagu]

hmm, sorry, maybe I'm not the right module owner to be answering the question. I
was looking at the component the bug is assigned to, rather than the location of
the file being patched.

Comment 87

[Brodie]

I have attached a patch for the MinGW header file shlobj.h in their bug
database. Would someone with MinGW please test this patch and see if it fixes
the MinGW build without the patch to Mozilla code provided by Masayuki? 

bug:
http://sourceforge.net/tracker/index.php?func=detail&aid=992313&group_id=2435&atid=102435

patch:
http://sourceforge.net/tracker/download.php?group_id=2435&atid=102435&file_id=94793&aid=992313

Since the w32api is released under a public domain licence, we could if
necessary copy this patch and modify it for use in our code. I would prefer to
say that in order to build Mozilla using MinGW you need to install the
appropriate patches for MinGW bugs.

Comment 88

[David G King]

Brodie: Refer comment #66.  If you want me to test with new, but functionally
equivalent patches, I can do that.

Comment 89

[Brodie]

David: the patch I have created is for the MinGW file shlobj.h itself, not for
Mozilla code. This fixes the problem that Masayuki is trying to work around. I
would appreciate if you would test it against your MinGW installation. You can
download it from the sourceforge site as listed above. Please ensure that you
remove Masayuki's patch first so that nsDataObj.h is unmodified when you build.

Comment 90

[David G King]

OK, I misunderstood. I'll pull a clean version of nsDataObj.h and get the
patched copy of shlobj.h

MinGW builds are slow, so I'll report back in a few hours.

Comment 91

[David G King]

Building Mozilla, Firefox and Thunderbird with an unpatched nsDataObj.h and a
patched mingw/include/shlobj.h works fine under Win32/MinGW/cygwin.

I'm still building Sunbird, but I doubt it will have any problems.

So, there are two solutions to this problem. Patch nsDataObj.h or patch
shlobj.h, patching shlobj is probably the best way, but I'm not sure if it is
the most expedient way.

Comment 92

[[e-mail]]

Even with the latest attachment I still can't build MinGW:
c:/mozilla/widget/src/windows/nsDataObj.cpp:82: warning: `CLSID_nsDataObj'
   initialized and declared `extern'
c:/mozilla/widget/src/windows/nsDataObj.cpp:82: warning: `__cdecl__' attribute
   only applies to function types
c:/mozilla/widget/src/windows/nsDataObj.cpp: In member function `virtual
   HRESULT nsDataObj::GetData(tagFORMATETC*, tagSTGMEDIUM*)':
c:/mozilla/widget/src/windows/nsDataObj.cpp:197: `CFSTR_FILEDESCRIPTORA'
   undeclared (first use this function)
c:/mozilla/widget/src/windows/nsDataObj.cpp:197: (Each undeclared identifier is
   reported only once for each function it appears in.)
c:/mozilla/widget/src/windows/nsDataObj.cpp:198: `CFSTR_FILEDESCRIPTORW'
   undeclared (first use this function)
c:/mozilla/widget/src/windows/nsDataObj.cpp: In function `void
   MangleTextToValidFilename(nsString&)':
c:/mozilla/widget/src/windows/nsDataObj.cpp:519: warning: comparison between
   signed and unsigned integer expressions
c:/mozilla/widget/src/windows/nsDataObj.cpp:523: warning: comparison between
   signed and unsigned integer expressions

Do I need another upgrade to MinGW?

Comment 93

[David G King]

The current method to get a build to work AFAIK is to apply the patch to
shlobj.h in mingw/include, as referred to in Comment #87. There is currently no
official MinGW release with this fix included.

Comment 94

[[e-mail]]

!!Even with the latest attachment!!

Comment 95

[David G King]

As detailed in Comment #89, use an unpatched nsDataObj.h and patch shlobj.h

However, this assumes you are not using MSVC. If you are using MSVC, then you
don't need to patch either file.

Comment 96

[[e-mail]]

(In reply to comment #95)
>As detailed in Comment #89, use an unpatched nsDataObj.h and patch shlobj.h
Yes, I tried both ways with the same errors before posting...
>However, this assumes you are not using MSVC.
Which is why I asked "Do I need another upgrade to MinGW?"...

Comment 97

[Brodie]

Attachment: Fix last 2 warnings of Neil's MinGW build

This will fix the last 2 warnings that Neil is getting. I wish MSVC would warn
consistently about this too... Not sure how to fix the first 2 warnings.

Neil & David: what version of MinGW are you using? David is obviously getting a
build with his version which must have those symbols defined. These symbols are
also defined in the current CVS version of shlobj.h in MinGW w32api.

Comment 98

[[e-mail]]

Don't worry about the warnings if they were there before, it's the errors that
are stopping me compiling!

The build page gives the following requirements:
# gcc >= 3.2.2 (20030208)
# binutils >= 2.14.90 (20030807)
# w32api >= 2.4
# mingw-runtime >= 2.4

g++ -v gives me various stuff, ending with:
gcc version 3.2.3 (mingw special 20030504-1)

Comment 99

[David G King]

I'm using MinGW gcc v3.4.1

Also, there are 2 seperate shlobj.h (which seem to be identical). One in
mingw/include, and one in cygwin/usr/include/w32api, make sure the patched one
is first in your environment setup.

Comment 100

[Brodie]

Neil: Obviously the warnings are not stopping you build, it's always good to 
get rid of them though. Let me know if the patch fixes the warnings for you. If 
you define the missing symbols in nsDataObj.h as follows, does it fix the build 
for you?

#ifndef CFSTR_FILEDESCRIPTORA
#define CFSTR_FILEDESCRIPTORA  "FileGroupDescriptor"
#endif
#ifndef CFSTR_FILEDESCRIPTORW
#define CFSTR_FILEDESCRIPTORW  "FileGroupDescriptorW"
#endif

Comment 101

[[e-mail]]

Yes, that patch does remove two warnings.

But more importantly, those #defines fix the build for me, thanks :-)

Comment 102

[Brodie]

Attachment: Fix warnings and MinGW build

Neil, here is the final patch with the fix for both the warnings and the build.
You've basically already reviewed it by testing it. Would you set the flags for
me and I'll check it in.

Comment 103

[Brodie]

Additionally, the bug I added to MinGW for shlobj.h missing these defines has 
been checked in to their CVS. What is the desired way to fix the MinGW build 
for this bug? Tell people to upgrade to the latest w32api version or fix it in 
Mozilla source? If the latter then we also need to merge in use Masayuki's 
patch.

http://sourceforge.net/tracker/?func=detail&atid=102435&aid=992313&group_id=2435

Comment 104

[Dan Mosedale (:dmosedale, :dmose)]

Brodie: I would suggest that we fix this in the Mozilla source for now, and then
as soon as MinGW puts out a new release with the necessary bits, we back out the
fix from Mozilla and insert a requirement on that new version of MinGW.

Comment 105

[Brodie]

Attachment: mingw patch 2

Ok. This patch combines my patch for Neil's problems with w32api 2.4 (warnings
and errors) and Masayuki's patch. I have copied and pasted the definition of
the new W structures from the new w32api/shlobj.h header file (thus should be
no licencing problems). Masayuki's patch broke the msvc build for me because he
was testing for typedef'd symbols with ifdef (is there a way to test for
typedef'd symbols?). To fix that I wrapped the definition with if MINGW and
w32api version checks.

David and Neil, would you please test this patch using a fresh unpatched copy
of w32api/shlobj.h. I want to close this bug off.

Comment 106

[David G King]

Builds are running. Clean shlobj.h and patch applied to nsDataObj.h and
nsDataObj.cpp

I should have something in a couple of hours...

Comment 107

[David G King]

Mozilla and Thunderbird builds on Win32/MinGW/cygwin have finished with no
errors (and a few less warnings). Firefox and Sunbird are still building,
however they both built nsDataObj.o with no errors.

It looks good to me.

Comment 108

[[e-mail]]

Comment on attachment 154326 [details] [diff] [review]
mingw patch 2

I wasn't able to find a w32api.h later than 2.5 so I was wondering why you were
comparing to version 3.0

Comment 109

[Brodie]

(In reply to comment #108)

Just to confuse everyone, it seems that the official CVS repository for w32api 
is at redhat not sourceforge. The latest version of w32api.h at redhat is 
version 3.0. So the next version to include all of these updates I am assuming 
will be the next one after that.

http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/winsup/w32api/include/?
cvsroot=src#dirlist

Comment 110

[[e-mail]]

Comment on attachment 154326 [details] [diff] [review]
mingw patch 2

Doh, I was looking for win32api.h by mistake :-[

Comment 111

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 154326 [details] [diff] [review]
mingw patch 2

>-  int nameLen;
>-  for (int n = 0; n < NS_ARRAY_LENGTH(forbiddenNames); ++n) {
>-    nameLen = strlen(forbiddenNames[n]);
>+  PRUint32 nameLen;
>+  for (size_t n = 0; n < NS_ARRAY_LENGTH(forbiddenNames); ++n) {
>+    nameLen = (PRUint32) strlen(forbiddenNames[n]);

Use a C++-style cast here (ie NS_STATIC_CAST), if you would.

With that change, sr=dmose.  I'd suggest either keeping this bug open or
immediately opening a new bug with regard to removing those lines of code once
the new MinGW version is released.

Comment 112

[Martijn Wargers (dead)]

The "mingw patch 2" has r+ and sr+, so it can be checked in?

Comment 113

[[e-mail]]

Checked in and marking fixed as per comment 105.