Bug 1559412 - Support web app manifest ImageResource.purpose r=baku
authorMarcos Cáceres <mcaceres@mozilla.com>
Thu, 15 Aug 2019 12:55:35 +0000
changeset 488430 90d0473fe6aa5e0ceacb3354a7e226cb7383e715
parent 488429 e54b58ba5f3a177e8d3abbb5de1e10903a83910d
child 488431 515ff404fcfe6f0c728799d1b64c6889748393ca
push id36443
push userccoroiu@mozilla.com
push dateFri, 16 Aug 2019 09:48:15 +0000
treeherdermozilla-central@5d4cbfe103bb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku
bugs1559412
milestone70.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1559412 - Support web app manifest ImageResource.purpose r=baku implementation of purpose member Differential Revision: https://phabricator.services.mozilla.com/D38627
dom/locales/en-US/chrome/dom/dom.properties
dom/manifest/ImageObjectProcessor.jsm
dom/manifest/ManifestObtainer.jsm
dom/manifest/ValueExtractor.jsm
dom/manifest/test/mochitest.ini
dom/manifest/test/test_ImageObjectProcessor_purpose.html
dom/manifest/test/test_ManifestProcessor_warnings.html
--- a/dom/locales/en-US/chrome/dom/dom.properties
+++ b/dom/locales/en-US/chrome/dom/dom.properties
@@ -245,16 +245,22 @@ ManifestStartURLShouldBeSameOrigin=The s
 # LOCALIZATION NOTE: %1$S is the name of the object whose property is invalid. %2$S is the name of the invalid property. %3$S is the expected type of the property value. E.g. "Expected the manifest's start_url member to be a string."
 ManifestInvalidType=Expected the %1$S’s %2$S member to be a %3$S.
 # LOCALIZATION NOTE: %1$S is the name of the property whose value is invalid. %2$S is the (invalid) value of the property. E.g. "theme_color: 42 is not a valid CSS color."
 ManifestInvalidCSSColor=%1$S: %2$S is not a valid CSS color.
 # LOCALIZATION NOTE: %1$S is the name of the property whose value is invalid. %2$S is the (invalid) value of the property. E.g. "lang: 42 is not a valid language code."
 ManifestLangIsInvalid=%1$S: %2$S is not a valid language code.
 # LOCALIZATION NOTE: %1$S is the name of the parent property whose value is invalid (e.g., "icons"). %2$S is the index of the image object that is invalid (from 0). %3$S is the name of actual member that is invalid. %4$S is the invalid value. E.g. "icons item at index 2 is invalid. The src member is an invalid URL http://:Invalid"
 ManifestImageURLIsInvalid=%1$S item at index %2$S is invalid. The %3$S member is an invalid URL %4$S
+# LOCALIZATION NOTE: %1$S is the name of the parent property that that contains the unusable image object (e.g., "icons"). %2$S is the index of the image object that is unusable (from 0). E.g. "icons item at index 2 lacks a usable purpose. It will be ignored."
+ManifestImageUnusable=%1$S item at index %2$S lacks a usable purpose. It will be ignored.
+# LOCALIZATION NOTE: %1$S is the name of the parent property that contains the unsupported value (e.g., "icons"). %2$S is the index of the image object that has the unsupported value (from 0). %3$S are the unknown purposes. E.g. "icons item at index 2 includes unsupported purpose(s): a b."
+ManifestImageUnsupportedPurposes=%1$S item at index %2$S includes unsupported purpose(s): %3$S.
+# LOCALIZATION NOTE: %1$S is the name of the parent property that has a repeated purpose (e.g., "icons"). %2$S is the index of the image object that has the repeated purpose (from 0). %3$S is the repeated purposes. E.g. "icons item at index 2 includes repeated purpose(s): a b."
+ManifestImageRepeatedPurposes=%1$S item at index %2$S includes repeated purpose(s): %3$S.
 PatternAttributeCompileFailure=Unable to check <input pattern='%S'> because the pattern is not a valid regexp: %S
 # LOCALIZATION NOTE: Do not translate "postMessage" or DOMWindow. %S values are origins, like https://domain.com:port
 TargetPrincipalDoesNotMatch=Failed to execute ‘postMessage’ on ‘DOMWindow’: The target origin provided (‘%S’) does not match the recipient window’s origin (‘%S’).
 # LOCALIZATION NOTE: Do not translate 'YouTube'. %S values are origins, like https://domain.com:port
 RewriteYouTubeEmbed=Rewriting old-style YouTube Flash embed (%S) to iframe embed (%S). Please update page to use iframe instead of embed/object, if possible.
 # LOCALIZATION NOTE: Do not translate 'YouTube'. %S values are origins, like https://domain.com:port
 RewriteYouTubeEmbedPathParams=Rewriting old-style YouTube Flash embed (%S) to iframe embed (%S). Params were unsupported by iframe embeds and converted. Please update page to use iframe instead of embed/object, if possible.
 # LOCALIZATION NOTE: This error is reported when the "Encryption" header for an
