Closed Bug 1250391 Opened 7 years ago Closed 7 years ago

[Sync] tabs record doesn't include the page favicon.

Categories

(Firefox for Android Graveyard :: Android Sync, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: markh, Assigned: markh, Mentored)

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(1 file)

The tabs record has the ability to store the favicon URL for a tab in an "icon" field. Desktop sets this via https://dxr.mozilla.org/mozilla-central/rev/789a12291942763bc1e3a89f97e0b82dc1c9d00b/services/sync/modules/engines/tabs.js#187

It would be great if Android also populated this field, so that when a user access Synced Tabs for a page that doesn't already have a favicon populated locally, a sane favicon is still able to be used.

Nick, is this viable and relatively simple?
Flags: needinfo?(nalexander)
Yeah, we can do this.  rnewman may even dupe this :)

It looks like we already have this handled in https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/background/db/Tab.java -- I see the DB reads and writes.

That means there's likely an issue populating the field in the DB.  I'll keep the NI to investigate one step further.  It may be that we should be fetching up-to-date favicons from our favicon service instead of using the DB field.
Mentor: nalexander
Whiteboard: [lang=java][good next bug]
Component: General → Android Sync
Flags: needinfo?(nalexander)
Product: Firefox for Android → Android Background Services
My experiments have shown that on loading a tab:

* Tab::addFavicon (https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/background/db/Tab.java) is called with the favicon url - this sticks the url in mAvailableFavicons.
* LocalTabsAccessor::insertLocalTabs (https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/LocalTabsAccessor.java) asks the tab for its favicon - it gets null as mFaviconUrl is still null.
* Tab::loadFavicon is then asked to load the favicon - it loads successfully, sets mFaviconUrl then does |Tabs.getInstance().notifyListeners(Tab.this, Tabs.TabEvents.FAVICON);|
* LocalTabsAccessor doesn't again attempt to request the favicon after the tab loads.

ISTM LocalTabsAccessor.java should possibly have a listener for the FAVICON event, but I wouldn't be surprised to hear it's not that simple ;) Nick, can you offer any advice on what should be happening here but isn't?
Flags: needinfo?(nalexander)
Attached patch t.patchSplinter Review
Tabs.java already has a listener for these events, but it doesn't do anything on the FAVICON notification. This (probably naive) patch adds a listener and queues the persisting of tabs when it fires. My Log() debugging tells me that this works - that after getting the notification the LocalTabsAccessor attempts to fetch the favicon and gets one.

Nick, does this seem like the correct approach?
Flags: needinfo?(nalexander)
Attachment #8727201 - Flags: feedback?(nalexander)
Comment on attachment 8727201 [details] [diff] [review]
t.patch

Review of attachment 8727201 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is sensible.  Push a try build, Robocop everything -- and accept my thanks for digging into the Fennec code :)
Attachment #8727201 - Flags: feedback?(nalexander) → review+
Thanks - my first Android patch \o/

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccb0940ab458
Assignee: nobody → markh
https://hg.mozilla.org/mozilla-central/rev/266ac4be0d70
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Android Background Services → Firefox for Android
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.