Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion - week of Aug 11, 2014 #61

Open
ezoehunt opened this issue Aug 13, 2014 · 13 comments
Open

Discussion - week of Aug 11, 2014 #61

ezoehunt opened this issue Aug 13, 2014 · 13 comments

Comments

@ezoehunt
Copy link
Member

This is where we'll discuss this week.

@eviljeff
Copy link

Copying my follow-ups to Bram's email on July 24 here as they don't appear to have been seen.

1 . Sample Manifest file (https://github.com/ezoehunt/manifest_testing/blob/gh-pages/sample_manifest.webapp) - have we covered the fields we need?

just missing 'role' property afaik.

2 . Sample Config file (https://github.com/brampitoyo/sample-marketplace-app/blob/gh-pages/CORRECT_ANSWERS_FOR_marketplace.config) - have we covered the fields we need? The config file should contain only marketplace-specific fields.

  • I think 'countries' should be 'regions'
  • you should indicate how 'rest-of-world' region is included and how to indicate that you want new regions auto added.
  • platforms is a current requirement
  • paid_upgrade_of : having this as free format text seems difficult to parse. Should it be a marketplace /slug instead?
  • feature buchets : not sure how we should handle these. Some can't be detected from the app and have to be manually set, whereas others could be detected from the app as now and are set by default but how would the developer override?

How should questions around app visibility on approval be asked (currently whether the app should be auto published). Its not strictly an app detail question but is something that would normally be asked on submission.

3 . Do the instructions for the Manifest file (http://ezoehunt.github.io/manifest_testing/mdn_instructions.html) make sense for the fields?

Is this a replacement for the existing Manifest page on MDN? (https://developer.mozilla.org/Apps/Build/Manifest) If not I'm concerned about us duplicating information.

Some comments:

  • .webapp isn't a requirement for hosted apps - should mention content type requirement somewhere for hosted apps.
  • for packaged apps the exact name manifest.webapp is a requirement
  • location doesn't need to be in the app's root directory for hosted apps.
  • I don't know what 'Internal paths also must be served from the same origin as the app.' means. (how can you have an internal path from a different origin?)
  • Don't encourage external paths for icons for packaged apps (I'm not sure if even works).
  • type mentions Certified apps in the 'submitting to Firefox Marketplace' section but you can't submit Certified apps to Marketplace.
  • default_locale (if locales is defined) is a requirement for Generic Open Web Apps also.
  • launch_path for packaged apps is a requirement for Generic Open Web Apps also.
  • icons aren't a requirement for Generic Open Web Apps
  • permissions don't just apply to Packaged Apps.
  • in 'origin' detail section mention is made to 'a single domain name': using the term 'domain' is confusing for packaged apps.
  • appcache is redundant for packaged apps (though I believe still available)

Was the detail section just for Marketplace Requirements? I wasn't sure. If its supposed to be for Generic Open Web apps too then square icons is a Marketplace requirement. Also, not having a solid background (and the other guidelines) are Marketplace recommendations (not requirements)

@operatorjen
Copy link

you should also take a look at the rules we've made on the node module but consolidated in scanJS with the security team https://github.com/cr/Foxalyzer/blob/master/rules/manifest.json - there are some special edge cases for certain properties

@ezoehunt
Copy link
Member Author

Thanks Jen and Andrew. I have a bunch of questions about dependencies between fields, so will follow up with you both directly as we develop the Manifest Builder.

@brampitoyo
Copy link

I had a look at Foxalyzer’s rules and noted a couple of things:

  • appcache_path seems to only apply to packaged apps. In our documentation, it applies to every type. We could change our documentation and builder to match.
  • There’s an error message that says “default_locale must match one of the keys in locales”. In our documentation and understanding, the language declared under default_locale must only be declared once, on the parent level, and cannot be declared again under locales. So this error message sounds odd to me?

@eviljeff
Copy link

appcache_path applies to both packaged and hosted apps. It only really makes sense for hosted apps though, even though it is available to packaged apps (there is no reason to cache something that is already locally available)

@eviljeff
Copy link

@brampitoyo your understanding is correct; the error message in foxalyzer's rules is wrong (and possibly the test). default_locale indicates what locale the manifest fields outside of the locales block are written in.

@eviljeff
Copy link

@ednapiranha should I comment on the foxalyzer issues there or here/with cr?

@ezoehunt
Copy link
Member Author

Question about launch_path:
MDN says it's required for packaged apps.
Does it make sense for hosted apps? What value/benefit does it provide for the developer of a hosted app?
Thanks.

@eviljeff
Copy link

@ezoehunt yes, launch_path is very much used for hosted apps - it allows an app to define where it is on the website. Without it every webapp would have to be effectively index.html in the root of the website.

@operatorjen
Copy link

@eviljeff you can file the bugs here https://github.com/mozilla/node-firefox-app-validator-manifest/ .. is it verified that the default_locale spec in https://github.com/mozilla/app-validator/blob/master/appvalidator is also correct? I may have misunderstood the restriction of default_locale on there but let me know.

@eviljeff
Copy link

@ednapiranha the spec is correct in https://github.com/mozilla/app-validator/blob/master/appvalidator/specs/webapps.py#L121 but only the rule about default_locale being mandatory if locales exists is enforced; that the default_locale shouldn't also exist in locales isn't enforced as far as I can see.

@operatorjen
Copy link

@eviljeff updated the default_locale rule here mozilla/firefox-app-validator-manifest@72991c2

this would state that 'if locales is provided, then default_locale is mandatory'. is this correct?

@brampitoyo
Copy link

Yes. That’s correct. default_locale must contain a value if locales contain a value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants