1043959 - [Meta] Remove the browser app

Status: RESOLVED FIXED

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

Description

[Ben Francis [:benfrancis]]

During upgrade to the system browser, we will need to remove the browser app. this bug is to track that work.

Comment 1

[Ben Francis [:benfrancis]]

One possibility is that in the upgrade from 2.0 to 2.1 we strip the browser down to just some code which can access the old places database, for the purpose of migrating data out of IndexedDB into the new Places DataStore.

Comment 2

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

We'll need to move places and bookmarks. Since I'm already doing this for bookmarks, I can take care of the removal part (which I've already written once anyways, but it's too bitrotted to bring over :| ). We can discuss what it'll take to move places, but it should look similar to bookmarks?

Comment 3

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

[Blocking Requested - why for this release]: Needs to be marked feature-b2g: 2.1+

Instead of stripping this for upgrade code, we are now going to move the IDB out from under the app. This means the app can be completely removed from the system on the 2.1 upgrade, instead of leaving a shell of it around.

This will still have to happen after system browser is viable to use and linked to all activities it needs. Is there a good blocking bug for that?

Comment 4

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

(Forgot to fb? bfrancis on last comment)

Comment 5

[Ben Francis [:benfrancis]]

Bug 945259? It would be good to have the migration code ready though, so we can pull the trigger once that is complete, which is likely to be at the last minute.

Comment 6

[Dietrich Ayala (:dietrich)]

Gregor, in triage we discussed and this seems more a feature bug than a blocker. Is it not possible to ship with the browser as-is?

Comment 7

[Gregor Wagner [:gwagner]]

