Closed
Bug 1076438
Opened 10 years ago
Closed 10 years ago
Distribution thumbnails missing for pinned sites
Categories
(Firefox for Android Graveyard :: Data Providers, defect)
Tracking
(firefox35 fixed, fennec35+)
RESOLVED
FIXED
Firefox 35
People
(Reporter: bnicholson, Assigned: bnicholson)
References
Details
Attachments
(2 files)
19.56 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
7.75 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
STR: 1) Install a distribution. 2) Pin one of the distribution tiles. The problem is that we have two different cursors for pinned sites and suggested sites, so a pinned site will fail the check here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/TopSitesCursorWrapper.java#251. Unfortunately, this also makes things more difficult for ID tracking since the pinned cursor will not be able to access the tracking ID for the same reasons. The easiest fix is probably to just back out bug 1073257, then add an ID getter in SuggestedSites like the image/bgcolor getters we had before.
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Updated•10 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Comment 1•10 years ago
|
||
This effectively backs out bug 1073257 and bug 1071039. I don't like doing this, but keeping this API would require more refactoring to move forward with the tiles tracking work. The pinned sites cursor is entirely independent from the suggested sites cursor, so I don't think there's good no way for us to access these properties from it (barring another cursor wrapper...). I'd be happy to hear otherwise!
Attachment #8498638 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 2•10 years ago
|
||
Replaces this with a URL-based lookup for the ID, like we have with the imageurl and bgcolor. Also, I noticed that the API at https://github.com/oyiptong/onyx/blob/master/README.md shows the IDs are numbers, not strings. I figured this would be a good opportunity to make that change.
Attachment #8498640 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #1) > so I don't think there's good no way for us That there not be no English I never didn't speak.
Comment 4•10 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #0) > STR: > 1) Install a distribution. > 2) Pin one of the distribution tiles. > > The problem is that we have two different cursors for pinned sites and > suggested sites, so a pinned site will fail the check here: > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/ > TopSitesCursorWrapper.java#251. > > Unfortunately, this also makes things more difficult for ID tracking since > the pinned cursor will not be able to access the tracking ID for the same > reasons. The easiest fix is probably to just back out bug 1073257, then add > an ID getter in SuggestedSites like the image/bgcolor getters we had before. Ha! And now I remember why I didn't put the image URL and bg color in TopSitesCursorWrapper! Sorry, I missed that in my review for bug 1073257.
Updated•10 years ago
|
Attachment #8498638 -
Flags: review?(lucasr.at.mozilla) → review+
Comment 5•10 years ago
|
||
Comment on attachment 8498640 [details] [diff] [review] Add tracking ID to SuggestedSites API Review of attachment 8498640 [details] [diff] [review]: ----------------------------------------------------------------- Yep. ::: mobile/android/base/db/SuggestedSites.java @@ +141,5 @@ > > public JSONObject toJSON() throws JSONException { > final JSONObject json = new JSONObject(); > > + if (trackingId >= 0) { Is 0 a valid tracking ID? ::: mobile/android/tests/browser/junit3/src/TestSuggestedSites.java @@ +375,5 @@ > } > > + public void testTrackingIds() { > + String content = "[{ trackingid: 42, url: \"url1\", title: \"title1\", imageurl: \"imageurl1\", bgcolor: \"bgcolor1\" }," + > + " { url: \"url2\", title: \"title2\", imageurl: \"imageurl2\", bgcolor: \"bgcolor2\" }]"; Why not reusing the code that generates JSON strings in TestSuggestedSites? This string is kinda hard to read.
Attachment #8498640 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #5) > Is 0 a valid tracking ID? I'm not sure. I don't see anything in the sample API that says otherwise, so it seems safer to assume it could be. > ::: mobile/android/tests/browser/junit3/src/TestSuggestedSites.java > Why not reusing the code that generates JSON strings in TestSuggestedSites? > This string is kinda hard to read. Added an includeIds boolean to generateSites to test two separate batches (one with IDs, and one without). https://hg.mozilla.org/integration/fx-team/rev/4a837d6936a0 https://hg.mozilla.org/integration/fx-team/rev/279afba13f4c
Updated•10 years ago
|
tracking-fennec: ? → 35+
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4a837d6936a0 https://hg.mozilla.org/mozilla-central/rev/279afba13f4c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8498640 [details] [diff] [review] Add tracking ID to SuggestedSites API Might as well get this landed now in preparation for bug 1068425. Approval Request Comment [Feature/regressing bug #]: Tacks on ID support to our existing distribution code. [User impact if declined]: We won't be able uplift tiles tracking (bug 1068425). [Describe test coverage new/current, TBPL]: Includes new tests for distributions with and without IDs. [Risks and why]: Very low risk. This just adds existing functionality to the distribution code. Has no effect on existing distributions. [String/UUID change made/needed]: None Note to uplifters: The first patch in this bug was a backout of other patches that never made it to Aurora, so this patch should be all that's needed.
Attachment #8498640 -
Flags: approval-mozilla-aurora?
Comment 9•10 years ago
|
||
Brian - Both this bug and bug 1068425 are tracking 35. Is the intention to uplift bug 1068425 to 34 this week and, if not, what is the value of uplifting this bug?
Flags: needinfo?(bnicholson)
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8498640 [details] [diff] [review] Add tracking ID to SuggestedSites API (In reply to Lawrence Mandel [:lmandel] from comment #9) > Brian - Both this bug and bug 1068425 are tracking 35. Is the intention to > uplift bug 1068425 to 34 this week and, if not, what is the value of > uplifting this bug? That was the goal, and I expect patches for bug 1068425 to be ready this week, though we'll still need server-side changes before this can actually work. I'll remove the approval flag for now until these are finalized.
Attachment #8498640 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(bnicholson)
status-firefox35:
--- → fixed
Flags: qe-verify+
Comment 11•6 years ago
|
||
Hi everyone, Seeing as this flag was set 4 years ago, I will remove the qe-verify+ flag. Thank you!
Flags: qe-verify+
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•