Closed Bug 1154554 Opened 7 years ago Closed 6 years ago

Upload tabs record

Categories

(Firefox for iOS :: Sync, defect)

All
iOS 8
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fxios + ---

People

(Reporter: rnewman, Assigned: fluffyemily, Mentored)

References

Details

Attachments

(2 files, 2 obsolete files)

47 bytes, text/x-github-pull-request
rnewman
: review+
Details | Review
47 bytes, text/x-github-pull-request
fluffyemily
: review+
Details | Review
Fleshing out Bug 1141847. As part of tab sync we wish to upload our own tabs.
Depends on: 1156076
Mentor: rnewman
tracking-fxios: --- → ?
Assignee: nobody → etoop
Status: NEW → ASSIGNED
FYI: TabManager takes a storage object to store these in a db (which I assume is needed here), but it isn't actually assigned right now. I've been meaning to fix that :)
Attached file WIP Feedback Request (obsolete) —
This all _seems_ to be working - I get a success response from the PUT, but I cannot then see the tabs I should have uploaded in about:sync-tabs when I sync my desktop.
Attachment #8636034 - Flags: feedback?(rnewman)
Attachment #8636034 - Flags: feedback?(rnewman) → feedback+
Attached file Pull request (obsolete) —
This one definitely works.
Attachment #8636565 - Flags: review?(rnewman)
Comment on attachment 8636565 [details] [review]
Pull request

Close!
Attachment #8636565 - Flags: review?(rnewman) → feedback+
Attached file Pull request
Attachment #8636034 - Attachment is obsolete: true
Attachment #8636565 - Attachment is obsolete: true
Attachment #8637210 - Flags: review?(rnewman)
Comment on attachment 8637210 [details] [review]
Pull request

Comments on the PR. Nice work!
Attachment #8637210 - Flags: review?(rnewman) → review+
This produces dodgy output:

    {
      "title" : "reddit: the front page of the internet",
      "icon" : null,
      "urlHistory" : [
        "http:\/\/www.reddit.com\/",
        "http:\/\/localhost:56412\/about\/home\/#panel=2",
        "http:\/\/www.reddit.com\/"
      ],
      "lastUsed" : "1437702183165"
    },
Here's the error:

                history: [displayURL] + browser.historyList,

    var historyList: [NSURL] {
        func listToUrl(item: WKBackForwardListItem) -> NSURL { return item.URL }
        var tabs = self.backList?.map(listToUrl) ?? [NSURL]()
        tabs.append(self.url!)
        return tabs
    }

so browser.historyList *already* includes every URL, including the current one. We then tag the URL on the front. That's wrong.
This breaks the remote tabs panel.


2015-07-23 19:54:51.021 [Error] [SQLiteRemoteClientsAndTabs.swift:218] getClientsAndTabs(): Couldn't cast tab Optional(<RemoteTab clientGUID: nil, URL: https://support.mozilla.org/en-US/questions, title: Support Forum | Mozilla Support, lastUsed: 1437706465816>) to RemoteTab.


        var acc = [String: [RemoteTab]]()
        for tab in tabCursor {
            if let tab = tab, guid = tab.clientGUID {                   <<<<
                if acc[guid] == nil {
                    acc[guid] = [tab]
                } else {
                    acc[guid]!.append(tab)
                }
            } else {
                log.error("Couldn't cast tab \(tab) to RemoteTab.")
            }
        }


Of course, it has no GUID, 'cos it's local. We don't filter them out when querying.
And another issue: on first sync we'll wipe the tabs DB as we apply incoming records. That'll throw away whatever we're about to upload. We should check for `clientGUID IS NULL` when wiping here.


    public func wipeTabs() -> Deferred<Result<()>> {
        return self.doWipe { (conn, inout err: NSError?) -> () in
            self.tabs.delete(conn, item: nil, err: &err)
        }
    }

…


            // If this is a fresh start, do a wipe.
            if self.lastFetched == 0 {
                log.info("Last fetch was 0. Wiping tabs.")
                return localTabs.wipeTabs()
                    >>== afterWipe
            }

            return afterWipe()
Attachment #8638291 - Flags: review?(etoop)
Attachment #8638291 - Flags: review?(etoop) → review+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.