Closed Bug 935157 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Version:
28 Branch
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox27 verified, firefox28 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 28
Tracking Flags:
Tracking Status
firefox27 --- verified
firefox28 --- verified

People

(Reporter: aaronmt, Assigned: rnewman)

References

Details

Attachments

(1 file)

Proposed patch. v1
1.92 KB, patch
Margaret
: review+
bajaj
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
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
status-firefox27: --- → affected
status-firefox28: --- → affected
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.
status-firefox28: affected → fixed
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: 11 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
status-firefox27: fixed → verified
status-firefox28: fixed → verified
Keywords: verifyme
Whiteboard: [qa+]
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: