1024527 - Integrate Android Search Activity strings into Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect, P1)

Description

[Nick Alexander :nalexander [he/him]]

Over in Bug 1021864, we're ironing out how to integrate the Search Activity into the Fennec APK.  For context, the Search Activity will be integrated in a similar manner to Android Sync: to date it has been developed as a separate (github) repository; it is essentially independent of Fennec proper; and we'd like to maintain some of that relationship for easier development and testing.

Comment 1

[Nick Alexander :nalexander [he/him]]

What I propose is that Search Activity's strings.xml be pre-processed (like Android Sync's).  strings.xml.in will contain search_strings.dtd(.in), just like Android Sync.  That dtd *should* live in

mobile/android/search/locales/en-US/search_strings.dtd

or similar.

*First ask*: is that okay for localizers?  Can we add new dtd roots into the tree?  (I really hope so.)

Once we have that dtd in place, I propose to do all the localization as part of mobile/android/base/locales.  That is, the Search Activity is only ever localized as part of building Fennec proper.  The alternative is to copy the mobile/android/base/locales logic to mobile/android/search/locales.  I really don't want to do that because 1) the paths are special-cased throughout the builders; 2) having the locales be handled separately makes the build system Makefiles much harder to maintain.

*Second ask*: is localizing "Fennec extras" like the Search Activity only as part of the Fennec APK acceptable?

Asking Pike for l10n perspective, glandium for build-system and l10n packaging perspective.

Comment 2

[Nick Alexander :nalexander [he/him]]

rnewman: I'm going to assert that the Search Activity ignores the Fennec locale chooser for a good long time, possibly indefinitely.  That is: the Search Activity runs in your system locale, regardless of your Fennec choice.  Claiming otherwise is equivalent to signing up to do the LocaleManager extraction-into-library work :)

Comment 3

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #2)
> Claiming otherwise is equivalent to signing up to do the
> LocaleManager extraction-into-library work :)

If it's in the same APK, can't we take the same approach that we do for Sync's activities?

