Implement the manifest processor part of W3C Manifest

RESOLVED FIXED in mozilla36

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: marcosc, Assigned: marcosc)

Tracking

(Blocks: 1 bug)

Trunk
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

3 years ago
Implement the part of the W3C manifest spec that just deals with processing a JSON file.
(Assignee)

Updated

3 years ago
Blocks: 997779
(Assignee)

Updated

3 years ago
Assignee: nobody → mcaceres
(Assignee)

Updated

3 years ago
(Assignee)

Comment 1

3 years ago
Created attachment 8505045 [details] [diff] [review]
Implementation of manifest processing +test suite
Attachment #8505045 - Flags: review?(ehsan.akhgari)
(Assignee)

Updated

3 years ago
Blocks: 1083410
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 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.)
Attachment #8505045 - Flags: review?(ehsan.akhgari) → review-
(Assignee)

Updated

3 years ago
Depends on: 1086997
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?
Flags: needinfo?(l10n)
(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.
Flags: needinfo?(l10n)
(Assignee)

Comment 6

3 years ago
Created attachment 8510486 [details] [diff] [review]
Updated files based on review.

Updated implementation, test suite, based on review.
Attachment #8505045 - Attachment is obsolete: true
Flags: needinfo?(ehsan.akhgari)
(Assignee)

Comment 7

3 years ago
Created attachment 8510515 [details] [diff] [review]
Updated patch... other one was incorrect.
Attachment #8510486 - Attachment is obsolete: true
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.
Attachment #8510515 - Flags: review-
Flags: needinfo?(ehsan.akhgari)
Marcos told me that he will deal with the l10n issue in bug 1086997, so this is good to go!
Attachment #8510515 - Flags: review- → review+
(Assignee)

Comment 10

3 years ago
Created attachment 8511570 [details] [diff] [review]
Final patch ready to be merged.
(Assignee)

Updated

3 years ago
Blocks: 1086997
No longer depends on: 1086997
(Assignee)

Comment 11

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

Comment 12

3 years ago
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.
Attachment #8510515 - Attachment is obsolete: true
Attachment #8511570 - Attachment is obsolete: true
Sent to try: https://tbpl.mozilla.org/?tree=Try&rev=fd513aac12cf
(Assignee)

Comment 14

3 years ago
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.
Attachment #8511577 - Attachment is obsolete: true
(Assignee)

Comment 15

3 years ago
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
(Assignee)

Comment 17

3 years ago
Created attachment 8514475 [details] [diff] [review]
0001-1079453-Implement-the-manifest-processor-part-of-W3C.patch

Strange... hopefully this one fixes that.
Attachment #8511579 - Attachment is obsolete: true
Sent to try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f14dc72fd74b

The patch applied correctly that time :)
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Marcos patch had some info from another patch included, new try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9a211f301486
Pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3624a370840
Target Milestone: --- → mozilla36
Flags: in-testsuite+
\o/

Thanks for your help, Mounir!
https://hg.mozilla.org/mozilla-central/rev/c3624a370840
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
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.
(Assignee)

Comment 24

3 years ago
(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.
(Assignee)

Comment 25

3 years ago
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.
(Assignee)

Comment 27

3 years ago
Thanks Fabrice, will file a new bug. Also made the fixes you suggested.
(Assignee)

Updated

2 years ago
Blocks: 1162729
You need to log in before you can comment on or make changes to this bug.