856799 - Please add the packaged marketplace to gaia

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Wil Clouser [:clouserw]]

Some work was already done via IRC, I think ending up with this commit: https://github.com/cvan/gaia/commit/1c92309699bfe198117b09a497cfe7a7f3f020a8

I don't know if it ended up in a pull request or not.  After that we'll need to adjust some documentation so carriers know how to customize the URL.

Let me know what else I can provide.  Thanks.

Comment 1

[Jason Smith [:jsmith]]

Needed for 1.0 based on what I've been told. Noming as such.

Ask Wil for details on why if need be.

Comment 2

[Jason Smith [:jsmith]]

FWIW, we should stagger landings on this bug such that:

1. Land this on master & v1-train first and wait a few days to make sure nothing blows up

2. Then land on 1.01

Comment 3

[[:fabrice] Fabrice Desré]

The zip has a harcoded link to https://marketplace.firefox.com/?carrier=telefonica for the iframe. I can't comment directly on github because it's inside a binary file, but this need to be done differently:
- use getSelf() to get the app object.
- use the manifestURL to get the carrier name if any
- add the carrier name to the iframe if needed.

Comment 4

[Christopher Van Wiemeersch [:cvan]]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> The zip has a harcoded link to
> https://marketplace.firefox.com/?carrier=telefonica for the iframe.

Thanks - updated: https://github.com/cvan/gaia/compare/packaged-marketplace

Comment 5

[Daniel Coloma:dcoloma]

What are the reasons for blocking on this?

CAn we land it in master first and then nominate this again as per comment 2?

Comment 6

[Matt Basta [:basta]]

If the packaged app doesn't land, then users will have a deprecated version of the Marketplace on their devices. We will be shutting off the current Marketplace as it exists now within a few months. Any device still pointing at it will be irreparably emborkened.

Comment 7

[Wil Clouser [:clouserw]]

This is to prevent further work in the future.  There is no upgrade path from a hosted app to a packaged app and since the Marketplace needs to be a packaged app to take advantage of privileged APIs it makes sense to get it packaged in the first version that ships.

Fabrice or Jason could answer your question about landing in master and nominating, he knows the flows better than I do.

Comment 8

[Jason Smith [:jsmith]]

We could blocking+ this but just indicate in the status flags to not to uplift to b2g 1.01 on immediate landing, wait a few days, then switch the flags to allow landing.

Comment 9

[Daniel Coloma:dcoloma]

