Implement the part of the W3C manifest spec that just deals with processing a JSON file.
Created attachment 8505045 [details] [diff] [review] Implementation of manifest processing +test suite
Sorry, I started this review today but didn't get very far. I need to ask you some questions in person to understand the flow of this code. Will do that tomorrow.
This patch adds a number of developer facing warnings that will be reported to the Web Console, and they're not localized. Axel, is that acceptable or do we need to put the strings in a .properties file in case our localizers are interested in localizing them?
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #4) > This patch adds a number of developer facing warnings that will be reported > to the Web Console, and they're not localized. Axel, is that acceptable or > do we need to put the strings in a .properties file in case our localizers > are interested in localizing them? Since they're exposed to users in the UI, even if hidden in the Web Console, it would be correct to make them localizable.
Created attachment 8510486 [details] [diff] [review] Updated files based on review. Updated implementation, test suite, based on review.
Created attachment 8510515 [details] [diff] [review] Updated patch... other one was incorrect.
3 years ago
Marcos told me that he will deal with the l10n issue in bug 1086997, so this is good to go!
3 years ago
To the updated patch, I just fixed a few double-quotes that should have been single quotes. Ehsan, if you have time, I need help merging this as I don't have the right public key on this machine and I'm away at TPAC :(
Created attachment 8511577 [details] [diff] [review] 0001-Bug-1079453-Implement-the-manifest-processor-part-of.patch Mounir tried the patch and it didn't apply cleanly, so I generated another.
Sent to try: https://tbpl.mozilla.org/?tree=Try&rev=fd513aac12cf
Created attachment 8511579 [details] [diff] [review] 0001-Bug-1079453-Implement-the-manifest-processor-part-of.patch Seems something changed with SpecialPowers as it no longer unwraps arrays properly. This is affecting some icons tests. I fixed a test to just check the length.
Mounir didn't apply the patch properly ^_^
There is something wrong with this patch. When I try to apply it, I get: patching file dom/manifest/ManifestProcessor.jsm patching file dom/manifest/moz.build patching file dom/manifest/test/common.js patching file dom/manifest/test/mochitest.ini patching file dom/manifest/test/test_IconsProcessor_density.html patching file dom/manifest/test/test_IconsProcessor_sizes.html patching file dom/manifest/test/test_IconsProcessor_src.html patching file dom/manifest/test/test_IconsProcessor_type.html patching file dom/manifest/test/test_ManifestProcessor_JSON.html patching file dom/manifest/test/test_ManifestProcessor_display.html patching file dom/manifest/test/test_ManifestProcessor_icons.html adding dom/manifest/ManifestProcessor.jsm adding dom/manifest/moz.build adding dom/manifest/test/common.js adding dom/manifest/test/mochitest.ini adding dom/manifest/test/test_IconsProcessor_density.html adding dom/manifest/test/test_IconsProcessor_sizes.html adding dom/manifest/test/test_IconsProcessor_src.html adding dom/manifest/test/test_IconsProcessor_type.html adding dom/manifest/test/test_ManifestProcessor_JSON.html adding dom/manifest/test/test_ManifestProcessor_display.html bad hunk #1 @@ -0,0 +1,30 @@ (1 0 30 30) dom/manifest/ManifestProcessor.jsm dom/manifest/moz.build dom/manifest/test/common.js dom/manifest/test/mochitest.ini dom/manifest/test/test_IconsProcessor_density.html dom/manifest/test/test_IconsProcessor_sizes.html dom/manifest/test/test_IconsProcessor_src.html dom/manifest/test/test_IconsProcessor_type.html dom/manifest/test/test_ManifestProcessor_JSON.html dom/manifest/test/test_ManifestProcessor_display.html patch failed, rejects left in working dir errors during apply, please fix and refresh attachment.cgi?id=8511579
Created attachment 8514475 [details] [diff] [review] 0001-1079453-Implement-the-manifest-processor-part-of-W3C.patch Strange... hopefully this one fixes that.
Sent to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f14dc72fd74b The patch applied correctly that time :)
Marcos patch had some info from another patch included, new try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9a211f301486
\o/ Thanks for your help, Mounir!
Why is process() taking a stringified version of the manifest as input? The most likely use case is to fetch the resource with xhr and directly get a json object by setting the xhr response type properly.
(In reply to Fabrice Desré [:fabrice] from comment #23) > Why is process() taking a stringified version of the manifest as input? Was keeping processing and error reporting in one place. > The > most likely use case is to fetch the resource with xhr and directly get a > json object by setting the xhr response type properly. Agree. Once I figure out how to route errors to the developer console that would make sense.
Created attachment 8552241 [details] [diff] [review] 0001-Refactored-ManifestProcessor.patch I refactored ManifestProcessor.jsm... what's the right process to get this added? Should I reopen this bug? What's new: * Removed dependency on securityManager and Services.io.newURI: - Now doing a straight origin to origin comparison. * Renamed 'docLocation' argument of process() to docURL * Fixed a small text formatting bug. * Updated tests to use new argument name * Renamed a few local variables * Cleaned up some loops to make the more functional Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1548bf8cd3a
You need to open a new bug. Also, while you do refactoring, fix the comments by adding a space between `//` and the first later of the first word. Also, start the sentences by a caiptal and end with a full stop.
Thanks Fabrice, will file a new bug. Also made the fixes you suggested.