[Meta] Remove the browser app

RESOLVED FIXED in 2.1 S6 (10oct)

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: benfrancis, Assigned: benfrancis)

Tracking

unspecified
2.1 S6 (10oct)
x86_64
Linux
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-b2g:2.1+, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [systemsfe])

Attachments

(2 attachments)

Assignee

Description

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

Comment 1

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

Updated

5 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?
Depends on: 940647
Assignee: nobody → kyle
Whiteboard: [systemsfe]
[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?
blocking-b2g: --- → 2.1?
(Forgot to fb? bfrancis on last comment)
Flags: needinfo?(bfrancis)
Assignee

Comment 5

5 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.
Flags: needinfo?(bfrancis)
Assignee

Updated

5 years ago
Blocks: rocketbar-mvp
No longer blocks: browser-chrome-mvp
Assignee

Updated

5 years ago
Blocks: 1055688
Depends on: 1058270
No longer blocks: 1055688
blocking-b2g: 2.1? → 2.1+
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?
Flags: needinfo?(anygregor)
Target Milestone: --- → 2.1 S4 (12sep)
(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.
Flags: needinfo?(anygregor)
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.
(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.
Flags: needinfo?(kyle)
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.
Depends on: 1062984
Flags: needinfo?(kyle)
Assignee

Comment 11

5 years ago
Let's see what tests break.
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)
Assignee

Comment 12

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

Rebased and trying again.
Depends on: 1069064
Assignee

Comment 13

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

Comment 14

5 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
Flags: needinfo?(kgrandon)
Depends on: 1071707
(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.
Flags: needinfo?(kgrandon)
Assignee

Comment 16

5 years ago
Thanks Kevin, I've rebased to see what TBPL looks like now.
Assignee

Comment 17

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

Comment 18

5 years ago
Stealing
Assignee: kyle → bfrancis
Assignee

Comment 19

5 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?
Flags: needinfo?(kgrandon)
Flags: needinfo?(crdlc)
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.
Flags: needinfo?(kgrandon)
I think that Kevin is right
Flags: needinfo?(crdlc)
Assignee

Comment 22

5 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
Flags: needinfo?(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.
Flags: needinfo?(kgrandon) → needinfo?(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.
Flags: needinfo?(crdlc)
Assignee

Comment 25

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

Comment 26

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

Pre-emptively flagging Kevin for review
Attachment #8490126 - Flags: review?(kgrandon)
Assignee

Comment 27

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

Or Dale might be able to review this
Attachment #8490126 - Flags: review?(dale)
Comment on attachment 8490126 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/24107

R+ assuming try is green. R.I.P. browser app.
Attachment #8490126 - Flags: review?(kgrandon) → review+
Assignee

Comment 29

5 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...
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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 :)
Attachment #8490126 - Flags: review?(dale) → review+
Assignee

Comment 31

5 years ago
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.
Attachment #8490126 - Flags: approval-gaia-v2.1?
(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?
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(community)
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
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(community)
Target Milestone: 2.1 S5 (26sep) → 2.1 S6 (10oct)
Assignee

Comment 34

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

Comment 35

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

Comment 38

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

Comment 41

5 years ago
Note that this is also expected to fix 2.1 blocker bug 1072738
Attachment #8490126 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
This is going to need rebasing for v2.1.
Flags: needinfo?(bfrancis)
Assignee

Comment 43

5 years ago
Posted file 2.1 branch patch
Flags: needinfo?(bfrancis)
Assignee

Comment 44

5 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?
Flags: needinfo?(ryanvm)
I think that at least the Linter failures in that Gaia-Try run are real.
Flags: needinfo?(ryanvm)
Assignee

Comment 46

5 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.
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(bfrancis)
Resolution: FIXED → ---
Assignee

Comment 49

5 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.
Flags: needinfo?(bfrancis)
Lets discuss the flatfish issue in bug 1075011.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Assignee

Comment 51

5 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.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.