Different suggestedsite tiles on first run for kidfox

VERIFIED FIXED in Firefox 42

Status

()

Firefox for Android
General
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: jchaulk, Assigned: ally)

Tracking

Trunk
Firefox 42
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
KidFox: Tiles: custom set of kid-appropriate default Tiles
Blocks: 1125710
OS: Windows 8.1 → Android
Hardware: x86_64 → All
Version: unspecified → Trunk
(Reporter)

Updated

3 years ago
Blocks: 1125984

Updated

3 years ago
No longer blocks: 1125984

Comment 1

3 years ago
Default thumbnails:
- Do not want add-ons and marketplace
- Replace them with 2 different sites
- Thumbnails will be replaced by history over time
- Reach out to mofo to determine first-run experience and sites to use
Blocks: 1125984
Summary: KidFox: Distribution file - Tiles → KidFox: Tiles for phase 1

Comment 2

3 years ago
How is this different than bug 1182108? Can we dupe these?
Flags: needinfo?(sfang)

Comment 3

3 years ago
(In reply to :Margaret Leibovic from comment #2)
> How is this different than bug 1182108? Can we dupe these?

Sam and I just talked about this - bug 1182108 is about a more complex phase 2 solution. This bug is just about replacing the default hardcoded set of tiles with a separate hardcoded set of tiles.
Flags: needinfo?(sfang)
(Assignee)

Comment 4

3 years ago
mfinkle raised today that there are legal/bd issues with including new tiles, or those with which we dont have an existing relationship with. 

Sam, how goes investigating webmaker & foundation sites for this?
Flags: needinfo?(sfang)
(Assignee)

Updated

3 years ago
Assignee: nobody → ally

Comment 5

3 years ago
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #4)
> mfinkle raised today that there are legal/bd issues with including new
> tiles, or those with which we dont have an existing relationship with. 
> 
> Sam, how goes investigating webmaker & foundation sites for this?

Hello Ally,

Since we are planning to work out the tiles with third-party filtering companies for phase 2.
If we replace the tiles with 2 of our own sites, then we might have to change them again during phase 2.
Its best if we just not include any tiles at all for phase 1.
Flags: needinfo?(sfang)
(Assignee)

Comment 6

3 years ago
Sold!

Comment 7

3 years ago
Should we still look at disabling the marketplace/AMO tiles, since app and add-on installation are disabled?
Flags: needinfo?(sfang)
Flags: needinfo?(ally)
(Assignee)

Comment 8

3 years ago
You mean prevent the user from visiting the site at all? 

The current spec already calls to remove all pre-installed tiles, including those 2. My current wip removes the as pre-install bookmarks as well.
Flags: needinfo?(ally)

Comment 9

3 years ago
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #8)
> You mean prevent the user from visiting the site at all? 

No, I just meant removing the tiles.

> The current spec already calls to remove all pre-installed tiles, including
> those 2. My current wip removes the as pre-install bookmarks as well.

Oh, I didn't see that mentioned anywhere in this bug. Can you update the bug with the current plan, and re-summarize as needed?
Flags: needinfo?(ally)
(Assignee)

Comment 10

3 years ago
Margaret, last line of comment 5 "Its best if we just not include any tiles at all for phase 1."
Flags: needinfo?(ally)
(Assignee)

Updated

3 years ago
Summary: KidFox: Tiles for phase 1 → KidFox: phase 1: No preselected tiles on first run
(Assignee)

Comment 11

3 years ago
I updated the title to reflect the current plan/desired outcome

Comment 12

3 years ago
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #10)
> Margaret, last line of comment 5 "Its best if we just not include any tiles
> at all for phase 1."

Oh, I read this as "it's best if we don't include any tiles changes for phase 1".

Sam, did you mean that you don't think we should have any tiles at all, or that we shouldn't have any changes to our current set of tiles?

If we don't have any tiles at all, first run will look a bit barren. Are we okay with that?

Comment 13

3 years ago
We talked about this with Karen this morning, and we do want to keep tiles to avoid an empty top sites panel on first run.

We talked about having the following tiles:
* mozilla.org
* webmaker.org
* SUMO article about restricted profile

You should ask Joni or Roland about a link for the SUMO article, and you should ask antlam for images for the tiles.

Let's not worry about bookmarks in this bug, but that's a good call, we should file a bug about that.

Updated

3 years ago
Flags: needinfo?(sfang)
(Assignee)

Comment 14

3 years ago
Well Anthony, We have 4 business days not counting today until the 42 deadline. Could I have those afore-mentioned icons stat?
Flags: needinfo?(alam)
I can look these up for you. What dimensions?
Flags: needinfo?(ally)
(In reply to Anthony Lam (:antlam) from comment #15)
> I can look these up for you. What dimensions?

Multiple:
http://mxr.mozilla.org/mozilla-central/find?string=suggestedsites_&tree=mozilla-central&hint=
Created attachment 8640225 [details]
webmaker_tile.zip

Located the designer responsible for the webmaker logo and they gave me this. I've exported it for ya!
Flags: needinfo?(alam)
(Assignee)

Comment 18

3 years ago
What about the one for this not-yet-created sumo article?
Flags: needinfo?(alam)
is there a reason to not use the current Mozilla Support wordmark?
Flags: needinfo?(alam)

Comment 20

3 years ago
(In reply to Anthony Lam (:antlam) from comment #19)
> is there a reason to not use the current Mozilla Support wordmark?

I think using the current support wordmark would be fine, provided we are just replacing the current support tile with one that goes directly to the SUMO article.

I didn't mention it in the bug here, but on IRC Robin also said she could help out with this.
(Assignee)

Comment 21

3 years ago
We do not appear to have the workmark already as a resource? I dont see anything turn up greping through http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/. 

 I did find that we use this icon for sumo in about:feedback http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/images/icon_floaty_xxhdpi.png ? I imagine that is not what you want.

So I guess I need a set of pngs with the current wordmark to go in?
Flags: needinfo?(ally) → needinfo?(alam)
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #21)
> We do not appear to have the workmark already as a resource? I dont see
> anything turn up greping through
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/.
> 
> 
>  I did find that we use this icon for sumo in about:feedback
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/
> images/icon_floaty_xxhdpi.png ? I imagine that is not what you want.
> 
> So I guess I need a set of pngs with the current wordmark to go in?

it's the fxsupport ones here: http://mxr.mozilla.org/mozilla-central/find?string=suggestedsites_&tree=mozilla-central&hint
Flags: needinfo?(alam) → needinfo?(ally)
(Assignee)

Comment 23

3 years ago
\o/
Flags: needinfo?(ally)
ally and I discussed two options.  Both include adding the relevant entries, say, 

browser.suggestedsites.kidfox.list.0=foo

and

browser.suggestedsites.foo.title=Kidfox only site title
...

to region.properties.

Option 1) Make the build system generate a second suggestedsites.kidfox.json into res/raw.  Then make SuggestedSites.java conditionally read from suggestedsites.json or suggestedsites.kidfox.json at runtime.

Option 2) Make the build system (generate_suggestedsites) read from a second list, browser.suggestedsites.kidfox.list, and add additional entries to the existing suggestedsites.json, tagged in some way.  The existing JSON format isn't fantastic for that, but we could add a per-site "profile" or "restriction" tag and then make SuggestedSites.java filter as appropriate.

I have no strong preference, but:

* Ally will probably find 2) easier, since it requires less Makefile foo (and more Python foo).
* 2) is more likely to be backwards compatible with existing distribution files, too, since the absence of the restricted flag can be anticipated.
* 2) should inherit whatever fallback logic we've built for the existing system.
* 2) should also avoid issues that might arise if a suggestedsites.kidfox.json doesn't exist (although I don't anticipate any issues).

