1003876 - [User Story] [W3C Manifest] Install an app/bookmark from a web page using app name from manifest

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect, P2)

Description

[Ben Francis [:benfrancis]]

As a developer I want to specify the title of my app in an app manifest.

Use the name property, as per the W3C Manifest draft specification http://w3c.github.io/manifest/#name-member

Comment 1

[Ben Francis [:benfrancis]]

Attachment: WIP patch

Comment 2

[Ben Francis [:benfrancis]]

Attachment: Proof of concept patch

Here is a proof of concept patch to start a conversation.

The system app listens for mozbrowsermanifestchange [0] events on browser windows. If a manifest URL is associated with a web page when the user goes to bookmark it, the system app tries to fetch the web manifest at that URL. If the manifest is fetched successfully and contains a name property, that name will be used for the bookmark name.

If no name property is found, it will will fall back to an application-name <meta> tag, and finally to a <title>.

It may be that we want the platform to fetch the manifest, sanitise it and pass it to content in some other way [1]. Also, it may be that for apps which specify a display mode [2] other than "browser", we want to call window.navigator.mozApps.install() with the manifest URL (or manifest file) rather than add a bookmark, so that we dynamically add an app into the app registry [3].

You can test this patch by navigating to http://tasks.webian.org in the system browser and selecting "Add to homescreen" from the overflow menu (although it won't do anything interesting!). That's a sample web page which references a web manifest in a link relation.

0. https://bugzilla.mozilla.org/show_bug.cgi?id=982800
1. https://bugzilla.mozilla.org/show_bug.cgi?id=997779
2. http://w3c.github.io/manifest/#display-modes
3. https://bugzilla.mozilla.org/show_bug.cgi?id=1075704

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8500641 [details] [review]
Proof of concept patch

This patch seems fine to me, just had one question before I leave F+.

Why does the mozbrowsermanifestchange event not come with the manifest payload? If we are always going to be using this, then I would imagine that we could optimize this from the platform by fetching this in parallel perhaps.

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Kevin Grandon :kgrandon from comment #3)

> Why does the mozbrowsermanifestchange event not come with the manifest
> payload? If we are always going to be using this, then I would imagine that
> we could optimize this from the platform by fetching this in parallel
> perhaps.

We could do that, but I'd like us to optimize this so that gecko only fetches the manifest if there's a mozbrowsermanifestchange event listener attached.

Comment 5

[Marcos Caceres [:marcosc]]

Shouldn't we only be d/ling the manifest when it's needed? I'm working on stuff locally here to add the following to BrowserElementParent.jsm. It fetches/processes the manifest as per W3C and gives you back a JS object:

```
   _getW3CManifest(){ 
    return new this._window.Promise((resolve,reject) => {
      W3CManifestObtainer
        .obtainManifest(this._window)
        .then(resolve,reject);
    });
  },

```

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8500641 [details] [review]
Proof of concept patch

f=me for appWindow change; and +1 to kevin, the "manifest change event then fetch the manifest info" mechanism in gaia looks strange. Atomic operation is desired to be consistent with all other events.

Comment 7

[Ben Francis [:benfrancis]]

Including the URL in the event payload and then letting the system app fetch the content from the URL is actually consistent with the mozbrowsericonchange event, but maybe that should be handled more by the platform too.

I don't have strong feelings whether the manifest is fetched by the platform or by content, but it would be nice to know which it's going to be before I go much further along with implementation.

One thing to note is that the current draft of the Web Manifest spec says that a valid manifest can exist at a "well known URI" [1] without needing to be referenced in a link relation. These manifests wouldn't currently be detected by a mozbrowsermanifestchange event. I personally think that the well known URI is just a way to create unnecessary network traffic and spam web server logs and I would be happy to omit support for it, but if that's what the spec says then we need to bear it in mind. It does seem to go against the "multiple apps per origin" idea of App Scope though.

