938171 - [User Story] Bookmark Migration from Browser to Homescreen

Status: VERIFIED FIXED

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

Description

[Peter Dolanjski [:pdol]]

User Story:
As Mozilla, during an OS update, I want to migrate existing users' Browser bookmarks to become homescreen bookmarks to support the concept of the Browser being part of the system rather than a stand alone app.

Acceptance Criteria:
1. After updating to the new OS version which includes the bookmark migration, all of websites which I had previously bookmarked within the Browser now display on my homescreen as icons.

Assumptions:
UX to define where the icons migrate to on app grid and if any modifications to the title of bookmark will occur.

Comment 1

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

hubert created https://moztrap.mozilla.org/manage/case/11455/

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Remove no longer needed files from browser app

Convenience patch so we don't have all of the file/function deletions getting in the way of the actual changes in later patches.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8470476 [details]
Patch 1 (v1) - Remove no longer needed files from browser app

Oops. Forgot this should just get a github pull request. Will wait on that.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Been working on when to launch the browser during upgrade boot. Trying to decide whether it should be FTU or System App. 

Tried having the FTU fire a "browser-migrate" activity. The problem being that when the browser uninstalls itself, we either get dropped to the homescreen in a weird state when using a "Window" context, or just a blank screen using "inline" context, because the system is not used to apps uninstalling themselves during an activity. I'm not sure if there's a way for the FTU to recover focus after the app uninstalls, unless I make some sort of fix for an activity detecting that the app its launched has been uninstalled.

I could try having the system app have a set of "upgrades" that it does before it launches the tutorial, but then we have to figure out how to know when the app has uninstalled itself.

fabrice, any opinions on this? I realize we're already well into special case land here in terms of what webapps do.

Comment 6

[[:fabrice] Fabrice Desré]

Are we launching the browser oop when it's used only for data migration?
If yes, one (hacky) thing you can do is to make the browser app use an api it doesn't have the permission for after it has uninstalled itself. That will make us kill the process and that should be caught properly by the system app to clean up the activity window.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

New strategy: 

In chrome, copy the indexed db database file from the browser app to the system app. This will need to happen after we read the webapps.json file, but before we uninstall the browser app, so timing is important. I've tested this method by hand and it does work; the system app ends up with a "browser" database with bookmarks in it.

Then, once the system app has launched and an upgrade is detected, we can run the upgrader to test for the existence of the browser database, do the IDB -> datastore migration, and delete the browser database.

This solution also lets us completely remove the browser app instead of just gutting it but leaving it as an upgrade path/zombie app.

Comment 8

[Ben Francis [:benfrancis]]

That does sound a lot cleaner if it works!

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, here's the full instructions for what needs to be done:

For gecko:

We need a component that lives in the b2g/components directory. It'll be an observer that listens for a message, something like "gaia-browser-app-migration". This message needs to fire after we've loaded the webapps.json registry, but BEFORE we run the installSystemApps() function in WebApps.jsm. Basically, there's one place it can go, right after line 613 in WebApps.jsm:

https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#613

That's where we'd need to fire the message. It does NOT need to be in the MOZ_WIDGET_GONK ifdef guard, as we should be able to test everything but the removal of browser on b2g desktop via integration tests and hopefully that will make our lives much easier in the long run.

The notification will cause the upgrader component to do a few things:

