Closed Bug 150451 Opened 19 years ago Closed 19 years ago

[FIX]Only a ".html" name suggested

Categories

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

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.2alpha

People

(Reporter: moz, Assigned: bzbarsky)

References

()

Details

Attachments

(1 file, 3 obsolete files)

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.
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. :)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Blocks: 115634
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.
Keywords: patch
Attached patch patch for contentAreaUtils.js (obsolete) — Splinter Review
Attachment #87129 - Attachment description: patch for contextAreaUtils.js → patch for contentAreaUtils.js
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → mozilla1.0.1
Nav triage team: nsbeta1-
Keywords: nsbeta1nsbeta1-
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?
Attached patch minimal solution (obsolete) — Splinter Review
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...
Attachment #87129 - Attachment is obsolete: true
Attached patch better patch? (obsolete) — Splinter Review
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 on attachment 88822 [details] [diff] [review]
better patch?

r=bzbarsky, but rename "aDocTitle" to "docTitle" -- "aFoo" is the naming
convention for function arguments
Attachment #88822 - Flags: review+
Attachment #88821 - Attachment is obsolete: true
Attachment #88822 - Attachment is obsolete: true
Attachment #89336 - Flags: review+
Comment on attachment 89336 [details] [diff] [review]
same 'better patch', with changed variable name

sr=jag
Attachment #89336 - Flags: superreview+
taking this to get it checked in.
Assignee: law → bzbarsky
Priority: -- → P1
Summary: Only a ".html" name suggested → [FIX]Only a ".html" name suggested
Target Milestone: mozilla1.0.1 → mozilla1.2alpha
Checked in on the trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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? :)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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!).
Status: REOPENED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
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).
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.