Closed Bug 1453580 Opened 6 years ago Closed 6 years ago

Reader mode and others are not specifying a preferred icon size

Categories

(Firefox :: General, enhancement, P2)

55 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox-esr60 --- fixed
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- verified

People

(Reporter: mak, Assigned: mak)

References

()

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

There are a few consumers of promiseFaviconLinkUrl that are no currently specifying a preferred icon size.
This is understandable, promiseFaviconLinkUrl itself doesn't expose an argument for that yet!

We should check these consumers, and see whether then can be converted to use page-icon (if they are in chrome context), or fix promiseFaviconLinkUrl to allow to specify a preferred size, and add that size to the exchanged message (if they are in content)
since about:sync-tabs doesn't exist anymore, looks like all the syncedTabs code is in chrome context, thus it can use page-icon. passing over a preferred size from the window is a bit more complicate.
The password manager looks trivial to change to page-icon
reader mode will hate to continue fetching the icon manually because content<->chrome, but at this point I'd probably remove promiseFaviconLinkUrl, for a single consumer that moreover uses the callback.
Flags: needinfo?(mak77)
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
Comment on attachment 8968437 [details]
Bug 1453580 - Remove promiseFaviconLinkUrl and fix its consumers.

https://reviewboard.mozilla.org/r/237140/#review242978

r=me with the sync case figured out.

::: services/sync/modules/SyncedTabs.jsm:52
(Diff revision 1)
>      if (showRemoteIcons) {
>        icon = tab.icon;
> -    }
> -    if (!icon) {
> +      if (!icon) {
> -      try {
> -        icon = (await PlacesUtils.promiseFaviconLinkUrl(url)).spec;
> -      } catch (ex) { /* no favicon avaiable */ }
> +        // By not specifying a size the favicon service will pick the default,
> +        // that is usually set through setDefaultIconURIPreferredSize by the
> +        // first browser window. Commonly it's 16px at current dpi.
> +        icon = "page-icon:" + url;
> -    }
> +      }

I'm a bit confused - why is this now inside the `showRemoteIcons` conditional?
Attachment #8968437 - Flags: review?(gijskruitbosch+bugs) → review+
Good question, I intended showRemoteIcons as "don't show an icon" since the description says:
"A preference used to disable the showing of icons in remote tab records"
the previous code was instead setting the Places icon if there was no remote tab icon.

Though, now that you make me think again about it, it's possible the pref description is just wrong and it intended to say 
"A preference used to disable the showing of REMOTE icons from tab records" (but will still show a local icon if available).

