Distribution thumbnails missing for pinned sites

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

Trunk
Firefox 35
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox35 fixed, fennec35+)

Details

Attachments

(2 attachments)

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.
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
Blocks: 1068425
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)
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)
(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.
(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.
Attachment #8498638 - Flags: review?(lucasr.at.mozilla) → review+
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+
(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
tracking-fennec: ? → 35+
https://hg.mozilla.org/mozilla-central/rev/4a837d6936a0
https://hg.mozilla.org/mozilla-central/rev/279afba13f4c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
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?
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)
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)
Flags: in-testsuite?
Flags: qe-verify+
Hi everyone,

Seeing as this flag was set 4 years ago, I will remove the qe-verify+ flag.

Thank you!
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.