Closed
Bug 627487
Opened 14 years ago
Closed 11 years ago
Add page and bookmark GUIDs to nsINavHistoryResultNode
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: philikon, Assigned: raymondlee)
References
(Depends on 1 open bug)
Details
(Whiteboard: [places-next-wanted])
Attachments
(1 file, 8 obsolete files)
(deleted),
patch
|
Gavin
:
superreview+
raymondlee
:
checkin+
|
Details | Diff | Splinter Review |
Bookmarks now have 12 character GUIDs which are used by Sync. Would make life easier for Sync if they were contained in the JSON backups.
Updated•14 years ago
|
Whiteboard: [places-next-wanted]
Assignee | ||
Comment 1•12 years ago
|
||
Any suggestions how I can obtain the bookmark guids? Both nsINavHistoryResultNode and nsINavBookmarksService don't provide a property or method to retrieve guid.
Doing a sql statement in PlaceUtils.jsm for each bookmark to get guid doesn't seem to be an option.
Comment 2•12 years ago
|
||
We can and should add it to nsINavHistoryResultNode, the only thing to figure out is whether we can do with just one guid attribute and associate to it either the bookmark or the page guid, or we should rather add both pageGuid and bookmarkGuid.
I suppose the latter is better since doing the former we may in future end up in need of the other guid (I quite hate having 2 different guids, but this is where we ended and changing it now would break consumers).
Shouldn't be too hard, matter of adding other 2 fields to all of the queries. In this changeset I added hidden, so should be good to mark points to modify in nsNavHistory.cpp and nsNavBookmarks.cpp: http://hg.mozilla.org/mozilla-central/rev/5356d4e7814c
Then one has to change RowToResult to set the found values into the node. There's no other way to do the same without changing all of those points, this code needs a rewrite from quite some time but it's pretty large and thus non-trivial.
Finally it would need a test running an history query and a bookmarks query and checking the guids make sense.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → raymond
Comment 3•12 years ago
|
||
Any progress on this, Raymond?
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Richard Newman [:rnewman] from comment #3)
> Any progress on this, Raymond?
Sorry, I am working on other stuff. Feel free to take it.
Assignee: raymond → nobody
Comment 5•12 years ago
|
||
Hey Raymond, we think this bug may actually help the development of PICL project and would like to ask if you may work on it
Flags: needinfo?(raymond)
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #5)
> Hey Raymond, we think this bug may actually help the development of PICL
> project and would like to ask if you may work on it
I am not very familiar with c++ so I will need some guidance for this. I will work on this next week based on comment 2
Flags: needinfo?(raymond)
Assignee | ||
Comment 7•12 years ago
|
||
I have tried to work on a patch for this but I am stuck with a compile error, and I am not clear how to fix it. I believe it's pretty straight for proficient C++ programmer.
Marco: could you give me some suggestions please?
Attachment #764811 -
Flags: feedback?(mak77)
Comment 8•11 years ago
|
||
Comment on attachment 764811 [details] [diff] [review]
WIP
I'm forwarding the request to mano since I don't have enough time atm to give prompt help.
Btw, I don't understand why you are querying for pages GUIDs, when the only thing you should need for bookmarks backups are bookmarks GUIds.
Attachment #764811 -
Flags: feedback?(mak77) → feedback?(mano)
Comment 9•11 years ago
|
||
Comment on attachment 764811 [details] [diff] [review]
WIP
It doesn't compile because you've not added it to the headers. See nsNavHistoryResult.h.
There are some macros there which you may find useful.
>diff --git a/toolkit/components/places/nsINavHistoryService.idl b/toolkit/components/places/nsINavHistoryService.idl
>--- a/toolkit/components/places/nsINavHistoryService.idl
>+++ b/toolkit/components/places/nsINavHistoryService.idl
>-[scriptable, uuid(081452e5-be5c-4038-a5ea-f1f34cb6fd81)]
>+[scriptable, uuid(fd9e65a3-3710-41de-9697-b5a43c98e74a)]
> interface nsINavHistoryResultNode : nsISupports
> {
> /**
> * Indentifies the parent result node in the result set. This is null for
> * top level nodes.
> */
> readonly attribute nsINavHistoryContainerResultNode parent;
>
>@@ -149,16 +149,21 @@ interface nsINavHistoryResultNode : nsIS
> */
> readonly attribute PRTime lastModified;
>
> /**
> * For uri nodes, this is a sorted list of the tags, delimited with commans,
> * for the uri represented by this node. Otherwise this is an empty string.
> */
> readonly attribute AString tags;
>+
>+ /**
>+ * The unique ID associated with the page.
>+ */
>+ readonly attribute AString guid;
There are two kinds of guids: page (uri) guids, and bookmarks guids, and I guess that for the purpose of this patch you actually want the later.
I suggest adding both to the interface (pageGUID, bookmarkGUID). bookmarkGUID should be empty for non-bookmark nodes.
>@@ -592,17 +597,17 @@ interface nsINavHistoryResult : nsISuppo
>
> /**
> * Similar to nsIRDFObserver for history. Note that we don't pass the data
> * source since that is always the global history.
> *
> * DANGER! If you are in the middle of a batch transaction, there may be a
> * database transaction active. You can still access the DB, but be careful.
> */
>-[scriptable, uuid(45e2970b-9b00-4473-9938-39d6beaf4248)]
>+[scriptable, uuid(830e9ba0-9164-484a-9c24-c87c4a4d54df)]
why? Nothing changed here.
>diff --git a/toolkit/components/places/nsNavBookmarks.cpp b/toolkit/components/places/nsNavBookmarks.cpp
>--- a/toolkit/components/places/nsNavBookmarks.cpp
>+++ b/toolkit/components/places/nsNavBookmarks.cpp
>@@ -49,16 +49,17 @@
>
> using namespace mozilla;
>
> // These columns sit to the right of the kGetInfoIndex_* columns.
> const int32_t nsNavBookmarks::kGetChildrenIndex_Position = 14;
> const int32_t nsNavBookmarks::kGetChildrenIndex_Type = 15;
> const int32_t nsNavBookmarks::kGetChildrenIndex_PlaceID = 16;
> const int32_t nsNavBookmarks::kGetChildrenIndex_Guid = 17;
>+const int32_t nsNavBookmarks::kGetChildrenIndex_PageGuid = 18;
>
The places queries already have the guid, why add it again?
Attachment #764811 -
Flags: feedback?(mano) → feedback-
Assignee | ||
Comment 10•11 years ago
|
||
Mano: I am able to compile at the end. I've added a test test_pageGuid_bookmarkGuid.js but the BookmarkGuid always returns 1 instead of random generated id like Bpk9kEsdqKk2, the PageGuid returns the similar thing. I believe it's something to do with the string type I used. Could you give me some suggestions how I can fix this please?
Attachment #764811 -
Attachment is obsolete: true
Attachment #781642 -
Flags: feedback?(mano)
Comment 11•11 years ago
|
||
Comment on attachment 781642 [details] [diff] [review]
WIP 2
I don't think it has anything to do with the string type, but rather with the fact that you didn't keep the query fields in sync with the list that's in nsNavBookmarks.cpp
53 const int32_t nsNavBookmarks::kGetChildrenIndex_Position = 14;
54 const int32_t nsNavBookmarks::kGetChildrenIndex_Type = 15;
55 const int32_t nsNavBookmarks::kGetChildrenIndex_PlaceID = 16;
56 const int32_t nsNavBookmarks::kGetChildrenIndex_Guid = 17;
So, that's for why it doesn't work (14 and 15 are already used, you'd need to adjust the indexes in nsNavBookmarks), but also keep in mind the last field above, which already selects the bookmark guid.
Generally speaking, when you tackle this sort of problems it's always a good idea to try pinging either me or Marco in #places. That's faster than waiting for feedback/review.
Attachment #781642 -
Flags: feedback?(mano) → feedback-
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Mano from comment #11)
> Comment on attachment 781642 [details] [diff] [review]
> WIP 2
>
> I don't think it has anything to do with the string type, but rather with
> the fact that you didn't keep the query fields in sync with the list that's
> in nsNavBookmarks.cpp
>
> 53 const int32_t nsNavBookmarks::kGetChildrenIndex_Position = 14;
> 54 const int32_t nsNavBookmarks::kGetChildrenIndex_Type = 15;
> 55 const int32_t nsNavBookmarks::kGetChildrenIndex_PlaceID = 16;
> 56 const int32_t nsNavBookmarks::kGetChildrenIndex_Guid = 17;
>
> So, that's for why it doesn't work (14 and 15 are already used, you'd need
> to adjust the indexes in nsNavBookmarks), but also keep in mind the last
> field above, which already selects the bookmark guid.
>
Thanks for the info.
> Generally speaking, when you tackle this sort of problems it's always a good
> idea to try pinging either me or Marco in #places. That's faster than
> waiting for feedback/review.
I will do that next time :-)
Assignee | ||
Comment 13•11 years ago
|
||
It returns the correct bookmark and Page GUIDs. Working on fixing the failing tests.
Attachment #781642 -
Attachment is obsolete: true
Assignee | ||
Comment 14•11 years ago
|
||
All tests passed in my machine.
Attachment #782575 -
Attachment is obsolete: true
Attachment #783063 -
Flags: review?(mano)
Comment 15•11 years ago
|
||
Comment on attachment 783063 [details] [diff] [review]
v1
Review of attachment 783063 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/places/nsINavHistoryService.idl
@@ +22,5 @@
> interface nsINavHistoryQueryOptions;
> interface nsINavHistoryResult;
> interface nsINavHistoryBatchCallback;
>
> +[scriptable, uuid(91d104bb-17ef-404b-9f9a-d9ed8de6824c)]
You need to change the guid for all the interfaces that inherit this one.
::: toolkit/components/places/nsNavBookmarks.h
@@ -368,5 @@
> static const int32_t kGetChildrenIndex_Position;
> static const int32_t kGetChildrenIndex_Type;
> static const int32_t kGetChildrenIndex_PlaceID;
> - static const int32_t kGetChildrenIndex_FolderTitle;
> - static const int32_t kGetChildrenIndex_Guid;
I don't get this. It's nsNavBookmarks that should select the bookmark-related fields.
::: toolkit/components/places/nsNavHistory.cpp
@@ +250,5 @@
> const int32_t nsNavHistory::kGetInfoIndex_ItemTags = 11;
> const int32_t nsNavHistory::kGetInfoIndex_Frecency = 12;
> const int32_t nsNavHistory::kGetInfoIndex_Hidden = 13;
> +const int32_t nsNavHistory::kGetInfoIndex_PageGuid = 14;
> +const int32_t nsNavHistory::kGetInfoIndex_BookmarkGuid = 15;
BookmarkGUID belongs to nsNavBookmarks (and it was already selected).
Attachment #783063 -
Flags: review?(mano) → review-
Assignee | ||
Comment 16•11 years ago
|
||
Updated based on on the comment #15
Attachment #783642 -
Flags: review?(mano)
Assignee | ||
Updated•11 years ago
|
Attachment #783642 -
Attachment description: bug-627487-v2.patch → v2
Assignee | ||
Updated•11 years ago
|
Attachment #783063 -
Attachment is obsolete: true
Comment 17•11 years ago
|
||
Comment on attachment 783642 [details] [diff] [review]
v2
Review of attachment 783642 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/places/nsINavHistoryService.idl
@@ +157,5 @@
> +
> + /**
> + * The unique ID associated with the page.
> + */
> + readonly attribute AUTF8String pageGuid;
ACString as we do in the observer interface.
@@ +161,5 @@
> + readonly attribute AUTF8String pageGuid;
> +
> + /**
> + * The unique ID associated with the bookmark. It returns null if the result
> + * node is not associated with a bookmark.
s/null/an empty string/
@@ +163,5 @@
> + /**
> + * The unique ID associated with the bookmark. It returns null if the result
> + * node is not associated with a bookmark.
> + */
> + readonly attribute AUTF8String bookmarkGuid;
ACString
::: toolkit/components/places/nsNavBookmarks.h
@@ +368,5 @@
> static const int32_t kGetChildrenIndex_Position;
> static const int32_t kGetChildrenIndex_Type;
> static const int32_t kGetChildrenIndex_PlaceID;
> + public:
> + static const int32_t kGetChildrenIndex_Guid;
public?
::: toolkit/components/places/nsNavHistory.cpp
@@ +3864,5 @@
> + nsAutoCString bookmarkGuid;
> + rv = aRow->GetUTF8String(nsNavBookmarks::kGetChildrenIndex_Guid, bookmarkGuid);
> + NS_ENSURE_SUCCESS(rv, rv);
> + resultNode->mBookmarkGuid = bookmarkGuid;
> +
I'm not sure why you're handling bookmarks guids here. Bookmark-specific stuff is handled in nsNavBookmarks::GetDescendantChildren
(And there we already take care of bookmark guids)
@@ +3887,5 @@
> + nsAutoCString bookmarkGuid;
> + rv = aRow->GetUTF8String(nsNavBookmarks::kGetChildrenIndex_Guid, bookmarkGuid);
> + NS_ENSURE_SUCCESS(rv, rv);
> + resultNode->mBookmarkGuid = bookmarkGuid;
> +
ditto.
::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +101,5 @@
> mFrecency(0),
> mHidden(false),
> + mTransitionType(0),
> + mPageGuid(nullptr),
> + mBookmarkGuid(nullptr)
Don't initialize strings to null.
::: toolkit/components/places/nsNavHistoryResult.h
@@ +190,5 @@
> // is a node (nsNavHistoryResult inherits from this), as well as every
> // leaf and branch on the tree.
>
> #define NS_NAVHISTORYRESULTNODE_IID \
> + { 0xd7a58c5b, 0xfe1b, 0x4c89, { 0xbd, 0x54, 0x7a, 0xf6, 0xa4, 0x9a, 0x2b, 0x95 } }
unnecessary
Attachment #783642 -
Flags: review?(mano) → review-
Assignee | ||
Comment 18•11 years ago
|
||
(In reply to Mano from comment #17)
> Comment on attachment 783642 [details] [diff] [review]
> v2
>
> ::: toolkit/components/places/nsNavBookmarks.h
> @@ +368,5 @@
> > static const int32_t kGetChildrenIndex_Position;
> > static const int32_t kGetChildrenIndex_Type;
> > static const int32_t kGetChildrenIndex_PlaceID;
> > + public:
> > + static const int32_t kGetChildrenIndex_Guid;
>
> public?
>
> ::: toolkit/components/places/nsNavHistory.cpp
> @@ +3864,5 @@
> > + nsAutoCString bookmarkGuid;
> > + rv = aRow->GetUTF8String(nsNavBookmarks::kGetChildrenIndex_Guid, bookmarkGuid);
> > + NS_ENSURE_SUCCESS(rv, rv);
> > + resultNode->mBookmarkGuid = bookmarkGuid;
> > +
>
> I'm not sure why you're handling bookmarks guids here. Bookmark-specific
> stuff is handled in nsNavBookmarks::GetDescendantChildren
>
> (And there we already take care of bookmark guids)
>
> @@ +3887,5 @@
> > + nsAutoCString bookmarkGuid;
> > + rv = aRow->GetUTF8String(nsNavBookmarks::kGetChildrenIndex_Guid, bookmarkGuid);
> > + NS_ENSURE_SUCCESS(rv, rv);
> > + resultNode->mBookmarkGuid = bookmarkGuid;
> > +
>
> ditto.
>
If we remove the above code in nsNavHistory.cpp, the root.getChild(0).bookmarkGuid would return empty string instead of the guid. Is it the expected result?
== test_pageGuid_bookmarkGuid.js ==
+ // add a bookmark to the folder
+ let b1 = bmsvc.insertBookmark(folder, uri("http://test1.com/"),
+ bmsvc.DEFAULT_INDEX, "1 title");
+ let b2 = bmsvc.insertBookmark(folder, uri("http://test2.com/"),
+ bmsvc.DEFAULT_INDEX, "2 title");
+ let b3 = bmsvc.insertBookmark(folder, uri("http://test3.com/"),
+ bmsvc.DEFAULT_INDEX, "3 title");
+
+ let options = histsvc.getNewQueryOptions();
+ options.queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS;
+ let query = histsvc.getNewQuery();
+ query.setFolders([folder], 1);
+ let result = histsvc.executeQuery(query, options);
+ let root = result.root;
+ root.containerOpen = true;
+ do_check_eq(root.childCount, 3);
+
+ // check bookmark guids
+ let bookmarkGuidZero = root.getChild(0).bookmarkGuid;
+ do_check_eq(bookmarkGuidZero.length, 0);
Flags: needinfo?(mano)
Comment 19•11 years ago
|
||
Of course not.
You cannot just remove the code, you need to make nsNavBookmarks::GetDescendantChildren set the bookmarkGUID attribute (as it does for position). AFAICT all bookmark nodes are generated there.
Flags: needinfo?(mano)
Assignee | ||
Comment 20•11 years ago
|
||
Moved some code related to bookmarkGuid to nsNavBookmarks.cpp
Attachment #783642 -
Attachment is obsolete: true
Attachment #792045 -
Flags: review?(mano)
Comment 21•11 years ago
|
||
Comment on attachment 792045 [details] [diff] [review]
bug-627487-v3.diff
Review of attachment 792045 [details] [diff] [review]:
-----------------------------------------------------------------
This iteration is much better, but there are still some minor issues to address.
It just came to mind though, that this doesn't play well with "restored" GUIDs (the thing I'm implementing for the new Undo Manager, and that you might have used in bookmarks backups). Worse, sync already "restores" page GUIDs, albeit given how it's used it should be a real problem.
You cannot address this here though because there is no notification for page GUIDs change (through updatePlaces), as far as I recall, and as for bookmark GUIDs, I was just setting them right into the db (as a temporary solution) in my Undo Manager patch.
However, I think this just means I need to write a proper solution for restoring GUIDs on my side, and figure out the necessary observer changes. All in all, I think we can move forward with this patch, but once it lands we must address GUIDs changes.
::: toolkit/components/places/nsINavHistoryService.idl
@@ +155,5 @@
> */
> readonly attribute AString tags;
> +
> + /**
> + * The unique ID associated with the page.
Please document what happens for non-URI nodes. They may actually have GUIDs for their place: uri, if any; or maybe they still don't. Please test or ask Marco.
::: toolkit/components/places/nsNavBookmarks.cpp
@@ +1828,5 @@
>
> + nsAutoCString bookmarkGuid;
> + rv = aRow->GetUTF8String(kGetChildrenIndex_Guid, bookmarkGuid);
> + NS_ENSURE_SUCCESS(rv, rv);
> + node->mBookmarkGuid = bookmarkGuid;
You've made this change under |if (itemType == TYPE_BOOKMARK) {|
But folders and separators also have GUIDs.
It seems to me this should be done right under the mBookmarkIndex set.
Please add tests for folders ands separators nodes.
::: toolkit/components/places/nsNavBookmarks.h
@@ -367,5 @@
>
> static const int32_t kGetChildrenIndex_Position;
> static const int32_t kGetChildrenIndex_Type;
> static const int32_t kGetChildrenIndex_PlaceID;
> - static const int32_t kGetChildrenIndex_FolderTitle;
Nice catch!
::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +113,5 @@
> mFrecency(0),
> mHidden(false),
> + mTransitionType(0),
> + mPageGuid(""),
> + mBookmarkGuid("")
unnecessary.
::: toolkit/components/places/tests/unit/test_pageGuid_bookmarkGuid.js
@@ +15,5 @@
> +try {
> + var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].getService(Ci.nsINavHistoryService);
> +} catch(ex) {
> + do_throw("Could not get history service\n");
> +}
Let's use PlacesUtils helpers in new tests.
@@ +21,5 @@
> +function run_test() {
> + run_next_test();
> +}
> +
> +add_task(function test_getPageGuidAndBookmarkGuid() {
For each test, please add a separate task.
Attachment #792045 -
Flags: review?(mano) → review-
Assignee | ||
Comment 22•11 years ago
|
||
(In reply to Mano from comment #21)
> Comment on attachment 792045 [details] [diff] [review]
> bug-627487-v3.diff
>
> Review of attachment 792045 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> This iteration is much better, but there are still some minor issues to
> address.
>
> It just came to mind though, that this doesn't play well with "restored"
> GUIDs (the thing I'm implementing for the new Undo Manager, and that you
> might have used in bookmarks backups). Worse, sync already "restores" page
> GUIDs, albeit given how it's used it should be a real problem.
>
> You cannot address this here though because there is no notification for
> page GUIDs change (through updatePlaces), as far as I recall, and as for
> bookmark GUIDs, I was just setting them right into the db (as a temporary
> solution) in my Undo Manager patch.
>
> However, I think this just means I need to write a proper solution for
> restoring GUIDs on my side, and figure out the necessary observer changes.
> All in all, I think we can move forward with this patch, but once it lands
> we must address GUIDs changes.
>
> ::: toolkit/components/places/nsINavHistoryService.idl
> @@ +155,5 @@
> > */
> > readonly attribute AString tags;
> > +
> > + /**
> > + * The unique ID associated with the page.
>
> Please document what happens for non-URI nodes. They may actually have GUIDs
> for their place: uri, if any; or maybe they still don't. Please test or ask
> Marco.
For separator and folder nodes, they return empty string. Added to the test.
>
> ::: toolkit/components/places/nsNavBookmarks.cpp
> @@ +1828,5 @@
> >
> > + nsAutoCString bookmarkGuid;
> > + rv = aRow->GetUTF8String(kGetChildrenIndex_Guid, bookmarkGuid);
> > + NS_ENSURE_SUCCESS(rv, rv);
> > + node->mBookmarkGuid = bookmarkGuid;
>
> You've made this change under |if (itemType == TYPE_BOOKMARK) {|
>
> But folders and separators also have GUIDs.
>
> It seems to me this should be done right under the mBookmarkIndex set.
>
> Please add tests for folders ands separators nodes.
Added
>
> ::: toolkit/components/places/nsNavBookmarks.h
> @@ -367,5 @@
> >
> > static const int32_t kGetChildrenIndex_Position;
> > static const int32_t kGetChildrenIndex_Type;
> > static const int32_t kGetChildrenIndex_PlaceID;
> > - static const int32_t kGetChildrenIndex_FolderTitle;
>
> Nice catch!
>
> ::: toolkit/components/places/nsNavHistoryResult.cpp
> @@ +113,5 @@
> > mFrecency(0),
> > mHidden(false),
> > + mTransitionType(0),
> > + mPageGuid(""),
> > + mBookmarkGuid("")
>
> unnecessary.
Removed
>
> ::: toolkit/components/places/tests/unit/test_pageGuid_bookmarkGuid.js
> @@ +15,5 @@
> > +try {
> > + var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].getService(Ci.nsINavHistoryService);
> > +} catch(ex) {
> > + do_throw("Could not get history service\n");
> > +}
>
> Let's use PlacesUtils helpers in new tests.
Updated to use those helpers.
>
> @@ +21,5 @@
> > +function run_test() {
> > + run_next_test();
> > +}
> > +
> > +add_task(function test_getPageGuidAndBookmarkGuid() {
>
> For each test, please add a separate task.
Attachment #796560 -
Flags: review?(mano)
Assignee | ||
Updated•11 years ago
|
Attachment #792045 -
Attachment is obsolete: true
Comment 23•11 years ago
|
||
(In reply to Raymond Lee [:raymondlee] from comment #22)
> For separator and folder nodes, they return empty string. Added to the test.
>
Separators and folders do have bookmarks guids, so if empty strings are returned, you didn't fix the issue i noted in nsNavBookmarks
Assignee | ||
Comment 24•11 years ago
|
||
(In reply to Mano from comment #23)
> (In reply to Raymond Lee [:raymondlee] from comment #22)
>
> > For separator and folder nodes, they return empty string. Added to the test.
> >
>
> Separators and folders do have bookmarks guids, so if empty strings are
> returned, you didn't fix the issue i noted in nsNavBookmarks
I was referring that separators and folders return empty strings for page guids. They do have bookmarks guids.
Comment 25•11 years ago
|
||
Comment on attachment 796560 [details] [diff] [review]
v4
Review of attachment 796560 [details] [diff] [review]:
-----------------------------------------------------------------
r=mano with the following comments addressed.
::: toolkit/components/places/nsINavHistoryService.idl
@@ +156,5 @@
> readonly attribute AString tags;
> +
> + /**
> + * The unique ID associated with the page. It returns an empty string
> + * if the result node is a non-URI node.
s/returns/may return/, because I think we're going to add GUIDs for place: uris.
@@ +162,5 @@
> + readonly attribute ACString pageGuid;
> +
> + /**
> + * The unique ID associated with the bookmark. It returns an empty string
> + * if the result node is not associated with bookmark, folder or separator.
a bookmark, a folder or a separator.
::: toolkit/components/places/nsNavBookmarks.cpp
@@ +1880,5 @@
>
> + nsAutoCString bookmarkGuid;
> + rv = aRow->GetUTF8String(kGetChildrenIndex_Guid, bookmarkGuid);
> + NS_ENSURE_SUCCESS(rv, rv);
> + node->mBookmarkGuid = bookmarkGuid;
I think you could just pass node->mBookmarkGuid to GetUTF8String.
::: toolkit/components/places/nsNavHistory.cpp
@@ +3858,5 @@
>
> + nsAutoCString pageGuid;
> + rv = aRow->GetUTF8String(kGetInfoIndex_Guid, pageGuid);
> + NS_ENSURE_SUCCESS(rv, rv);
> + resultNode->mPageGuid = pageGuid;
I think you could pass resultNode->mPageGuid to GetUTF8String.
@@ +3876,5 @@
>
> + nsAutoCString pageGuid;
> + rv = aRow->GetUTF8String(kGetInfoIndex_Guid, pageGuid);
> + NS_ENSURE_SUCCESS(rv, rv);
> + resultNode->mPageGuid = pageGuid;
Ditto.
::: toolkit/components/places/tests/unit/test_pageGuid_bookmarkGuid.js
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var bmsvc;
> +var histsvc;
const? and you could just initialize them here, I think.
@@ +33,5 @@
> + let root = result.root;
> + root.containerOpen = true;
> + do_check_eq(root.childCount, 5);
> +
> + // check bookmark guids
*C*heck bookmark guids*.*
Same goes for all other comments.
@@ +110,5 @@
> + yield promiseAddVisits({ uri: sourceURI });
> + do_check_eq(bmsvc.getBookmarkedURIFor(sourceURI), null);
> +
> + options = histsvc.getNewQueryOptions();
> + query = histsvc.getNewQuery();
let query, let options
@@ +113,5 @@
> + options = histsvc.getNewQueryOptions();
> + query = histsvc.getNewQuery();
> + query.uri = sourceURI;
> + result = histsvc.executeQuery(query, options);
> + root = result.root
let root, missing ;
::: toolkit/components/places/tests/unit/xpcshell.ini
@@ +125,5 @@
> [test_PlacesUtils_lazyobservers.js]
> [test_placesTxn.js]
> [test_telemetry.js]
> [test_getPlacesInfo.js]
> +[test_pageGuid_bookmarkGuid.js]
\ No newline at end of file
Add a new empty line at the end of file to shut the diff warning.
Attachment #796560 -
Flags: review?(mano) → review+
Assignee | ||
Comment 26•11 years ago
|
||
Fixed the patch based on comment 25
Pushed to try and waiting for results.
https://tbpl.mozilla.org/?tree=Try&rev=84a5d17593f4
Assignee: nobody → raymond
Attachment #796560 -
Attachment is obsolete: true
Assignee | ||
Comment 27•11 years ago
|
||
(In reply to Raymond Lee [:raymondlee] from comment #26)
> Created attachment 798687 [details] [diff] [review]
> Patch for check in
>
> Fixed the patch based on comment 25
>
> Pushed to try and waiting for results.
> https://tbpl.mozilla.org/?tree=Try&rev=84a5d17593f4
Passed try!
Keywords: checkin-needed
Comment 28•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 29•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a4fc8aae3053
https://hg.mozilla.org/mozilla-central/rev/26b027841efe
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 30•11 years ago
|
||
Comment on attachment 798687 [details] [diff] [review]
Patch for check in
This should have landed with sr due to the interface changes. Asking post-facto.
Attachment #798687 -
Flags: superreview?(gavin.sharp)
Comment 31•11 years ago
|
||
BTW, is the issue reported in the summary "Bookmark JSON backup should contain new-style GUIDs" actually addressed now? I expected a second patch here.
Updated•11 years ago
|
Attachment #798687 -
Flags: superreview?(gavin.sharp) → superreview+
Assignee | ||
Comment 32•11 years ago
|
||
(In reply to Mano from comment #31)
> BTW, is the issue reported in the summary "Bookmark JSON backup should
> contain new-style GUIDs" actually addressed now? I expected a second patch
> here.
I will look into adding the guid into the JSON backups.
Assignee | ||
Updated•11 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•11 years ago
|
Attachment #798687 -
Flags: checkin+
Assignee | ||
Comment 33•11 years ago
|
||
Require bug 887043
Comment 34•11 years ago
|
||
Comment on attachment 803136 [details] [diff] [review]
Part 2 - add guid to json backups
Oh well. So it seems you don't actually need the new results API after all, right? Not that I suggest backing it out, because it sure has (and will have) other use cases.
However I do suggest morphing this bug to cover just the new results API and taking the GUID addition somewhere else (either to a new bug or just to bug 887043).
Assignee | ||
Comment 35•11 years ago
|
||
(In reply to Mano from comment #34)
> Comment on attachment 803136 [details] [diff] [review]
> Part 2 - add guid to json backups
>
> Oh well. So it seems you don't actually need the new results API after all,
> right? Not that I suggest backing it out, because it sure has (and will
> have) other use cases.
>
> However I do suggest morphing this bug to cover just the new results API and
> taking the GUID addition somewhere else (either to a new bug or just to bug
> 887043).
OK, let give the GUID code to bug 887043.
Assignee | ||
Updated•11 years ago
|
Attachment #803136 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•