the most recent update adds a /redirects/ manifest entry to log in via evernote. Unfortunately this isn't working. After providing evernote login details and authentificating an error is shown because the page 'http://ffos-notes.local/redirect.html' is loaded, rather than /redirect.html. I'm installing via the reviewer tools so this is a signed package installed properly, rather than sideloaded, etc.

Hey ochameau, as this looks very similar to bug 889356, maybe you can have a look. I only saw redirects working in preloaded gaia apps; maybe something in the webapps source is not picking them up on install? Or is this even simpler and might be fixed by the patch from bug 889356.

That's a bug in the app. http://ffos-notes.local/redirect.html isn't a valid URL. The from part of a redirect needs to be a valid URL that reports a 302.

(In reply to Jason Smith [:jsmith] from comment #3) > That's a bug in the app. http://ffos-notes.local/redirect.html isn't a valid > URL. The from part of a redirect needs to be a valid URL that reports a 302. Oh, okay. I read the manifest docs on MDN as meaning that the url (http://ffos-notes.local/redirect.html) had to be /requested/ via a 302 from the evernote website. What Location does the from URL have to report a redirect to?

I'm going to let fabrice clarify more of details here. I need to dig into how this a little more myself.

The `from` field has to be the redirection target, not the redirection source, so let say evernote.com/signed-in/ redirect to mozilla.org/notes/, the `from` field being set to http://mozilla.org/notes/, the final redirection of evernotes.com/signed-in will be replaced by the value of `to` field. I manually tested that feature with the following test case in bug 889356 comment 26. But only when sideloading the app, with the required fix. I haven't tried via the marketplace.

Ah, comment 6 is right, I think I misinterpreted something here. It isn't the URL reporting the 302, it's the URL target that you 302 sends you to should be set in the from field. I do wonder if the URL has to be a valid source, however.

Putting needinfo on me to investigate.

so I guess its either: a) from needs to be a valid url b) evernote isn't using a 302 redirect c) the implementation on fxos is broken

okay, I took the test case that ochameau provided; finished and uploaded it to Marketplace: https://marketplace.firefox.com/reviewers/apps/review/redirected - it fails when installed via Marketplace. Assuming the server config is still correct then I'm leaning towards c) in #comment #9

Can you check that this is a privileged app? redirects are not usable by non-privileged apps. Also, I'd like to see the webapps.json file (in /data/local/webapps/webapps.json on a device).

Checked this myself - I can confirm what Andrew is talking about. This privileged app redirects property works when sideloaded, but fails when installed from marketplace. This definitely looks like a bug on the DOM Apps side.

We were not sending the updated app object to child processes after downloading the package and reading the redirects in the manifest. So if you start the app right after installing it, it uses the preallocated process that has an outdated version.

Comment on attachment 779941 [details] [diff] [review] patch Review of attachment 779941 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/AppsService.js @@ +110,5 @@ > return Services.io.newURI(to, null, null); > } > } > + // No matching redirect. > + return null; nit: I think you can just use the |return null;| at line 116 removing the else. ::: dom/apps/src/Webapps.jsm @@ +2004,5 @@ > app.downloading = false; > app.downloadAvailable = false; > this._saveApps((function() { > this.updateAppHandlers(null, aManifest, appObject); > + this.broadcastMessage("Webapps:AddApp", { id: aId, app: appObject }); Shouldn't we update and notify with 'app' [1] instead? Like: this.updateAppHandlers(null, aManifest, app); this.broadcastMessage("Webapps:AddApp", { id: aId, app: app }); [1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#2088

Comment on attachment 779941 [details] [diff] [review] patch Review of attachment 779941 [details] [diff] [review]: ----------------------------------------------------------------- This was clarified via IRC

Has this been merged into 1.0.1 and 1.1 branches also?

(In reply to Andrew Williamson [:eviljeff] from comment #18) > Has this been merged into 1.0.1 and 1.1 branches also? leo+, will be merged soon.

(In reply to Alex Keybl [:akeybl] from comment #19) > (In reply to Andrew Williamson [:eviljeff] from comment #18) > > Has this been merged into 1.0.1 and 1.1 branches also? > > leo+, will be merged soon. ... is that a no?

It's a "not yet"

Note to Andrew - this is being fixed in 1.1, not 1.01.

(In reply to Jason Smith [:jsmith] from comment #22) > Note to Andrew - this is being fixed in 1.1, not 1.01. okay. If we approve the update now then everyone on 1.0.1 will get a broken app. So generally we won't approve such an update/submission until 1.1 is pushed to everyone on 1.0.1. Although in this case the app is broken anyway due to evernote changes so we'll probably approve it regardless.

The redirected sanity test is passing now. So marking as verified.