Closed Bug 840210 Opened 7 years ago Closed 7 years ago

search engine must be customizable at build time

Categories

(Firefox OS Graveyard :: Gaia::Browser, defect, P2)

x86
macOS
defect

Tracking

(b2g18+ affected, b2g18-v1.0.1 wontfix)

VERIFIED FIXED
Tracking Status
b2g18 + affected
b2g18-v1.0.1 --- wontfix

People

(Reporter: dietrich, Assigned: benfrancis)

References

Details

(Whiteboard: [sprintready][TAIPEI_FND_TRACKING], c=browser u=user)

Attachments

(1 file, 2 obsolete files)

for contractual requirements, we must be able to allow mobile operators to customize the default search engine.
Assignee: nobody → bfrancis
tracking-b2g18: --- → ?
Duplicate to Bug 808732 ?
Assignee: bfrancis → gasolin
Attached file pull request redirect to github (obsolete) —
Use localStorage to store default search engine.

Can define multiple search provider in browser.json/init.json
for the flexibility to support change search provider in app. (Bug 808732)
Attachment #743035 - Flags: review?(bfrancis)
Comment on attachment 743035 [details]
pull request redirect to github

Thanks for changing this not to use localStorage Fred. I personally would have used IndexedDB, but maybe asyncStorage is OK, I've not used it before. Sorry for my ignorance but can you explain how this works asynchronously? The code looks like it waits for the value to return before continuing. Shouldn't asyncStorage.getItem() use a callback of some kind?

Can we remove the code for multiple search engines as it isn't actually used. That work should happen in bug 808732

Also, this patch will try to read a string from disk up to three times every time you type a letter in the address bar. This is probably a bad idea. We should probably retrieve the default search engine data only once at startup and store it in memory. Also, we should probably try to lazy load the async storage library.
Attachment #743035 - Flags: review?(bfrancis) → review-
Thanks Ben, I'll try it again once this issue get higher priority :)
Just saw the discussion from thread about not use DOM Storage (localStorage).

'IndexedDB, Storage, DOM Storage - Which to use ?' in dev-gaia maillist
https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.gaia/7WC4Bq9gbfE


'Never ever use DOMStorage (aka localStorage) in gaia or any built-in apps. 
Though it's fine most of the time it can cause terrible performance issues.'
-- Jonas Sicking
Whiteboard: [TAIPEI_FND_TRACKING]
What does [TAIPEI_FND_TRACKING] mean?
Whiteboard: [TAIPEI_FND_TRACKING] → [TAIPEI_FND_TRACKING], c=browser u=user
(In reply to Ben Francis [:benfrancis] from comment #6)
> What does [TAIPEI_FND_TRACKING] mean?

Means Taipei will drive this issue once we clean up blocking+ issues ...
WIP: 

- move customization related function(populateDefaultData) into a separate `customize.js` file.
- Organized within a `customize` object, call it as customize.populateDefaultData()
- lazyload js/customize.js when `firstrun` flag is set.

will do:

integrate last PR into customize.js, and modified the asyncStorage.getItem part.
Priority: -- → P2
WIP:

https://github.com/mozilla-b2g/gaia/pull/10202
works in firefox nightly, but cannot lazyload correctly on device

need to figure out why lazyload callback is not run on device.
Attached file 2nd pull request redirect to github (obsolete) —
above comment issue solved. send PR.

The reason of lazyloader not work in device is because of we need to add a line in html file to let build script include the correspondent share/ script.

also add test cases to ensure the refactor works.
Attachment #758411 - Flags: review?(bfrancis)
Whiteboard: [TAIPEI_FND_TRACKING], c=browser u=user → [sprintready][TAIPEI_FND_TRACKING], c=browser u=user
Comment on attachment 758411 [details]
2nd pull request redirect to github

Unfortunately this code would never run if people are upgrading from 1.1 to 1.2 because the first run would already have happened.

We need a new mechanism to detect first run for 1.2 and a better upgrade path for IndexedDB for existing users. This will require some refactoring which I'm happy to do.
Attachment #758411 - Flags: review?(bfrancis) → review-
Stealing
Assignee: gasolin → bfrancis
Ben thanks to make this in progress.

I think this will only effect new users and should not run while upgrading to 1.2 since its not user's `firstrun` in this device?
Attachment #743035 - Attachment is obsolete: true
Attachment #758411 - Attachment is obsolete: true
Attachment #786300 - Flags: review?(dale)
Blocks: 808732
Comment on attachment 786300 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11379

This looks great, cheers

Theres a bunch of nits, the ontxncomplete and async_storage comments arent really important and can be ignored.

I would like to clean up the upgrade logic and preferably remove the duplication of the default search provider (mostly having a big base64 file sitting in our source tree is annoying), I have put in some suggestions, I definitely think at the least the upgrade logic should be contained to a BrowserDB and not leak to the main Browser object and if we have no need to assume that no application-data exists then we dont need the redundant fallback

As its a large patch will clear r? and take a look at it again ammended
Attachment #786300 - Flags: review?(dale)
Attachment #786300 - Flags: review?(dale)
Comment on attachment 786300 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11379

There is still a reference to Places in js/toolbar.js, the rest is looking good but will retest with everything working
Attachment #786300 - Flags: review?(dale) → review-
Attachment #786300 - Flags: review?(dale) → review+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Note to QA: this could do with some thorough testing during verification. I say this because a) A lot of re-factoring was done and b) There are quite a lot of different edge cases.

Some particular things worth testing:
1. Different configurations for different operator variants (different bookmarks and search engines specified for different MCC/MNC combinations) and checking they apply correctly when different SIM cards are entered.
2. The upgrade path from 1.1 to 1.2 as a database upgrade occurs to add new object stores.

Let me know if you need more information. Thanks!
Keywords: qawanted
Naoki - Can you look into this?
Flags: needinfo?(nhirata.bugzilla)
Keywords: qawantedverifyme
sure jsmith.  it's in sprint 2 for browser so I would be looking at it.
Flags: needinfo?(nhirata.bugzilla)
(In reply to Ben Francis [:benfrancis] from comment #19)
> Note to QA: this could do with some thorough testing during verification. I
> say this because a) A lot of re-factoring was done and b) There are quite a
> lot of different edge cases.
> 
> Some particular things worth testing:
> 1. Different configurations for different operator variants (different
> bookmarks and search engines specified for different MCC/MNC combinations)
> and checking they apply correctly when different SIM cards are entered.


