Closed
Bug 1164510
Opened 9 years ago
Closed 9 years ago
Context needs a default favicon for the panel room list
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(firefox40 verified, firefox41 verified)
People
(Reporter: standard8, Assigned: mikedeboer)
References
Details
(Whiteboard: [context])
Attachments
(3 files, 2 obsolete files)
60.35 KB,
image/svg+xml
|
Details | |
90.39 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
92.49 KB,
patch
|
mikedeboer
:
review+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
When we don't have a favicon, we need some sort of indicator in the panel list that the room has context attached. Otherwise, there will just be a blank block which the user won't know to click, or a broken image that will confuse the user.
Flags: firefox-backlog+
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(sfranks)
Reporter | ||
Updated•9 years ago
|
Rank: 15
Priority: -- → P1
Comment 1•9 years ago
|
||
Let's use darker of the identity-icons-generic.png http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/identity-icons-generic.png
Flags: needinfo?(sfranks)
Assignee | ||
Comment 2•9 years ago
|
||
(In reply to Sevaan Franks [:sevaan] from comment #1) > Let's use darker of the identity-icons-generic.png > > http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/identity- > icons-generic.png Michael, could you provide a 16x16 SVG version of the globe icon? Thanks!
Flags: needinfo?(mmaslaney)
Updated•9 years ago
|
Whiteboard: [context][ux] → [context]
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 41.2 - Jun 8
Points: --- → 1
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8615937 -
Flags: review?(standard8)
Assignee | ||
Updated•9 years ago
|
Flags: qe-verify+
Reporter | ||
Comment 5•9 years ago
|
||
Comment on attachment 8615937 [details] [diff] [review] Patch v1: show a globe favicon as default fallback Review of attachment 8615937 [details] [diff] [review]: ----------------------------------------------------------------- Any reason for not making the globe svg part of the 16x16 icons? Also, I think this needs to adjust the fallback on the standalone views as well.
Attachment #8615937 -
Flags: review?(standard8)
Updated•9 years ago
|
Rank: 15 → 12
Assignee | ||
Comment 6•9 years ago
|
||
Now part of the SVG map. Somehow this didn't work for me the first time around, so I worked it out with a separate file. A retry was successful.
Attachment #8615937 -
Attachment is obsolete: true
Attachment #8617239 -
Flags: review?(standard8)
Assignee | ||
Updated•9 years ago
|
Points: 1 → 3
Assignee | ||
Updated•9 years ago
|
Iteration: 41.2 - Jun 8 → 41.3 - Jun 29
Reporter | ||
Comment 7•9 years ago
|
||
Comment on attachment 8617239 [details] [diff] [review] Patch v2: show a globe favicon as default fallback Review of attachment 8617239 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-loop.js @@ +8,5 @@ > (function() { > const kNSXUL = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; > const kBrowserSharingNotificationId = "loop-sharing-notification"; > const kPrefBrowserSharingInfoBar = "browserSharing.showInfoBar"; > + const kDefaultFavicon = "data:image/x-icon;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8/9hAAAArklEQVR42t2TMQ4CIRBF" + Can you add a comment about where this is obtained from? So that people searching for changes might have a chance of catching why the default favicon is now wrong?
Attachment #8617239 -
Flags: review?(standard8) → review+
Comment 9•9 years ago
|
||
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3394442&repo=fx-team
Flags: needinfo?(mdeboer)
Assignee | ||
Comment 10•9 years ago
|
||
Try push with fix for the test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3a15433d6c3
Flags: needinfo?(mdeboer)
Assignee | ||
Comment 11•9 years ago
|
||
Canceled that one, now running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=594dbed6cefd
Assignee | ||
Comment 12•9 years ago
|
||
Marco, can you check the code I changed/ added in browser-social.js? I think it's the right approach, because it makes sure we don't fetch favicons for pages that don't have one and fetches hi-res icons when needed via the moz-anno:favicon protocol. Thanks for your help earlier!
Attachment #8617239 -
Attachment is obsolete: true
Attachment #8620387 -
Flags: review?(standard8)
Attachment #8620387 -
Flags: feedback?(mak77)
Comment 13•9 years ago
|
||
Comment on attachment 8620387 [details] [diff] [review] Patch v3: show a globe favicon as default fallback Review of attachment 8620387 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-loop.js @@ +531,3 @@ > > + this.PlacesUtils.promiseFaviconLinkUrl(pageURI).then(uri => { > + uri = this.PlacesUtils.getImageURLForResolution(window, uri.spec); yes, this should do what you want. But if you just need to fetch the icon data, why don't you directly use PlacesUtils.favicons.getFaviconDataForPage instead of xhr? I think it's possible to build the data url you want from the data returned by its callback (data octets, mime type)
Attachment #8620387 -
Flags: feedback?(mak77)
Comment 14•9 years ago
|
||
though, getFaviconDataForPage won't give you only the right resolution image, you would get the .ico data, but after building the data url you could still use getImageURLForResolution on it. Depending on how much you care about perf one of the 2 solutions could be preferred, while the other could be simplet (I'd expect getFaviconDataForPage to be faster than xhr).
Assignee | ||
Comment 15•9 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #14) > though, getFaviconDataForPage won't give you only the right resolution > image, you would get the .ico data, but after building the data url you > could still use getImageURLForResolution on it. > Depending on how much you care about perf one of the 2 solutions could be > preferred, while the other could be simplet (I'd expect > getFaviconDataForPage to be faster than xhr). True, this is what I implemented before. Except, using `btoa` on the data octets threw an error due to its unicode incompatibility. Using nsIScriptableBase64Encoder like in `BookmarkHTMLUtils.jsm` works, but would introduce another function. I'm not too fussed about speed here. But how can I use getImageURLForResolution on a data-uri? AFAICT it's only supposed to work on moz-anno:favicon URLs and getFaviconDataForPage requires the page URI, not that of the favicon. So that's why I opted for this solution eventually.
Comment 16•9 years ago
|
||
Backout: https://hg.mozilla.org/mozilla-central/rev/cbc9af5b38ee
Reporter | ||
Updated•9 years ago
|
Attachment #8620387 -
Flags: review?(standard8) → review+
Comment 18•9 years ago
|
||
Backed out for causing frequent browser_UITour_loop.js failures. They were hitting on both OSX and Windows at a ~10-20% rate. https://treeherder.mozilla.org/logviewer.html#?job_id=3423175&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/80e89b5bfe4f
Assignee | ||
Comment 19•9 years ago
|
||
I was able to reproduce this test failure locally yesterday and located it to Loop/ Hello code. Will attach an updated patch soon.
Assignee | ||
Comment 21•9 years ago
|
||
I just landed it right away, because the failure was so darn obvious (100% reproducable locally when run in single-test-mode).
Comment 22•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d02a12bf32f9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•9 years ago
|
QA Contact: bogdan.maris
Comment 23•9 years ago
|
||
Reproduced the initial issue using Aurora build, verified that using latest Nightly across platforms (Windows 7 64-bit, Mac OS X 10.9.4 and Ubuntu 14.04 32-bit) on websites without favicon, globe icon is properly displayed in conversation window, hello panel.
Updated•9 years ago
|
status-firefox40:
--- → affected
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8620387 [details] [diff] [review] Patch v3: show a globe favicon as default fallback Approval Request Comment [Feature/regressing bug #]: Loop/ Hello context in conversations feature - bug 1115340 [User impact if declined]: User will see the wrong, missing and/ or dashed square favicon when no favicon resource is available for a URL attached to a conversation as context. [Describe test coverage new/current, TreeHerder]: landed on m-c, m-a and tests pass. Baked well-done. [Risks and why]: minor. [String/UUID change made/needed]: n/a.
Attachment #8620387 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 26•9 years ago
|
||
Beta patch try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae8527ff8823
Mike, I noticed that this bug has two patches and you have requested beta-uplift on the one that is NOT called "for beta". I just wanted to make sure there wasn't a mistake.
Flags: needinfo?(mdeboer)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mdeboer)
Attachment #8620387 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 28•9 years ago
|
||
Comment on attachment 8637182 [details] [diff] [review] Patch v3.beta: BETA only patch See comment 24 for approval request.
Attachment #8637182 -
Flags: approval-mozilla-aurora?
Comment on attachment 8637182 [details] [diff] [review] Patch v3.beta: BETA only patch This patch was verified by QE and has an automated test too. Let's uplift to Aurora.
Attachment #8637182 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 30•9 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #29) > This patch was verified by QE and has an automated test too. Let's uplift to > Aurora. So sorry, this is already on Aurora. I meant to request BETA. :'(
Assignee | ||
Updated•9 years ago
|
Attachment #8637182 -
Flags: approval-mozilla-aurora+ → approval-mozilla-beta?
Comment 31•9 years ago
|
||
Comment on attachment 8637182 [details] [diff] [review] Patch v3.beta: BETA only patch Although this is a somewhat large change, the fix has been on m-c for a month and has been verified by QE. I think this is safe enough to take in beta8. Beta+ Mike - We're late in the beta cycle. Can you please verify that the fix does what you intended in tomorrow's beta8 build?
Flags: needinfo?(mdeboer)
Attachment #8637182 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 32•9 years ago
|
||
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #31) > Mike - We're late in the beta cycle. Can you please verify that the fix does > what you intended in tomorrow's beta8 build? Yes, certainly. Leaving n-i for that purpose.
Comment 34•9 years ago
|
||
Verified that using Firefox 40 beta 8 across platforms (Windows 7 64-bit, Mac OS X 10.10.4 and Ubuntu 14.04 64-bit) on websites without favicon, globe icon is properly displayed in conversation window, hello panel and standaloneUI.
Assignee | ||
Comment 35•9 years ago
|
||
I can also confirm that everything is working as expected on beta 8.
Flags: needinfo?(mdeboer)
You need to log in
before you can comment on or make changes to this bug.
Description
•