Let's NI the Sync team to be sure of the expected outcome.
Flags: needinfo?(markh)
(In reply to Marco Bonardo [::mak] from comment #4)
> Though, now that you make me think again about it, it's possible the pref
> description is just wrong and it intended to say 
> "A preference used to disable the showing of REMOTE icons from tab records"
> (but will still show a local icon if available).

Yes, the comment (and the pref name) isn't clear, but what you write is closer to the intent - we don't want to *fetch* a remote icon due to privacy concerns (which is the same basic privacy concern as bug 428378).

IIUC, that privacy concern is, roughly, "this device is more sensitive than my other devices. If this device has never visited, say, facebook.com, the fact that I use Sync shouldn't itself cause this device to visit facebook.com"

Given that, the current code appears correct, as promiseFaviconLinkUrl() wouldn't know what icon to use for facebook.com - but does mean that even if |services.sync.syncedTabs.showRemoteIcons| is false we will still display some remote icons if they are known locally. That's somewhat counter-intuitive, but (a) that's what happens now and (b) I suspect the number of users who toggle that pref can be counted on one hand.

So assuming that using |"page-icon:" + url;| will never cause us to hit the URL, I think that block should remain outside of the preference check.
Flags: needinfo?(markh)
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/897bbf35e781
Remove promiseFaviconLinkUrl and fix its consumers. r=Gijs
 Backed out changeset 897bbf35e781 (bug 1453580) for XPCShell failure on services/sync/tests/unit/test_syncedtabs.js. CLOSED TREE 

Failure push:  https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=897bbf35e781fc7067b7e358379fad54d5342a4f

Backout: https://hg.mozilla.org/integration/autoland/rev/09c4cd5c8946259f133b10c0844380712c02c912

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=174287893&repo=autoland&lineNumber=2586

[task 2018-04-18T10:42:32.303Z] 10:42:32     INFO -  TEST-START | services/sync/tests/unit/test_syncedtabs.js
[task 2018-04-18T10:42:33.145Z] 10:42:33  WARNING -  TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_syncedtabs.js | xpcshell return code: 0
[task 2018-04-18T10:42:33.145Z] 10:42:33     INFO -  TEST-INFO took 840ms
[task 2018-04-18T10:42:33.145Z] 10:42:33     INFO -  >>>>>>>
[task 2018-04-18T10:42:33.145Z] 10:42:33     INFO -  PID 13529 | JavaScript strict warning: /builds/worker/workspace/build/tests/xpcshell/tests/services/sync/tests/unit/head_helpers.js -> resource://testing-common/sinon-2.3.2.js, line 8941: ReferenceError: reference to undefined property "iso-8859-8-i"
[task 2018-04-18T10:42:33.145Z] 10:42:33     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-04-18T10:42:33.145Z] 10:42:33     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2018-04-18T10:42:33.145Z] 10:42:33     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2018-04-18T10:42:33.145Z] 10:42:33     INFO -  running event loop
[task 2018-04-18T10:42:33.146Z] 10:42:33     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "iso-8859-8-i"" {file: "/builds/worker/workspace/build/tests/xpcshell/tests/services/sync/tests/unit/head_helpers.js -> resource://testing-common/sinon-2.3.2.js" line: 8941}]"
[task 2018-04-18T10:42:33.146Z] 10:42:33     INFO -  services/sync/tests/unit/test_syncedtabs.js | Starting head_setup
[task 2018-04-18T10:42:33.146Z] 10:42:33     INFO -  (xpcshell/head.js) | test head_setup pending (2)
[task 2018-04-18T10:42:33.147Z] 10:42:33     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2018-04-18T10:42:33.147Z] 10:42:33     INFO -  (xpcshell/head.js) | test run_next_test 1 pending (2)
[task 2018-04-18T10:42:33.147Z] 10:42:33     INFO -  (xpcshell/head.js) | test head_setup finished (2)
[task 2018-04-18T10:42:33.148Z] 10:42:33     INFO -  PID 13529 | 1524048152652	FirefoxAccounts	TRACE	initializing of new storage manager done
[task 2018-04-18T10:42:33.148Z] 10:42:33     INFO -  services/sync/tests/unit/test_syncedtabs.js | Starting setup
[task 2018-04-18T10:42:33.148Z] 10:42:33     INFO -  (xpcshell/head.js) | test setup pending (2)
[task 2018-04-18T10:42:33.149Z] 10:42:33     INFO -  PID 13529 | 1524048152673	Sync.SyncScheduler	TRACE	Setting SyncScheduler policy values to defaults.
[task 2018-04-18T10:42:33.149Z] 10:42:33     INFO -  PID 13529 | 1524048152673	Sync.SyncScheduler	DEBUG	Clearing sync triggers and the global score.
[task 2018-04-18T10:42:33.150Z] 10:42:33     INFO -  PID 13529 | JavaScript strict warning: resource://services-sync/policies.js, line 106: ReferenceError: reference to undefined property "_globalScore"
[task 2018-04-18T10:42:33.150Z] 10:42:33     INFO -  PID 13529 | 1524048152675	Sync.Status	DEBUG	Status.login: success.login => error.login.reason.no_username
[task 2018-04-18T10:42:33.151Z] 10:42:33     INFO -  PID 13529 | 1524048152675	Sync.Status	DEBUG	Status.service: success.status_ok => service.client_not_configured
[task 2018-04-18T10:42:33.151Z] 10:42:33     INFO -  PID 13529 | 1524048152675	Sync.Status	DEBUG	Status.service: service.client_not_configured => service.client_not_configured
[task 2018-04-18T10:42:33.152Z] 10:42:33     INFO -  PID 13529 | 1524048152679	Sync.Service	INFO	Loading Weave 1.63.0
[task 2018-04-18T10:42:33.153Z] 10:42:33     INFO -  PID 13529 | 1524048152682	Sync.Engine.Clients	DEBUG	Engine constructed
[task 2018-04-18T10:42:33.154Z] 10:42:33     INFO -  PID 13529 | 1524048152683	Sync.Engine.Clients	DEBUG	Resetting clients last sync time
[task 2018-04-18T10:42:33.155Z] 10:42:33     INFO -  (xpcshell/head.js) | test run_next_test 1 finished (2)
[task 2018-04-18T10:42:33.156Z] 10:42:33     INFO -  PID 13529 | 1524048152691	Sync.Engine.Addons	DEBUG	Engine constructed
[task 2018-04-18T10:42:33.157Z] 10:42:33     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "_globalScore"" {file: "resource://services-sync/policies.js" line: 106}]"
[task 2018-04-18T10:42:33.164Z] 10:42:33     INFO -  PID 13529 | 1524048152693	Sync.Engine.Addons	DEBUG	SyncEngine initialized: addons
[task 2018-04-18T10:42:33.164Z] 10:42:33     INFO -  PID 13529 | 1524048152694	Sync.AddonsReconciler	TRACE	Loading json from disk: addonsreconciler
[task 2018-04-18T10:42:33.165Z] 10:42:33     INFO -  PID 13529 | 1524048152695	Sync.AddonsReconciler	DEBUG	No data seen in loaded file: addonsreconciler
[task 2018-04-18T10:42:33.166Z] 10:42:33     INFO -  PID 13529 | 1524048152699	Sync.Engine.Forms	DEBUG	Engine constructed
[task 2018-04-18T10:42:33.167Z] 10:42:33     INFO -  PID 13529 | 1524048152700	Sync.Engine.Forms	DEBUG	SyncEngine initialized: forms
[task 2018-04-18T10:42:33.168Z] 10:42:33     INFO -  PID 13529 | 1524048152704	Sync.Engine.History	DEBUG	Engine constructed
[task 2018-04-18T10:42:33.168Z] 10:42:33     INFO -  PID 13529 | 1524048152706	Sync.Engine.History	DEBUG	SyncEngine initialized: history
[task 2018-04-18T10:42:33.169Z] 10:42:33     INFO -  PID 13529 | 1524048152710	Sync.Engine.Passwords	DEBUG	Engine constructed
[task 2018-04-18T10:42:33.170Z] 10:42:33     INFO -  PID 13529 | 1524048152712	Sync.Engine.Passwords	DEBUG	SyncEngine initialized: passwords
[task 2018-04-18T10:42:33.171Z] 10:42:33     INFO -  PID 13529 | 1524048152715	Sync.Engine.Prefs	DEBUG	Engine constructed
[task 2018-04-18T10:42:33.171Z] 10:42:33     INFO -  PID 13529 | 1524048152718	Sync.Engine.Prefs	DEBUG	SyncEngine initialized: prefs
[task 2018-04-18T10:42:33.173Z] 10:42:33     INFO -  PID 13529 | 1524048152721	Sync.Engine.Tabs	DEBUG	Engine constructed
[task 2018-04-18T10:42:33.173Z] 10:42:33     INFO -  PID 13529 | 1524048152724	Sync.Engine.Tabs	DEBUG	SyncEngine initialized: tabs
[task 2018-04-18T10:42:33.174Z] 10:42:33     INFO -  PID 13529 | 1524048152724	Sync.Engine.Tabs	DEBUG	Resetting tabs last sync time
[task 2018-04-18T10:42:33.176Z] 10:42:33     INFO -  PID 13529 | 1524048152726	Sync.Engine.Extension-Storage	DEBUG	Engine constructed
[task 2018-04-18T10:42:33.178Z] 10:42:33     INFO -  PID 13529 | 1524048152728	Sync.Engine.Extension-Storage	DEBUG	SyncEngine initialized: extension-storage
[task 2018-04-18T10:42:33.179Z] 10:42:33     INFO -  PID 13529 | 1524048152734	Sync.Engine.Bookmarks	DEBUG	Engine constructed
[task 2018-04-18T10:42:33.180Z] 10:42:33     INFO -  PID 13529 | 1524048152735	Sync.Engine.Bookmarks	DEBUG	SyncEngine initialized: bookmarks
[task 2018-04-18T10:42:33.183Z] 10:42:33     INFO -  PID 13529 | 1524048152736	Sync.Service	INFO	Mozilla/5.0 (X11; Linux i686 on x86_64; rv:61.0) Gecko/20100101 XPCShell/1
[task 2018-04-18T10:42:33.185Z] 10:42:33     INFO -  PID 13529 | 1524048152759	Sync.Status	DEBUG	Status.login: error.login.reason.no_username => error.login.reason.no_username
[task 2018-04-18T10:42:33.186Z] 10:42:33     INFO -  PID 13529 | 1524048152760	Sync.Status	DEBUG	Status.service: service.client_not_configured => service.client_not_configured
[task 2018-04-18T10:42:33.188Z] 10:42:33     INFO -  PID 13529 | 1524048152760	Sync.Status	DEBUG	Status.service: service.client_not_configured => service.client_not_configured
[task 2018-04-18T10:42:33.190Z] 10:42:33     INFO -  PID 13529 | 1524048152760	Sync.Engine.Tabs.Tracker	TRACE	stop().
[task 2018-04-18T10:42:33.192Z] 10:42:33     INFO -  PID 13529 | 1524048152764	Sync.SyncScheduler	TRACE	Handling weave:service:ready
[task 2018-04-18T10:42:33.193Z] 10:42:33     INFO -  (xpcshell/head.js) | test run_next_test 2 pending (2)
[task 2018-04-18T10:42:33.193Z] 10:42:33     INFO -  (xpcshell/head.js) | test setup finished (2)
[task 2018-04-18T10:42:33.194Z] 10:42:33     INFO -  services/sync/tests/unit/test_syncedtabs.js | Starting test_noClients
[task 2018-04-18T10:42:33.195Z] 10:42:33     INFO -  (xpcshell/head.js) | test test_noClients pending (2)
Flags: needinfo?(mak77)
The test just needed an update for page-icon.
Flags: needinfo?(mak77)
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/9923d93eddd8
Remove promiseFaviconLinkUrl and fix its consumers. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/9923d93eddd8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Verified fixed in Nightly 61.0a1 build 2018-04-19. Thanks!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.