1247894 - Move about:loop* definitions into the Loop add-on

Status: RESOLVED INCOMPLETE

(Hello (Loop) :: Client, defect, P1)

Description

[Mark Banner (:standard8)]

Currently the about:loop* definitions exist in the core of the desktop browser:

http://hg.mozilla.org/mozilla-central/annotate/576a6dcde5b6/browser/components/about/AboutRedirector.cpp#l104

We should move these to the add-on so that its easier for us to add/change them independently of the Firefox releases. Pocket has already done this:

http://hg.mozilla.org/mozilla-central/annotate/576a6dcde5b6/browser/extensions/pocket/bootstrap.js#l77

I think we should do the same.

Note: we'll need to additionally implement getIndexedDBOriginPostfix in the about page handler to return the appropriate value if the uri matches.

Comment 1

[Mark Banner (:standard8)]

I think we want to bump this up the priority queue - its looking like we need to alter the about: uris for the work we're doing so having this in place would be the obvious thing to do.

Comment 2

[GitHub Autolander Bot]

Attachment: Link to Github pull-request: https://github.com/mozilla/loop/pull/360

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8741456 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/360

The functional tests should explode badly if things go wrong in this code, so that seems like sufficient coverage.

Comment 4

[Mark Banner (:standard8)]

Attachment: about:loop* removal from browser/

This is the patch I used to test the add-on against FF.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8741456 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/360

I left some comments in the PR, I think there's still things in the conversation window to sort out at least.

Comment 6

[Mark Banner (:standard8)]

Attachment: about:loop* removal from browser/ v2

bitrot fix

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8741456 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/360

Review/feedback/thoughts requested here, including about tests.  Lots of the latest context in the PR.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8741456 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/360

Ok, I've had a play around with this. Adding

<em:multiprocessCompatible>true</em:multiprocessCompatible>

just after the `em:type` in install.rdf.in fixes the issues when run with my patch (note: *.in files don't auto-rebuild via runserver). Which means we're still hitting an issue with the shims even doing it the new way.

Due to the fact that akita needs an extra about: page for the toc, we're going to need to do something. That means turning off multiprocess compatible for us.

I'd still like to poke at the shims and see if we can get them fixed, however, for now, how about we retarget this for landing on Akita?

We can land with the patch roughly as-is, but with the multiprocess compat flag set to true, and not land the FF removals for the pages.

That will unblock the akita work, and get us something that we can start testing. If the shims can be fixed, we can consider what we want to do later wrt master - I'm still keen on getting this into FF land for some earlier testing.

I've not thought about tests too much yet. It doesn't look super scary. I think testing shutdown/unload with anything other than manual is going to be potentially difficult anyway. Lets punt on that we do the restartless testing (it doesn't really affect FF users, only people manually installing the add-on).

Comment 9

[GitHub Autolander Bot]

Attachment: [loop] dmose:about-loop-backout-reland > mozilla:akita

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

This PR attempts to back out the old version of the about:loop branch from the akita patch (which turned out to be more exciting than anticipated due to some interleaved commits), and land the most up-to-date version.  There's at least one problem where the sidebar doesn't load that needs to be tracked down, plus some manual testing to make sure nothing else got broken is necessary.  I also haven't yet made the tweaks suggested in comment 8, which is why this is still WIP.

Comment 11

[Mark Banner (:standard8)]

Just to follow-up on comment 8, I've filed bug 1271244 on the core issue. We'll work out how to deal with this as we move forward.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8750173 [details] [review]
[loop] dmose:about-loop-backout-reland > mozilla:akita

This seems to be basically working, at least well-enough for the akita branch...

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8750173 [details] [review]
[loop] dmose:about-loop-backout-reland > mozilla:akita

The code is good. r=Standard8 with the comments addressed.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Adding a needinfo to myself to summarize the state of both akita and master, and file any spin-off bugs, including testing ones.

Comment 15

[Mark Banner (:standard8)]

Support for Hello/Loop has been discontinued.

https://support.mozilla.org/kb/hello-status

Hence closing the old bugs. Thank you for your support.