Closed
Bug 150451
Opened 23 years ago
Closed 22 years ago
[FIX]Only a ".html" name suggested
Categories
(Core Graveyard :: File Handling, defect, P1)
Core Graveyard
File Handling
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.2alpha
People
(Reporter: moz, Assigned: bzbarsky)
References
()
Details
Attachments
(1 file, 3 obsolete files)
732 bytes,
patch
|
svl-bmo
:
review+
jag+mozilla
:
superreview+
|
Details | Diff | Splinter Review |
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
Comment 2•23 years ago
|
||
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
Keywords: mozilla1.0.1,
nsbeta1
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
Attachment #87129 -
Attachment description: patch for contextAreaUtils.js → patch for contentAreaUtils.js
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → mozilla1.0.1
![]() |
Assignee | |
Comment 6•23 years ago
|
||
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?
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
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/
![]() |
Assignee | |
Comment 9•23 years ago
|
||
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+
Comment 10•23 years ago
|
||
Attachment #88821 -
Attachment is obsolete: true
Attachment #88822 -
Attachment is obsolete: true
Attachment #89336 -
Flags: review+
Comment 11•22 years ago
|
||
Comment on attachment 89336 [details] [diff] [review] same 'better patch', with changed variable name sr=jag
Attachment #89336 -
Flags: superreview+
![]() |
Assignee | |
Comment 12•22 years ago
|
||
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
Updated•22 years ago
|
![]() |
Assignee | |
Comment 13•22 years ago
|
||
Checked in on the trunk.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 14•22 years ago
|
||
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 → ---
![]() |
Assignee | |
Comment 15•22 years ago
|
||
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: 22 years ago → 22 years ago
Resolution: --- → FIXED
Comment 16•22 years ago
|
||
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
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•