To get started on 2), iterate over two lists at line https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/generate_suggestedsites.py#81.  For the second (kidfox) list, add a "sublist" key (like kidfox) to each JSON record.

I can help with this if necessary.

Is an empty set of suggestedsites for Kidfox okay?  (I.e., when localizers haven't set any.)
(Reporter)

Updated

3 years ago
Status: NEW → ASSIGNED
See Also: → bug 1189790
The scope of this bug is now only about having *no* tiles, right? Sam filed bug 1189790 about having specific KidFox tiles. Do we need less effort for showing no tiles or is this basically the same like showing different tiles? Either we keep both bugs or we merge them.
See Also: bug 1189790
(In reply to Sebastian Kaspari (:sebastian) from comment #25)
> The scope of this bug is now only about having *no* tiles, right? Sam filed
> bug 1189790 about having specific KidFox tiles. Do we need less effort for
> showing no tiles or is this basically the same like showing different tiles?
> Either we keep both bugs or we merge them.

The other bug was actually about bookmarks. Nevermind. :)
(In reply to Sebastian Kaspari (:sebastian) from comment #25)
> The scope of this bug is now only about having *no* tiles, right? Sam filed
> bug 1189790 about having specific KidFox tiles. Do we need less effort for
> showing no tiles or is this basically the same like showing different tiles?
> Either we keep both bugs or we merge them.

For the record, no tiles is a trivial Java change to SuggestedSites.java -- switch on the context, return no tiles :)
(Assignee)

Updated

3 years ago
Depends on: 1189920
Created attachment 8641882 [details]
MozReview Request: Bug 1125280 - Add list of 'restricted' suggestedsites. f?ally, r?gps

Bug 1125280 - Add list of 'restricted' suggestedsites. f?ally, r?gps

This ticket adds a second list of suggestedsites, generated and
localized at build time.  The JSON in the second list is tagged with
an additional 'restricted': true flag in order to be easily filtered
at runtime.

gps: I didn't see making the set of lists configurable in the
region.properties file, but I nodded in that direction by making the
list of lists a data structure, and using a general default item JSON
object (rather than just a flag).

ally: the resources are required (to prevent runtime failures) but the
ones I've included will need to be corrected.
Attachment #8641882 - Flags: review?(gps)
Comment on attachment 8641882 [details]
MozReview Request: Bug 1125280 - Add list of 'restricted' suggestedsites. f?ally, r?gps

https://reviewboard.mozilla.org/r/14633/#review13231

I'm not too sure what all this is related to. But the Python code looks good. Might want to run it by someone who knows what's going on if you aren't super confident about what you are doing. But I'll give you r+.

::: python/mozbuild/mozbuild/action/generate_suggestedsites.py:117
(Diff revision 1)
> +    lists = [
> +        ('browser.suggestedsites.list', {}),
> +        ('browser.suggestedsites.restricted.list', {'restricted': True}),
> +    ]

Should this list be extracted to a module-level variable so it is more visible?
Attachment #8641882 - Flags: review?(gps) → review+
(Assignee)

Comment 30

3 years ago
Created attachment 8642672 [details] [diff] [review]
kidfox-no-tiles-hatchet

Pending the regression that is 1190597
(Assignee)

Comment 31

3 years ago
Comment on attachment 8642672 [details] [diff] [review]
kidfox-no-tiles-hatchet

I had to hardcode isUsedRestricted() to return true to move forward around that nasty regression. :/
Attachment #8642672 - Flags: review?(s.kaspari)
(Assignee)

Comment 32

3 years ago
Nick's patch as-is can't land. the webmaker & specific kidfox page show up in regular firefox, where theyre not supposed to.
(Assignee)

Comment 33

3 years ago
Cachedsites has been claiming to have 5 records instead of the 7 introduced by Nick's patch. So regular fennec is only showing 2/4 required tiles even if I filter on the restricted property.

I traced we now have entries with duplicate urls, one entry with a restricted tag, and one without, but use a hashmap where there cannot be 2 entries with the same url. When the json processor tries to add the second entry, it silently throws it out. I wish it had thrown.

It did not help that the frame where this happens is not included in the stack trace. 

I have a sketch for a workaround.

Comment 34

3 years ago
(In reply to Nick Alexander :nalexander from comment #27)
> (In reply to Sebastian Kaspari (:sebastian) from comment #25)
> > The scope of this bug is now only about having *no* tiles, right? Sam filed
> > bug 1189790 about having specific KidFox tiles. Do we need less effort for
> > showing no tiles or is this basically the same like showing different tiles?
> > Either we keep both bugs or we merge them.
> 
> For the record, no tiles is a trivial Java change to SuggestedSites.java --
> switch on the context, return no tiles :)

As I understand it, this bug is about trying to get rid of the marketplace and AMO suggested sites, not all of the suggested sites.

No tiles seems like a fine back-up option for 42 if we can't get a proper solution done in time, but we should still try to find a way to disable some subset of the suggested sites for restricted profiles.

If we are going to just disable tiles for this bug, we should file a separate bug for Nick's work.
Comment on attachment 8642672 [details] [diff] [review]
kidfox-no-tiles-hatchet

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

::: mobile/android/base/db/SuggestedSites.java
@@ +469,5 @@
>              return cursor;
>          }
>  
> +        // Return an empty cursor for both guest mode & restricted profile
> +        if (RestrictedProfiles.isUserRestricted(context)) {

I assume we do not want to remove the default sites for a guest profile? Does it make sense to make isRestrictedProfile public? Or better remember the decision made in createConfiguration() and make it accessible?
Attachment #8642672 - Flags: review?(s.kaspari) → feedback+
(Assignee)

Comment 36

3 years ago
Created attachment 8644161 [details] [diff] [review]
Kidfox-newstartingtiles-Part2-wip

So we, mostly I, am in a bit of a spot. I can switch over the sites object to handle the duplicate entries introduced by kidfox, but the cachedsites is a bigger issue. What do we do with the cachedsites list when it contains two almost duplicate entries? Which do we black list? ....

So tomorrow I throw most of this out the window for now, and come up with a way to keep the existing hashmap and still give us what we want.
(Assignee)

Comment 37

3 years ago
Created attachment 8644584 [details] [diff] [review]
kidfox-starting-tiles-hack - works

has a few issues (webmaker uses marketplace icon, printfs galore, inot quite the right if check), but is otherwise functional
(Assignee)

Comment 38

3 years ago
Created attachment 8644796 [details] [diff] [review]
kidfox-starting-tiles-hack

- the biggest issue is that the existing data structure can't handle duplicate urls (in particular the blocklist mechanism can't) and we can't break regular fennec. So there's a hack in here where the mozilla.org links can't be the exact same url

- built on top of Nick's patch, some assets need to be changed, and the filtering for regular fennec is needed.
Attachment #8642672 - Attachment is obsolete: true
Attachment #8644161 - Attachment is obsolete: true
Attachment #8644584 - Attachment is obsolete: true
Attachment #8644796 - Flags: review?(s.kaspari)
Comment on attachment 8644796 [details] [diff] [review]
kidfox-starting-tiles-hack

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

::: mobile/android/base/RestrictedProfiles.java
@@ +52,5 @@
>      private static boolean isGuestProfile() {
>          return GeckoAppShell.getGeckoInterface().getProfile().inGuestMode();
>      }
>      @TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR2)
> +    public static boolean isRestrictedProfile(Context context) {

I did this in the patch for bug 1184190 too. Let's see who's faster landing this.. ;)

::: mobile/android/base/db/SuggestedSites.java
@@ +100,3 @@
>          public Site(JSONObject json) throws JSONException {
>              this.trackingId = json.isNull(JSON_KEY_TRACKING_ID) ? TRACKING_ID_NONE : json.getInt(JSON_KEY_TRACKING_ID);
> +            this.restricted =  !json.isNull(JSON_KEY_RESTRICTED);

So by definition this will never be in the JSON and be false, right?

@@ +146,5 @@
>              final JSONObject json = new JSONObject();
>              if (trackingId >= 0) {
>                  json.put(JSON_KEY_TRACKING_ID, trackingId);
>              }
> +            if(restricted) {

NIT: space

@@ +511,5 @@
>                  continue;
>              }
> +            final boolean restrictedProfile =  RestrictedProfiles.isRestrictedProfile(context);
> +            final boolean addSite = ( restrictedProfile &&  site.restricted) ||
> +                                    (!restrictedProfile && !site.restricted);

A shorter version would be
> addSite = restrictedProfile == site.restricted;

But I guess that's also confusing.
Attachment #8644796 - Flags: review?(s.kaspari) → review+
(Assignee)

Updated

3 years ago
Summary: KidFox: phase 1: No preselected tiles on first run → Different suggestedsite tiles on first run for kidfox
https://hg.mozilla.org/mozilla-central/rev/b8f3ee0be9aa
https://hg.mozilla.org/mozilla-central/rev/bd1474381abd
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42

Comment 42

3 years ago
verified as fixed on latest Aurora.
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified
You need to log in before you can comment on or make changes to this bug.