1380740 - Bookmark created with blank/no name still has bookmark name in some cases after landing patch from bug #1360872

Status: VERIFIED FIXED

(Toolkit :: Places, defect, P2)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: screencast.mp4

+++ This bug was initially created as a clone of Bug #1379412 +++


[Tracking Requested - why for this release]: Regression

Regression caused by bug:
Bug #1360872 - Return empty strings for `null` bookmark titles


STR:
Better reproducible STR:
1. Bookmark this website page ( http://www.ldoceonline.com/ ) with blank/no name in "Bookmark Toolbar" folder
and see that it still have name in:
- toolbar (after hovering bookmark)
- bookmark name
but it doesn't have name in:
- bookmark doorhanger

Workaround: Clear name second time to get blank name
New workaround (added by bug #1379412): Restart browser

Comment 1

[Lina Butler [:lina]]

Attachment: macos.gif

Thanks for the screencast! I couldn't reproduce on macOS, but you said this is on Windows, so let me try there. (The default title showing up when I switch the folder from "Other Bookmarks" to "Bookmarks Toolbar" doesn't seem related; I can reproduce that on 54, too. It goes away once I click "Done").

Comment 2

[Lina Butler [:lina]]

OK, I reproduced this. Removing the untitled bookmark, then re-adding, switching folders, and clearing the title *usually* keeps the old title.

Comment 3

[Lina Butler [:lina]]

1) If I star the bookmark, clear the title, then choose "Bookmarks Toolbar" from the drop-down, *without clicking the down arrow*, the bookmark has the default title. When I click "Done", it correctly switches to blank. This isn't a regression, I can reproduce on Release 54.

2) If I star the bookmark, choose "Bookmarks Toolbar" from the drop-down (without clicking the down arrow), then clear the title, I get the same result as (1).

3) If I star the bookmark, clear the title, click the down arrow, and choose "Bookmarks Toolbar" from the expanded tree view, the bookmark also has the default title, even after I click "Done". I added some logging, and saw the default title comes from http://searchfox.org/mozilla-central/rev/cbd628b085ac809bf5a536109e6288aa91cbdff0/toolkit/components/places/nsNavHistory.cpp#4104. This is the regression; on 54, bookmark shows up in the toolbar with a blank title as soon as I choose "Bookmarks Toolbar".

4) If I star the bookmark, clear the title, click the down arrow, and choose "Bookmarks Toolbar" from the drop-down, the bookmark has the default title, even after I click "Done". Same as (3).

5) If I star the bookmark, click the down arrow, clear the title, then choose "Bookmarks Toolbar" *from the expanded tree view*, the bookmark shows up in the toolbar without a title. I don't need to click "Done" to see the blank title. Same on 54.

6) If I star the bookmark, click the down arrow, clear the title, then choose "Bookmarks Toolbar" *from the drop-down instead of the tree view*, the bookmark shows in the toolbar *with* the default title, but correctly switches to blank once I click "Done". Same as (1) and (2), and on 54.

7) If I star the bookmark, click the down arrow, and choose "Bookmarks Toolbar" from the expanded tree view, the bookmark shows in the toolbar with the default title. If I then clear the title and click "Done", it switches to blank. Same on 54.

8) If I star the bookmark, click the down arrow, and choose "Bookmarks Toolbar" from the drop-down instead of the tree view, I get the same results as (7).

I'm happy to upload screencasts of all the above if it helps.

Replacing `COALESCE(b.title, h.title)` with `b.title` in http://searchfox.org/mozilla-central/rev/cbd628b085ac809bf5a536109e6288aa91cbdff0/toolkit/components/places/nsNavHistory.cpp#1543,1567,4104,4144 will fix (3) and (4), but it's not clear to me *why* we fall back to the visit *title*. ISTM all our views will fall back to the *URL*.

Mak, do you think I can safely remove those `COALESCE(b.title, h.title)` calls, and always use `b.title`? Do you know why `h.title` is there now?

Comment 4

[Lina Butler [:lina]]

Attachment: Bug 1380740 - Remove remaining visit title fallbacks for untitled bookmarks.

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

Comment 5

[Marco Bonardo [:mak]]

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #3)
> Mak, do you think I can safely remove those `COALESCE(b.title, h.title)`
> calls, and always use `b.title`? Do you know why `h.title` is there now?

I think the original intent was to fallback to history title and if that was not provided to the url. that was probably when there was also a user_title column. It's not critical functionality so I'd not care that much.

The thing that came to my mind now, is that in the future we may want some way to know if the user changed the title or not, because:
1. it would be handy to be able to update a bookmark title with the most recent history title, when the user didn't change it. Currently the bookmarks titles are frozen at the time they were created.
2. it would be of great benefit avoiding to store the bookmark title when it's exactly the same as the history title, for db size and fragmentation concerns.

Both of these may indeed benefit from a null vs empty difference, and would likely require this kind of coalescing. But I don't think it's worth blocking current work on future possibilities, we can figure out a way out when it's the time, and at least at that point we'll start from a coherent situation.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8886460 [details]
Bug 1380740 - Remove remaining visit title fallbacks for untitled bookmarks.

https://reviewboard.mozilla.org/r/157248/#review162518

Comment 7

[Lina Butler [:lina]]

Comment on attachment 8886460 [details]
Bug 1380740 - Remove remaining visit title fallbacks for untitled bookmarks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157248/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/52ab5df3d1ca
Remove remaining visit title fallbacks for untitled bookmarks. r=mak

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/52ab5df3d1ca

Comment 10

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm confirming that this issue is fixed, starting since Mozilla Firefox Nightly 56.0a1 (2017-07-16).
Thank you very much! \o/