150451 - [FIX]Only a ".html" name suggested

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect, P1)

Description

[moz]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 
BuildID:    2002053012

Reproducible: Always
Steps to Reproduce:
1. Go to http://latinmoz.f2g.net/mozillation/
2. File/Save page
3. No name, only a " .html" is suggested. 
There should be a default "index.html", the name of the domain, or whatever. The novices will get confused otherwise. Maybe the name function is not working correctly.

Comment 1

[Sander]

We use the page title as suggested filename when no filename is found in the
url. This page however has the title set to a space.

Maybe we already check for empty titles and if no title is found we default back
to domain (which was the old behaviour), in which case the check should
incorporate a trim, otherwise this check should be added.
Confirming for now, though I'll spend a few more minutes to see if this isn't a
dupe after all.

related: bug 115176

From the patch from there, we do:
if (aDocument && aDocument.title != "") 
    return validateFileName(aDocument.title)    // 2) Use the document title
and that should be something like:
if (aDocument && trim(aDocument.title) != "") 
though I don't know if we have a trim function (I assume we do), and if it's
actually called that. :)

Comment 2

[Andrew Hagen]

No dupes found. Confirming on Mozilla 1.1 alpha 2002060704 Windows 98. 

In this case, maybe we should just use whatever filename we have, even if it's
something like "index.html."

This bug seems to block bug 115634.

Nominating for Mozilla 1.0.1 and nsbeta1. This bug shouldn't be hard to fix.

Comment 3

[Sander]

Attaching a patch momentarily, simply trimming whitespace at the beginning and
end of the title before the check of != "" is applied. (Upon which the suggested
filename reverts back to the [domainname].html)

The patch works (pretty trivial), however, when creating the diff with
patchmaker I got this error:
Warning: no CVS equivalent found for the following files:
comm/content/communicator/contentAreaUtils.js

The CVS version of your patch may not work correctly.
Patch size is 553 bytes

- I have no idea whatsoever what this means and if this could give problems when
applying the patch; hoping someone who does can clear that up.

Comment 4

[Sander]

Attachment: patch for contentAreaUtils.js

Comment 5

[Samir Gehani]

Nav triage team: nsbeta1-

Comment 6

[Boris Zbarsky [:bzbarsky]]

Couldn't this use .match() instead of .replace()?  Something like:

+  if (aDocument && !aDocument.match(/^\s*$/)) 

Or is the idea that a title of " foo " should lead to a name of "foo.html"?  If
that's the idea, does it actually work?

Comment 7

[Sander]

Attachment: minimal solution

Sorry this took a while; was hoping I'd hear something from Gerv on the
patchmaker problem. I haven't, so for now I just looked real close at how the
diff file is constructed and manually changed what I think is the problem, the
file location. In the worst case this won't apply "as is", which it wouldn't
anyway; in the best case this should just work.

As for the replace in the previous version of my patch; yes, this was overkill
- I was too hooked up to the trim() idea. And no, it didn't actually trim on
the eventually suggested filename - only on testing. This minimal patch does
the same, but then utilizing test() so it's a lot more efficient...

Comment 8

[Sander]

Attachment: better patch?

However, your question did set me thinking. We really _should_ trim the title
as well. And what if someone would have a title for a page like this: " * * * *
* * * "? - on windows, after the validateFileName function, this would still
result in a suggested useless filename of "	.html"
So this is a slightly more extensive patch that first calls validateFileName
and then trims the result of that before testing. I'm however not sure if this
is what we really want to do (I think so though), so I'm also providing the
previous minimal patch.

I also set up three testcases so you can see how this behaves on for example
the trimming case and the " * * * * * * * " title.
http://juima.org/stuff/mozilla/test1/
http://juima.org/stuff/mozilla/test2/
http://juima.org/stuff/mozilla/test3/

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 88822 [details] [diff] [review]
better patch?

r=bzbarsky, but rename "aDocTitle" to "docTitle" -- "aFoo" is the naming
convention for function arguments

Comment 10

[Sander]

Attachment: same 'better patch', with changed variable name

Comment 11

[jag (Peter Annema)]

Comment on attachment 89336 [details] [diff] [review]
same 'better patch', with changed variable name

sr=jag

Comment 12

[Boris Zbarsky [:bzbarsky]]

taking this to get it checked in.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Checked in on the trunk.

Comment 14

[Sander]

Boris, what you checked in still does:
if (docTitle != "") {
  return validateFileName(aDocument.title);
}

Yet the point of this second version of the patch was that we'd return the 
trimmed title, so:
if (docTitle != "") {
  return docTitle;
}

(We already do the validate when creating the docTitle:
var docTitle = validateFileName(aDocument.title).replace(/^\s+|\s+$/g, "");
)

Was this a conscious decision, or merely the result of juggling three patches 
all affecting the same 20 lines or so? :)

Comment 15

[Boris Zbarsky [:bzbarsky]]

It was the three patches.  Manual merging and insufficient attention, wonderful
combination...

-      return validateFileName(aDocument.title);
+      return docTitle;

checked in.  Thank you for catching that (and for the patch, of course!).

Comment 16

[sairuh (rarely reading bugmail)]

tested using 2002.09.16.08 comm trunk builds, with cases from comment 8.

on win2k: all tests saved as expected.

on linux rh7.2 and mac 10.1.5: test1 suggested ********.html, test2 suggested
juima.org.html, and test3 suggested ***MOZILLA ROCKS***.html (as expected).