Marcos, are you suggesting that the system app should call iframe.getW3CManifest(callback) and wait for a callback containing a JavaScript object? I'm not opposed to this, but it may be simpler to either just include the manifest object in the manifestchange event payload or leave it to content to fetch the manifest file.

Fabrice has suggested modifying window.navigator.mozApps.install() to accept a manifest object rather than a URL so that once we have the manifest data in Gaia, Gecko doesn't have to fetch it again in order to install an app into the app registry.

1. http://w3c.github.io/manifest/#the-manifest.json-well-known-uri

Comment 8

[Marcos Caceres [:marcosc]]

(In reply to Ben Francis [:benfrancis] from comment #7)
> Including the URL in the event payload and then letting the system app fetch
> the content from the URL is actually consistent with the
> mozbrowsericonchange event, but maybe that should be handled more by the
> platform too.

My concern is that this will lead to redundant requests; at least, until we work out how to actually apply the contents of a manifest to a web application. Currently, the manifest content is assumed to be applied after a subsequent restart of the web application - and requires some kind of "installation" step, even if that happens transparently.  

> I don't have strong feelings whether the manifest is fetched by the platform
> or by content, but it would be nice to know which it's going to be before I
> go much further along with implementation.

I would prefer we don't double up on effort here. If you go much further you will just end up reimplementing everything I've already implemented (i.e., about 3 weeks worth of work). I think that would be a waste of resources given that I spent quite a bit of time making sure it matches the spec.

To make this happen, I've also allocated 95% of my time to just working on this. I'm trying my best to get up to speed with all the Gecko quirks. Again, I'm on IRC and happy to collaborate on anything in whatever way is most efficient - being new to the code base and flying mostly blind isn't doing wonders for my productivity, but I think I'm getting there:)   
 
> One thing to note is that the current draft of the Web Manifest spec says
> that a valid manifest can exist at a "well known URI" [1] without needing to
> be referenced in a link relation. These manifests wouldn't currently be
> detected by a mozbrowsermanifestchange event.

The point is that manifest should only be applied when needed. This is why I don't see the value of the mozbrowsermanifestchange as the principal trigger for obtaining a manifest. The change event can play a role later though, but its utility might be limited to invalidating an previously acquired manifest. 

>  I personally think that the
> well known URI is just a way to create unnecessary network traffic and spam
> web server logs and I would be happy to omit support for it, but if that's
> what the spec says then we need to bear it in mind. 

FWIW, I've already got support for .well-known/manifest in my implementation. 

> It does seem to go
> against the "multiple apps per origin" idea of App Scope though.

No, it allows authors freedom to choose which model they opt into. 
 
> Marcos, are you suggesting that the system app should call
> iframe.getW3CManifest(callback) and wait for a callback containing a
> JavaScript object?

It returns a promise, but yes exactly that. 

> I'm not opposed to this, but it may be simpler to either
> just include the manifest object in the manifestchange event payload or
> leave it to content to fetch the manifest file.

Again, I think fetching before you use the manifest leads to unnecessary requests. However, it would be trivial to add support for this though it would need to deal with a bunch of error cases (e.g., manifest change, but 404 returned). 
 
> Fabrice has suggested modifying window.navigator.mozApps.install() to accept
> a manifest object rather than a URL so that once we have the manifest data
> in Gaia, Gecko doesn't have to fetch it again in order to install an app
> into the app registry.

I don't know if it's a good idea to mix the proprietary stuff with the standardized stuff - specially as we made the decision to not include an imperative API for the W3C side of things. But I haven't thought about it too much.
 
> 1. http://w3c.github.io/manifest/#the-manifest.json-well-known-uri

Comment 9

[Ben Francis [:benfrancis]]

(In reply to Marcos Caceres [:marcosc] from comment #8)
> My concern is that this will lead to redundant requests; at least, until we
> work out how to actually apply the contents of a manifest to a web
> application. Currently, the manifest content is assumed to be applied after
> a subsequent restart of the web application - and requires some kind of
> "installation" step, even if that happens transparently.

The use case covered by this bug is specifically bookmarking an app to the homescreen, this patch will only fetch the manifest when the user explicitly requests it.

I'm not trying to solve the "progressive enhancement" to a browser window case here because I think that's still very loosely defined and UX aren't even sure if that's something we want in Firefox OS (it's under discussion). I'm currently working under the analogy of web apps as a "slice of the web" (defined by URL scope) which you separate out from the browser into its own app only when you install/bookmark the app. You can read more about that analogy in my blog post from last week if you're interested http://tola.me.uk/blog/2014/10/03/what-is-a-web-app/

> I would prefer we don't double up on effort here. If you go much further you
> will just end up reimplementing everything I've already implemented (i.e.,
> about 3 weeks worth of work). I think that would be a waste of resources
> given that I spent quite a bit of time making sure it matches the spec.
> 
> To make this happen, I've also allocated 95% of my time to just working on
> this. I'm trying my best to get up to speed with all the Gecko quirks.
> Again, I'm on IRC and happy to collaborate on anything in whatever way is
> most efficient - being new to the code base and flying mostly blind isn't
> doing wonders for my productivity, but I think I'm getting there:)

I don't have any intentions of duplicating the work you've done on parsing manifests, I'm working on prototyping the Gaia parts which means the UI presented to the user and hooking up the bookmarks datastore/app registry in Firefox OS to actually apply the manifest. Currently that requires fetching the manifest using XHR because only the manifest URL is exposed to content, but I can change that if the Web API changes.

I'm sorry I'm no help to you with the Gecko pieces.
  
> FWIW, I've already got support for .well-known/manifest in my
> implementation.

Having worked on code that has to deal with the "well known" /favicon.ico URL for icons in Firefox OS, and administered a web servers with logs full of 404s for /favicon.ico, I'm not a big fan of this part of the spec. But I missed the discussion around that so maybe there are good reasons for it.

> Again, I think fetching before you use the manifest leads to unnecessary
> requests.

We could either only fetch the manifest from the system app when the user initiates the bookmarking process, or optimise the platform to only fetch the manifest when a manifestchange listener is registered like Fabrice suggested. It doesn't necessarily result in unneeded requests.

> > Fabrice has suggested modifying window.navigator.mozApps.install() to accept
> > a manifest object rather than a URL so that once we have the manifest data
> > in Gaia, Gecko doesn't have to fetch it again in order to install an app
> > into the app registry.
> 
> I don't know if it's a good idea to mix the proprietary stuff with the
> standardized stuff - specially as we made the decision to not include an
> imperative API for the W3C side of things. But I haven't thought about it
> too much.

This is just for the system app to add an app to the app registry internally so that "standalone" apps defined by a web manifest get treated as apps rather than bookmarks in Firefox OS, based on suggestions from Fabrice and Jonas.

Jonas do you have a view on whether the Web API should include the manifest object in a manifestchange event or the system app should request it via iframe.getW3CManifest()? Or should we just leave it to the system app to fetch the manifest from a URL included in a manifestchange event?

Comment 10

[Ben Francis [:benfrancis]]

(In reply to Ben Francis [:benfrancis] from comment #9)
> it may be that for apps
> which specify a display mode [2] other than "browser", we want to call
> window.navigator.mozApps.install() with the manifest URL (or manifest file)
> rather than add a bookmark, so that we dynamically add an app into the app
> registry [3].

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1003883#c1

Comment 11

[Ben Francis [:benfrancis]]

(In reply to Marcos Caceres [:marcosc] from comment #5)
> Shouldn't we only be d/ling the manifest when it's needed? I'm working on
> stuff locally here to add the following to BrowserElementParent.jsm. It
> fetches/processes the manifest as per W3C and gives you back a JS object:
> 
> ```
>    _getW3CManifest(){ 
>     return new this._window.Promise((resolve,reject) => {
>       W3CManifestObtainer
>         .obtainManifest(this._window)
>         .then(resolve,reject);
>     });
>   },
> 
> ```

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1077620

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 8500641 [details] [review]
Proof of concept patch

Getting the manifest using an xhr from gaia will not guarantee that you fetch it with the correct load context. Eg if you had to authenticate yourself to a site, you won't have the http auth or cookies set and fetching the manifest will fail.

Comment 13

[Ben Francis [:benfrancis]]

That's a fair point, but does that not also apply to mozbrowsericonchange and mozbrowseropensearch? Both of those events provide a URL to Gaia which Gaia has to fetch over XHR, and would fail for an HTTP authenticated URL.

So how do you think this should be done? Should we add a getW3CManifest() method to the browser API as Marcos is suggesting in bug 1077620, or include the manifest data in the payload of the manifestchange event? Shouldn't whatever we do be made consistent with the other parts of the Browser API?

Comment 14

[[:fabrice] Fabrice Desré]

(In reply to Ben Francis [:benfrancis] from comment #13)
> That's a fair point, but does that not also apply to mozbrowsericonchange
> and mozbrowseropensearch? Both of those events provide a URL to Gaia which
> Gaia has to fetch over XHR, and would fail for an HTTP authenticated URL.
> 
> So how do you think this should be done? Should we add a getW3CManifest()
> method to the browser API as Marcos is suggesting in bug 1077620, or include
> the manifest data in the payload of the manifestchange event? Shouldn't
> whatever we do be made consistent with the other parts of the Browser API?

We should probably fix the api, yes. Remember that we added the download() method also for the same kind of reasons. For icons and search plugins we could have a generic getResource() to fetch data on behalf of the page.

Manifests may be a bit different if we want gecko to sanitize them. And if we expect to support w3c manifests in fx desktop / android, we'll need that code in gecko.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Fabrice Desré [:fabrice] from comment #14)
> We should probably fix the api, yes. Remember that we added the download()
> method also for the same kind of reasons. For icons and search plugins we
> could have a generic getResource() to fetch data on behalf of the page.

Yeah, I was thinking exactly this. It might be worth doing this as an addition to XMLHttpRequest so that we can reuse its features, like the ability to set headers and the ability to get the result as JSON/blob/arraybuffer.

So maybe something like:

browserElement.createXHR();
or
new XMLHttpRequest({ context: browserElement })

> Manifests may be a bit different if we want gecko to sanitize them. And if
> we expect to support w3c manifests in fx desktop / android, we'll need that
> code in gecko.

The network request still needs to use the cookie jar of the of the app. So I think we still need to use getResource() or some such to download the manifest.

As for sanitizing and interpreting the manifest (for example figure out which icon to display, or if a given URL is within scope/stay-in-app) we could write a JS library to do that. Such a library could then be used both by the system app, and by gecko.

Comment 16

[Ben Francis [:benfrancis]]

Thanks for the feedback! I suggest moving this discussion to bug 1003890 as it applies to web manifests more broadly.

Comment 17

[Marcos Caceres [:marcosc]]

Comment on attachment 8500641 [details] [review]
Proof of concept patch

I don't have any significant comments on the code. Tho, I'm looking forward to seeing what comes out of this.

Comment 18

[Ben Francis [:benfrancis]]

Attachment: WIP Patch

New work in progress patch. Adds a bookmark for "browser" display mode and installs an app for all other valid display modes.

Can be tested with the test apps at http://people.mozilla.org/~bfrancis/test_apps/ but depends on the Gecko patch in bug 1075716 to install an app with a manifest served with the Web Manifest MIME type.

Comment 19

[Ben Francis [:benfrancis]]

Attachment: Mockup of web app installation

Francis, I'm looking for some early UX feedback on this because discussions in bug 1003890 indicate that the UI design we choose may influence the way the required platform features are implemented.

The attached mockup shows roughly the effect that the change in this patch would have. If a browser window overflow menu is opened on a web page which provides a web manifest, an additional option appears.

In the mockup I've called this option "Install App" and renamed "Add to homescreen" to "Bookmark Page" to differentiate the two actions. One alternative is that we use the terms "Add page to homescreen" and "Add app to homescreen" but that might be a bit long winded.

The idea here is that if a web page specifies a web manifest, we can provide the option to install the web app directly from the page. This would install an app with the name provided in the manifest which opens in an app window, as opposed to bookmarking the page which would use the page title and open in a browser window.

Note that a web manifest specifies a display mode. Only the "fullscreen", "minimal-ui" or "standalone" display modes will result in an app being installed. The "browser" display mode will just add a bookmark which opens in a normal browser window.

I'm looking for feedback on the basic idea of an extra option in the overflow menu to install an app, which will only appear if the current page specifies a web manifest.

Comment 20

[Kevin Grandon :kgrandon]

Why should we differentiate the options at all? Is there still a use-case for the current "bookmarking" flow if the site has a web manifest?

Comment 21

[Ben Francis [:benfrancis]]

(In reply to Kevin Grandon :kgrandon from comment #20)
> Why should we differentiate the options at all? Is there still a use-case
> for the current "bookmarking" flow if the site has a web manifest?

Yes, I think there is. Bookmarking allows deep linking to a page inside an app.

For example, say I navigated to a news article at http://www.bbc.co.uk/news/technology-27793464

"Bookmark Page" would add a bookmark to the news article with the article title and page icon which would open in a browser window, whereas "Install App" would add an app launcher for the BBC News app at the start_url of http://www.bbc.co.uk/news/ which could open in an app window separate from the browser. If both were added then the article bookmark could in future link directly to that article inside the app using App Scope.

See: https://github.com/w3c/manifest/issues/167

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree with Ben, we definitely need to allow deep-linking into an app.

The tricky scenario that I can think of is what to do if the user creates a deep-link into an app, but doesn't have the app installed. I.e. what to do if the user's at a page which links to a manifest and then chooses "bookmark this page" without having ever "installed" the manifest.

Do we at that point create a new "app", but make that the homescreen icon just not launch the startURL of that app? Or do we just create something which opens in the browser when clicked?

We might want to visually separate "bookmarks" from "apps". That would make it more clear to the user what does and does not show up in the permissions manager and other places where we enable users to configure apps.

Comment 23

[Kevin Grandon :kgrandon]

I think users are definitely going to be confused with the different options, especially our target market. I think you should only have a single option, and that option should be to add the current page to your home screen. If it has a web manifest, we could install and deep link to it.

Comment 24

[Ben Francis [:benfrancis]]

(In reply to Jonas Sicking (:sicking) from comment #22)
> The tricky scenario that I can think of is what to do if the user creates a
> deep-link into an app, but doesn't have the app installed. I.e. what to do
> if the user's at a page which links to a manifest and then chooses "bookmark
> this page" without having ever "installed" the manifest.

I think this is fine. If the bookmarked URL falls within the scope of an installed app, it opens in the app, otherwise it opens in the browser. If the user later installs the app to indicate they want to use this slice of the web separately from the browser, then the bookmark will start to open inside the app.
 
> Do we at that point create a new "app", but make that the homescreen icon
> just not launch the startURL of that app? Or do we just create something
> which opens in the browser when clicked?

I think the latter. We shouldn't assume that because the user is bookmarking a BBC News article that they want to install the BBC app as well.

> We might want to visually separate "bookmarks" from "apps". That would make
> it more clear to the user what does and does not show up in the permissions
> manager and other places where we enable users to configure apps.

I agree we should consider visually differentiating bookmarks from app launchers. I think this is similar to how a Word document has a different icon to the Word application in Windows. The Word document is a piece of content which opens in the Word application.

(In reply to Kevin Grandon :kgrandon from comment #23)
> I think users are definitely going to be confused with the different
> options, especially our target market. 

I originally had the same view as you. But after spending a *lot* of time prototyping and thinking about this I've come to the conclusion that apps and bookmarks are actually different and complimentary things. I believe we can build a UX around this which users won't find confusing, but let's let UX chime in on that.

> I think you should only have a single
> option, and that option should be to add the current page to your home
> screen. If it has a web manifest, we could install and deep link to it.

Like I said, I don't think we should assume that just because someone is bookmarking a web page, they want to install the whole app.

The conclusion I have reached is that bookmarks are a shortcut to a web page at a particular URL on the web which may fall within the scope of an installed app, or it may open in the browser.

If a bookmark is a marker for a single page in a book, then the app is the book.


Renaming this bug to reflect the scope of the patch.

Comment 26

[Ben Francis [:benfrancis]]

Attachment: Proposed basic UX spec from Francis

Comment 27

[Ben Francis [:benfrancis]]

Attachment: WIP Patch

I have created an updated prototype based on Francis' UX spec.

Comment 28

[Francis Djabri [:djabber]]

Updated UX spec (version 0.2) available at: https://mozilla.box.com/s/bi1k1eg3delrtgd9o5hs

Comment 29

[Francis Djabri [:djabber]]

Comment on attachment 8509561 [details]
Mockup of web app installation

Hi Ben, I agree with Kevin that we should keep a single option, but allow some choice with the page is being added to the homescreen. The latest spec is available at: Updated UX spec (version 0.2) available at: https://mozilla.box.com/s/bi1k1eg3delrtgd9o5hs

Comment 30

[GitHub Autolander Bot]

Attachment: [PullReq] benfrancis:1003876-4 to mozilla-b2g:master

Comment 31

[Ben Francis [:benfrancis]]

Added a new work in progress patch.

Comment 32

[Ben Francis [:benfrancis]]

Eric, I'm implementing the Web Manifest spec and the "Add" button in the add to homescreen dialog in the visual spec is *really* small. Does this meet our minimum touch target size? I wonder if we should make it bigger, and preferably not use images for the buttons as it makes their size inflexible for localisation?

Also, any idea whether we should use short_name or name of the app by default in this view?

Comment 33

[GitHub Autolander Bot]

Attachment: [PullReq] benfrancis:1003876 to mozilla-b2g:master

Comment 34

[Ben Francis [:benfrancis]]

Comment on attachment 8559355 [details] [review]
[PullReq] benfrancis:1003876-4 to mozilla-b2g:master

Updated patch, hopefully just needs tests. The only add to homescreen integration tests I've been able to find are in the verticalhome app.

Comment 35

[Eric Pang [:epang] UX]

(In reply to Ben Francis [:benfrancis] from comment #32)
> Eric, I'm implementing the Web Manifest spec and the "Add" button in the add
> to homescreen dialog in the visual spec is *really* small. Does this meet
> our minimum touch target size? I wonder if we should make it bigger, and
> preferably not use images for the buttons as it makes their size inflexible
> for localisation?
> 
> Also, any idea whether we should use short_name or name of the app by
> default in this view?

Hey Ben,

I've updated the visual spec to use the button from the building blocks.  I've also shifted a few elements around.
https://mozilla.box.com/s/fnuq2vthajkvodwemrq1

Let me know if you have any questions.

Thanks!

Comment 36

[Ben Francis [:benfrancis]]

Thanks Eric, updating user story with updated spec, patch to follow shortly.

Comment 37

[Ben Francis [:benfrancis]]

Comment on attachment 8559355 [details] [review]
[PullReq] benfrancis:1003876-4 to mozilla-b2g:master

Updated to match new visual spec

Comment 38

[Ben Francis [:benfrancis]]

Comment on attachment 8559355 [details] [review]
[PullReq] benfrancis:1003876-4 to mozilla-b2g:master

Hi Cristian,

Would you be able to review this? It's the first patch for the implementation of the 2.2 Web Manifest spec (see the links in the user story).

This basically detects a <link rel=manifest"> link relation in a web page and displays an extra bit of UI in the "Add to Home" dialog on pages with a Web Manifest which allows the user to install the app from the page.

There will be follow-on patches to add the icon to the dialog and add support to the system app for some of the W3C web app manifest properties.

I notice that there aren't any integration tests in the bookmark app folder, where would you advise adding integration tests for this change?

Comment 39

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8559355 [details] [review]
[PullReq] benfrancis:1003876-4 to mozilla-b2g:master

LGTM, good job. I reviewed the bookmark code and seems excellent

Comment 40

[Cristian Rodriguez (:crdlc)]

> I notice that there aren't any integration tests in the bookmark app folder, where would you advise adding integration tests for this change?

Honestly I don't remember if we did some integration test or not here when it was implemented :) I would suggest to do this perhaps in bookmarks app although there are four files here https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/test/marionette/ related to bookmarks. Maybe you can take advantage of the code already done in vertical home or after checking my first commits there were integration tests in the legacy home https://github.com/mozilla-b2g/gaia/commit/5960beb98780d70ea409c5397924ddc89a048cdb

Comment 41

[Cristian Rodriguez (:crdlc)]

Left some comments in GH although nothing really important

Comment 42

[Ben Francis [:benfrancis]]

Thanks Cristian, I've addressed the review comments, rebased against master since there were conflicts with Kevin's private browsing patch and I've added an integration test.

I wasn't able to test everything I wanted to test in the integration test because I need Marionette's HTTP server to return the manifest file with the correct Content-Type until bug 1091786 is fixed.

James, do you know if it's possible to get the HTTP server Marionette uses to set the Content-Type of a response header for a served resource?

Waiting to see Treeherder results because I think I may have broken the linter tests.

Comment 43

[Cristian Rodriguez (:crdlc)]

(In reply to Ben Francis [:benfrancis] from comment #42)
> Thanks Cristian, I've addressed the review comments, rebased against master
> since there were conflicts with Kevin's private browsing patch and I've
> added an integration test.
> 
> I wasn't able to test everything I wanted to test in the integration test
> because I need Marionette's HTTP server to return the manifest file with the
> correct Content-Type until bug 1091786 is fixed.
> 
> James, do you know if it's possible to get the HTTP server Marionette uses
> to set the Content-Type of a response header for a served resource?
> 
> Waiting to see Treeherder results because I think I may have broken the
> linter tests.

Thanks to you Ben for your excellent work

Comment 44

[Cristian Rodriguez (:crdlc)]

Left a comment about removing the listener and binding, the rest is perfect, I finished the review, go ahead for me. Thanks a lot Ben!

Comment 45

[Ben Francis [:benfrancis]]

(In reply to Cristian Rodriguez (:crdlc) from comment #44)
> Left a comment about removing the listener and binding, the rest is perfect,
> I finished the review, go ahead for me. Thanks a lot Ben!

Done, and fixed linter tests.

Waiting for green from Treeherder.

Comment 46

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/189d6b3c4e44cfd28a92ce7dee4e06739f479feb

Comment 48

[Hermes Cheng (inactive after July 27, 2015)]

Is the patch going to uplift to v2.2?

Comment 49

[James Lal [:lightsofapollo]]

https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/test/marionette/server/child.js#L208 (this is for vertialhome and the one I am familiar with) should be very straightforward to edit the headers as needed.

Comment 50

[Ben Francis [:benfrancis]]

(In reply to Hermes Cheng[:hermescheng] from comment #48)
> Is the patch going to uplift to v2.2?

No, it doesn't look like it.

(In reply to James Lal [:lightsofapollo] from comment #49)
> https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/test/
> marionette/server/child.js#L208 (this is for vertialhome and the one I am
> familiar with) should be very straightforward to edit the headers as needed.

Thanks James