230349 - Adding multiple history entries to bookmarks fails and context menu reads "Bookmark this Page(L)"

Status: RESOLVED FIXED

(Core Graveyard :: History: Global, defect)

Description

[Andreas Kunz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031209
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6b) Gecko/20031209

As summary says...

Reproducible: Always

Steps to Reproduce:
1. Open bookmarks window (e.g. Ctrl-H)
2. Select two or more history entries (but no 'folder'!)
3. Right-click
4. Read label of context menu

Actual Results:  
"Bookmark this Page(L)"

Expected Results:  
"Add Links to Bookmarks" (or similar)

The label is taken from
http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/locale/en-US/contentAreaCommands.dtd#18
which earlier said "Add to Bookmarks", but was changed:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/xpfe/communicator/resources/locale/en-US&command=DIFF_FRAMESET&file=contentAreaCommands.dtd&rev2=1.15&rev1=1.14
probably without this usage in mind...

Shortcut letter is "L" which makes it look even more strange... therefore I
suggested something with "Link" in it.
This should probably go into history's own files (I cannot think of any other
place where you can bookmark two URLs at once...).

Comment 1

[Andreas Kunz]

Attachment: proposed patch

This is my proposal how to fix this bug:
Put a new label/accesskey for bookmarking more than one selected history
entries into the file historyTreeOverlay.dtd. That file only contains tree
labels until now, but I guess it's the best place for the new label because it
is used in historyTreeOverlay.xul. And only there because only in history there
can be several URLs bookmarked at once. 

The other possibility would have been to put it where it's "brother" entry for
bookmarking _single_ history entries is: 
http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/locale/en-US/contentAreaCommands.dtd#53

If this is considered better, I would change the patch to do so. This would
allow for other users of that string if there ever are some...

This patch also fixed the accesskey: previously only the label was changed, the
accesskey remained the same: "L" (this is why there is a "L" in brackets after
the context menu entry).
With my patch it is still "L", but now there is an "L" in the entry so it looks
normal. So it is now switched from "L" to "L" all the time. This is obviously
not necessary at the moment but allows for changes in the future without
glitches as seen in this bug.

Comment 2

[Andreas Kunz]

Comment on attachment 138658 [details] [diff] [review]
proposed patch

Requesting r= from Neil and sr= from alecf.

If you test this and encounter problems in the context menu it's probably
covered by my patch in bug 133590 or bug 131182 or several other ones (there
are lots of problems there...), so this patch is really most probably not the
culprit.

Don't be reluctant to say it if you'd like me to change my patch in some way.

Comment 3

[Alec Flett]

Comment on attachment 138658 [details] [diff] [review]
proposed patch

sr=alecf

Comment 4

[[e-mail]]

Comment on attachment 138658 [details] [diff] [review]
proposed patch

Unfortunately adding multiple bookmarks doesn't actually work :-/

Comment 5

[Andreas Kunz]

Right, but with this patch it looks nicer... ;-)

(Did I mention that I also don't think I will ever use that feature?)

Comment 6

[Andreas Kunz]

Adding multiple bookmarks fails in 
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/bookmarks.js#1565
because BMSVC is not defined.
The containing function addBookmark is called from history.js with
aShowDialog=false so the bookmarks are added directly.

When only adding one bookmark, aShowDialog is true and that dialog manages the
adding, so this error does not occur.

pch said on IRC that initServices() and initBMService() have to be used to init
BMSVC properly. Neil mentioned that this might be overkill, but adding a single
bookmark also does this before the "Add Bookmark" dialog appears.

The solutions I currently see are:
1) use the "add Bookmark" dialog also for multiple bookmarks. Easiest solution,
but hell for sombody who adds more than a dozen bookmarks at once.
2) init Bookmark Service with initServices() and initBMService()
3) try to find a smaller set of instructions to init BMSVC

Extending bug summary.

Comment 7

[Andreas Kunz]

Calling addBookmark with "undefined" as charset makes the called function use
the charset of the currently open document:

var fw = document.commandDispatcher.focusedWindow;
aCharset = fw.document.characterSet;

(http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/resources/bookmarks.js#1553)

- does this make any sense???

Wouldn't it make more sense to use a "mostly" working charset instead of one
chosen by random?

But I'm not sure what the charset of a bookmark is for, so this might all be
nonsense...

Comment 8

[Andreas Kunz]

Comment on attachment 138658 [details] [diff] [review]
proposed patch

First patch is obsolete. New patch(es) coming...

Comment 9

[Andreas Kunz]

Attachment: Patch v2 A: do the same as the last one plus init the Bookmarks Service

v2 A: do the same as the first patch plus init the Bookmarks Service

Comment 10

[Andreas Kunz]

Attachment: Patch v2 B: do the same as the first one plus do several other things...

v2 A: do the same as the last one plus several other changes:

- as mentioned in comment #7, the charset of the currently open document -
which happens to be "UTF-8" for the history window - is used for the added
bookmarks. 
(Don't know about the sidebar case, but it certainly doesn't produce a better
result)
So I guess using that charset UTF-8 directly cannot hurt. I'm not sure of any
possible problems when a bookmark has a wrong charset attached, but IF there
are any, they already existed for"ever".
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/src/nsBookmarksService.cpp#3226

is where that charset is finally being used, just in case anybody wants to have
a look.

- don't call the bookmarks.js init functions because they init a lot of stuff
we don't need. Instead only init BMSVC and do the adding call to it directly.

- when only one entry is selected to be added, now bookmarks.js cannot do
anything for us, all it does is calling openDialog, so we can do this
ourselves.

Comment 11

[Andreas Kunz]

Comment on attachment 138753 [details] [diff] [review]
Patch v2 B: do the same as the first one plus do several other things...

Requesting Neil's opinion on the two possible patches.

Please say what looks better to you. Combinations of these patches are also
possible.
E.g. I might leave alone the "one bookmark" case (maybe changing to UTF-8) but
use the "multiple bookmarks" solution from the second patch. Or...

Comment 12

[[e-mail]]

I think I'd like pch to comment on the use of BookmarkUtils.addBookmark
(especially given the comment about aShowDialog) and also the charset issue.

Comment 13

[Andreas Kunz]

As a note: firebird also uses "undefined" for charset:
http://lxr.mozilla.org/mozilla/source/browser/components/history/content/history.js#230

But pch probably knows best what to do, so...

Comment 14

[Pierre Chanial]

Sorry for the delay.

- about the aShowDialog flag: addBookmarkImmediately is obsolete, because the
modifications to the bookmarks datasource are not processed through the
transaction manager.

- the behavior of createBookmarks when the charset is obviously bogus, it was
written for the case in which the page is bookmarked from the browser window,
not from another window a sidebar panel.
However, we need to infer a charset... We may look at the cache (if it has this
info) but I don't know what to do (except relying on the automatic charset
detection) by not asserting the charset in the bookmarks datasource. Another
solution comes to my mind.
Do we really need the ability to add a bookmark from the history???
I think that loading the page then bookmarking it is a good workaround, in the
improbable case that the user hasn't already done that.

Comment 15

[Pierre Chanial]

s/don't know what to do/don't know what to do when cache of the page is expired

Comment 16

[[e-mail]]

OK, so two more questions...
1. Does Bookmarks/Bookmark This Page add a bookmark immediately correctly?
2. What charset does Bookmark Manager's File/New/Bookmark use?

Comment 17

[Pierre Chanial]

1) adding a bookmark via Bookmarks > Bookmarks This Page go through
BookmarksUtils.addBookmarkForBrowser that picks the charset of the doc shell but
it's not correctly handled by the transaction manager.

2) adding a new bookmark exhibits the same pb that when adding a bookmark from
the history. The null or "" charset is sent to BookmarksUtils.createBookmark
that  wrongly changes it to the add bookmark window one that turns out to be
"UTF-8" for chrome windows.

We should remove...
    if (!aCharSet) {
      var fw = document.commandDispatcher.focusedWindow;
      if (fw)
        aCharSet = fw.document.characterSet;
    }
... that is really bogus and check copy/paste and drag and drop (see bug 174197,
that relies on the snipet) operations.

Comment 18

[[e-mail]]

Comment on attachment 138753 [details] [diff] [review]
Patch v2 B: do the same as the first one plus do several other things...

I think I would stick to using initBMService and addBookmark for now, but with
a third parameter of null rather than undefined as I think that any implicit or
explicit UTF-8 charset would be wrong. I also don't see why you need both init
calls, and definitely not inside the loop!

Comment 19

[Andreas Kunz]

Attachment: revised patch

Both init calls are necessary because the first one inits some variables used
by the second (namely e.g. RDF). And the second inits the service.

Comment 20

[Andreas Kunz]

Comment on attachment 139381 [details] [diff] [review]
revised patch

Requesting r= from Neil.

Comment 21

[[e-mail]]

Comment on attachment 139381 [details] [diff] [review]
revised patch

>-    BookmarksUtils.addBookmark(url, title, undefined, true);
>+    BookmarksUtils.addBookmark(url, title, null, true);
You need to do this for the multiple bookmark case too.

>+    initServices();
>+    initBMService();
Might be worth wrapping this in an if (!BMSVC)

Comment 22

[Andreas Kunz]

Attachment: again revised

Changes those two things. BMSVC is only _not_ null if you already have
bookmarked several history items during the lifetime of the currently open
history window. No matter what other bookmarks activities are going on. But
yes, it's probably still worth it.

Comment 23

[Andreas Kunz]

Comment on attachment 139419 [details] [diff] [review]
again revised

Requesting r= from Neil.

Comment 24

[Andreas Kunz]

Comment on attachment 139419 [details] [diff] [review]
again revised

Requesting sr= from alecf.

Comment 25

[Alec Flett]

Comment on attachment 139419 [details] [diff] [review]
again revised

sr=alecf

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in