1347248 - Bookmarks.jsm's insert() shouldn't use "nonexistent" as a 'url' for inserts

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P3)

Description

[:Gijs (he/him)]

See:

https://dxr.mozilla.org/mozilla-central/rev/6d38ad302429c98115c354d643e81987ecec5d3c/toolkit/components/places/Bookmarks.jsm#1032


      // Insert the bookmark into the database.
      yield db.executeCached(
        `INSERT INTO moz_bookmarks (fk, type, parent, position, title,
                                    dateAdded, lastModified, guid,
                                    syncChangeCounter, syncStatus)
         VALUES ((SELECT id FROM moz_places WHERE url_hash = hash(:url) AND url = :url), :type, :parent,
                 :index, :title, :date_added, :last_modified, :guid,
                 :syncChangeCounter, :syncStatus)
        `, { url: item.hasOwnProperty("url") ? item.url.href : "nonexistent",
             type: item.type, parent: parent._id, index: item.index,
             title: item.title, date_added: PlacesUtils.toPRTime(item.dateAdded),
             last_modified: PlacesUtils.toPRTime(item.lastModified), guid: item.guid,
             syncChangeCounter: syncChangeDelta, syncStatus });

This was a nit in bug 1344282 comment 14 for the insertTree implementation, but it was simply copied from insert().

This shouldn't be too hard to fix.

Comment 1

[Santiago Paez [:tiago]]

Hi!

This would be my first bug, I'd like to give it a try.

I have a couple questions:

The nit you mentioned suggests to change the string "nonexistent" for null. 
Should I also change the way the bookmark is queried to take advantage of the "nonexistant" for null change?

Are there any other changes that I'm not thinking of?

Thanks in advance.

Comment 2

[Marco Bonardo [:mak]]

(In reply to Santiago Paez from comment #1)
> The nit you mentioned suggests to change the string "nonexistent" for null. 
> Should I also change the way the bookmark is queried to take advantage of
> the "nonexistant" for null change?

Yes, the query should also change, see how the patch in bug 1344282 implemented it through a CASE WHEN.

> Are there any other changes that I'm not thinking of?

Ideally this is already covered by tests, it should be enough to run the tests in ./mach test toolkit/components/placea/tests/bookmarks/

Comment 3

[Santiago Paez [:tiago]]

Attachment: Bug 1347248 - Change "nonexistent" for null and query to use CASE WHEN

Review commit: https://reviewboard.mozilla.org/r/121806/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121806/

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8848950 [details]
Bug 1347248 - Change "nonexistent" for null and query to use CASE WHEN

https://reviewboard.mozilla.org/r/121806/#review124840

LGTM, thank you.
I started a Try run to verify tests.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0418c7323163
Change "nonexistent" for null and query to use CASE WHEN r=mak

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0418c7323163