- Look up the local Ids of the system and browser app (we can do this via their app URLs, since they're certified)

- Form the path of the browser storage directory and browser DB filename, as well as the storage directory path it needs to move to in the system app storage.

 - For the storage directory, this is usually something like [profile directory]/storage/persistent/22+f+app+++browser.gaiamobile.org/idb. The directory name can be formed by changing out the local app id and the url (i.e. [local id]+f+app+++[app url]).

 - The browser DB name is always the same due to the way we hash idb names: 2959517650brreosw.sqlite

- Make sure the browser DB actually exists (may happen on an upgrade where the browser app was never run). If this is the case, stop now.

- Copy the file from the browser storage directory to the system storage directory. As the next step after we leave this function is to uninstall all non-included apps, we /have/ to make sure the copy is finished before we continue, because it will be deleted in the next step.

-------------------------

After that's done, we can continue on with startup. The next step happens in gaia:

- We need to add a "browser_migration.js" module to the system app. This will run on startup when an upgrade is detected.

- This module will need to try and open an db called "browser". We can bring the code from browser_db.js in the browser app to do this work. The problem with IDB is that if a DB doesn't exist, it'll create one when you open it. Since we're not going to be changing the DB version, if the "needs upgrade" function runs, we'll know we /don't/ need to migrate, because that would only run if there wasn't a DB in the first place. If "needs upgrade" doesn't run, we'll know we have a DB that matches the version we expect, at which point we should migrate.
 
- Migration is simply running a getAllBookmarks and getAllPlaces (note, getAllPlaces function does not currently exist in that form but should be easy enough) and making the needed calls to their respective datastores to import the information.

- Right now, the assumption is that this will be fast enough to be unnoticeable from normal boot time. I have not load tested yet.

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - WIP: Move browser app database file in gecko

Current WIP for moving database file. The problem happens in the GaiaBrowserAppUpgrade.js (which I realize is a bad filename for it but stick with me here). When call AppService function, we receive what is basically a new DOMApplicationRegistry object. The webapps.json for it is not filled in, so we can't resolve local IDs.

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Move browser app database file in gecko

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 8475677 [details] [diff] [review]
Patch 1 (v2) - Move browser app database file in gecko

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

::: b2g/components/B2GAppMigrator.js
@@ +15,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +Cu.import("resource://gre/modules/AppsUtils.jsm");

nit: add a blank line.

@@ +18,5 @@
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +Cu.import("resource://gre/modules/AppsUtils.jsm");
> +XPCOMUtils.defineLazyServiceGetter(this, "appsService",
> +                                   "@mozilla.org/AppsService;1",
> +                                   "nsIAppsService");

and here too

@@ +20,5 @@
> +XPCOMUtils.defineLazyServiceGetter(this, "appsService",
> +                                   "@mozilla.org/AppsService;1",
> +                                   "nsIAppsService");
> +function B2GAppMigrator() {
> +  if (AppsUtils.isFirstRun(Services.prefs)) {

Calling isFirstRun(Services.prefs) here will make it return |false| later when Webapps.jsm does the same call. If you really need it, use another pref branch.

@@ +25,5 @@
> +    if (DEBUG) debug("System in upgrade mode, adding migration observer");
> +    Services.obs.addObserver(this, "b2g-app-migration", false);
> +  } else {
> +    if (DEBUG) debug("System not upgrading, not adding migration observer");
> +  }  

nit: trailing whitespace

@@ +42,5 @@
> +    // the app
> +    let browserLocalAppId = appsService.getAppLocalIdByManifestURL("app://browser.gaiamobile.org/manifest.webapp");
> +    let systemLocalAppId = appsService.getAppLocalIdByManifestURL("app://system.gaiamobile.org/manifest.webapp");
> +    let browserAppStorageDirName = browserLocalAppId + "+f+app+++browser.gaiamobile.org";
> +    let systemAppStorageDirName = systemLocalAppId + "+f+app+++system.gaiamobile.org";      

trailing whitespace.

@@ +56,5 @@
> +                                          "persistent",
> +                                          systemAppStorageDirName,
> +                                          "idb",
> +                                          browserDBFileName], true);
> +    

here too.

@@ +88,5 @@
> +        createInstance(Components.interfaces.nsIBinaryOutputStream);
> +    bostream.setOutputStream(foStream);
> +    bostream.writeBytes(bytes, bytes.length);
> +    bostream.close();
> +    if (DEBUG) debug("Browser DB copied successfully");

Why are you not using nsIFile.copyTo() ?

@@ +90,5 @@
> +    bostream.writeBytes(bytes, bytes.length);
> +    bostream.close();
> +    if (DEBUG) debug("Browser DB copied successfully");
> +  },
> +  

nit: trailing whitespace

::: b2g/components/moz.build
@@ +25,4 @@
>      'SystemMessageGlue.js',
>      'TelProtocolHandler.js',
>      'WebappsUpdateTimer.js',
> +    'YoutubeProtocolHandler.js'

please don't change that.

::: b2g/installer/package-manifest.in
@@ +831,4 @@
>  @BINPATH@/components/DownloadsUI.js
>  @BINPATH@/components/InterAppCommUIGlue.js
>  @BINPATH@/components/SystemMessageGlue.js
> +@BINPATH@/components/GaiaBrowserAppUpgrade.js

wrong name. How did that work?!

::: dom/apps/src/Webapps.jsm
@@ +607,4 @@
>    loadAndUpdateApps: function() {
>      return Task.spawn(function() {
>        let runUpdate = AppsUtils.isFirstRun(Services.prefs);
> +      

nit: trailing whitespace.

@@ +612,5 @@
>  
>        if (runUpdate) {
> +        // At this point, we want to move the gaia browser app
> +        // database if it still exists, as otherwise we will delete
> +        // the browser app before we can migrate bookmarks

I would not explain the use case here. Just explain where in the startup sequence we trigger that.

@@ +613,5 @@
>        if (runUpdate) {
> +        // At this point, we want to move the gaia browser app
> +        // database if it still exists, as otherwise we will delete
> +        // the browser app before we can migrate bookmarks
> +        Services.obs.notifyObservers(null, "b2g-app-migration", null);        

nit: trailing whitespace.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Move browser app database file in gecko

Fixed nits, replaced stream copy with copyTo, fixed manifest. We now also always register for the observer, though I'm not sure how that affects lifetime of the object if the observer is never called and unregistered?

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 8476164 [details] [diff] [review]
Patch 1 (v3) - Move browser app database file in gecko

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

Almost there, I just want to re-check before we land.

::: b2g/components/B2GAppMigrator.js
@@ +15,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +Cu.import("resource://gre/modules/AppsUtils.jsm");

You don't use this one anymore.

@@ +22,5 @@
> +                                   "@mozilla.org/AppsService;1",
> +                                   "nsIAppsService");
> +
> +function B2GAppMigrator() {
> +  Services.obs.addObserver(this, "b2g-app-migration", false);

So indeed it would be nice to remove this observer in an xpcom-shutdown observer.

@@ +47,5 @@
> +                                           "persistent",
> +                                           browserAppStorageDirName,
> +                                           "idb",
> +                                           browserDBFileName], true);
> +    let systemDBDir = FileUtils.getFile("ProfD",

any reason for not using getDir here? https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/FileUtils.jsm#62

@@ +59,5 @@
> +      debug("System local id: " + systemLocalAppId + "");
> +      debug("Browser DB file path: " + browserDBFile.path + "");
> +      debug("System DB directory path: " + systemDBDir.path + "");
> +    }
> +    if (!browserDBFile.exists()) {

move this check before |let systemDBDir|

@@ +67,5 @@
> +    if (DEBUG) debug("Browser DB file exists, copying");
> +    try {
> +      browserDBFile.copyTo(systemDBDir, browserDBFileName);
> +    } catch (e) {
> +      debug("File copy caused error! " + e.name); 

nit: trailing whitespace.

::: dom/apps/src/Webapps.jsm
@@ +611,5 @@
>        yield this.loadCurrentRegistry();
>  
>        if (runUpdate) {
> +
> +        // Run migration before uninstall of core apps happens

nit: full stop at the end of comment.

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v4) - Move browser app database file in gecko

File copy now just uses copyTo, fixed nits

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - Browser Bookmark Migration in Gaia

The gaia side of bookmark migration.

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 8476291 [details] [diff] [review]
Patch 1 (v4) - Move browser app database file in gecko

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

r=me with nit fixed.

::: dom/apps/src/Webapps.jsm
@@ +612,5 @@
>  
>        if (runUpdate) {
> +
> +        // Run migration before uninstall of core apps happens.
> +        Services.obs.notifyObservers(null, "b2g-app-migration", null);

this is not inside a MOZ_B2G block, so rename the topic to something product-agnostic. Naming is hard... what about "webapps-before-update-merge" ?

Comment 18

[Kevin Grandon :kgrandon]

Comment on attachment 8476407 [details] [review]
Patch 2 (v1) - Browser Bookmark Migration in Gaia

Since this is essentially a new module in the system app, a system peer probably needs to sign off as well. Adding Tim to either take the review or re-assign it. (I'm looking as well)

Comment 19

[Kevin Grandon :kgrandon]

Comment on attachment 8476407 [details] [review]
Patch 2 (v1) - Browser Bookmark Migration in Gaia

This looks pretty good to me, my only concern is having everything loaded in the system app all the time. Would like to either hear a counter-argument or see some file shuffled around and I would be happy with it. Thanks!

Comment 20

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I'm totally good with lazy loading. We can do this similar to how we do the operator variant code.

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8476407 [details] [review]
Patch 2 (v1) - Browser Bookmark Migration in Gaia

I would love to see this being done entirely either in Gecko or Gaia but I don't think that's possible :'(

Comment 22

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v2) - Browser Bookmark Migration in Gaia

Converted app migrator to generic system with browser migrator loading. Also collapsed browser db file into the migrator.

Comment 23

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #21)
> Comment on attachment 8476407 [details] [review]
> Patch 2 (v1) - Browser Bookmark Migration in Gaia
> 
> I would love to see this being done entirely either in Gecko or Gaia but I
> don't think that's possible :'(

Believe me, we wanted that too. Unfortunately, we can't hit datastores from chrome js (there's a bug filed to do so but it's nowhere near high priority), and the pure gaia solution can't fully remove a certified app from the system. So, worst of both worlds it is. :/

Comment 24

[Kevin Grandon :kgrandon]

Comment on attachment 8477182 [details] [review]
Patch 2 (v2) - Browser Bookmark Migration in Gaia

The code looks good to me. I'm going to be honest and say that I was a bad reviewer and didn't actually test that a migration worked, but I will keep an eye on this will try to test it with a 2.1 OTA update once it lands. Thanks for the quick execution on this.

Comment 25

[Ben Francis [:benfrancis]]

Comment on attachment 8477182 [details] [review]
Patch 2 (v2) - Browser Bookmark Migration in Gaia

r+ from me.

This is going to need thorough testing by QA I think.

Is this code going to run for dogfooders getting OTA updates with nightly builds? It would be bad if we deleted their browser database before we implement history migration.

Comment 26

[Jason Smith [:jsmith]]

(In reply to Ben Francis [:benfrancis] from comment #25)
> Comment on attachment 8477182 [details] [review]
> Patch 2 (v2) - Browser Bookmark Migration in Gaia
> 
> r+ from me.
> 
> This is going to need thorough testing by QA I think.

Drive-by comment - this might be a good case to use the qa-approval flag on someone to test this before landing.

Comment 27

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Fabrice just recommended a simplification for Patch 1 on IRC, putting it here so I don't forget.

<fabrice> qDot: because I think we could do something a bit simpler... Instead of observer notification, just set the contactID of the component (eg @mozilla.org/dom/apps/migration;1) and make it implement  nsIObserver, then do a createInstance() and call observe() on the component

Comment 28

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

In the interest of getting this landed, I'm decoupling the idea in Comment 27 to bug 1058180. Should still be done by end of 2.1 feature sprints, but would like to get this in now so we can have parallel work on the gaia side.

Comment 29

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Gecko patch: https://hg.mozilla.org/integration/b2g-inbound/rev/25338e7384da
Gaia patch: https://github.com/mozilla-b2g/gaia/commit/a4bf1ffcb798825c14b7860052b0639763286380

Comment 30

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v1) - Add app_migrator to bootstrap unit test

Comment 31

[Aus Lacroix [:aus]]

Comment on attachment 8478696 [details] [review]
Patch 3 (v1) - Add app_migrator to bootstrap unit test

lgtm!

Comment 32

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Gaia unit test patch: https://github.com/mozilla-b2g/gaia/pull/23287

Comment 33

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Gaia patch backed out due to gaia bootstrap unit test failures.

https://github.com/mozilla-b2g/gaia/commit/d35f1c9a22c87e96f0289135955f7316900de6ab
https://github.com/mozilla-b2g/gaia/commit/0acb88a5663af1fa4ec8beb84b7cb657acba19b2

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25338e7384da

Comment 36

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Relanded to gaia: https://github.com/mozilla-b2g/gaia/commit/f11ccb2c4e317cf13a7868801fbb0d490d582c58

Comment 37

[Yeojin Chung [:YeojinC]]

This issue is verified on Flame 2.1:

[Before OTA - Flame 2.0]
 
Environmental Variables:
Device: Flame 2.0 (319mb)(Full Flash)
BuildID: 20141003000204
Gaia: fa797854bfe708129ed54a158ad4336df1015c39
Gecko: 9b7fd1f78a15
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 32.0 (2.0)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
  
[After OTA - Flame 2.1]

Environmental Variables:
Device: Flame 2.1
BuildID: 20141003000203
Gaia: 9861c61ec302fb0316c753a2e1c0f592180515fd
Gecko: da68900d1c66
Gonk: 2c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Bookmarks are migrated properly on the homescreen after upgrade to 2.1.