171468 - "Save as type" drop-down box in "Save Page As" dialog always saves as html only in Win9x

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Koike Kazuhiko]

File type dialog in "Save Page As" dialog shows an invalid option
or shows "Web page, complete" only.

This bug is reproducable from 2002092404-trunk/Win98.

Comment 1

[Chris Petersen]

I can't reproduce this problem in the 2002-10-09-08 trunk build. Tested under
Windows ME. Reporter, can you try with a newer trunk build ?

Comment 2

[doctor__j]

I can reproduce the problem in Win32 trunk build 2002111204.

Comment 3

[doctor__j]

Attachment: screenshot showing the problem

Comment 4

[Jun MUTO]

This bug had gone once, since 2002-09-29.
Now it returned after 2002-11-11-04-trunk/Win98SE.

Comment 5

[Jun MUTO]

I suspect patch of bug 104934 would cause this bug.
It was checked in on 09/23/2002 12:16, backed out on 09/28/2002 08:52 
and checked in again on 11/08/2002 14:46-14:47.

This bug can be reproduced between 2002092404-trunk and 20020928??-trunk, 
and 2002111104-trunk or later.
It cannot be seen from 2002092908-trunk to 2002110808-trunk.

Comment 6

[Roy Yokoyama]

reporter: is this Win98/ME only problem?
          Which language OS is this?  

assign bug to myself

Comment 7

[Jun MUTO]

I have only Japanese Windows 98 Second Edition. Therefore I can confirm 
this bug on it.

Per Bugzilla-JP comments, this can be reproduced on Japanese Windows 98, 
not on Japanese Windows XP. There is no report about Japanese Windows Me.
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=2696 

Comment 8

[Warner Young]

Same build (2002112108) exhibits this problem on Win98, but not on Win2K, so it
probably is Win9x only.

Comment 9

[Torben]

*** Bug 181608 has been marked as a duplicate of this bug. ***

Comment 10

[Torben]

I see this on Norwegian Win95 with trunk-build 2002111804. The 1.2 branch-build
2002111817 is not affected. 

Duplicate bug 181608 is reported with en-US win98.

Comment 11

[K Chayka]

Also noted in duplicate bug 181608: The only valid option in the File Types list
is "Web Page, complete" but selecting this saves the page as Text, not HTML,
does not create the _files directory or download any other files.  It behaves
like Save As Text.