--- a/dom/manifest/ImageObjectProcessor.jsm
+++ b/dom/manifest/ImageObjectProcessor.jsm
@@ -29,16 +29,18 @@ const { Services } = ChromeUtils.import(
 XPCOMUtils.defineLazyGlobalGetters(this, ["URL"]);
 
 function ImageObjectProcessor(aErrors, aExtractor, aBundle) {
   this.errors = aErrors;
   this.extractor = aExtractor;
   this.domBundle = aBundle;
 }
 
+const iconPurposes = Object.freeze(["any", "maskable"]);
+
 // Static getters
 Object.defineProperties(ImageObjectProcessor, {
   decimals: {
     get() {
       return /^\d+$/;
     },
   },
   anyRegEx: {
@@ -59,30 +61,121 @@ ImageObjectProcessor.prototype.process =
     property: aMemberName,
     expectedType: "array",
     trim: false,
   };
   const { domBundle, extractor, errors } = this;
   const images = [];
   const value = extractor.extractValue(spec);
   if (Array.isArray(value)) {
-    // Filter out images whose "src" is not useful.
     value
-      .filter((item, index) => !!processSrcMember(item, aBaseURL, index))
       .map(toImageObject)
+      // Filter out images that resulted in "failure", per spec.
+      .filter(image => image)
       .forEach(image => images.push(image));
   }
   return images;
 
-  function toImageObject(aImageSpec) {
-    return {
-      src: processSrcMember(aImageSpec, aBaseURL),
-      type: processTypeMember(aImageSpec),
-      sizes: processSizesMember(aImageSpec),
+  function toImageObject(aImageSpec, index) {
+    let img; // if "failure" happens below, we return undefined.
+    try {
+      // can throw
+      const src = processSrcMember(aImageSpec, aBaseURL, index);
+      // can throw
+      const purpose = processPurposeMember(aImageSpec, index);
+      const type = processTypeMember(aImageSpec);
+      const sizes = processSizesMember(aImageSpec);
+      img = {
+        src,
+        purpose,
+        type,
+        sizes,
+      };
+    } catch (err) {
+      /* Errors are collected by each process* function */
+    }
+    return img;
+  }
+
+  function processPurposeMember(aImage, index) {
+    const spec = {
+      objectName: "image",
+      object: aImage,
+      property: "purpose",
+      expectedType: "string",
+      trim: true,
+      throwTypeError: true,
     };
+
+    // Type errors are treated at "any"...
+    let value;
+    try {
+      value = extractor.extractValue(spec);
+    } catch (err) {
+      return ["any"];
+    }
+
+    // Was only whitespace...
+    if (!value) {
+      return ["any"];
+    }
+
+    const keywords = value.split(/\s+/);
+
+    // Emtpy is treated as "any"...
+    if (keywords.length === 0) {
+      return ["any"];
+    }
+
+    // We iterate over keywords and classify them into:
+    const purposes = new Set();
+    const unknownPurposes = new Set();
+    const repeatedPurposes = new Set();
+
+    for (const keyword of keywords) {
+      const canonicalKeyword = keyword.toLowerCase();
+
+      if (purposes.has(canonicalKeyword)) {
+        repeatedPurposes.add(keyword);
+        continue;
+      }
+
+      iconPurposes.includes(canonicalKeyword)
+        ? purposes.add(canonicalKeyword)
+        : unknownPurposes.add(keyword);
+    }
+
+    // Tell developer about unknown purposes...
+    if (unknownPurposes.size) {
+      const warn = domBundle.formatStringFromName(
+        "ManifestImageUnsupportedPurposes",
+        [aMemberName, index, [...unknownPurposes].join(" ")]
+      );
+      errors.push({ warn });
+    }
+
+    // Tell developer about repeated purposes...
+    if (repeatedPurposes.size) {
+      const warn = domBundle.formatStringFromName(
+        "ManifestImageRepeatedPurposes",
+        [aMemberName, index, [...repeatedPurposes].join(" ")]
+      );
+      errors.push({ warn });
+    }
+
+    if (purposes.size === 0) {
+      const warn = domBundle.formatStringFromName("ManifestImageUnusable", [
+        aMemberName,
+        index,
+      ]);
+      errors.push({ warn });
+      throw new TypeError(warn);
+    }
+
+    return [...purposes];
   }
 
   function processTypeMember(aImage) {
     const charset = {};
     const hadCharset = {};
     const spec = {
       objectName: "image",
       object: aImage,
@@ -103,28 +196,35 @@ ImageObjectProcessor.prototype.process =
 
   function processSrcMember(aImage, aBaseURL, index) {
     const spec = {
       objectName: aMemberName,
       object: aImage,
       property: "src",
       expectedType: "string",
       trim: false,
+      throwTypeError: true,
     };
     const value = extractor.extractValue(spec);
     let url;
+    if (typeof value === "undefined" || value === "") {
+      // We throw here as the value is unusable,
+      // but it's not an developer error.
+      throw new TypeError();
+    }
     if (value && value.length) {
       try {
         url = new URL(value, aBaseURL).href;
       } catch (e) {
         const warn = domBundle.formatStringFromName(
           "ManifestImageURLIsInvalid",
           [aMemberName, index, "src", value]
         );
         errors.push({ warn });
+        throw e;
       }
     }
     return url;
   }
 
   function processSizesMember(aImage) {
     const sizes = new Set();
     const spec = {
--- a/dom/manifest/ManifestObtainer.jsm
+++ b/dom/manifest/ManifestObtainer.jsm
@@ -64,24 +64,28 @@ var ManifestObtainer = {
   /**
    * Public interface for obtaining a web manifest from a XUL browser.
    * @param {Window} aContent A content Window from which to extract the manifest.
    * @param {Object} aOptions
    * @param {Boolean} aOptions.checkConformance If spec conformance messages should be collected.
    *                                            Adds proprietary moz_* members to manifest.
    * @return {Promise<Object>} The processed manifest.
    */
-  contentObtainManifest(aContent, aOptions = { checkConformance: false }) {
+  async contentObtainManifest(
+    aContent,
+    aOptions = { checkConformance: false }
+  ) {
     if (!aContent || isXULBrowser(aContent)) {
       const err = new TypeError("Invalid input. Expected a DOM Window.");
       return Promise.reject(err);
     }
-    return fetchManifest(aContent).then(response =>
-      processResponse(response, aContent, aOptions)
-    );
+    const response = await fetchManifest(aContent);
+    const result = await processResponse(response, aContent, aOptions);
+    const clone = Cu.cloneInto(result, aContent);
+    return clone;
   },
 };
 
 function toError(aErrorClone) {
   let error;
   switch (aErrorClone.name) {
     case "TypeError":
       error = new TypeError();
--- a/dom/manifest/ValueExtractor.jsm
+++ b/dom/manifest/ValueExtractor.jsm
@@ -23,28 +23,40 @@ ValueExtractor.prototype = {
   // This function takes a 'spec' object and destructures
   // it to extract a value. If the value is of th wrong type, it
   // warns the developer and returns undefined.
   //  expectType: is the type of a JS primitive (string, number, etc.)
   //  object: is the object from which to extract the value.
   //  objectName: string used to construct the developer warning.
   //  property: the name of the property being extracted.
   //  trim: boolean, if the value should be trimmed (used by string type).
-  extractValue({ expectedType, object, objectName, property, trim }) {
+  //  throwTypeError: boolean, throw a TypeError if the type is incorrect.
+  extractValue(options) {
+    const {
+      expectedType,
+      object,
+      objectName,
+      property,
+      throwTypeError,
+      trim,
+    } = options;
     const value = object[property];
     const isArray = Array.isArray(value);
     // We need to special-case "array", as it's not a JS primitive.
     const type = isArray ? "array" : typeof value;
     if (type !== expectedType) {
       if (type !== "undefined") {
         const warn = this.domBundle.formatStringFromName(
           "ManifestInvalidType",
           [objectName, property, expectedType]
         );
         this.errors.push({ warn });
+        if (throwTypeError) {
+          throw new TypeError(warn);
+        }
       }
       return undefined;
     }
     // Trim string and returned undefined if the empty string.
     const shouldTrim = expectedType === "string" && value && trim;
     if (shouldTrim) {
       return value.trim() || undefined;
     }
--- a/dom/manifest/test/mochitest.ini
+++ b/dom/manifest/test/mochitest.ini
@@ -1,15 +1,16 @@
 [DEFAULT]
 support-files =
   common.js
   resource.sjs
   manifestLoader.html
   file_reg_appinstalled_event.html
   file_testserver.sjs
+[test_ImageObjectProcessor_purpose.html]
 [test_ImageObjectProcessor_sizes.html]
 [test_ImageObjectProcessor_src.html]
 [test_ImageObjectProcessor_type.html]
 [test_ManifestProcessor_background_color.html]
 [test_ManifestProcessor_dir.html]
 [test_ManifestProcessor_display.html]
 [test_ManifestProcessor_icons.html]
 [test_ManifestProcessor_JSON.html]
new file mode 100644
--- /dev/null
+++ b/dom/manifest/test/test_ImageObjectProcessor_purpose.html
@@ -0,0 +1,100 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug </title>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" href="/tests/SimpleTest/test.css"/>
+  <script src="common.js"></script>
+  <script>
+    /**
+     * Image object's purpose member
+     * https://w3c.github.io/manifest/#purpose-member
+     **/
+
+    "use strict";
+    const testManifest = {
+      icons: [{
+        src: "test",
+      }],
+    };
+
+    const invalidPurposeTypes = [
+      [],
+      123,
+      {},
+      null,
+    ]
+
+    invalidPurposeTypes.forEach(invalidType => {
+      const expected = `Invalid types get treated as 'any'.`;
+      testManifest.icons[0].purpose = invalidType;
+      data.jsonText = JSON.stringify(testManifest);
+      const result = processor.process(data);
+      is(result.icons.length, 1, expected);
+      is(result.icons[0].purpose.length, 1, expected);
+      is(result.icons[0].purpose[0], "any", expected);
+    });
+
+    const invalidPurposes = [
+      "not-known-test-purpose",
+      "invalid-purpose invalid-purpose",
+      "no-purpose invalid-purpose some-other-non-valid-purpose",
+    ];
+
+    invalidPurposes.forEach(invalidPurpose => {
+      const expected = `Expect invalid purposes to invalidate the icon.`;
+      testManifest.icons[0].purpose = invalidPurpose;
+      data.jsonText = JSON.stringify(testManifest);
+      const result = processor.process(data);
+      is(result.icons.length, 0, expected);
+    });
+
+    const mixedValidAndInvalidPurposes = [
+      "not-known-test-purpose maskable",
+      "maskable invalid-purpose invalid-purpose",
+      "no-purpose invalid-purpose maskable some-other-non-valid-purpose",
+    ];
+
+    mixedValidAndInvalidPurposes.forEach(mixedPurpose => {
+      const expected = `Expect on 'maskable' to remain.`;
+      testManifest.icons[0].purpose = mixedPurpose;
+      data.jsonText = JSON.stringify(testManifest);
+      const result = processor.process(data);
+      is(result.icons.length, 1, expected);
+      is(result.icons[0].purpose.join(), "maskable", expected);
+    });
+
+    const validPurposes = [
+      "maskable",
+      "any",
+      "any maskable",
+      "maskable any",
+    ];
+
+    validPurposes.forEach(purpose => {
+      testManifest.icons[0].purpose = purpose;
+      data.jsonText = JSON.stringify(testManifest);
+      var manifest = processor.process(data);
+      is(manifest.icons[0].purpose.join(" "), purpose, `Expected "${purpose}" as purpose.`);
+    });
+
+    const validWhiteSpace = [
+      "",
+      whiteSpace, // defined in common.js
+      `${whiteSpace}any`,
+      `any${whiteSpace}`,
+      `${whiteSpace}any${whiteSpace}`,
+    ];
+
+    validWhiteSpace.forEach(purpose => {
+      testManifest.icons[0].purpose = purpose;
+      data.jsonText = JSON.stringify(testManifest);
+      var manifest = processor.process(data);
+      is(manifest.icons[0].purpose.join(), "any");
+    });
+  </script>
+</head>
--- a/dom/manifest/test/test_ManifestProcessor_warnings.html
+++ b/dom/manifest/test/test_ManifestProcessor_warnings.html
@@ -56,25 +56,75 @@ const options = {...data, checkConforman
     func: () => options.jsonText = JSON.stringify({
       background_color: "42",
     }),
     warn: "background_color: 42 is not a valid CSS color.",
   },
   {
     func: () => options.jsonText = JSON.stringify({
       icons: [
-        { "src": "http://exmaple.com", "sizes": "48x48"},
+        { "src": "http://example.com", "sizes": "48x48"},
         { "src": "http://:Invalid", "sizes": "48x48"},
       ],
     }),
     warn: "icons item at index 1 is invalid. The src member is an invalid URL http://:Invalid",
   },
+  // testing dom.properties: ManifestImageUnusable
+  {
+    func() {
+      return (options.jsonText = JSON.stringify({
+        icons: [
+          { src: "http://example.com", purpose: "any" }, // valid
+          { src: "http://example.com", purpose: "banana" }, // generates error
+        ],
+      }));
+    },
+    get warn() {
+      // Returns 2 warnings... array here is just to keep them organized
+      return [
+        "icons item at index 1 includes unsupported purpose(s): banana.",
+        "icons item at index 1 lacks a usable purpose. It will be ignored.",
+      ].join(" ");
+    },
+  },
+  // testing dom.properties: ManifestImageUnsupportedPurposes
+  {
+    func() {
+      return (options.jsonText = JSON.stringify({
+        icons: [
+          { src: "http://example.com", purpose: "any" }, // valid
+          { src: "http://example.com", purpose: "any foo bar baz bar bar baz" }, // generates error
+        ],
+      }));
+    },
+    warn: "icons item at index 1 includes unsupported purpose(s): foo bar baz.",
+  },
+  // testing dom.properties: ManifestImageRepeatedPurposes
+  {
+    func() {
+      return (options.jsonText = JSON.stringify({
+        icons: [
+          { src: "http://example.com", purpose: "any" }, // valid
+          {
+            src: "http://example.com",
+            purpose: "any maskable any maskable maskable", // generates error
+          },
+        ],
+      }));
+    },
+    warn: "icons item at index 1 includes repeated purpose(s): any maskable.",
+  },
 ].forEach((test, index) => {
   test.func();
   const result = processor.process(options);
-  const [message] = result.moz_validation;
-  is(message.warn, test.warn, "Check warning.");
-  options.manifestURL = manifestURL;
+  let messages = [];
+  // Poking directly at "warn" triggers xray security wrapper.
+  for (const validationError of result.moz_validation) {
+    const { warn } = validationError;
+    messages.push(warn);
+  }
+  is(messages.join(" "), test.warn, "Check warning.");
+    options.manifestURL = manifestURL;
   options.docURL = docURL;
 });
 
   </script>
 </head>