1079453 - Implement the manifest processor part of W3C Manifest

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Marcos Caceres [:marcosc]]

Implement the part of the W3C manifest spec that just deals with processing a JSON file.

Comment 1

[Marcos Caceres [:marcosc]]

Attachment: Implementation of manifest processing +test suite

Comment 2

[(no longer active)]

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.

Comment 3

[(no longer active)]

Comment on attachment 8505045 [details] [diff] [review]
Implementation of manifest processing +test suite

Review of attachment 8505045 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/manifest/IconsProcessor.jsm
@@ +10,5 @@
> +  interfaces: Ci
> +} = Components;
> +
> +
> +(function(exports) {

Nit: JS modules are loaded isolated from all other JS code, so this idiom doesn't really make sense for them.  The only symbol that is visible from the outside world is the ones added to EXPORTED_SYMBOLS.

@@ +21,5 @@
> +  function IconsProcessor(valueExtractor) {
> +    if (!valueExtractor) {
> +      throw new TypeError('Value extractor is required.');
> +    }
> +    extractValue = valueExtractor;

IconsPreprocessor taking a valueExtractor argument kind of threw me off when reviewing this code.  Wouldn't it be better to merge this file with ManifestProcessor and use the same machinery that we have there?

@@ +23,5 @@
> +      throw new TypeError('Value extractor is required.');
> +    }
> +    extractValue = valueExtractor;
> +  }
> +  exports.IconsProcessor = IconsProcessor;

This can go away too.

@@ +34,5 @@
> +      },
> +      icons = [],
> +      value = extractValue(obj);
> +
> +    if (Array.isArray(value)) {

How can this not be an array?  And what should we do in that case?  Should we just ignore it?

@@ +37,5 @@
> +
> +    if (Array.isArray(value)) {
> +      //filter out icons with no "src" or src is empty string
> +      let processableIcons = value.filter(
> +        icon => icon && Object.prototype.hasOwnProperty.call(icon, 'src') && icon.src !== ''

Shouldn't we handle the case where src is not a string property as per <https://w3c.github.io/manifest/#dfn-steps-for-processing-the-src-member-of-an-icon>?

@@ +65,5 @@
> +        expectedType: 'string'
> +      },
> +      value = extractValue(obj),
> +      isString = (typeof value === 'string');
> +    value = (isString && value.length > 0) ? netutil.parseContentType(value.trim(), charset, hadCharset) : undefined;

I think it would be a bit cleaner if charset and hadCharset were locals here.

@@ +73,5 @@
> +  function processDensityMember(icon, manifestURL) {
> +    let hasDensity = Object.prototype.hasOwnProperty.call(icon, 'density'),
> +      rawValue = (hasDensity) ? icon.density : undefined,
> +      value = parseFloat(rawValue),
> +      result = (Number.isNaN(value) || value === +Infinity || value <= 0 || undefined) ? 1.0 : value;

I think you meant value === undefined or some such.  Please add a test that would have caught this.

@@ +111,5 @@
> +    Implementation of HTML's link@size attribute checker
> +    */
> +    function isValidSizeValue(size) {
> +      const onlyDecimals = /^\d+$/,
> +        any = new RegExp('any', 'i');

Nit: please move these regexes to the caller so that they get compiled only once.

::: dom/manifest/ManifestProcessor.jsm
@@ +17,5 @@
> +
> +/*
> +encapsulate scope, so it doesn't leak onto the backstage pass
> +*/
> +(function(exports) {

Ditto.

@@ +26,5 @@
> +  Cu.importGlobalProperties(['URL']);
> +  Cu.import('resource://gre/modules/Services.jsm', imports);
> +  Cu.import('resource://gre/modules/IconsProcessor.jsm', imports);
> +  const securityManager = imports.Services.scriptSecurityManager;
> +  const iconsProcessor = new imports.IconsProcessor(extractValue);

This assumes that the IconsProcessor object doesn't have internal state that matters here, which is true for now, but it's risky to rely on that.  I'd move creating this object down to the consumption sites.

@@ +63,5 @@
> +
> +  /**
> +   * Helper function
> +   */
> +  function toNsIURI(URL) {

Conventionally, we call this function "makeURI" elsewhere in the codebase.  Please use the same name here.

Also, can you call the argument something else that doesn't clash with the global URL property?

@@ +90,5 @@
> +    }
> +
> +    if (typeof manifest !== 'object' || manifest === null) {
> +      let msg = 'Manifest needs to be an object.';
> +      issueDeveloperWarning(msg);

If jsonText is invalid JSON, the catch block above would issue one developer warning, and this one would report a second one.  Perhaps we should remove the call to issueDeveloperWarning from the catch block?  I think this one handles the case where the JSON.parse() call throws, assuming that you don't intialize |manifest| to {}.

@@ +91,5 @@
> +
> +    if (typeof manifest !== 'object' || manifest === null) {
> +      let msg = 'Manifest needs to be an object.';
> +      issueDeveloperWarning(msg);
> +      manifest = {};

Shouldn't we just return here?

@@ +183,5 @@
> +    } catch (e) {
> +      issueDeveloperWarning('Invalid URL.');
> +      return result;
> +    }
> +    targetURI = toNsIURI(docURL);

I'd avoid parsing docURL as an nsIURI twice here.

@@ +218,5 @@
> +    return value;
> +  }
> +
> +  function issueDeveloperWarning(msg) {
> +    //console.log(msg);

Please file a follow-up bug and include its number here for fixing this.

::: dom/manifest/test/IconsProcessor_density.js
@@ +22,5 @@
> +  var expected = `Expect density to default to 1.0.`;
> +  testIcon.icons[0].density = density;
> +  data.jsonText = JSON.stringify(testIcon);
> +  var result = processor.process(data);
> +  SimpleTest.is(result.icons[0].density, 1.0, expected);

Nit: just say is(...).  Also, beware that is() just does a == check.  If you want a === check, you should use ise().

::: dom/manifest/test/IconsProcessor_sizes.js
@@ +20,5 @@
> +  expect: ['any']
> +}, {
> +  test: 'Any',
> +  expect: ['Any']
> +}];

Please include other tests for other interesting valid cases such as "any 32x32", "16x32", "17x33", "32x32 32x32", "32X32", etc.

@@ +51,5 @@
> +    sizes: undefined
> +  }]
> +};
> +
> +var invalidSizes = ['invalid', '', ' ', '16 x 16', '32', '21'];

Please test a string containing two x's.

::: dom/manifest/test/IconsProcessor_src.js
@@ +2,5 @@
> + * icon member's src member
> + * https://w3c.github.io/manifest/#src-member
> + **/
> +'use strict';
> +var noSrc = {

Please add a test for the non-string src case.

::: dom/manifest/test/ManifestProcessor_JSON.js
@@ +7,5 @@
> +invalidJson.forEach((testString) => {
> +  var expected = `Expect to recover from invalid JSON: ${testString}`;
> +  data.jsonText = testString;
> +  var result = processor.process(data);
> +  SimpleTest.ok(result.start_url.host === docLocation.host, true, expected);

I'd check .origin instead.

@@ +15,5 @@
> +validButUnhelpful.forEach((testString) => {
> +  var expected = `Expect to recover from invalid JSON: ${testString}`;
> +  data.jsonText = testString;
> +  var result = processor.process(data);
> +  SimpleTest.ok(result.start_url.host === docLocation.host, true, expected);

Same here.

::: dom/manifest/test/ManifestProcessor_display.js
@@ +26,5 @@
> +
> +//trim tests
> +validModes.forEach((display) => {
> +  var expected = `Expect trimmed display mode to be returned.`;
> +  var expandedDisplay = `${seperators}${lineTerminators}${display}${lineTerminators}${seperators}`;

Please use +.

@@ +34,5 @@
> +  var result = processor.process(data);
> +  SimpleTest.is(result.display, display, expected);
> +});
> +
> +//Unknow modes

*Unknown

::: dom/manifest/test/ManifestProcessor_name_and_short_name.js
@@ +49,5 @@
> +   pass pass pass pass pass pass pass pass pass pass pass pass pass pass
> +   pass pass pass pass pass pass pass pass pass pass pass pass pass pass
> +   pass pass pass pass pass pass pass pass pass pass pass pass`,
> +  'これは許容できる名前です',
> +  'ນີ້ແມ່ນຊື່ທີ່ຍອມຮັບໄດ້'

Nice!

@@ +59,5 @@
> +    var obj = {};
> +    obj[prop] = name;
> +    data.jsonText = JSON.stringify(obj);
> +    var result = processor.process(data);
> +    SimpleTest.ok(result[prop] === name, true, expected);

You probably want ise() here.

::: dom/manifest/test/common.js
@@ +2,5 @@
> + * Common infrastructure for manifest tests.
> + **/
> +
> +'use strict';
> +const bsp = SpecialPowers.Cu.import('resource://gre/modules/ManifestProcessor.jsm'),

Please don't capture the return value of import, and just access ManifestProcessor off of the global.

@@ +8,5 @@
> +  manifestURL = new URL(document.location.origin + '/manifest.json'),
> +  docLocation = new URL('', document.location.origin),
> +  seperators = '\u2028\u2029\u0020\u00A0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200A\u202F\u205F\u3000',
> +  lineTerminators = '\u000D\u000A\u2028\u2029',
> +  whiteSpace = `${seperators}${lineTerminators}`,

|separators + lineTerminators| seems much more readable.

::: dom/manifest/test/mochitest.ini
@@ +3,5 @@
> +
> +support-files =
> +	../IconsProcessor.jsm
> +	../ManifestObtainer.jsm
> +	../ManifestProcessor.jsm

You don't need to package the JSMs in the test manifest.

::: dom/manifest/test/test_IconsProcessor_density.html
@@ +8,5 @@
> +  <title>Test for Bug 1079453</title>
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +  <script src="common.js"></script>
> +  <script src="IconsProcessor_density.js"></script>

We usually just include the script inline inside the test body.  Basically, try to keep as much of the test self-contained in one file as you can (common.js is fine, of course.)

Comment 4

[(no longer active)]

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?

Comment 5

[Francesco Lodolo [:flod]]

(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.

Comment 6

[Marcos Caceres [:marcosc]]

Attachment: Updated files based on review.

Updated implementation, test suite, based on review.

Comment 7

[Marcos Caceres [:marcosc]]

Attachment: Updated patch... other one was incorrect.

Comment 8

[(no longer active)]

Comment on attachment 8510515 [details] [diff] [review]
Updated patch... other one was incorrect.

Review of attachment 8510515 [details] [diff] [review]:
-----------------------------------------------------------------

Based on comment 5, we need to make sure that the strings that will be displayed to the user are localizable.  For that, you need to put the strings in a .properties file, and use the string bundle service <https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Miscellaneous#Using_string_bundles_from_JavaScript> in order to format the error messages, potentially using formatStringFromName if you need to inject placeholders into the string.  Sorry for the additional review round-trip!

::: dom/manifest/ManifestProcessor.jsm
@@ +193,5 @@
> +      },
> +      icons = [];
> +    let value = extractValue(obj);
> +
> +    if (Array.isArray(value)) {

Repeat of the previous comment:

How can this not be an array? And what should we do in that case? Should we just ignore it?

@@ +200,5 @@
> +        icon => icon && Object.prototype.hasOwnProperty.call(icon, 'src') && icon.src !== ''
> +      );
> +      for (let potentialIcon of processableIcons) {
> +        let src = processSrcMember(potentialIcon, baseURL)
> +        if(src !== undefined){

Nit: space around the parentheses.

Comment 9

[(no longer active)]

Marcos told me that he will deal with the l10n issue in bug 1086997, so this is good to go!

Comment 10

[Marcos Caceres [:marcosc]]

Attachment: Final patch ready to be merged.

Comment 11

[Marcos Caceres [:marcosc]]

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 :(

Comment 12

[Marcos Caceres [:marcosc]]

Attachment: 0001-Bug-1079453-Implement-the-manifest-processor-part-of.patch

Mounir tried the patch and it didn't apply cleanly, so I generated another.

Comment 13

[Mounir Lamouri (:mounir)]

Sent to try: https://tbpl.mozilla.org/?tree=Try&rev=fd513aac12cf

Comment 14

[Marcos Caceres [:marcosc]]

Attachment: 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.

Comment 15

[Marcos Caceres [:marcosc]]

Mounir didn't apply the patch properly ^_^

Comment 16

[(no longer active)]

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

Comment 17

[Marcos Caceres [:marcosc]]

Attachment: 0001-1079453-Implement-the-manifest-processor-part-of-W3C.patch

Strange... hopefully this one fixes that.

Comment 18

[Mounir Lamouri (:mounir)]

Sent to try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f14dc72fd74b

The patch applied correctly that time :)

Comment 19

[Mounir Lamouri (:mounir)]

Marcos patch had some info from another patch included, new try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9a211f301486

Comment 20

[Mounir Lamouri (:mounir)]

Pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3624a370840

Comment 21

[(no longer active)]

\o/

Thanks for your help, Mounir!

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c3624a370840

Comment 23

[[:fabrice] Fabrice Desré]

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.

Comment 24

[Marcos Caceres [:marcosc]]

(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.

Comment 25

[Marcos Caceres [:marcosc]]

Attachment: 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

Comment 26

[[:fabrice] Fabrice Desré]

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.

Comment 27

[Marcos Caceres [:marcosc]]

Thanks Fabrice, will file a new bug. Also made the fixes you suggested.