1249828 - Synced tabs from iOS don't include title, and might include localhost URLs as the title

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Ryan Feeley [:rfeeley]]

Attachment: synced-tabs.png

Synced tabs from iOS only showing URL in desktop, not the page title. Not sure if this is an iOS bug, a desktop bug or a toolkit bug?

Comment 1

[Chris Karlof [:ckarlof]]

I'm guessing all your pages don't have titles. :)

Most of my tabs for iOS have titles.

Comment 2

[Richard Newman [:rnewman]]

I can repro this.

JSON.stringify(Weave.Service.engineManager.get("tabs")._store._remoteClients)
=>

"{"ekhsSAMMigaU":{"id":"ekhsSAMMigaU","tabs":[{"title":"http://www.buffalosystemsusa.com/mobile/Product.aspx?ProductCode=belay-jacket-limited","icon":null,"urlHistory":["http://www.buffalosystemsusa.com/mobile/Product.aspx?ProductCode=belay-jacket-limited"],"lastUsed":"1455910687846"}…

Comment 3

[Richard Newman [:rnewman]]

Everything storage on down is right, so finger points at this:

    var displayTitle: String {
        if let title = webView?.title {
            if !title.isEmpty {
                return title
            }
        }
        return displayURL?.absoluteString ?? lastTitle ?? ""
    }

… in other words, we're not able to get a title from the webview at the point we write out the tabs, so we're writing the URL instead.

This'll need a little more analysis to find out what's happening when.

Comment 4

[Aaron Train [:aaronmt]]

Attachment: remote-tabs.png

Currently none of my remote tabs are shown – only panels

Comment 5

[Aaron Train [:aaronmt]]

This can't ship as broken.

Comment 6

[Richard Newman [:rnewman]]

Looks like you're seeing something else, Aaron: you're seeing the same home panel URL, presumably because we flushed to the database before restoring tabs. Likely the same root cause, but not the same symptom.

Dumping this in 'General' 'cos this is almost certainly a tabs persistence issue.

Comment 7

[Richard Newman [:rnewman]]

Emily will look at this: related test failure.

Comment 9

[Emily Toop (:fluffyemily)]

Attachment: Pull request

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8728465 [details] [review]
Pull request

Follow-on/more to think about: are we persisting at the right times?

Comment 11

[Emily Toop (:fluffyemily)]

(In reply to Richard Newman [:rnewman] from comment #10)
> Comment on attachment 8728465 [details] [review]
> Pull request
> 
> Follow-on/more to think about: are we persisting at the right times?

That's definitely something to consider. Right now we are persisting on restore, every time we complete a URL load in a webView and every time we create a new tab. 

We don't really need to persist on restore - we are restoring the state as it was when we last closed the app, so the current DB and disk state on before tab restoration will be identical that what it is after.

We don't need to persist to RemoteTabs when we create a new, blank tab. All this does is replace the existing DB state with ... exactly the same DB state as until we have something loaded in that new tab remote tabs just don't care.

Every time we complete a URL load, sure. This is sensible. This is when we should be persisting.

There may be other times we persist, but nothing that I can see right now.

Comment 12

[Richard Newman [:rnewman]]

Assuming that we don't use this particular pile of persisted tab state for anything but Sync, we really just want to persist at two times:

* Immediately before a sync while the app is in the foreground.
* Immediately before we're backgrounded (because if we do it any later, the webviews are gone!).

There's a second decision, which is when/whether to trigger a sync as a result of browsing activity -- browse enough and we should probably sync your tabs… and in the course of so doing we will persist them.

Does that sound sane to you, Emily?

Comment 13

[Emily Toop (:fluffyemily)]

#merged c0b1939721a0ed563fb6582714738b9ddd945e1d

Comment 14

[Stephan Leroux [:sleroux]]

v3.x 2df73de6d3e2a43a53c12f8ac9346bc4c141329a