519479 - places default title behavior shouldn't use part of the URL

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Aakash Desai [:aakashd]]

Build ID:
Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20090928
Fennec/1.0b4pre

Litmus Testcase https://litmus.mozilla.org/show_test.cgi?id=7612

Steps to Reproduce:
1. Go to http://ed.agadak.net/blan
2. Go to the awesome bar and view the page title listed there

Actual Results:
"blank" shows up in the page title and not the url

Expected Results:
Due to the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=456084 , the url should show up in the title.

Comment 1

[Aakash Desai [:aakashd]]

This is also inconsistent with the bookmarks manager as the name field in the edit bookmark dialog shows the title to be the url there.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

bug 456084 only fixed blanks for the URLBar itself. A different bug fixed blanks in bookmarks.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

"blank" seems to be coming from the autocomplete controller, which kinda means that we are somehow saving it as "blank" - but Firefox doesn't

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Turns out this is places' intended behavior. It tries to generate a title based on the URL for history entries that have no title:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistory.cpp#7425 (GenerateTitleFromURI)

This happens in Firefox as well. We could turn this into a bug asking for that behavior to be changed, but otherwise I think it is INVALID.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(That page just happens to be "blank" because that's the last part of its URL)

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #5)
> (That page just happens to be "blank" because that's the last part of its URL)

So it's working as designed. I think we should close this bug too.

Comment 7

[Aakash Desai [:aakashd]]

There's an inconsistency to the user for the bookmarks dialog to have a different page title than the one in the awesome bar. This is definitely a bug.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Not a Fennec bug, though...

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Actually saving null titles is probably the best option - the consumer can decide what to actually show (e.g. by duplicating GenerateTitleFromURI in front-end code).

Note also that there is an inconsistency in behavior, which probably explains why we sometimes didn't see this bug in testing - InternalAddNewPage calls GenerateTitleFromURI if the title is void, but setPageTitle() called from nsDocShell::SetTitle doesn't, and seems to just save null titles.

Comment 10

[Marco Bonardo [:mak]]

i completely agree with Gavin as i said or IRC, toolkit should not make assumption on titles, and leave decisions on how to fill empty titles to the frontend.
Saving a part of the uri as title means deciding that's the best option, and as we see here, is it not always true.
Saving meaningless informations is always bad from a db point of view, and from a user point of view.
We should really get rid of that code and just save a null title.

Comment 11

[Aakash Desai [:aakashd]]

This'll make changes to litmus test case on the fennec 1.0 test run:

https://litmus.mozilla.org/show_test.cgi?id=7612

Comment 12

[Marco Bonardo [:mak]]

the interesting question is:
is an empty string title different from a null title?

or in other words: should returned title be empty string or null, or should we always return an empty string (for example from getPageTitle)?

From a code point of view, since we allow to set a null title or an empty string title makes sense to allow distinguish them. But from a "contents" point of view this is the title of a page, and the page either has a title or has not.

I think i'll go for the widest impl, allowing to set and get both an empty string or a null title, so that an implementer can choice to only override null titles while setting empty string titles for some wanted page.

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 14

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/86f3d9c54c95

Comment 16

[Marco Bonardo [:mak]]

pushed an additional changeset to fix an orange: http://hg.mozilla.org/mozilla-central/rev/dcfa9c3a2388

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 406441 [details] [diff] [review]
patch v1.1

this is blocking fennec, would probably need approval

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 406441 [details] [diff] [review]
patch v1.1

Minusing this version because 1) we missed 1.9.2.2 and 2) this patch apparently needs a test fix before landing.

Does this really block Fennec? Because it doesn't seem to have... Anyway, if you still need this please create a merged 1.9.2.x patch with text fix and request 1.9.2.3 approval

Comment 20

[Marco Bonardo [:mak]]

(In reply to comment #19)
> Does this really block Fennec?

not blocking in the strict sense, but pretty annoying unless they did workaround it (empty selection from the locationbar), that they should tell us.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Fennec didn't work around it, AFAIK. It's makes awesomebar results somewhat annoying for pages without titles, but it's certainly not the end of the world.