(In reply to Dietrich Ayala (:dietrich) from comment #6)
> Gregor, in triage we discussed and this seems more a feature bug than a
> blocker. Is it not possible to ship with the browser as-is?

No, we can't ship with the system browser and the browser app. All our databases, bookmark, history logic isn't written in a way to support it.

Comment 8

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

To clarify: the browser was hidden in bug 1058270, which landed shortly after bookmark migration. It's no longer accessible to anything, but still exists on the file system and it's data will stay in the phone profile. This bug was to completely remove it from the repo, and therefore the build, which means on an OTA update it will be completely removed from the system, and its data will be delete from profile on update (after migration, of course).

We could probably live with it hidden for 2.1, I'm just pushing on this to happen because, well, less code is better code. Large floating piece of dead code worry me.

Comment 9

[bhavana bajaj [:bajaj]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #8)
> To clarify: the browser was hidden in bug 1058270, which landed shortly
> after bookmark migration. It's no longer accessible to anything, but still
> exists on the file system and it's data will stay in the phone profile. This
> bug was to completely remove it from the repo, and therefore the build,
> which means on an OTA update it will be completely removed from the system,
> and its data will be delete from profile on update (after migration, of
> course).
> 
> We could probably live with it hidden for 2.1, I'm just pushing on this to
> happen because, well, less code is better code. Large floating piece of dead
> code worry me.

Kyle, looks like all dependencies have landed, so can we close this ? If not can you confirm what's remaining here as QA was concerned on testing around this area.

Comment 10

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

Until bookmark migration is solid, we can't remove the browser, otherwise we'll have data loss. This last dependency should hopefully get taken care of today or tomorrow, has just been blocked on me trying to finish settings work.

Comment 11

[Ben Francis [:benfrancis]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/24107

Let's see what tests break.

Comment 12

[Ben Francis [:benfrancis]]

Build and integration tests failed with test harness failures https://tbpl.mozilla.org/?rev=15af28d2f62b1c5c02d83256ebb21f85f97ba323&tree=Gaia-Try

Rebased and trying again.

Comment 13

[Ben Francis [:benfrancis]]

Looks like the integration test failure is real, it's looking or a file in the browser app directory.

Comment 14

[Ben Francis [:benfrancis]]

Kevin, this vertical homescreen integration test references the browser mock which currently lives in the browser app folder. What's your preferred way to make this test pass?

https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/test/marionette/bookmark_uninstall_test.js

Comment 15

[Kevin Grandon :kgrandon]

(In reply to Ben Francis [:benfrancis] from comment #14)
> Kevin, this vertical homescreen integration test references the browser mock
> which currently lives in the browser app folder. What's your preferred way
> to make this test pass?
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/test/
> marionette/bookmark_uninstall_test.js

I've filed bug 1071707 to remove the browser app dependency from the test. I'll try to get to that asap to unblock you.

Comment 16

[Ben Francis [:benfrancis]]

Thanks Kevin, I've rebased to see what TBPL looks like now.

Comment 17

[Ben Francis [:benfrancis]]

There was a failing test in the build directory which I have hopefully now fixed, there was also a failure in the homescreen which seems unrelated so let's see if it was an intermittent.

Comment 18

[Ben Francis [:benfrancis]]

Stealing

Comment 19

[Ben Francis [:benfrancis]]

Same integration test failure.

NoSuchElement: (7) Unable to locate element: li[data-manifest-u-r-l*="sms.gaiamobile.org"]

in /apps/homescreen/test/marionette/active_icons_test.js:24
and /apps/homescreen/test/marionette/active_icons_test.js:33

Not sure how this is realted, and it's the old homescreen.

Kevin or Christian, any ideas?

Comment 20

[Kevin Grandon :kgrandon]

It looks like this error is getting logged: [marionette error] app://homescreen.gaiamobile.org/js/grid.js:1099 TypeError: descriptor is null

My guess is that you need to remove the browser app from places that reference it in the build/ folder and the home screen folder if you haven't.

Comment 21

[Cristian Rodriguez (:crdlc)]

I think that Kevin is right

Comment 22

[Ben Francis [:benfrancis]]

OK, I've removed all the references I can find in the build folder.

There are quite a lot of integration tests in the old homescreen app which reference the browser for bookmarking, I'm not sure what we want to do with these? Re-write them or disable them? I'm unsure of the level of support we are still providing for the old homescreen?

https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/test/marionette/install_bookmark_test.js
https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/test/marionette/remove_bookmark_test.js
https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/test/marionette/update_bookmark_test.js

https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/test/marionette/lib/browser.js

Comment 23

[Kevin Grandon :kgrandon]

The only reason we still have old home screen code around is because we have not yet ported flatfish over to use the vertical home screen. I would normally say that we should just update these tests to use the system browser, but because this is something blocking 2.1, I think that removing or disabling these tests might be an option - though I would want Cristian to make the final decision as this relates to the old home screen.

Comment 24

[Cristian Rodriguez (:crdlc)]

I think so, we could remove/disable those tests because I don't have time to update those right now, maybe the best idea would be disabled it because it's a blocker. This is my opinion, although I don't have problem if someone takes it and fix the problem.

(In reply to Kevin Grandon :kgrandon from comment #23)
> The only reason we still have old home screen code around is because we have
> not yet ported flatfish over to use the vertical home screen. I would
> normally say that we should just update these tests to use the system
> browser, but because this is something blocking 2.1, I think that removing
> or disabling these tests might be an option - though I would want Cristian
> to make the final decision as this relates to the old home screen.

Comment 25

[Ben Francis [:benfrancis]]

Doing a test run with those integration tests disabled https://tbpl.mozilla.org/?rev=32206300f7d3936d20f2cc2e6bd67970222646ff&tree=Gaia-Try

Comment 26

[Ben Francis [:benfrancis]]

Comment on attachment 8490126 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/24107

Pre-emptively flagging Kevin for review

Comment 27

[Ben Francis [:benfrancis]]

Comment on attachment 8490126 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/24107

Or Dale might be able to review this

Comment 28

[Kevin Grandon :kgrandon]

Comment on attachment 8490126 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/24107

R+ assuming try is green. R.I.P. browser app.

Comment 29

[Ben Francis [:benfrancis]]

Passed on TBPL https://tbpl.mozilla.org/?rev=32206300f7d3936d20f2cc2e6bd67970222646ff&tree=Gaia-Try

Fixed a little merge conflict and landed on master https://github.com/mozilla-b2g/gaia/commit/6d1b717271ec411498f6f4660de51e010a5ffd30

Let's see if it sticks...

Comment 30

[Dale Harvey (:daleharvey)]

Comment on attachment 8490126 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/24107

Seen this land on twitter, so assuming I dont need to review, but yay :)

Comment 31

[Ben Francis [:benfrancis]]

Comment on attachment 8490126 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/24107

If this sticks on master, requesting uplift to 2.1.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): n/a
[User impact] if declined: Shipping a ghost browser app taking up disk space
[Testing completed]: TBPL is green!
[Risk to taking this patch] (and alternatives if risky): This patch removes ~10,000 lines of code and touches configuration files in the build system. We should make sure it well and truly sticks on master before uplifting.
[String changes made]: Many strings deleted, none added.

Comment 32

[bhavana bajaj [:bajaj]]

(In reply to Ben Francis [:benfrancis] from comment #31)
> Comment on attachment 8490126 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/24107
> 
> If this sticks on master, requesting uplift to 2.1.
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): n/a
> [User impact] if declined: Shipping a ghost browser app taking up disk space
> [Testing completed]: TBPL is green!
> [Risk to taking this patch] (and alternatives if risky): This patch removes
> ~10,000 lines of code and touches configuration files in the build system.
> We should make sure it well and truly sticks on master before uplifting.
> [String changes made]: Many strings deleted, none added.

Removing strings is still a change that breaks string freeze per my understanding...But given we do not want to ship a ghost browser obviously, lets see if l10n/:flod is willing to accommodate this request giving we do not want folks to wast time on something that's not usable

On a side note, why was this not prioritized before the freeze?

Comment 33

[Francesco Lodolo [:flod]]

Did you NI :l10n (community alias) but actually wanted :pike? CCing him anyway.

(In reply to bhavana bajaj [:bajaj] from comment #32)
> Removing strings is still a change that breaks string freeze per my
> understanding...But given we do not want to ship a ghost browser obviously,
> lets see if l10n/:flod is willing to accommodate this request giving we do
> not want folks to wast time on something that's not usable

Yes, removing strings or files is still breaking string freeze. It's a change that's going to create unnecessary noise on files and repos that are not supposed to change at this point.

If you really need to land this on 2.1 (210 changed files, that looks scar…), please create a patch that doesn't touch localization files. That means /browser/locales, but also manifest.webapp (Pike, do you need anything else for your extraction tool?).

That would also protect us from the fall-out in case this doesn't stick on 2.1

Comment 34

[Ben Francis [:benfrancis]]

(In reply to bhavana bajaj [:bajaj] from comment #32)
> On a side note, why was this not prioritized before the freeze?

Due to the way the data migration works we couldn't delete the old browser app until users of the master and Aurora nightly builds had migrated their browser bookmarks and history using an OTA update, so it wasn't possible to land this before branching 2.1.

We have been very vocal on mailing lists for many months about the fact the browser app was going away, and members of the localisation team have replied to those threads. I would be very disappointed if localisers have been wasting their time translating strings for the browser app in 2.1. If that's the case then that's a serious breakdown in communication and we should figure out how to prevent that happening in future. This is likely to happen again with apps like the old homescreen.

Comment 35

[Ben Francis [:benfrancis]]

(In reply to Francesco Lodolo [:flod] from comment #33)
> Yes, removing strings or files is still breaking string freeze. It's a
> change that's going to create unnecessary noise on files and repos that are
> not supposed to change at this point.

I'm sorry that this has come as a surprise, it shouldn't have. I wish we could have made this change earlier, but as explained above we needed to make sure we didn't cause data loss for users.

> If you really need to land this on 2.1 (210 changed files, that looks
> scar…),

It looks like a lot of changes but it's really just removing an unused directory, and disabling some integration tests in the old (ghost) homescreen app which depended on the browser app.

> please create a patch that doesn't touch localization files. That
> means /browser/locales, but also manifest.webapp (Pike, do you need anything
> else for your extraction tool?).
> 
> That would also protect us from the fall-out in case this doesn't stick on
> 2.1

I don't really see the value in creating a patch that removes all the browser app files except for the localisation files. Regardless of whether or not this patch sticks on 2.1 (which I fully expect it to if it's landed there), those strings are never exposed to the user, the browser app is completely hidden. It's a waste of time to localise those strings and they should be deleted.

Comment 36

[Kevin Grandon :kgrandon]

Yes, I also wouldn't want to ship just a manifest inside an empty directory - then the app is still technically there and just won't function. We'd probably want to make sure that the icons are there as well, so it kind of defeats the purpose of removal in the first place.

Comment 37

[Axel Hecht [:Pike]]

I expected a ton of "more problems" if we keep the strings.

I think we'll be fine with removing the app completely, but that'll involve a significant risk in case this needs to be backed out. This needs to be rock solid no way going back done deal perfect.


As for the comms around this, yes, I heard that the browser app will eventually go away. Going away at this point on a stable and string frozen branch? That's not something I picked up from the conversations.

For future revamps of our organization, I expect these to land as part of the regular development cycle, including profile migration. That's the best way to do it, and the least reason for that is that we know that we can migrate user's profiles out in the wild.

Comment 38

[Ben Francis [:benfrancis]]

(In reply to Axel Hecht [:Pike] from comment #37)
> I think we'll be fine with removing the app completely, but that'll involve
> a significant risk in case this needs to be backed out. This needs to be
> rock solid no way going back done deal perfect.

Great. I'm confident that the browser app is not coming back at this stage. There's no pref to switch off the system browser since the Rocketbar re-write and I can't imagine many bugs which would be so hard to fix that we'd want to back out the whole system browser instead.

I'm working with the Devices team to ensure some downstream projects are using the new system browser as a base.

> As for the comms around this, yes, I heard that the browser app will
> eventually go away. Going away at this point on a stable and string frozen
> branch? That's not something I picked up from the conversations.

FWIW, the browser app has been hidden from users since long before 2.1 branched. Is there a way that we can notify localisers in future if there are special cases of strings still checked in that aren't actually exposed to users for whatever reason?

> For future revamps of our organization, I expect these to land as part of
> the regular development cycle, including profile migration. That's the best
> way to do it, and the least reason for that is that we know that we can
> migrate user's profiles out in the wild.

I didn't work on the data migration so I can't comment on how feasible that is.

This patch seems to have stuck on master. I recommend that we uplift it to 2.1 as soon as possible.

Comment 39

[Axel Hecht [:Pike]]

Re messaging, I don't have high hopes for the time being. Too many of our messaging and comments go unread, or read and not understood/forgot.

Comment 41

[Ben Francis [:benfrancis]]

Note that this is also expected to fix 2.1 blocker bug 1072738

Comment 42

[Ryan VanderMeulen [:RyanVM]]

This is going to need rebasing for v2.1.

Comment 43

[Ben Francis [:benfrancis]]

Attachment: 2.1 branch patch

Comment 44

[Ben Francis [:benfrancis]]

Hi Ryan, I created a pull request with a rebased patch against the v2.1 branch, just waiting to see if tests pass.

Does this look OK? Will you merge this or shall I?

Comment 45

[Ryan VanderMeulen [:RyanVM]]

I think that at least the Linter failures in that Gaia-Try run are real.

Comment 46

[Ben Francis [:benfrancis]]

Oops, looks like I resolved a conflict wrongly and removed some things from the xfail list which shouldn't be removed on the 2.1 branch. I've pushed to try again.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/1c5f728f3f66e176eae9104a7b62e37f70fd3583

Comment 48

[Safwan Rahman (:safwan)]

It seems that, according to Bug 1075011, the  commit: [58e3bdf9b7a7776451f74ad3bfe7af5ad58dffca] of this bug affected Badly the build of Flatfish Device. So need to fix it in order to build Firefox OS for flatfish.

Comment 49

[Ben Francis [:benfrancis]]

I'm trying to create a flatfish build to see what happened, but I don't think a flatfish bug should block the 2.1 release.

I commented in bug 1059697 with a suggestion to show the new browser icon on the old homescreen app. If no icons are showing at all on the homescreen and that regressed with this patch then that might mean we broke the build configuration for flatfish and that's a wider issue.

Comment 50

[Gregor Wagner [:gwagner]]

Lets discuss the flatfish issue in bug 1075011.

Comment 51

[Ben Francis [:benfrancis]]

We discussed this bug in the integration test peer review session and concluded there probably isn't much we can test here with integration tests as there were no user facing changes, the browser app was hidden, this was just removing the un-used directory.