Last Comment Bug 710878 - Bookmark label can omit part of the text in the link
: Bookmark label can omit part of the text in the link
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 11
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-14 13:46 PST by Paul Rouget [:paul]
Modified: 2011-12-17 00:38 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test (78 bytes, text/html)
2011-12-14 13:46 PST, Paul Rouget [:paul]
no flags Details
patch v1 (1.30 KB, patch)
2011-12-14 13:53 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch with test (3.25 KB, patch)
2011-12-15 10:43 PST, Paul Rouget [:paul]
dietrich: review+
Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2011-12-14 13:46:59 PST
Created attachment 581767 [details]
test

STR:

1) open the attached HTML file
2) right click on the link
3) click "bookmark this link"

result:
In the dialog, the name of the bookmark is "word1 word2".

expected result:
It should be "word1 word2 word3".
Comment 1 Paul Rouget [:paul] 2011-12-14 13:53:07 PST
Created attachment 581771 [details] [diff] [review]
patch v1

Twitter work:

Patch by @DvdBng.
Bug found an reported (with a mxr link to the faulty function) by @manchurian.
Comment 2 Dietrich Ayala (:dietrich) 2011-12-14 15:04:52 PST
Comment on attachment 581771 [details] [diff] [review]
patch v1

Review of attachment 581771 [details] [diff] [review]:
-----------------------------------------------------------------

change looks fine, thanks for fixing this! attach an automated test along with the code change and i can r+ this.
Comment 3 Paul Rouget [:paul] 2011-12-15 10:43:09 PST
Created attachment 582028 [details] [diff] [review]
patch with test
Comment 4 Paul Rouget [:paul] 2011-12-15 10:48:47 PST
Comment on attachment 582028 [details] [diff] [review]
patch with test

Do you think a try run is necessary?
Comment 5 Dietrich Ayala (:dietrich) 2011-12-15 10:53:50 PST
Comment on attachment 582028 [details] [diff] [review]
patch with test

Review of attachment 582028 [details] [diff] [review]:
-----------------------------------------------------------------

looks great, r=me. thanks!

no need for try test on this... assuming it's passing locally ;)
Comment 6 Paul Rouget [:paul] 2011-12-15 10:59:10 PST
(In reply to Dietrich Ayala (:dietrich) from comment #5)
> Comment on attachment 582028 [details] [diff] [review]
> patch with test
> 
> Review of attachment 582028 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks great, r=me. thanks!
> 
> no need for try test on this... assuming it's passing locally ;)

It is! Thank you.
Comment 7 Rob Campbell [:rc] (:robcee) 2011-12-16 06:03:28 PST
https://hg.mozilla.org/integration/fx-team/rev/67500e02c844
Comment 8 Rob Campbell [:rc] (:robcee) 2011-12-16 11:20:14 PST
https://hg.mozilla.org/mozilla-central/rev/67500e02c844
Comment 9 neil@parkwaycc.co.uk 2011-12-16 12:07:32 PST
Happy 7th birthday! I am of course referring to the original bug 274486, which even has a comment describing the correct way to fix the bug rather than monkeypatching gatherTextUnder into a treewalker like we did for Thunderbird in bug 369341 only 4.75 years ago...

Note You need to log in before you can comment on or make changes to this bug.