(In reply to Jason Smith [:jsmith] from comment #8)
> We could blocking+ this but just indicate in the status flags to not to
> uplift to b2g 1.01 on immediate landing, wait a few days, then switch the
> flags to allow landing.

tef+ is then. I leave up to you setting up the right flags

Comment 10

[Jason Smith [:jsmith]]

Flags set for initial landing.

Comment 11

[Alex Keybl [:akeybl]]

David - can you help us assign this bug?

Comment 12

[Christopher Van Wiemeersch [:cvan]]

r? https://github.com/mozilla-b2g/gaia/pull/8973

If there's anything else that needs to be done, please let me know. Thanks much!

Comment 13

[Jason Smith [:jsmith]]

Attachment: Pointer to Github Pull Request: https://github.com/mozilla-b2g/gaia/pull/8973

Comment 14

[Jason Smith [:jsmith]]

Comment on attachment 733196 [details]
Pointer to Github Pull Request: https://github.com/mozilla-b2g/gaia/pull/8973

Took a quick look. review- for missing etags. Comments in github for stuff that's in the diff there.

For the zip file, comments below:

      * {
        margin: 0;
        overflow: hidden;
        padding: 0;
      }

What's that star doing in the style tag?

Comment 15

[Matt Basta [:basta]]

(In reply to Jason Smith [:jsmith] from comment #14)
> Comment on attachment 733196 [details]
> Pointer to Github Pull Request: https://github.com/mozilla-b2g/gaia/pull/8973
> 
> Took a quick look. review- for missing etags. Comments in github for stuff
> that's in the diff there.

Where is the documentation for this?

> What's that star doing in the style tag?

Per the CSS 2.1 spec:

    The universal selector, written "*", matches the name of any
    element type. It matches any single element in the document tree.

You can read more, if interested, at http://www.w3.org/TR/CSS2/selector.html#universal-selector

We're matching every element in the app and removing margin, padding, and hiding overflow (to prevent scrollbars and weird scrolling).

Comment 16

[Jason Smith [:jsmith]]

(In reply to Matt Basta [:basta] from comment #15)
> (In reply to Jason Smith [:jsmith] from comment #14)
> > Comment on attachment 733196 [details]
> > Pointer to Github Pull Request: https://github.com/mozilla-b2g/gaia/pull/8973
> > 
> > Took a quick look. review- for missing etags. Comments in github for stuff
> > that's in the diff there.
> 
> Where is the documentation for this?

https://bugzilla.mozilla.org/show_bug.cgi?id=820630 and https://github.com/mozilla-b2g/gaia/blob/master/build/webapp-manifests.js#L158 is the best I have for info on this (not documented, sadly). In short, you need to specify:

etag <--- this is the manifest etag
packageEtag <--- this is the packaged app etag

If the package etag isn't specified, then a user will always get prompted for an app update immediately upon starting usage of the phone.

Comment 17

[Matt Basta [:basta]]

(In reply to Jason Smith [:jsmith] from comment #16)
> If the package etag isn't specified, then a user will always get prompted
> for an app update immediately upon starting usage of the phone.

I can't say that I'd be displeased with this behavior, since that'll probably happen anyway.

Comment 18

[Jason Smith [:jsmith]]

(In reply to Matt Basta [:basta] from comment #17)
> (In reply to Jason Smith [:jsmith] from comment #16)
> > If the package etag isn't specified, then a user will always get prompted
> > for an app update immediately upon starting usage of the phone.
> 
> I can't say that I'd be displeased with this behavior, since that'll
> probably happen anyway.

True, although dogfooders will probably get noisy with this behavior (they already have for maps not declaring those etags, for example).

Comment 19

[Julien Wajsberg [:julienw]]

giving it back to :cvan ;)

Chris, please ping me if you need any help !

Comment 20

[Julien Wajsberg [:julienw]]

I've added this on the PR, but saying it here too: the file applications-data.js must be changed too because you changed the marketplace directory name.

Comment 21

[Christopher Van Wiemeersch [:cvan]]

Attachment: Pointer to Github Pull Request: https://github.com/mozilla-b2g/gaia/pull/8973

Everything working with fresh `make clean profile`

Comment 22

[Jason Smith [:jsmith]]

Comment on attachment 733954 [details]
Pointer to Github Pull Request: https://github.com/mozilla-b2g/gaia/pull/8973

Package app looks okay to me from the QA side.

Julien can handle the rest of the review pieces for formal landing.

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 733954 [details]
Pointer to Github Pull Request: https://github.com/mozilla-b2g/gaia/pull/8973

r=me with packageEtag either removed or modified to reflect a real value (which AFAIK it doesn't right now)

Comment 24

[Jason Smith [:jsmith]]

Comment on attachment 733954 [details]
Pointer to Github Pull Request: https://github.com/mozilla-b2g/gaia/pull/8973

I'm still questioning the etags here that are being used. As it stands right now, I would not check this in without resolving etag problem. r- from me.

Comment 25

[Matt Basta [:basta]]

Slightly off-topic, but is it indeed the case that etags are necessary on all packaged apps for all developers? If so, we haven't communicated this at all and documentation needs to be updated. Can you please post a link to information about etag requirements and how they need to be used?

Comment 26

[Julien Wajsberg [:julienw]]

This is not necessary, but this is the only current way to prevent downloading of an app that didn't change. ie we don't support If-Modified-Since. See Bug 823648 and especially Bug 823648 comment 7 for a possible solution.

If you don't specific an ETag, we download the file and then we compute a hash that will be compared to the previous stored hash, so that we don't reinstall the app if it hasn't changed. I think we don't support setting a hash for a preinstalled app.

Comment 27

[Matt Basta [:basta]]

Is nothing in the minifest (version, for instance) considered for determining when to update?

Comment 28

[Julien Wajsberg [:julienw]]

Matt> yes of course, we're not trying to download the package unless the minifest changed. Which uses the same mechanism: ETag or hash. But if the minifest changes (for example: developer or description changed) but not the package, we don't want to redownload the package.

Comment 29

[Christopher Van Wiemeersch [:cvan]]

Julien, thanks for the review - are we good here?

Comment 30

[Matt Basta [:basta]]

(In reply to Julien Wajsberg [:julienw] from comment #28)
> Matt> yes of course, we're not trying to download the package unless the
> minifest changed. Which uses the same mechanism: ETag or hash. But if the
> minifest changes (for example: developer or description changed) but not the
> package, we don't want to redownload the package.

If the Marketplace promises not to change the minifest unless there's an update, can we get by without the etags in the short term so this can land? We have no intention to change the package (yulelog, as we call it) for at least a few weeks, and at that point we'll have infrastructure stood up to serve packages in a better way. Is that an agreeable compromise?

Comment 31

[Christopher Van Wiemeersch [:cvan]]

(In reply to Matt Basta [:basta] from comment #30)
> (In reply to Julien Wajsberg [:julienw] from comment #28)
> > Matt> yes of course, we're not trying to download the package unless the
> > minifest changed. Which uses the same mechanism: ETag or hash. But if the
> > minifest changes (for example: developer or description changed) but not the
> > package, we don't want to redownload the package.
> 
> If the Marketplace promises not to change the minifest unless there's an
> update, can we get by without the etags in the short term so this can land?

As of today in production, there are no ETag headers on the .zip - but I have since changed this on -dev and that change will roll out to production tomorrow at 2:00 PDT.

(z)(Chris-MacBook-Pro.local) %% curl -I https://marketplace-dev.allizom.org/package.zip
HTTP/1.1 200 OK
Server: gunicorn/0.15.0
Vary: X-Requested-With, Accept-Language, Cookie, X-Mobile, User-Agent
X-Backend-Server: dev1.addons.phx1.mozilla.com
Content-Type: application/zip
Access-Control-Expose-Headers: X-API-Version, X-API-Status
Strict-Transport-Security: max-age=2592000
Date: Wed, 10 Apr 2013 18:35:50 GMT
Transfer-Encoding: chunked
ETag: "9be1d52a14b87fdf18ab00b4cdebdedc"

Comment 32

[Julien Wajsberg [:julienw]]

Matt: that was my proposition :) but now I think we should just use the Etag that will be used. so that could land as is.

anyway as long as we don't break the build, we can still make adjustments later if necessary.

Comment 33

[Jason Smith [:jsmith]]

Comment on attachment 733954 [details]
Pointer to Github Pull Request: https://github.com/mozilla-b2g/gaia/pull/8973

looks good now per discussion on the etags piece.

We're good to land then, right?

Comment 34

[Julien Wajsberg [:julienw]]

master: https://github.com/mozilla-b2g/gaia/commit/9b1fa182e4ac13643326394efeda26e7d4e98145

Comment 35

[Julien Wajsberg [:julienw]]

v1-train: e439fb1b7a5bcd341edf28fb66ca2b3164e55d2b

Comment 36

[Julien Wajsberg [:julienw]]

will wait before landing on v1.0.1 as was requested.

Comment 37

[Jason Smith [:jsmith]]

Nothing outrageously blew up, so good to land on next branch.

Comment 38

[John Ford [:jhford] CET/CEST Berlin Time]

v1.0.1: b1855ee0a4e1e6ed3185d9d5cf5e23e8f6d3d82d

Comment 39

[Cody Roesch [:croesch]]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 40

[Jason Smith [:jsmith]]

It's automatically verified - this is part of the build and we use marketplace in our smoke tests daily.