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)
Tracking
()
VERIFIED
FIXED
Firefox 61
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)
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(mak77)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 4•6 years ago
|
||
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)
Comment 5•6 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/897bbf35e781 Remove promiseFaviconLinkUrl and fix its consumers. r=Gijs
Comment 8•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
The test just needed an update for page-icon.
Flags: needinfo?(mak77)
Comment 11•6 years ago
|
||
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/9923d93eddd8 Remove promiseFaviconLinkUrl and fix its consumers. r=Gijs
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9923d93eddd8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 13•6 years ago
|
||
Verified fixed in Nightly 61.0a1 build 2018-04-19. Thanks!
Status: RESOLVED → VERIFIED
status-firefox59:
--- → wontfix
status-firefox60:
--- → wontfix
status-firefox-esr60:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•