1125280 - Different suggestedsite tiles on first run for kidfox

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jenn Chaulk (:jchaulk)]

KidFox: Tiles: custom set of kid-appropriate default Tiles

Comment 1

[Sam Fang [:sfang]]

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

Comment 2

[:Margaret Leibovic]

How is this different than bug 1182108? Can we dupe these?

Comment 3

[:Margaret Leibovic]

(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.

Comment 4

[Allison Naaktgeboren :ally]

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?

Comment 5

[Sam Fang [:sfang]]

(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.

Comment 6

[Allison Naaktgeboren :ally]

Sold!

Comment 7

[:Margaret Leibovic]

Should we still look at disabling the marketplace/AMO tiles, since app and add-on installation are disabled?

Comment 8

[Allison Naaktgeboren :ally]

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.

Comment 9

[:Margaret Leibovic]

(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?

Comment 10

[Allison Naaktgeboren :ally]

Margaret, last line of comment 5 "Its best if we just not include any tiles at all for phase 1."

Comment 11

[Allison Naaktgeboren :ally]

I updated the title to reflect the current plan/desired outcome

Comment 12

[:Margaret Leibovic]

(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

[:Margaret Leibovic]

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.

Comment 14

[Allison Naaktgeboren :ally]

Well Anthony, We have 4 business days not counting today until the 42 deadline. Could I have those afore-mentioned icons stat?

Comment 15

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I can look these up for you. What dimensions?

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(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=

Comment 17

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: webmaker_tile.zip

Located the designer responsible for the webmaker logo and they gave me this. I've exported it for ya!

Comment 18

[Allison Naaktgeboren :ally]

What about the one for this not-yet-created sumo article?

Comment 19

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

is there a reason to not use the current Mozilla Support wordmark?

Comment 20

[:Margaret Leibovic]

(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.

Comment 21

[Allison Naaktgeboren :ally]

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?

Comment 22

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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

Comment 23

[Allison Naaktgeboren :ally]

\o/

Comment 24

[Nick Alexander :nalexander [he/him]]

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.)

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

(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. :)

Comment 27

[Nick Alexander :nalexander [he/him]]

(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 :)

Comment 28

[Nick Alexander :nalexander [he/him]]

Attachment: 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.

Comment 29

[Gregory Szorc [: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?

Comment 30

[Allison Naaktgeboren :ally]

Attachment: kidfox-no-tiles-hatchet

Pending the regression that is 1190597

Comment 31

[Allison Naaktgeboren :ally]

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. :/

Comment 32

[Allison Naaktgeboren :ally]

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

Comment 33

[Allison Naaktgeboren :ally]

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

[:Margaret Leibovic]

(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 35

[Sebastian Kaspari (:sebastian; :pocmo)]

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?

Comment 36

[Allison Naaktgeboren :ally]

Attachment: 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.

Comment 37

[Allison Naaktgeboren :ally]

Attachment: 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

Comment 38

[Allison Naaktgeboren :ally]

Attachment: 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.

Comment 39

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b8f3ee0be9aa
https://hg.mozilla.org/integration/fx-team/rev/bd1474381abd

Comment 41

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b8f3ee0be9aa
https://hg.mozilla.org/mozilla-central/rev/bd1474381abd

Comment 42

[u549602]

verified as fixed on latest Aurora.