Ben Francis [:benfrancis] Assignee
	Description • 10 years ago

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

	Ben Francis [:benfrancis] Assignee
	Comment 1 • 10 years ago

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.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 2 • 10 years ago

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?

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 3 • 10 years ago

[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?

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 4 • 10 years ago

(Forgot to fb? bfrancis on last comment)

	Ben Francis [:benfrancis] Assignee
	Comment 5 • 10 years ago

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.

	Dietrich Ayala (:dietrich)
	Comment 6 • 10 years ago

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?

	Gregor Wagner [:gwagner]
	Comment 7 • 10 years ago

(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.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 8 • 10 years ago

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.

	bhavana bajaj [:bajaj]
	Comment 9 • 10 years ago

(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.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 10 • 10 years ago

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.

	Ben Francis [:benfrancis] Assignee
	Comment 11 • 10 years ago

Let's see what tests break.

	Ben Francis [:benfrancis] Assignee
	Comment 12 • 10 years ago

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

Rebased and trying again.

	Ben Francis [:benfrancis] Assignee
	Comment 13 • 10 years ago

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

	Ben Francis [:benfrancis] Assignee
	Comment 14 • 10 years ago

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

	Kevin Grandon :kgrandon
	Comment 15 • 10 years ago

(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.

	Ben Francis [:benfrancis] Assignee
	Comment 16 • 10 years ago

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

	Ben Francis [:benfrancis] Assignee
	Comment 17 • 10 years ago

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.

	Ben Francis [:benfrancis] Assignee
	Comment 18 • 10 years ago

Stealing

	Ben Francis [:benfrancis] Assignee
	Comment 19 • 10 years ago

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?

	Kevin Grandon :kgrandon
	Comment 20 • 10 years ago

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.

	Cristian Rodriguez (:crdlc)
	Comment 21 • 10 years ago

I think that Kevin is right

	Ben Francis [:benfrancis] Assignee
	Comment 22 • 10 years ago

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

	Kevin Grandon :kgrandon
	Comment 23 • 10 years ago

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.

	Cristian Rodriguez (:crdlc)
	Comment 24 • 10 years ago

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.

	Ben Francis [:benfrancis] Assignee
	Comment 25 • 10 years ago

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

	Ben Francis [:benfrancis] Assignee
	Comment 26 • 10 years ago

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

Pre-emptively flagging Kevin for review

	Ben Francis [:benfrancis] Assignee
	Comment 27 • 10 years ago

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

Or Dale might be able to review this

	Kevin Grandon :kgrandon
	Comment 28 • 10 years ago

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

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

	Ben Francis [:benfrancis] Assignee
	Comment 29 • 10 years ago

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...

	Dale Harvey (:daleharvey)
	Comment 30 • 10 years ago

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

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

	Ben Francis [:benfrancis] Assignee
	Comment 31 • 10 years ago

Comment on attachment 8490126 [details]
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.

	bhavana bajaj [:bajaj]
	Comment 32 • 10 years ago

(In reply to Ben Francis [:benfrancis] from comment #31)
> Comment on attachment 8490126 [details]
> 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?

	Francesco Lodolo [:flod]
	Comment 33 • 10 years ago

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

	Ben Francis [:benfrancis] Assignee
	Comment 34 • 10 years ago

(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.

	Ben Francis [:benfrancis] Assignee
	Comment 35 • 10 years ago

(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.

	Kevin Grandon :kgrandon
	Comment 36 • 10 years ago

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.

	Axel Hecht [:Pike]
	Comment 37 • 10 years ago

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.

	Ben Francis [:benfrancis] Assignee
	Comment 38 • 10 years ago

(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.

	Axel Hecht [:Pike]
	Comment 39 • 10 years ago

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.

	Ben Francis [:benfrancis] Assignee
	Comment 41 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 42 • 10 years ago

This is going to need rebasing for v2.1.

	Ben Francis [:benfrancis] Assignee
	Comment 44 • 10 years ago

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?

	Ryan VanderMeulen [:RyanVM]
	Comment 45 • 10 years ago

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

	Ben Francis [:benfrancis] Assignee
	Comment 46 • 10 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 47 • 10 years ago

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

	Safwan Rahman (:safwan)
	Comment 48 • 10 years ago

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.

	Ben Francis [:benfrancis] Assignee
	Comment 49 • 10 years ago

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.

	Gregor Wagner [:gwagner]
	Comment 50 • 10 years ago

Lets discuss the flatfish issue in bug 1075011.

	Ben Francis [:benfrancis] Assignee
	Comment 51 • 10 years ago

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.