Closed Bug 1164510 Opened 5 years ago Closed 5 years ago

Context needs a default favicon for the panel room list

Categories

(Hello (Loop) :: Client, defect, P1)

defect
Points:
3

Tracking

(firefox40 verified, firefox41 verified)

VERIFIED FIXED
mozilla41
Iteration:
41.3 - Jun 29
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: standard8, Assigned: mikedeboer)

References

Details

(Whiteboard: [context])

Attachments

(3 files, 2 obsolete files)

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+
Flags: needinfo?(sfranks)
Rank: 15
Priority: -- → P1
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)
(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)
Attached image Globe SVG
SVG attached.
Flags: needinfo?(mmaslaney)
Whiteboard: [context][ux] → [context]
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 41.2 - Jun 8
Points: --- → 1
Attachment #8615937 - Flags: review?(standard8)
Flags: qe-verify+
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)
Rank: 15 → 12
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)
Points: 1 → 3
Iteration: 41.2 - Jun 8 → 41.3 - Jun 29
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+
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)
Try push with fix for the test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3a15433d6c3
Flags: needinfo?(mdeboer)
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 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)
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).
(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.
Attachment #8620387 - Flags: review?(standard8) → review+
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
I was able to reproduce this test failure locally yesterday and located it to Loop/ Hello code. Will attach an updated patch soon.
I just landed it right away, because the failure was so darn obvious (100% reproducable locally when run in single-test-mode).
https://hg.mozilla.org/mozilla-central/rev/d02a12bf32f9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
QA Contact: bogdan.maris
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
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?
Carrying over r=Standard8.
Attachment #8637182 - Flags: review+
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)
Flags: needinfo?(mdeboer)
Attachment #8620387 - Flags: approval-mozilla-beta?
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+
(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. :'(
Attachment #8637182 - Flags: approval-mozilla-aurora+ → approval-mozilla-beta?
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+
(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.
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.
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.