1031524 - Search Engine based on MNC/MCC in browser.json is not included in the Settings -> Homescreen -> Search Engine

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Search, defect)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: browser.json

1. modify browser.json with a different search provider for the mcc/mnc for your SIM card
2. modify the providers.json to have a different set of search providers
3. compile gaia onto the device with : GAIA_DISTRIBUTION_DIR=./customization make production
4. go to  Settings -> Homescreen -> Search Engine

Expected: the search engine drop down is also included in the mnc/mcc search engine
Actual: it does not

Note:
1. browser.json's bookmarks do show
2. the search engines in Provider.json do show.

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

according to bug 1009353 there should be an allowance to change the search provider based on MNC/MCC

Comment 2

[Kevin Grandon :kgrandon]

We don't use browser.json for the vertical homescreen, but instead we use provider.json only. It looks like MNC/MCC support is not quite finished for this feature - oops.

Doing a needinfo? on Ben and Carmen - Could one of you guys take this?

Comment 3

[Carmen Jimenez Cabezas]

I'm sorry, I don't really have bandwidth to take this now. I'll pass it to Albert

Comment 4

[:gerard-majax]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #0)
> Created attachment 8447404 [details]
> browser.json
> 
> 1. modify browser.json with a different search provider for the mcc/mnc for
> your SIM card
> 2. modify the providers.json to have a different set of search providers
> 3. compile gaia onto the device with : GAIA_DISTRIBUTION_DIR=./customization
> make production
> 4. go to  Settings -> Homescreen -> Search Engine
> 
> Expected: the search engine drop down is also included in the mnc/mcc search
> engine
> Actual: it does not
> 
> Note:
> 1. browser.json's bookmarks do show
> 2. the search engines in Provider.json do show.

Could you be seeing bug 1031164 ?

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug1031524.patch

Ben, I changed a bit the file format. Not sure it makes sense to use browser.json for the search engine list now that it's part of the system.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Taking as an assignee even if I don't mind if someone steal it if this patch is not correct.

Comment 7

[Kevin Grandon :kgrandon]

Comment on attachment 8448695 [details] [diff] [review]
bug1031524.patch

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

Stealing review because Ben is on PTO, and I was asked :) I am not too familiar with this type of customization, but the code looks good to me. Thanks!

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Thanks.

https://github.com/mozilla-b2g/gaia/commit/a4f2792725c81a78898d82663a185a57e4a34013

Comment 9

[Kevin Grandon :kgrandon]

2.0: https://github.com/mozilla-b2g/gaia/commit/de7448291f57121314dcfbad0a4fa966b2308bd4

Comment 10

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

ok, I think if I understand this the browser.json remains untouched and that's where the custom search engine for browser is updated until bug 1026031 lands for 2.1

Meanwhile, the MNC/MCC is customizable within the providers.json for Rocketbar.

I think there might need to be an update to the Mozilla Docs, so I am ni?cmills.

Comment 11

[Chris Mills [:cmills]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #10)
> ok, I think if I understand this the browser.json remains untouched and
> that's where the custom search engine for browser is updated until bug
> 1026031 lands for 2.1
> 
> Meanwhile, the MNC/MCC is customizable within the providers.json for
> Rocketbar.
> 
> I think there might need to be an update to the Mozilla Docs, so I am
> ni?cmills.

Ok, I'm not completely sure I understood this correctly, I added in this section:

https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Market_customizations_guide#Custom_search_providers_for_Rocketbar

Plus a note at the top of 

https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Market_customizations_guide#Browser_bookmarks_and_default_search_engine

Can you check these and let me know if they are ok?

Comment 12

[Ben Francis [:benfrancis]]

I'm not sure I understand what is going on in this bug. Wasn't this already implemented in bug 1020167 ?

As of bug 1009353 the default Rocketbar search provider (on enter press) can be configured in settings.json
https://github.com/mozilla-b2g/gaia/blob/master/customization/settings.json

and the list of providers  is specified in providers.json
https://github.com/mozilla-b2g/gaia/blob/master/customization/search/providers.json

But as of bug 1020167 the default Rocketbar search provider can also be customized per MCC/MNC on first SIM card entry, for example
https://github.com/mozilla-b2g/gaia/blob/master/customization/mobizilla/mobizilla_default_search.json

and the list of providers per MCC/MNC is specified here for example https://github.com/mozilla-b2g/gaia/blob/master/customization/mobizilla/mobizilla_search.json

Did that not work? Aren't we now configuring this in two places and re-implementing logic from the single variant app?

I'll review the documentation once this all gets sorted out because there seems to be a lot of confusion here.

Also, won't this change the list of available options if you change your SIM card after first SIM card entry? That would be incorrect.

Comment 13

[Albert [:albert]]

As Ben said, there are two ways to configure search engines.

- Normal configuration: search engines that will be added always. These are defined in providers.json and the default one is configured in settings.json

- Single variant configuration: search engines that will be added only for a given SIM mcc/mnc the first time it is inserted. These are defined in a json file having the same format that providers.json. So we need one file per mcc/mnc, then varian.json defines which search file (json) is used for each mcc/mnc pair. variant.json is placed in distribution_dir, for example: https://github.com/mozilla-b2g/gaia/blob/master/customization/variant.json

In order to apply the single variant configuration you have to set the GAIA_DISTRIBUTION_DIR variable with the path of your variant folder when doing building gaia.

Comment 14

[Kevin Grandon :kgrandon]

Ok, so it seems that we should already support this, and that we just did not properly apply the customization? Sorry about the confusion, I should've waited for you guys to be available for review :)

It sounds like we should revert this patch - does anyone have objections to doing so?

Comment 15

[Chris Mills [:cmills]]

(In reply to Ben Francis [:benfrancis] from comment #12)

> 
> I'll review the documentation once this all gets sorted out because there
> seems to be a lot of confusion here.

Apologies for the confusion, and thanks Ben/Albert for the information. I've made some more changes; can you now review?

1. Default Rocketbar search provider: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Market_customizations_guide#Default_Rocketbar_search_provider
2. Default providers: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Market_customizations_guide#Search_provider_customization
3. Single variant configuration: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Market_customizations_guide#Single_variant_customization_to_override_list_of_providers_and_Rocketbar_search_provider

Comment 16

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Based on what Ben is stating, this bug is invalid in that the Provider.json shouldn't control the MNC/MCC, the variant.json should be controlling it.

I think I'm doing something wrong; because I can't seem to get the variant.json to add MNC/MCC search engines.  I reverted the patch and then changed the mobizilla_search.json to add two more search engines based on the comments.  Ben, I'll try to ping you on irc.

Comment 17

[Ben Francis [:benfrancis]]

Albert, are you able to help Naoki in testing the feature you implemented?

Kevin, I think we're OK to back this out.

Comment 18

[Albert [:albert]]

Sure, once backed out I can take it.

Comment 19

[Kevin Grandon :kgrandon]

Ok, based on Ben's and Albert's comments I have backed this out on master: https://github.com/mozilla-b2g/gaia/commit/7a69046ea6f85062885db2be8a2dde1e9deef135

Once we have verification from QA that this is working on master, we can uplift the backout. Naoki - can we get this verified in the next 1-2 days on latest master before uplift (not 2.0)?

Comment 20

[Kevin Grandon :kgrandon]

Sorry, going to leave this as fixed for now, as it should technically work - we just need to verify.

Comment 21

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Yes, I sent Albert an email; I'll try to work it out with him.  It's on my list.

Comment 22

[Albert [:albert]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #21)
> Yes, I sent Albert an email; I'll try to work it out with him.  It's on my
> list.

I tried your variant json config file and worked fine. I'll try to talk with you in IRC to help you.

Comment 23

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Working with Albert, he had used the hamachi where as I used the flame.  When I used the hamachi, I do see the single variant working on master.

When I use the flame, I did not see it work.  I saw the providers.json instead.  I saw bug 1009443 and I turned on the ril debug before the launch of the phone.  I then got the single variant to work.  It appears that there's a bug on the flame device of not showing the MCC/MNC until the ril debug is turned on.

Albert mentioned that he will try checking on the Flame and comment here tomorrow.  I think we may need to reopen bug 1009443 as we need the MCC/MNC to work without having to turn on the RIL debug.

Comment 24

[Kevin Grandon :kgrandon]

Backed out of 2.0 for details in comment 23: https://github.com/mozilla-b2g/gaia/commit/29b78711df232870bfb57f6b335f94063b83c36b

Changing to invalid to reflect the proper state of the bug.

Comment 25

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Verifying work following the Single Variant will be tracked in bug 1020167 per comment 12.

Comment 26

[Albert [:albert]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #23)
> Working with Albert, he had used the hamachi where as I used the flame. 
> When I used the hamachi, I do see the single variant working on master.
> 
> When I use the flame, I did not see it work.  I saw the providers.json
> instead.  I saw bug 1009443 and I turned on the ril debug before the launch
> of the phone.  I then got the single variant to work.  It appears that
> there's a bug on the flame device of not showing the MCC/MNC until the ril
> debug is turned on.
> 
> Albert mentioned that he will try checking on the Flame and comment here
> tomorrow.  I think we may need to reopen bug 1009443 as we need the MCC/MNC
> to work without having to turn on the RIL debug.

After some testing I see that the root problem is a race condition when sending the broadcast system message from OperatorApps to OperatorVariant. We saw that issue one month ago, I though it was fixed but is still open.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1031164