627487 - Add page and bookmark GUIDs to nsINavHistoryResultNode

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Philipp von Weitershausen [:philikon]]

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.

Comment 1

[Raymond Lee [:raymondlee]]

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

[Marco Bonardo [:mak]]

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.

Comment 3

[Richard Newman [:rnewman]]

Any progress on this, Raymond?

Comment 4

[Raymond Lee [:raymondlee]]

(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.

Comment 5

[Marco Bonardo [:mak]]

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

Comment 6

[Raymond Lee [:raymondlee]]

(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

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: WIP

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?

Comment 8

[Marco Bonardo [:mak]]

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.

Comment 9

[Asaf Romano (gone)]

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?

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: WIP 2

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?

Comment 11

[Asaf Romano (gone)]

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.

Comment 12

[Raymond Lee [:raymondlee]]

(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 :-)

Comment 13

[Raymond Lee [:raymondlee]]

Attachment: WIP 3

It returns the correct bookmark and Page GUIDs.  Working on fixing the failing tests.

Comment 14

[Raymond Lee [:raymondlee]]

Attachment: v1

All tests passed in my machine.

Comment 15

[Asaf Romano (gone)]

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).

Comment 16

[Raymond Lee [:raymondlee]]

Attachment: v2

Updated based on on the comment #15

Comment 17

[Asaf Romano (gone)]

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

Comment 18

[Raymond Lee [:raymondlee]]

(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);

Comment 19

[Asaf Romano (gone)]

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.

Comment 20

[Raymond Lee [:raymondlee]]

Attachment: bug-627487-v3.diff

Moved some code related to bookmarkGuid to nsNavBookmarks.cpp

Comment 21

[Asaf Romano (gone)]

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.

Comment 22

[Raymond Lee [:raymondlee]]

Attachment: v4

(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.

Comment 23

[Asaf Romano (gone)]

(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

Comment 24

[Raymond Lee [:raymondlee]]

(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

[Asaf Romano (gone)]

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.

Comment 26

[Raymond Lee [:raymondlee]]

Attachment: 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

Comment 27

[Raymond Lee [:raymondlee]]

(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!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4fc8aae3053

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a4fc8aae3053
https://hg.mozilla.org/mozilla-central/rev/26b027841efe

Comment 30

[Asaf Romano (gone)]

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.

Comment 31

[Asaf Romano (gone)]

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.

Comment 32

[Raymond Lee [:raymondlee]]

(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.

Comment 33

[Raymond Lee [:raymondlee]]

Attachment: Part 2 - add guid to json backups

Require bug 887043

Comment 34

[Asaf Romano (gone)]

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).

Comment 35

[Raymond Lee [:raymondlee]]

(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.