Save Page is totally broken. :(

Comment 12

[Roy Yokoyama]

Attachment: Need to give a proper filter length to WideCharToMultiByte()

Calling WideCharToMultiByte() with -1 for the Wstring will
causes to convert the string only to the first null terminated string.
Filter string is a pointer to a buffer containing pairs 
of null-terminated filter strings

We need to find the correct filter string length before
calling WideCharToMultiByte() and provide the length.

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/WinUI/WindowsUserInterface/UserInput/CommonDialogBoxLibrary/CommonDialogBoxReference/CommonDialogBoxStructures/OPENFILENAME.asp


shanjian: can you review?

Comment 13

[Shanjian Li]

Comment on attachment 107420 [details] [diff] [review]
Need to give a proper filter length to WideCharToMultiByte()

Fix the nits and mark r=shanjian.
>+    while ((aFileNameW->lpstrFilter[len] != L'\0') 
>+          || (aFileNameW->lpstrFilter[len+1] != L'\0'))
>+    {
>+      len += 1;
>+    }
    while ((aFileNameW->lpstrFilter[len] != L'\0') ||
	   (aFileNameW->lpstrFilter[len+1] != L'\0'))
    {
      ++len;
    }
>+
>+    len = WideCharToMultiByte(CP_ACP, 0,
>+                                  aFileNameW->lpstrFilter,
>+                                  len,
>+                                  filterA,
>+                                  MAX_FILTER_NAME, NULL, NULL);
    len = WideCharToMultiByte(CP_ACP, 0,
			      aFileNameW->lpstrFilter,
			      len,
			      filterA,
			      MAX_FILTER_NAME, NULL, NULL);
>+    filterA[len] = '\0';
>+    filterA[len+1] = '\0';
>     ofnA.lpstrFilter = filterA; 
>   }
>   if (aFileNameW->lpstrCustomFilter)  {

Comment 14

[Roy Yokoyama]

Attachment: Fixing the nits

Comment 15

[Roy Yokoyama]

Comment on attachment 107642 [details] [diff] [review]
Fixing the nits

as per shanjian's comment.

Comment 16

[kinmoz]

Comment on attachment 107642 [details] [diff] [review]
Fixing the nits

==== Why do we need to check |aFileNameW->lpstrFilter[len+1]!= L'\0'| too? If
we ever hit that expression, that means that [len] was zero, so doesn't that
mean we will be looking at a character beyond the string terminator? Also
|L'\0'| is the same as |0| ... can't we just get away with checking
|while(aFileNameW->lpstrFilter[len])|?


+    while ((aFileNameW->lpstrFilter[len] != L'\0') ||
+	    (aFileNameW->lpstrFilter[len+1] != L'\0'))
+    {
+      ++len;
+    }

Comment 17

[kinmoz]

Comment on attachment 107642 [details] [diff] [review]
Fixing the nits

Ok, duh, I read the URL provided by yokoyama above regarding the fact that the 
string is actually a collection of null terminated strings, plus an extra null
at the end of the last string in the buffer, so  disregard my questions above
about the  [len+1] check and the |strlen()|.

So can we still remove the need for |L'\0'|?

sr=[e-mail]

Comment 18

[Roy Yokoyama]

>So can we still remove the need for |L'\0'|?
Thanks and I'll remove the |L'\0'|.

Comment 19

[Roy Yokoyama]

fix checked into the trunk.

Comment 20

[Jun MUTO]

We can choose types in "Save as type" drop-down box now.

But it does not work at all.
When type is changed, new type does not be effective. The previous 
saving type is still applyed.

Reproducable on 2002-12-03-04-trunk (Win98SE Japanese).

Comment 21

[Hector Fabio Meza]

Confirming comment #20 on win98 spanish with 2002120308, no matter what type
it's selected, the pages is saved as html only. I suggest reopening this bug.

Comment 22

[Torben]

Confirming the behaviour in comment 20 and comment 21, build 2002120308 win95NO.
Reopening.

Comment 23

[Roy Yokoyama]

changing the summary to reflect the real problem.
I'll take a look.

Comment 24

[Roy Yokoyama]

Attachment: assign the new filter index

nsFilePicker needs two elements back from the Open/Save dlgbox.
lpstrFile and nFilterIndex.  We missed the nsFilterIndex. 

shanjian: can you review?

Comment 25

[Roy Yokoyama]

Comment on attachment 108234 [details] [diff] [review]
assign the new filter index

kin: can you super review?

Comment 26

[kinmoz]

Comment on attachment 108234 [details] [diff] [review]
assign the new filter index

sr=[e-mail]

Comment 27

[Roy Yokoyama]

Comment on attachment 108234 [details] [diff] [review]
assign the new filter index

Impact:
- Open/SaveAs dlgbox
- Windows 9x platforms only

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 108234 [details] [diff] [review]
assign the new filter index

a=asa for checkin to 1.3a

Comment 29

[Roy Yokoyama]

fix checked in

Comment 30

[[e-mail]]

Comment on attachment 108234 [details] [diff] [review]
assign the new filter index

>   if (ofnA.lpstrFile) {
>     ConvertAtoW(ofnA.lpstrFile, MAX_PATH, aFileNameW->lpstrFile);
>   }

Hey, I forget the bug number, but is this the code that stops you attaching
multiple files to emails?

Comment 31

[Roy Yokoyama]

neil: not sure.  I never knew such bug exists.  If the code you point out
      is the cause, then the code only gets executed in Win9x.  Is the bug 
      only in Win9x?

Comment 32

[Roy Yokoyama]

neil: Never mind my previous question.  You are correct. I have
      a patch ready for review; but I can't find the mail bug you are referring.
      Do you know the bug number?

Seth: do you recall?  

Comment 33

[Roy Yokoyama]

It's 180333.  I'll attach a patch.

Comment 34

[Chris Petersen]

Since I don't have access to a Win 98 machine, could the reporter please verify
this with the latest trunk ?

Comment 35

[[e-mail]]

I can verify that this is now fixed on Windows 95/98.