Closed Bug 1117833 Opened 5 years ago Closed 3 years ago

Top-Site thumbnail summary displays page URL instead of page title; fixed on refresh

Categories

(Firefox for Android :: Awesomescreen, defect, P5)

37 Branch
ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
fennec + ---
firefox50 --- fixed

People

(Reporter: aaronmt, Assigned: twointofive, Mentored)

Details

(Whiteboard: [lang=java])

Attachments

(1 file)

Currently our Top-Site thumbnail summary will display the page URL instead of the page title. 

  Visit http://people.mozilla.org/~atrain/mobile/tests/title.html
  Visit about:home, see the page URL below the thumbnail
  Refresh http://people.mozilla.org/~atrain/mobile/tests/title.html
  Re-visit about:home, see the page title now

Why are we showing the page URL instead of the title initially?

Probably a long-standing issue but I just noticed it just now testing the recent URL/title changes.

--
Nightly (01/05)
LG Nexus 5 (Android 5.1)
Mentor: michael.l.comella
tracking-fennec: ? → +
Whiteboard: [lang=java]
(In reply to Aaron Train [:aaronmt] from comment #0)
> Why are we showing the page URL instead of the title initially?

We initially only have the url, which is temporarily used as the page title until the page title is retrieved from Gecko (as can be seen when loading pages with "Page name" enabled), so we are likely writing to the database with the initial "page title" (i.e. the url).
Because of the 3 second "is this a redirect?" delay in nsAndroidHistory.cpp, LocalBrowserDB.updateHistoryTitle gets called with a title update before LocalBrowserDB.updateVisitedHistory gets called to create a history entry.
Tom, what is your patch intended to do? Given my analysis in comment 1, I don't actually know what the expected behavior would be here, beyond making the title update in about:home as soon as we retrieve it.

I'm also wondering if this is an issue that might better be fixed in Java (but I don't regularly work in the C++ code so I'm missing context).
Assignee: nobody → twointofive
Flags: needinfo?(twointofive)
(In reply to Michael Comella (:mcomella) from comment #4)
> I'm also wondering if this is an issue that might better be fixed in Java
> (but I don't regularly work in the C++ code so I'm missing context).

I'll get someone else to look at the C++ if this patch results in expected behavior.
What I'm seeing without the patch the first time we visit a given non-redirect uri is:

We enter nsAndroidHistory::VisitURI where we decide to wait three seconds to see if aURI is going to wind up being redirected [1].
During that three seconds the page loads and we get a title update in nsAndroidHistory::SetURITitle [2], but when we pass that title update into java the db update fails because there's no entry with the given aURI already in the db (that's a silent fail (as it should be)).
Then the three second timer pops in nsAndroidHistory and we call nsAndroidHistory::Notify [3] which calls SaveVisitURI, which calls java and actually adds the URI to the db.  (At that point the title update will succeed the next time that site is visited.)

With the patch I assume (correctly?) that if we've processed a page title for a given URI, then that URI isn't a redirect, so when we get the title, if the URI is still on the 3-second-is-this-a-redirect list, we just go ahead and remove it from that list and do the SaveVisitURI right away (which puts the URI in the db and allows the subsequent title update to succeed) instead of waiting for the timer to pop.

So the only thing the patch is intended to do is to save a URI visit as soon as we have a title, with the side effect that that means the title update on a first URI visit will succeed.  Robocop tests looked good.

It seemed more difficult to do that in java because in java you don't have that this-uri-is-being-visited-unless-a-redirect's-about-to-occur list (i.e. mRecentlyVisitedURIs), and so it's more difficult to filter out title updates for URIs coming from frames/iframes in the page that's being loaded (you could, for example, check that some tab is visiting a given uri, but you might get a different tab than the one you're opening that just happens to be visiting the uri for some iframe in your page, and then you're doing title updates for a different tab than the one you're loading (though I guess that can happen anyway?)).  But all of this code is new to me, so maybe you can point me to a better way :)  Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/build/nsAndroidHistory.cpp#264
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/build/nsAndroidHistory.cpp#291
[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/build/nsAndroidHistory.cpp#190
Flags: needinfo?(twointofive)
Comment on attachment 8768034 [details]
Bug 1117833 - Cancel pending visit and save immediately when title is updated.

Given that I know little about the C++, seems like a reasonable change to me – let's ask Snorp to do the deep-dive.
Attachment #8768034 - Flags: review?(michael.l.comella) → review?(snorp)
Priority: -- → P5
Comment on attachment 8768034 [details]
Bug 1117833 - Cancel pending visit and save immediately when title is updated.

https://reviewboard.mozilla.org/r/62382/#review61924
Attachment #8768034 - Flags: review?(snorp) → review+
Pushed by twointofive@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/96c3ed67980c
Cancel pending visit and save immediately when title is updated. r=snorp
https://hg.mozilla.org/mozilla-central/rev/96c3ed67980c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.