That is, dump a stub {Browser,}LocaleManager into the search activity repo, and call BrowserLocaleManager.getInstance() during activity creation (or later if it doesn't immediately show strings).

The right thing will happen when the non-stub class is available.

See Bug 970176 for reference.

Am I missing something?

Comment 4

[Axel Hecht [:Pike]]

Sadly, the file locations we can can use for Android are special, because of the clusterfucked \ escaping story in xml that we need for android, and must not have anywhere else.

That logic is only triggered for android/base/locales/en-US and embedding/android/locales/en-US.

It'd be an easy patch to change, but a whole lot of shebang to actually deploy it everywhere.

If we care about the search activity being part of the fennec l10n process really depends on what the intent is, and how big of a project it is/will become.

Comment 5

[Mike Hommey [:glandium]]

I'm not sure what the build system question is.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Axel Hecht [:Pike] from comment #4)
> Sadly, the file locations we can can use for Android are special, because of
> the clusterfucked \ escaping story in xml that we need for android, and must
> not have anywhere else.
> 
> That logic is only triggered for android/base/locales/en-US and
> embedding/android/locales/en-US.

Ah, I see this in compare-locales: special treatment for a few directories in compare-locales/lib/Mozilla/Checks.py.

I should point out that we're including mobile/android/branding/*dtd in strings.xml, presumably without these checks, but also without issue.  I reckon that's 'cuz the branding string set is small and essentially static.

> It'd be an easy patch to change, but a whole lot of shebang to actually
> deploy it everywhere.

Roger that.
 
> If we care about the search activity being part of the fennec l10n process
> really depends on what the intent is, and how big of a project it is/will
> become.

I anticipate the Search Activity being a big deal, but also that additional Projects that aren't in mobile/android/base will require Android strings (the most likely next project being the Mozilla Stumbler).

My preference is to land a new DTD in mobile/android/search/locales/en-US/search_strings.dtd and forego the special Android checking, and retro-fit our tools as necessary; but I could be convinced otherwise.

Comment 7

[:Margaret Leibovic]

What's the status of this bug?

I do see there is a search_strings.dtd in the tree. Does this bug mean that localizers can't actually access and localize this file?

Marking this as a P1 to make sure that the search activity can be localized.

Comment 8

[Axel Hecht [:Pike]]

Yes, that file is not exposed to localizers.

Also, there's no intent to spread more directories with android badness on my side. It's just way to cumbersome to do that in any reasonable timeframe.

Comment 9

[Richard Newman [:rnewman]]

(See Bug 709877 for historical context for sync_strings.dtd.)

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Axel Hecht [:Pike] from comment #8)
> Yes, that file is not exposed to localizers.
> 
> Also, there's no intent to spread more directories with android badness on
> my side. It's just way to cumbersome to do that in any reasonable timeframe.

Pike, let me make sure I'm understanding this correctly: localizers don't currently see:

* mobile/android/search/locales/en-US/search_strings.dtd

And you don't want to expose that location to localizers.  From my perspective, this is unfortunate; we want to grow *more* locations for localizers as we modularize pieces of Fennec, not *fewer*.  But we can work around, for now, perhaps: will localizers see (without additional changes):

* mobile/android/base/locales/en-US/search_strings.dtd

If so, we'll move search_strings.dtd there, and update our import scripts.

Comment 11

[Axel Hecht [:Pike]]

According to my notes, the file is at http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/strings/search_strings.dtd.

If it moved to mobile/android/search/locales/en-US/search_strings.dtd, it'd be possible to hook that up to l10n.ini, but that wouldn't trigger the awful android logic on there.

mobile/android/base/locales/en-US/search_strings.dtd will be fine.

I strongly believe that we do not want to spread localization files through various totally disconnected sections in the source code. The product we're localizing is fennec, just fennec. There are a few strings, just keep them together.

Comment 12

[:Margaret Leibovic]

nalexander, can you own this bug?

Comment 13

[Nick Alexander :nalexander [he/him]]

Yes.  Leaving the NI so it's in my queue.

Comment 14

[Nick Alexander :nalexander [he/him]]

Attachment: Link to Github pull-request: https://github.com/mozilla/fennec-search/pull/86

Comment 15

[Nick Alexander :nalexander [he/him]]

Attachment: Move search_strings.dtd into m/a/base/locales/en-US. r=Pike

Pike: can you confirm that no further changes are needed in l10n.ini
for localizers to see the new search_strings.dtd?  Thanks.

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 8486629 [details] [diff] [review]
Move search_strings.dtd into m/a/base/locales/en-US. r=Pike

Technically, this is OK.

But the search settings stuff should be migrated into region.properties, http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/strings/search_strings.dtd#23 is just a big basket of localizer fail.

Also, Karen, what's the policy around that feature?

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to Axel Hecht [:Pike] from comment #16)
> Comment on attachment 8486629 [details] [diff] [review]
> Move search_strings.dtd into m/a/base/locales/en-US. r=Pike
> 
> Technically, this is OK.

Thanks for the quick turn-around.
 
> But the search settings stuff should be migrated into region.properties,
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/strings/
> search_strings.dtd#23 is just a big basket of localizer fail.

Migrating these settings into region.properties is almost certainly out of scope for Fx 35 due to technical divisions between Fennec proper and the Search Activity.  I propose we land this or something very close to it so that we can localize in 35; and then figure out what a region.properties system looks like.

Can you elaborate on what you mean by "localizer fail" here?  Do you mean that localizers have no particular knowledge of searchplugins, so exposing something like this to localizers at all is a bad idea?

Comment 18

[Axel Hecht [:Pike]]

The thing is, you're not going to get this properly localized by some entry in a DTD.

They're going to do all kinds of things to the "yahoo" string, ignoring the comment you put on top. And even if we landed the right string once, they're still going to regress it merrily. Sad, but true.

region.properties on the other hand is not exposed to regular translation tools, but only changes when technically savvy people edit it. Which is why I asked kar for context on how and why to do what.

Can you bundle region.properties into the search activity in addition to what search_strings.dtd does? It's going to require different code to get the localized value, but it'll be the stable way forward, given our experience with these kinds of settings.

Comment 19

[Francesco Lodolo [:flod]]

Adding some more thoughts on the subject.

* region.properties
That's our 'safe' place for productization settings. Localizers are instructed to not touch those files, and changes are easier to spot when reviewing sign-offs (localizers sign-off a changeset for shipping, l10n-drivers review it for errors before accepting it).

Also, we have external localization tools that explicitly ignore that file, so localizers won't even see it.

Take a look at this page
http://l10n.mozilla-community.org/~flod/p12n/errors/

"Firefox Mobile" is now in a clean state (ga-IE has been fixed, we have a bug on file for zh-CN), as Firefox desktop. They weren't a year ago before I started cleaning up.

* Policy
As Pike said, there's also a "policy" question: are localized supposed to keep that value, or can they change it? That's probably a question for BD.

* default_engine_identifier
I'm not sure about the choice of using the filename as value. For a similar setting, we use the 'name' attribute of the searchplugin, not the filename.
http://hg.mozilla.org/mozilla-central/file/152ef25e89ae/mobile/locales/en-US/chrome/region.properties#l6

What happens if localizers screw up and specify a non existing searchplugin? Do we have a fallback mechanism?

Comment 20

[:Margaret Leibovic]

(In reply to Francesco Lodolo [:flod] from comment #19)
> Adding some more thoughts on the subject.
> 
> * region.properties
> That's our 'safe' place for productization settings. Localizers are
> instructed to not touch those files, and changes are easier to spot when
> reviewing sign-offs (localizers sign-off a changeset for shipping,
> l10n-drivers review it for errors before accepting it).
> 
> Also, we have external localization tools that explicitly ignore that file,
> so localizers won't even see it.
>
> Take a look at this page
> http://l10n.mozilla-community.org/~flod/p12n/errors/
> 
> "Firefox Mobile" is now in a clean state (ga-IE has been fixed, we have a
> bug on file for zh-CN), as Firefox desktop. They weren't a year ago before I
> started cleaning up.
> 
> * Policy
> As Pike said, there's also a "policy" question: are localized supposed to
> keep that value, or can they change it? That's probably a question for BD.
> 
> * default_engine_identifier
> I'm not sure about the choice of using the filename as value. For a similar
> setting, we use the 'name' attribute of the searchplugin, not the filename.
> http://hg.mozilla.org/mozilla-central/file/152ef25e89ae/mobile/locales/en-US/
> chrome/region.properties#l6

I decided to use the file name (the 'identifier' attribute for the plugin) because that seemed simpler than the 'name' attirbute in the plugin. Do you happen to know the history behind that decision for the regular search engines?

> What happens if localizers screw up and specify a non existing searchplugin?
> Do we have a fallback mechanism?

This is bug 1065123, which I am going to fix :)

nalexander, is there a way for us to get at region.properties in the search activity without needing to interact with gecko? It would be nice to be able to manage all our search engine defaults in one place.

Comment 21

[:Margaret Leibovic]

Comment on attachment 8486625 [details] [review]
Link to Github pull-request: https://github.com/mozilla/fennec-search/pull/86

This looks good to me, but we do need to sort out the default_engine_identifier issue. However, I think that should happen in a separate patch, either here or in a follow-up bug.

Comment 22

[Francesco Lodolo [:flod]]

If this lands, I would be much happier with an hard-coded "Yahoo" somewhere out of string files while we figure out the story behind it (especially the policy part).

Comment 23

[Francesco Lodolo [:flod]]

(In reply to :Margaret Leibovic from comment #20)
> I decided to use the file name (the 'identifier' attribute for the plugin)
> because that seemed simpler than the 'name' attirbute in the plugin. Do you
> happen to know the history behind that decision for the regular search
> engines?

It's too far away in the past for me, maybe gavin or mconnor (or pike) could have some ideas.

Comment 24

[:Margaret Leibovic]

Update: In bug 1065123 I'm going to write a patch to remove this string from search_strings.dtd, so that we won't be blocked on that problem anymore.

I filed bug 1065306 to come up with a real solution for this localized-default engine issue.

Comment 25

[Karen Rudnitski [:kar]]

Regarding policy, I presume this refers to things like the list itself (number of providers, order, etc). My expectation is that it mirrors that of our current search provider options. I would want the default that someone sets in the settings menu to also reflect that in the search activity page. Same goes for the list - if DE has a set of providers today, that same list should be reflected in the search activity. This also goes for any 'rules' for default that we have (ie we decide to change Bing across a set of locales, this also must be reflected in the search activity page).

Is that what we're referring to here? And does the above make sense?

Comment 26

[Francesco Lodolo [:flod]]

Policy = default search engine, does it need to be Yahoo (it's Google for the standard search)? 
Can it be changed by locales? For example I'd need to check, but we don't have Yahoo in all locales we ship on Fennec.

Comment 27

[Karen Rudnitski [:kar]]

I'm going to suggest that we mimic the list of the locale's current list of search providers. We can look at possibly modifying the list, but I would think that would really confuse our users (two separate provider lists).

That does mean if a user changes their default engine to, say, DDG, then that would also be reflected in the activity. I say this again because I worry users wouldn't understand the difference - for them, a search is a search is a search under the Firefox umbrella.

Right now, Google is the default provider globally. When we look at moving Bing into that position, we have been discussing making that change on a subset of locales (at first). So we need that flexibility. I would also maintain that after any stated defaults and order that may be required, then the locales can choose the engines that best serve their community as per current guidelines. Again, however, to mimic the current list of providers reflected when typing in the URL bar.

Comment 28

[Axel Hecht [:Pike]]

So, reading Kar's statements, we actually don't want and need a different configuration for search activity settings? Also, that the current setting in en-US as yahoo is a bug?

Comment 29

[Karen Rudnitski [:kar]]

the original intent of this project was to investigate and be bolder in terms of how we launch searches, how we present queries and how we present results. More freedom was required in designing this as to see what could work well. Also Yahoo provided more creative freedom than Google as well around how results were presented.

the 'first launch' of this project, however, is more conservative in nature. Therefore I assert that we need to preserve the flexibility to have different settings, but upon initial implementation should mimic the current provider lists for each locale.

we're phasing these bits and I expect us to be more creative in nightly while more conservative in our beta & release channels to ensure we get the 'product' right (avoid user confusion, ensure provider buy-in, get the right UX on all fronts, etc)

Comment 30

[:Margaret Leibovic]

(In reply to Karen Rudnitski [:kar] from comment #27)
> I'm going to suggest that we mimic the list of the locale's current list of
> search providers. We can look at possibly modifying the list, but I would
> think that would really confuse our users (two separate provider lists).

The way it's currently implemented, the list is the same, since we're pulling the search plugins out of the same directory.

> That does mean if a user changes their default engine to, say, DDG, then
> that would also be reflected in the activity. I say this again because I
> worry users wouldn't understand the difference - for them, a search is a
> search is a search under the Firefox umbrella.

This is different right now because search settings are managed by gecko, and the search activity doesn't depend on gecko at all (and we don't want it to, for performance reasons).

(In reply to Karen Rudnitski [:kar] from comment #29)
> the original intent of this project was to investigate and be bolder in
> terms of how we launch searches, how we present queries and how we present
> results. More freedom was required in designing this as to see what could
> work well. Also Yahoo provided more creative freedom than Google as well
> around how results were presented.
> 
> the 'first launch' of this project, however, is more conservative in nature.
> Therefore I assert that we need to preserve the flexibility to have
> different settings, but upon initial implementation should mimic the current
> provider lists for each locale.

To be clear, we don't have anything in place to support a different list of search engines for the search activity, but we *do* have a separate default engine pref for it.

If it's not a blocker to shipping, I would really prefer to keep the search activity search engine pref separate from the Fennec search engine pref (if the user changes them), since making them the same would require a non-trivial amount of work.

Comment 31

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

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

> To be clear, we don't have anything in place to support a different list of
> search engines for the search activity, but we *do* have a separate default
> engine pref for it.
> 
> If it's not a blocker to shipping, I would really prefer to keep the search
> activity search engine pref separate from the Fennec search engine pref (if
> the user changes them), since making them the same would require a
> non-trivial amount of work.

I think the separate default engine pref makes sense since Search and Fennec are two different use cases. I want to see how it plays out: Do people tend to want to keep Search and Fennec defaults the same or not?

Comment 32

[Karen Rudnitski [:kar]]

It makes sense provided a user understands these are separated, which I'm not sure they would (I certainly wouldn't). I'm also thinking that our providers would feel that this devalues default and may request a certain behaviour if we're included in the agreements [just keeping this in the back of our minds]. 

However, I would suggest the following:
- ensure the default set on shipping is the same default as the current list
- upon first release, we can see how many people change the default(s) and use the search activity vs in-browser search (and whether they notice a difference in default). 
- ensure we *can* link the two for possibly commercial reasons. We'll know whether this is required or not in the coming month or two, I reckon. 

TBH, I'm still concerned about user expectations. My hypothesis is that users have no idea that these are separate things, and I'm not sure in this case that they should be treated as different thing. But I'm willing to let it test out a bit.

Comment 33

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

(In reply to Karen Rudnitski [:kar] from comment #32)
 
> TBH, I'm still concerned about user expectations. My hypothesis is that
> users have no idea that these are separate things, and I'm not sure in this
> case that they should be treated as different thing. But I'm willing to let
> it test out a bit.

I take the opposite stance. They are two different applications. Why would people assume the defaults are the same? I wouldn't.

Comment 34

[Karen Rudnitski [:kar]]

I would :)

So let's check out the hypotheses and how it pans out.

Comment 35

[Nick Alexander :nalexander [he/him]]

(In reply to Francesco Lodolo [:flod] from comment #22)
> If this lands, I would be much happier with an hard-coded "Yahoo" somewhere
> out of string files while we figure out the story behind it (especially the
> policy part).

OK, this has landed: Bug 1065123.  So the immediate l10n blocker is out of the way.

I am going to interpret:

1) this change, which removes Pike's (and flod's) immediate concern;
2) follow-up Bug 1065123, which will address Pike's longer concern; and
3) Pike's "technically OK" in https://bugzilla.mozilla.org/show_bug.cgi?id=1024527#c16

as r=margaret,Pike.  Let's get these strings out to localizers (well, the few who localize Nightly) and move on to Bug 1065123.

Comment 36

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/10c93ff9783c
https://github.com/mozilla/fennec-search/commit/5298df7e91286c866eef7c466c92b889e4d12139

Comment 37

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #35)
> (In reply to Francesco Lodolo [:flod] from comment #22)
> > If this lands, I would be much happier with an hard-coded "Yahoo" somewhere
> > out of string files while we figure out the story behind it (especially the
> > policy part).
> 
> OK, this has landed: Bug 1065123.  So the immediate l10n blocker is out of
> the way.
> 
> I am going to interpret:
> 
> 1) this change, which removes Pike's (and flod's) immediate concern;
> 2) follow-up Bug 1065123, which will address Pike's longer concern; and

That should be Bug 1065306.

> 3) Pike's "technically OK" in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1024527#c16
> 
> as r=margaret,Pike.  Let's get these strings out to localizers (well, the
> few who localize Nightly) and move on to Bug 1065123.

Also, that should be Bug 1065306.

Comment 38

[:Margaret Leibovic]

FYI, I filed bug 1065891 about keeping the default engine prefs in sync.

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10c93ff9783c