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 on attachment 152612 [details] [diff] [review] patch rv.1 Sorry this patch is buggy.

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

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.

> 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.

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)

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)'.

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

>> 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?

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

>> 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.

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());

> 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.

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?).

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.

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.

> 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?

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.

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.

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.

> 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...

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."

> 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?

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.

>> 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?

My mistake - I missed the +1.

> // 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.

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 on attachment 152908 [details] [diff] [review] patch rv1.4.1(final?) Sorry my mistake.

> 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 on attachment 152909 [details] [diff] [review] patch rv.1.4.1 Delegating review request to Brodie.

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 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.

> 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 '}'?

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

> 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 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.

> 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.

Fix checked in for Masayuki.

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

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++.

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

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?

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

Brodie: Thank you for the patch. Should I create the patch for definition of |FILEGROUPDESCRIPTORW| and |LPFILEGROUPDESCRIPTORW|?

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.

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

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

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

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].

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 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?

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.

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

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 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 on attachment 153409 [details] [diff] [review] patch for MinGW(Better version?) You need to resolve the licensing issues first. Talk to gerv.

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.

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

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?

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?).

(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.

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

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

Sorry my mistake.

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

"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

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?

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

> Do you accept the patch? It's the module owner's call, I'd say. Gerv

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.

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.

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.

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

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.

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.

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.

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?

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.

!!Even with the latest attachment!!

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.

(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?"...

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.

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)

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.

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

Yes, that patch does remove two warnings. But more importantly, those #defines fix the build for me, thanks :-)

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.

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

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.

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.

Builds are running. Clean shlobj.h and patch applied to nsDataObj.h and nsDataObj.cpp I should have something in a couple of hours...

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 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

(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 on attachment 154326 [details] [diff] [review] mingw patch 2 Doh, I was looking for win32api.h by mistake :-[

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.

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

Checked in and marking fixed as per comment 105.