Open
Bug 845363
Opened 11 years ago
Updated 2 years ago
clean up gatherTextUnder
Categories
(Firefox :: Menus, defect)
Firefox
Menus
Tracking
()
NEW
People
(Reporter: areinald.bug, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
Text is appended to a temporary variable before being returned. This variable is initialized on every occurrence of an image which has its alt field filled.
Reporter | ||
Comment 2•11 years ago
|
||
Attachment #718463 -
Flags: review?(paul)
Flags: needinfo?(areinald)
Updated•11 years ago
|
Component: DOM → General
Product: Core → Firefox
Reporter | ||
Comment 3•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #1) > How exactly are you doing this conversion? I don't know when this code is called, I just noticed this obvious bug while reading the code. It may be dead code, and never be called though.
Reporter | ||
Comment 4•11 years ago
|
||
The code seems to simply walk the sub-tree starting at a specified node. No conversion is performed on the way. Only white space is removed before returning the text buffer.
Comment 5•11 years ago
|
||
Looks like this is called from some context menu methods. But what I was really after was which code you were talking about; I didn't realize you already had a patch for it. ;)
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → areinald
Comment 6•11 years ago
|
||
Comment on attachment 718463 [details] [diff] [review] Patch to fix the issue, the alt field text is appended to the "text" variable. I can't review this (wrong :paul I guess).
Attachment #718463 -
Flags: review?(paul)
Comment 7•11 years ago
|
||
Comment on attachment 718463 [details] [diff] [review] Patch to fix the issue, the alt field text is appended to the "text" variable. Paul, you actually have the only blame in this function after the migration to Mercurial, which is why I assume you were fingered. The blame on the original code goes back to CVS rev 1.1, which was blakeross doing initial Firefox UI development back in 2002. I'm going to claim that I'm qualified to review this code. ;) André, do you have push access, or do you need this pushed to inbound? In the latter case, can you please include the bug number in your commit message?
Attachment #718463 -
Flags: review+
Comment 8•11 years ago
|
||
This code is pretty broken in other ways, too: - "altText && altText != """ is redundant - bug 539193 - should use .trim() rather than those regexes to strip whitespace - seems to only be used to gather the text for a link element, so it could probably be simplified
Comment 9•11 years ago
|
||
Gavin, you want to take over the reviews here? You're probably a better fit anyway. ;)
Comment 10•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #7) > Paul, you actually have the only blame in this function after the migration > to Mercurial, which is why I assume you were fingered. > > The blame on the original code goes back to CVS rev 1.1, which was blakeross > doing initial Firefox UI development back in 2002. Another Paul. Back then, I didn't even know what internet was :)
Comment 11•11 years ago
|
||
No, definitely you. The change after Blake's stuff (which was Blake, not you) was bug 710878.
Comment 12•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #11) > No, definitely you. The change after Blake's stuff (which was Blake, not > you) was bug 710878. Damn, you're right. Forgot about this one.
Reporter | ||
Comment 13•11 years ago
|
||
At second thought, the "text = altText" was there on purpose: it meant to use the alt text of an image node, and only use that. But that was probably meant to work only when (depth == 1), not for an image further down the tree. So either I add (depth == 1) as a condition and keep the break instruction, or I remove the break too. My previous patch was halfway and would result in "word1 word2 alt1" instead of "word1 word2 alt1 word3 alt2 word4" I picked the 2nd option, and removed the break. Plus I took into account Gavin's comment and used trim instead of 2 regex replaces.
Attachment #718463 -
Attachment is obsolete: true
Attachment #719040 -
Flags: review?(paul)
Updated•11 years ago
|
Attachment #719040 -
Attachment is patch: true
Comment 14•11 years ago
|
||
I think we should probably step back here and figure out what this function should be doing, given that it's extremely old and crufty. It seems to only be used for: - obtaining a default file name when you alt+click a link to save it (and have browser.altClickSave set to true) - obtaining a default file name when using "save link as" - pre-filling the "Name" field when using "bookmark this link" Wouldn't just using node.textContent be sufficient for most of these cases? We could also optionally support checking for whether "node" is an image and using .alt in that case. Since this is just used as a default file/bookmark name, it really doesn't have to be perfect.
Comment 15•11 years ago
|
||
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #14) > It seems to only be used for: > - obtaining a default file name when you alt+click a link to save it (and > have browser.altClickSave set to true) > - obtaining a default file name when using "save link as" And even in these cases it's very rarely actually used, since the code in contentAreaUtils.js getDefaultFilename() uses it only as a fallback in case the URI being saved is not an nsIURL/doesn't have a .fileName. And in the "save link as", we only even fall back to that code if we hit NS_ERROR_SAVE_LINK_AS_TIMEOUT.
Comment 16•11 years ago
|
||
Comment on attachment 719040 [details] [diff] [review] Replace 1st patch which was flawed and takes into account gavin's comment. (cancelling review until Gavin's comment is addressed)
Attachment #719040 -
Flags: review?(paul)
Comment 18•11 years ago
|
||
The core issue that led to this bug being filed was fixed in bug 906465, but there is still some cleanup needed here (comment 8/comment 14)
Assignee: areinald → nobody
Summary: While converting HTML to text, if an "alt" field is filled for an image element, the buffer is emptied → clean up gatherTextUnder
Updated•11 years ago
|
Component: General → Menus
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•