Include bookmarks for Kinderfox

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sfang, Assigned: ally)

Tracking

42 Branch
Firefox 43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 affected, firefox43 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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
I think you're talking about suggested site tiles (the big ones on Top Sites), not bookmarks. Please re-correct if necessary!
Summary: Show different bookmarks → Include different suggested sites for Kidfox profiles
See Also: → bug 1125280
(Reporter)

Comment 2

3 years ago
(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.
(Reporter)

Updated

3 years ago
Summary: Include different suggested sites for Kidfox profiles → Include different bookmarks for Kinderfox
See Also: bug 1125280
@ally: Is the list of bookmarks filled from the same mechanism like top sites?
Flags: needinfo?(ally)

Comment 4

3 years ago
(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?
Assignee: nobody → ally

Comment 5

3 years ago
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.
(Assignee)

Comment 6

3 years ago
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.
Flags: needinfo?(ally)

Comment 7

3 years ago
(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.
(Assignee)

Comment 8

3 years ago
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.
(Assignee)

Updated

3 years ago
Summary: Include different bookmarks for Kinderfox → Include mozilla & support bookmarks for Kinderfox
(Assignee)

Comment 9

3 years ago
Created attachment 8645978 [details] [diff] [review]
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.
(Assignee)

Comment 10

3 years ago
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?
Flags: needinfo?(alam)
(Assignee)

Comment 11

3 years ago
Created attachment 8646061 [details] [diff] [review]
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
Attachment #8646061 - Flags: feedback?(s.kaspari)
Attachment #8646061 - Flags: feedback?(margaret.leibovic)
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. :)
Attachment #8646061 - Flags: feedback?(s.kaspari) → feedback+
(Assignee)

Updated

3 years ago
Summary: Include mozilla & support bookmarks for Kinderfox → Include bookmarks for Kinderfox
Created attachment 8646493 [details]
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.
Flags: needinfo?(alam) → needinfo?(ally)
(Assignee)

Comment 14

3 years ago
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.
Flags: needinfo?(ally)
(Assignee)

Comment 15

3 years ago
Created attachment 8646580 [details] [diff] [review]
kidfox-bookmarks
Attachment #8645978 - Attachment is obsolete: true
Attachment #8646061 - Attachment is obsolete: true
Attachment #8646061 - Flags: feedback?(margaret.leibovic)
Attachment #8646580 - Flags: review?(s.kaspari)
Attachment #8646580 - Flags: feedback?(margaret.leibovic)
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()
Attachment #8646580 - Flags: review?(s.kaspari) → review+

Comment 17

3 years ago
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.
Attachment #8646580 - Flags: feedback?(margaret.leibovic) → feedback+
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
https://hg.mozilla.org/mozilla-central/rev/8e42d6f9b45c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
(Assignee)

Updated

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

Comment 23

3 years ago
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
Attachment #8646580 - Flags: approval-mozilla-aurora?

Comment 24

3 years ago
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.
Flags: needinfo?(bbermes)
Flags: needinfo?(ally)
(Assignee)

Comment 25

3 years ago
It was on the v1 list. Afaik that means it is wanted for 42
Flags: needinfo?(ally)
(Reporter)

Comment 26

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

3 years ago
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

3 years ago
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.
Flags: needinfo?(ally)
(Assignee)

Comment 30

3 years ago
(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
Flags: needinfo?(ally)
(Assignee)

Updated

3 years ago
Attachment #8646580 - Flags: approval-mozilla-aurora?

Comment 31

3 years ago
(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.
Flags: needinfo?(ally)
(Reporter)

Comment 32

3 years ago
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.
Flags: needinfo?(bbermes)
(Assignee)

Comment 33

3 years ago
(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?
Flags: needinfo?(ally) → needinfo?(francesco.lodolo)
(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.
Flags: needinfo?(francesco.lodolo)

Comment 35

3 years ago
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?
(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.
Flags: needinfo?(bbermes)
Looks fine for me but I'd like to understand the concern if/why this was raised?
Flags: needinfo?(bbermes)

Comment 38

3 years ago
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.
You need to log in before you can comment on or make changes to this bug.