How does one setup a test SIM with a customizable search engine for testing?  Or is this something we need to get operators to give us?  (if so, what do they need to do to setup the sim?)

or can we just modify a local pref to customize the search engine?  (but that would defeat the purpose of searching MNC/MCC)

> 2. The upgrade path from 1.1 to 1.2 as a database upgrade occurs to add new
> object stores.

This one's harder, we'll need a FOTA server setup, which we dont have.  and since OEMs really deal with this, we'd want external help.  but we can see if we can ask for a OTA server internally.
> 
> Let me know if you need more information. Thanks!
Question 1: If I understand things correctly I think we need to change : 
https://mxr.mozilla.org/gaia/source/build/applications-data.js#219 
to have that and build gaia on top to include the MCC/MNC of the SIM cards that we are using.

Is that correct?

Question 2: Would starting from 1.1 and then doing a make reset-gaia be ok for testing the upgrade path?  Or would I need to do an OTA update?
Flags: needinfo?(bfrancis)
(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #23)
> Question 1: If I understand things correctly I think we need to change : 
> https://mxr.mozilla.org/gaia/source/build/applications-data.js#219 
> to have that and build gaia on top to include the MCC/MNC of the SIM cards
> that we are using.
> 
> Is that correct?

Yes, that's correct.

> Question 2: Would starting from 1.1 and then doing a make reset-gaia be ok
> for testing the upgrade path?  Or would I need to do an OTA update?

I don't think that would work actually because Gaia from 1.2 probably requires Gecko from 1.2 so you need to upgrade both Gecko and Gaia.

I just tested the upgrade path by:
1. Flash a nightly mozilla-central build from before this patch landed (e.g. 2013-08-13)
2. Checkout gaia from before just before this patch landed (e.g. git checkout b6aec1845e535ed0bcb8bcc346e6ee0190704480)
3. Do make reset-gaia (this is necessary because doing install-gaia over the top of a production build will not boot)
4. Open browser app
5. Checkout the current master (git checkout master)
6. Do make install-gaia
7. Open browser app

This showed that the upgrade did work. No bookmarks were added because there were no default bookmarks specified at step 3 but the default search engine was set because it was specified at step 7.

Repeating from step 2 to step 7 with different data specified in applications-data.js and different SIM cards installed at step 6 should yield different results.

This isn't a perfect test of a 1.1 to 1.2 upgrade, but I can't think how to create a more realistic test without a OTA update server.
Flags: needinfo?(bfrancis)
Found out that you can change the channel and get the updates already.  The update script to change the hamachi is attached in bug 906231; I sent kar an update script for unagi so she should be able to get updates on her unagi once she runs that script on her device.

I am still blocked from testing this due to Bug 905831
Depends on: 905831
Waiting for acceptance
Flags: needinfo?(krudnitski)
Will accept but looking to QA to do their (his!) best to verify that a requested default search provider from a partner appears on a 'fresh' device as expected.
Flags: needinfo?(krudnitski)
There's an issue with the releng build; if you build your own it seems to work fine.

Also tried using an update script.  Found one issue that is severe enough that we may want someone to look into when updating from 1.1 to 1.2: Bug 909885

There some other smaller bugs and 1.1HD bugs.  Marking this one as verified:
2013-08-27-04-02-01
Unagi
Status: RESOLVED → VERIFIED
This may have possibly been 

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

Which is an error in the build configuration, it actually looks like it is possibly a racy bug which would explain why it couldnt be reproduced
Depends on: 919515
No longer depends on: 919515
Attachment mime type: text/plain → text/x-github-pull-request
Removing verifyme keyword per comment 28
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.