Closed Bug 411160 Opened 17 years ago Closed 16 years ago

Front-end code must be able to distinguish simple-folder query nodes

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 3 beta3

People

(Reporter: asaf, Assigned: asaf)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
As things stand, if a simple query node is created for a simple place:folder=X, a result node is created with node.itemId set to X rather than the shortcuts's itemId. This makes it impossible for front-end code distinguish shortcut nodes from concrete nodes,  and also causes us (and probably future extensions as well) bugs like bug 406089. It also makes bugs like bug 408660 impossible to fix properly.

Note that for complex query nodes, the correct itemId is used
Attachment #295801 - Flags: review?(dietrich)
Assignee: nobody → mano
Severity: normal → major
Flags: blocking-firefox3?
OS: Mac OS X → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → Firefox 3 M11
Status: NEW → ASSIGNED
Comment on attachment 295801 [details] [diff] [review]
patch


>+    
>+    // We should never expose the history title for query nodes if the
>+    // bookmark-item's title is set to null (the history title may be the
>+    // query string without the place: prefix). Thus we call getItemTitle
>+    // explicitly. Unfurtunatly, query-bookamrks cannot be distinguished in the
>+    // sql query.

s/Unfurtunatly/unfortunately/
s/bookamrks/bookmarks/

generally looks ok though. it's unfortunate that itemId might return different values for seemingly same objects, but i don't see a way of achieving this w/o making things more complicated. a couple of other things:

- can you explain why the UTF8 changes are necessary? fixing this bug doesn't seem to depend on those changes, so would be better to spin that off into a different bug. will it have any affect on consumers such as weave?

- both the browser and toolkit places unit tests crashed in different places w/ this patch applied. might be a local problem, so can you confirm that they're ok on your end?
(In reply to comment #1)
> (From update of attachment 295801 [details] [diff] [review])
> 
> >+    
> >+    // We should never expose the history title for query nodes if the
> >+    // bookmark-item's title is set to null (the history title may be the
> >+    // query string without the place: prefix). Thus we call getItemTitle
> >+    // explicitly. Unfurtunatly, query-bookamrks cannot be distinguished in the
> >+    // sql query.
> 
> s/Unfurtunatly/unfortunately/
> s/bookamrks/bookmarks/
> 

fixed

> 
> - can you explain why the UTF8 changes are necessary? fixing this bug doesn't
> seem to depend on those changes, so would be better to spin that off into a
> different bug. will it have any affect on consumers such as weave?
> 
I didn't want to introduce more conversions. Unfortunately, i had to so in migration code after all (we can tweak that later by using cstring getters, esp. in IEProfileMigrator).

> - both the browser and toolkit places unit tests crashed in different places w/
> this patch applied. might be a local problem, so can you confirm that they're
> ok on your end?
> 

fixed now.
Attached patch v2Splinter Review
Attachment #295801 - Attachment is obsolete: true
Attachment #295960 - Flags: review?(dietrich)
Attachment #295801 - Flags: review?(dietrich)
Comment on attachment 295960 [details] [diff] [review]
v2


>@@ -3335,16 +3348,17 @@ nsNavHistoryFolderResultNode::OnItemRemo
> 
> NS_IMETHODIMP
> nsNavHistoryResultNode::OnItemChanged(PRInt64 aItemId,
>                                       const nsACString& aProperty,
>                                       PRBool aIsAnnotationProperty,
>                                       const nsACString& aValue)
> {
>   if (aProperty.EqualsLiteral("title")) {
>+    // XXX: what should we do if the new title is void?

some people have title-less bookmarks on the toolbar.
Attachment #295960 - Flags: review?(dietrich) → review+
dietrich: title less != void titles.
Flags: blocking-firefox3? → blocking-firefox3+
mozilla/browser/components/migration/src/nsBrowserProfileMigratorUtils.cpp 1.28
mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp 1.69
mozilla/browser/components/migration/src/nsOperaProfileMigrator.cpp 1.75
mozilla/browser/components/migration/src/nsSafariProfileMigrator.cpp 1.48
mozilla/browser/components/places/src/nsPlacesImportExportService.cpp 1.40
mozilla/toolkit/components/places/public/nsINavBookmarksService.idl 1.52
mozilla/toolkit/components/places/src/nsNavBookmarks.cpp 1.139
mozilla/toolkit/components/places/src/nsNavBookmarks.h 1.50
mozilla/toolkit/components/places/src/nsNavHistory.cpp 1.225
mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.126
mozilla/toolkit/components/places/src/nsNavHistoryResult.h 1.52
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 413107
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: