405296 - Places View for Menus broken after landing of bug 387746

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Ronny Perinke]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.10pre) Gecko/20071122 Firefox/2.0.0.10pre (Sephiroth/SSE2)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9b2pre) Gecko/2007112405 Minefield/3.0b2pre

Uncaught exception, when a submenu of a places-generated menu should open.

Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getFolderIdForItem]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/places/utils.js :: anonymous :: line 1662"  data: no]

Reproducible: Always

Steps to Reproduce:
1. download the attached rar file and extract it in your firefox directory, it
contains a manifest and a xul file

2. restart firefox and open "chrome://bug388148/content/"

3. try out the 3 history menus, where only the third (no grouping) will work
Actual Results:  
exception and the menus of the first two history menus won't open

Expected Results:  
the opposite. no exception and the menus are filled with the history items sorted by date with a folder for each day (or host)

This is similar to bug 388148, which was caused by bug 337855.

Comment 1

[Ronny Perinke]

Attachment: testcase

Comment 2

[Ronny Perinke]

Attachment: patch v1

A quick analyze showed, that the id of the folders for the days/hosts is -1 which is not valid for getFolderIdForItem() (http://mxr.mozilla.org/mozilla/source/toolkit/components/places/tests/bookmarks/test_bookmarks.js#128).

Comment 3

[Asaf Romano (gone)]

Comment on attachment 290096 [details] [diff] [review]
patch v1

>Index: browser/components/places/content/utils.js
>===================================================================
>@@ -1659,17 +1659,17 @@ var PlacesUtils = {
>       else if (this.containerTypes.indexOf(type) != -1) {
>         element = document.createElement("menu");
>         element.setAttribute("container", "true");
> 
>         if (aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY)
>           element.setAttribute("query", "true");
>         if (this.nodeIsLivemarkContainer(aNode))
>           element.setAttribute("livemark", "true");
>-        else if (this.bookmarks
>+        else if (aNode.itemId > 0 && this.bookmarks
>                      .getFolderIdForItem(aNode.itemId) == this.tagsFolderId) {
>             element.setAttribute("tagContainer", "true");
>         }
> 

so, considering non-folder containers, the following path is probably better:

if (aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY)
  element.setAttribute("query", "true");
else if (aNode.itemId !- 1) {
  if (this.nodeIsLivemarkContainer(aNode))
    element.setAttribute("livemark", "true");
  else if (this.bookmarks....)
    element.setAttribute("tagContainer", "true")
}

Comment 4

[Ronny Perinke]

Attachment: patch v2

(In reply to comment #3)
> so, considering non-folder containers, the following path is probably better:
> 
> if (aNode.type == Ci.nsINavHistoryResultNode.RESULT_TYPE_QUERY)
>   element.setAttribute("query", "true");
> else if (aNode.itemId !- 1) {
>   if (this.nodeIsLivemarkContainer(aNode))
>     element.setAttribute("livemark", "true");
>   else if (this.bookmarks....)
>     element.setAttribute("tagContainer", "true")
> }
> 

your probably right, because history folders are no livemark or tag container at all (at least it would not make much sense)

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 292153 [details] [diff] [review]
patch v2

r=me

drivers: very low impact/risk

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 292153 [details] [diff] [review]
patch v2

removing request, it's already a blocker

Comment 7

[Dietrich Ayala (:dietrich)]

check me in once tree allows b4 checkins.

Comment 8

[Reed Loden [:reed]]

Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.101; previous revision: 1.100
done

Comment 9

[Tracy Walker [:tracy]]

Verified with rc2 on Win XP

Comment 10

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv