Closed Bug 1031524 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0+, b2g-v2.0 unaffected, b2g-v2.1 unaffected)

RESOLVED FIXED
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- unaffected
b2g-v2.1 --- unaffected

People

(Reporter: nhirata, Assigned: vingtetun)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files)

Attached file 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.
blocking-b2g: --- → 2.0?
Summary: Search provider based on MNC/MCC in browser.json is not included in the Settings -> Homescreen -> Search Engine → Search Engine based on MNC/MCC in browser.json is not included in the Settings -> Homescreen -> Search Engine
according to bug 1009353 there should be an allowance to change the search provider based on MNC/MCC
Blocks: 1015336
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?]
Whiteboard: [systemsfe]
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?
Flags: needinfo?(cjc)
Flags: needinfo?(bfrancis)
I'm sorry, I don't really have bandwidth to take this now. I'll pass it to Albert
Flags: needinfo?(cjc) → needinfo?(alberto.crespellperez)
blocking-b2g: 2.0? → 2.0+
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?] → [VH-FL-blocking-][VH-FC-blocking+]
(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 ?
Flags: needinfo?(nhirata.bugzilla)
Attached patch bug1031524.patchSplinter Review
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.
Attachment #8448695 - Flags: review?(bfrancis)
Taking as an assignee even if I don't mind if someone steal it if this patch is not correct.
Assignee: nobody → 21
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!
Attachment #8448695 - Flags: review?(bfrancis) → review+
Flags: needinfo?(nhirata.bugzilla)
2.0: https://github.com/mozilla-b2g/gaia/commit/de7448291f57121314dcfbad0a4fa966b2308bd4
Flags: needinfo?(bfrancis)
Flags: needinfo?(alberto.crespellperez)
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.
Flags: needinfo?(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?
Flags: needinfo?(cmills)
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.
Flags: needinfo?(alberto.crespellperez)
Flags: needinfo?(21)
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.
Flags: needinfo?(alberto.crespellperez)
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?
(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
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.
Flags: needinfo?(bfrancis)
Albert, are you able to help Naoki in testing the feature you implemented?

Kevin, I think we're OK to back this out.
Flags: needinfo?(kgrandon)
Flags: needinfo?(bfrancis)
Flags: needinfo?(alberto.crespellperez)
Sure, once backed out I can take it.
Flags: needinfo?(alberto.crespellperez)
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)?
Status: RESOLVED → REOPENED
Flags: needinfo?(nhirata.bugzilla)
Flags: needinfo?(kgrandon)
Flags: needinfo?(21)
Resolution: FIXED → ---
Sorry, going to leave this as fixed for now, as it should technically work - we just need to verify.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Yes, I sent Albert an email; I'll try to work it out with him.  It's on my list.
Flags: needinfo?(nhirata.bugzilla)
(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.
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.
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.
Resolution: FIXED → INVALID
Verifying work following the Single Variant will be tracked in bug 1020167 per comment 12.
(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
Resolution: INVALID → FIXED
You need to log in before you can comment on or make changes to this bug.