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)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 3 beta3
People
(Reporter: asaf, Assigned: asaf)
References
Details
Attachments
(1 file, 1 obsolete file)
52.95 KB,
patch
|
dietrich
:
review+
|
Details | Diff | 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 | ||
Updated•17 years ago
|
Assignee: nobody → mano
Severity: normal → major
Flags: blocking-firefox3?
OS: Mac OS X → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → Firefox 3 M11
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Comment 1•17 years ago
|
||
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?
Assignee | ||
Comment 2•17 years ago
|
||
(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.
Assignee | ||
Comment 3•17 years ago
|
||
Attachment #295801 -
Attachment is obsolete: true
Attachment #295960 -
Flags: review?(dietrich)
Attachment #295801 -
Flags: review?(dietrich)
Comment 4•17 years ago
|
||
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+
Assignee | ||
Comment 5•16 years ago
|
||
dietrich: title less != void titles.
Updated•16 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Assignee | ||
Comment 6•16 years ago
|
||
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
Comment 7•15 years ago
|
||
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.
Description
•