1189790 - Include bookmarks for Kinderfox

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sam Fang [:sfang]]

From the 4 existing bookmarks (mozilla, support, amo, marketplace)

- Remove amo and marketplace
- Replace general support with a direct link to the SUMO article explaining what the restricted version of the browser is
- Keep mozilla
- Add webmaker

Comment 1

[Richard Newman [:rnewman]]

I think you're talking about suggested site tiles (the big ones on Top Sites), not bookmarks. Please re-correct if necessary!

Comment 2

[Sam Fang [:sfang]]

(In reply to Richard Newman [:rnewman] from comment #1)
> I think you're talking about suggested site tiles (the big ones on Top
> Sites), not bookmarks. Please re-correct if necessary!

Its actually for bookmarks. We want bookmarks to show the same content on the suggest tiles.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

@ally: Is the list of bookmarks filled from the same mechanism like top sites?

Comment 4

[:Margaret Leibovic]

(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> @ally: Is the list of bookmarks filled from the same mechanism like top
> sites?

No, unfortunately, it's different.

Ally, you already had a patch for this, right? Can you post it?

Comment 5

[:Margaret Leibovic]

Oh, this already seems to be addressed in bug 1189447... but we can use this bug to still include the other 2 default bookmarks that we want, rather than just disabling all of them.

Comment 6

[Allison Naaktgeboren :ally]

I had a patch that skipped adding them. :/ I can try to play the reflection game to try and keep the 2 we want from the orignal set. Adding new ones not in the default set could be messier than the tiles.

Comment 7

[:Margaret Leibovic]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #6)
> I had a patch that skipped adding them. :/ I can try to play the reflection
> game to try and keep the 2 we want from the orignal set. Adding new ones not
> in the default set could be messier than the tiles.

Yes, I wouldn't try adding new ones, I would just try not adding AMO and Marketplace.

This is where they're created:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#220

Maybe instead of matching a pattern like this, we should just be explicit about the 4 fields we're looking for, and exclude AMO and Marketplace for restricted profiles. These fields don't change for different locales, and we have never changed this set of default bookmarks, so I'm not sure why we bother with this pattern matching business.

We could also take the opportunity to make this logic better beyond Firefox 42.

Comment 8

[Allison Naaktgeboren :ally]

Well, let's use this bug to get the two of the original bookmarks inserted, and followup with how we'd like to deal with this going forward. Do we treat it as a distribution? Wire it up to behave like suggested sites? etc.

Comment 9

[Allison Naaktgeboren :ally]

Attachment: kidfox-restore-some-defualt-bookmarks

-restores mozilla.org & sumo.

I'd like to see if I can shove in the two additional bookmarks, but the built-in favicon invariant might get the better of me. we'll see.

Comment 10

[Allison Naaktgeboren :ally]

The webmaker .pngs from the tiles are transparent, so is being an invisible favicon :/. Anthony, can I get a nontransparent png for use in the web?

Comment 11

[Allison Naaktgeboren :ally]

Attachment: kidfox--kidfox-bookmarks

- waiting on a proper favicon for webmaker since the existing one is transparent and looks like there's nothing there.

feedback:
- margaret for the localization/autogeneration bits
- sebastian for the java

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8646061 [details] [diff] [review]
kidfox--kidfox-bookmarks

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

::: mobile/android/base/db/LocalBrowserDB.java
@@ +228,5 @@
> +                if (RestrictedProfiles.isUserRestricted()) {
> +                    // matching on variable name from strings.xml.in
> +                    final String addons = "bookmarkdefaults_title_addons";
> +                    final String marketplace = "bookmarkdefaults_title_marketplace";
> +                    final String regular_sumo = "bookmarkdefaults_title_support";

In Java we use camelCase for method and variable names.

@@ +232,5 @@
> +                    final String regular_sumo = "bookmarkdefaults_title_support";
> +                    if (name ==  addons
> +                            || name == marketplace
> +                            || name == regular_sumo) {
> +                        continue;

You should use the equals() method to compare Strings (or Objects in general). == is only comparing references. If this has been working here then you have been lucky and some optimization has kicked in and both variables are referencing the same String (as they are immutable by design this optimization is perfectly fine but not guaranteed).

@@ +239,5 @@
> +                if (!RestrictedProfiles.isUserRestricted()) {
> +                    // if we're not in kidfox, skip the kidfox specific bookmarks
> +                    final Pattern r_p = Pattern.compile("^bookmarkdefaults_title_restricted");
> +                    final Matcher r_m = r_p.matcher(name);
> +                    if (r_m.find()) {

As long as you are only interested in matching a prefix name.startsWith("...") can do that for you. :)

Comment 13

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

Attachment: favicon_webmaker.png

Here's the favicon 64x64. I got it from the Webmaker folks so it should be good from a branding point of view.

Comment 14

[Allison Naaktgeboren :ally]

awesome. That's good to hear that's there's been some standarization. The webmaker branding stuff from them hanging on the walls of this office are a zillion colors and multiple icons in multiple weights.

Comment 15

[Allison Naaktgeboren :ally]

Attachment: kidfox-bookmarks

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8646580 [details] [diff] [review]
kidfox-bookmarks

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

r+ with the changes below. :)

::: mobile/android/base/db/LocalBrowserDB.java
@@ +223,5 @@
>              if (!m.find()) {
>                  continue;
>              }
>              try {
> +                if (RestrictedProfiles.isUserRestricted()) {

This probably should be RestrictedProfiles.isRestrictedProfile() to only show these new bookmarks for restricted profiles and not when using a guest profile? This class and its methods are super confusing and I filed a bug to find a better naming.

@@ +232,5 @@
> +                    if (name.equals(addons) || name.equals(marketplace) || name.equals(regularSumo)) {
> +                        continue;
> +                    }
> +                }
> +                if (!RestrictedProfiles.isUserRestricted()) {

RestrictedProfiles.isRestrictedProfile()

Comment 17

[:Margaret Leibovic]

Comment on attachment 8646580 [details] [diff] [review]
kidfox-bookmarks

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

The strings bit looks fine to me.

::: mobile/android/base/strings.xml.in
@@ +463,5 @@
>    <!-- Icon is automatically generated from R.drawable.bookmarkdefaults_favicon_marketplace -->
>    <string name="bookmarkdefaults_title_marketplace">@bookmarks_marketplace@</string>
>    <string name="bookmarkdefaults_url_marketplace">https://marketplace.firefox.com/</string>
> +  <!-- anaaktge todo doublcheck urls and add strings to proper localization file-->
> +  <!-- anaaktge todo make icons show up -->

Did you address these TODOs? You should remove these comments before landing.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/965133e168d9

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/6bb8ca8a857c for android build failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=4259937&repo=fx-team

Comment 20

[Francesco Lodolo [:flod]]

Oh, new bookmarks, I foresee busted builds in Aurora :-\ 

You're going to love me for this comment, but since we're touching this file, and this was backed out, can we at least add a note explaining that straight quotes (single or double) must be escaped? In the past we also suggested to move these strings into android_strings.dtd too (bug 964946 comment 1).

This current localization note doesn't make any sense
http://hg.mozilla.org/mozilla-central/file/2ddfc9180971/mobile/locales/en-US/profile/bookmarks.inc#l6

en-US should be removed from this, even if it's just a comment
http://hg.mozilla.org/mozilla-central/file/2ddfc9180971/mobile/locales/en-US/profile/bookmarks.inc#l22

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8e42d6f9b45c

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e42d6f9b45c

Comment 23

[Allison Naaktgeboren :ally]

Comment on attachment 8646580 [details] [diff] [review]
kidfox-bookmarks

Approval Request Comment
[Feature/regressing bug #]: Kidfox, 
[User impact if declined]: kids get no bookmarks
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: minimal
[String/UUID change made/needed]: strings added

Comment 24

[:Margaret Leibovic]

Is this actually a requirement for 42? I was under the impression that it would be okay to let this slip to 43 to avoid the string churn for localizers.

Comment 25

[Allison Naaktgeboren :ally]

It was on the v1 list. Afaik that means it is wanted for 42

Comment 26

[Sam Fang [:sfang]]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #25)
> It was on the v1 list. Afaik that means it is wanted for 42

Originally we wanted the bookmarks to show the same default sites like the tiles. However, like Margaret said, for 42 we are okay for just having no default bookmarks shown. :)

Comment 27

[Francesco Lodolo [:flod]]

I'd really like this to stay in m-c and ride the trains. Besides having agreed on being string frozen, see also comment 20, I'm confident this is going to bust builds, not just one time. And the sad part is that we're going to notice it only *after* the multilocale build is busted.

Comment 28

[Axel Hecht [:Pike]]

Ugh, I'd actually prefer to r- this patch.

bookmarks.inc was used to share bookmarks between xul and native fennec, IIRC, and bookmarks themselves aren't in the json anymore, so I think we should just add the bookmarks titles to the android_strings.dtd as we do anywhere else. URLs being hard-coded is still the right thing to do.

Comment 29

[:Margaret Leibovic]

Given flod and Pike's comments, I think we should consider backing out the patch that landed here, and working on fixing the way we implement default bookmarks to avoid these l10n problems.

Ally, did you already file another bug about changing this default bookmark implementation? I remember us talking about how this could use some engineering investment.

Comment 30

[Allison Naaktgeboren :ally]

(In reply to Francesco Lodolo [:flod] from comment #27)
> I'd really like this to stay in m-c and ride the trains. 

So it sounds like flod would prefer it to ride just not be uplifted. 

I/We was talking about suggested sites/tiles, not bookmarks. None of that work would affect bookmarks.  

It sounds like Axel is asking to retire bookmarks.inc from fennec in the general case for l10n reasons

Comment 31

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #29)

> Ally, did you already file another bug about changing this default bookmark
> implementation? I remember us talking about how this could use some
> engineering investment.

Looks like you cleared the NI without answering this question - is there a bug? We can use a separate bug to discuss implementation details with Pike and flod.

I also still think we should back out the patch here to avoid busting builds, as flod mentioned in comment 20.

Comment 32

[Sam Fang [:sfang]]

Just to confirm, for the Kinderfox demo we did, the default bookmarks were included (same sites as the default Top Sites). So for the current Nightly, it is working as intended.

Comment 33

[Allison Naaktgeboren :ally]

(In reply to :Margaret Leibovic from comment #31)
> (In reply to :Margaret Leibovic from comment #29)
> 
> > Ally, did you already file another bug about changing this default bookmark
> > implementation? I remember us talking about how this could use some
> > engineering investment.
> 
> Looks like you cleared the NI without answering this question - is there a
> bug? We can use a separate bug to discuss implementation details with Pike
> and flod.
> 
> I also still think we should back out the patch here to avoid busting
> builds, as flod mentioned in comment 20.

No, there is no current bug here. See Sam's comment 32. There is fragile and not easily extensible code which is my concern for the future. Flod also raises concerns about bookmarks.inc not being great for the localizers either. So I think it would be wise to refactor and update this code soon. 

Comment 20 mentions Aurora explicitly. Comment 27 talks about string freeze, which is not a concern on Nightly. My read was that the _uplift_ would bust the multi-locale build.

Flod, are the bookmarks busted on Nightly?

Comment 34

[Francesco Lodolo [:flod]]

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #33)
> Flod, are the bookmarks busted on Nightly?

No, but we only have 3 locales so far (fr, it, sk), and teams working on l10n-central are kind of safe. 

As I said, tools won't provide any feedback because this issue is a fall-out from the switch to native that nobody ever considered. If I send an email to these teams, or file bugs, they'll react decently fast.

When we move to mozilla-aurora it's a whole different world, with an audience of less technical localizers working on external tools. Which means an error is introduced, multi-locale build is busted, sheriffs ping me, and I can't fix the issue on Mercurial because the external tool will simply revert it within an hour. It might take days between the build breaking and the fix (I see you found bug 1036465 already for example), which is OK for a single locale build, but clearly not for the multilocale build.

Comment 35

[u549602]

On latest nightly, 3 tiles are displayed on Top Sites(Mozilla Support, Mozilla Webmaker and Mozilla Project) and in the bookmarks section 3 items(About:home, Mozilla Support and Mozilla Webmaker).
Since the default Top sites are listed as Bookmarks (except Mozilla Project replaced by About:home bookmark) is this the intended behavior?

Comment 36

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to mihai.ninu from comment #35)
> On latest nightly, 3 tiles are displayed on Top Sites(Mozilla Support,
> Mozilla Webmaker and Mozilla Project) and in the bookmarks section 3
> items(About:home, Mozilla Support and Mozilla Webmaker).
> Since the default Top sites are listed as Bookmarks (except Mozilla Project
> replaced by About:home bookmark) is this the intended behavior?

I think yes, that's what we agreed on but I'm NI-ing barbara.

Comment 37

[Barbara Bermes [:barbara] - NI please!]

Looks fine for me but I'd like to understand the concern if/why this was raised?

Comment 38

[u549602]

As mentioned in Comment 35, Mozilla Project link that is present in the Top Sites menu is replaced by the About:home bookmark at a first log in. 
From my understanding, the Bookmarked links should be the same as the ones present in the Top Sites page.