Closed Bug 935157 Opened 6 years ago Closed 6 years ago

Follow-up: correctly insert favicons into memory cache, and don't log page URLs

Categories

(Firefox for Android :: General, defect)

28 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 28
Tracking Status
firefox27 --- verified
firefox28 --- verified

People

(Reporter: aaronmt, Assigned: rnewman)

References

Details

Attachments

(1 file)

W/FaviconCache( 9419): Cannot compute dominant color of non-cached favicon <URL>

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/favicons/cache/FaviconCache.java#436

Seems to happen way too frequently just scrolling through my history list and even logging repeat URL's. Oh, and we shouldn't be logging URL's anyways (I think).
This bug is mostly "why the heck isn't it in the cache", IMO. I'll fix the PII log level while I'm there!
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Yup, in removing setFaviconWithUrl, the "url" argument was replaced by the page URL, not the favicon URL. Fix momentarily.

This will need to be uplifted.
Blocks: 931843
Summary: Reduce FaviconCache logging about the inability to compute dominant colours in a non-cached favicon → Follow-up: correctly insert favicons into memory cache, and don't log page URLs
[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Bug 931843 (just landed and uplifted).
 
User impact if declined: 
  Favicon in-memory cache won't work correctly -- it'll fill, but never get hits, and background color calculation won't work.

Testing completed (on m-c, etc.): 
  Manual testing of trivial patch, visually compared flawed diff.

Risk to taking this patch (and alternatives if risky): 
  None.

String or IDL/UUID changes made by this patch:
  None.
Attachment #827751 - Flags: review?(margaret.leibovic)
Attachment #827751 - Flags: approval-mozilla-aurora?
Comment on attachment 827751 [details] [diff] [review]
Proposed patch. v1

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

Makes sense.
Attachment #827751 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/integration/fx-team/rev/21eb1180a51e

QA: favicons should now have a correct dominant-color background set.
Whiteboard: [fixed in fx-team][qa+]
Target Milestone: --- → Firefox 28
Comment on attachment 827751 [details] [diff] [review]
Proposed patch. v1

low risk patch, fixing fallout from favicon changes in Fx27. Adding verifyme to help with QA verfication
Attachment #827751 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
https://hg.mozilla.org/mozilla-central/rev/21eb1180a51e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in fx-team][qa+] → [qa+]
This doesn't apply to Aurora due to bug 931843 not being present there. Not sure if this is worth uplifting at this point?
Flags: needinfo?(rnewman)
Status: RESOLVED → VERIFIED
Keywords: verifyme
Whiteboard: [qa+]
You need to log in before you can comment on or make changes to this bug.