Bug 1563460 - Collect console messages generated by processing manifest for Dev Tools r=baku

This gets rid of the sending warnings to the browser console. Instead, when the processor is explicitly asked to do so, it now collects spec violations into a `moz_validation` member. To access the new manifest member, you can now pass a second argument to `ManifestObtainer.contentObtainManifest()` like so: ``` const manifest = await ManifestObtainer.contentObtainManifest( this.targetActor.window, { checkConformance: true } ); manifest. moz_validation; // 🎉 ``` Differential Revision: https://phabricator.services.mozilla.com/D36885 --HG-- extra : moz-landing-system : lando
2019-07-09 02:33:39 +00:00 · 2019-07-09 02:33:39 +00:00 · 1118e89526
--- a/dom/locales/en-US/chrome/dom/dom.properties
+++ b/dom/locales/en-US/chrome/dom/dom.properties
@ -250,6 +250,8 @@ ManifestInvalidType=Expected the %1$S’s %2$S member to be a %3$S.
 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
 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’).
--- a/dom/manifest/ImageObjectProcessor.jsm
+++ b/dom/manifest/ImageObjectProcessor.jsm
@ -28,9 +28,10 @@ const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");

 XPCOMUtils.defineLazyGlobalGetters(this, ["URL"]);

-function ImageObjectProcessor(aConsole, aExtractor) {
-  this.console = aConsole;
+function ImageObjectProcessor(aErrors, aExtractor, aBundle) {
+  this.errors = aErrors;
  this.extractor = aExtractor;
+  this.domBundle = aBundle;
 }

 // Static getters
@ -59,13 +60,13 @@ ImageObjectProcessor.prototype.process = function(
    expectedType: "array",
    trim: false,
  };
-  const extractor = this.extractor;
+  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 => !!processSrcMember(item, aBaseURL))
+      .filter((item, index) => !!processSrcMember(item, aBaseURL, index))
      .map(toImageObject)
      .forEach(image => images.push(image));
  }
@ -100,9 +101,9 @@ ImageObjectProcessor.prototype.process = function(
    return value || undefined;
  }

-  function processSrcMember(aImage, aBaseURL) {
+  function processSrcMember(aImage, aBaseURL, index) {
    const spec = {
-      objectName: "image",
+      objectName: aMemberName,
      object: aImage,
      property: "src",
      expectedType: "string",
@ -113,7 +114,13 @@ ImageObjectProcessor.prototype.process = function(
    if (value && value.length) {
      try {
        url = new URL(value, aBaseURL).href;
-      } catch (e) {}
+      } catch (e) {
+        const warn = domBundle.formatStringFromName(
+          "ManifestImageURLIsInvalid",
+          [aMemberName, index, "src", value]
+        );
+        errors.push({ warn });
+      }
    }
    return url;
  }
--- a/dom/manifest/ManifestObtainer.jsm
+++ b/dom/manifest/ManifestObtainer.jsm
@ -57,15 +57,18 @@ var ManifestObtainer = {
  },
  /**
   * Public interface for obtaining a web manifest from a XUL browser.
-   * @param  {Window} The content Window from which to extract the manifest.
+   * @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.
   * @return {Promise<Object>} The processed manifest.
   */
-  contentObtainManifest(aContent) {
+  contentObtainManifest(aContent, aOptions = { checkConformance: false }) {
    if (!aContent || isXULBrowser(aContent)) {
-      throw new TypeError("Invalid input. Expected a DOM Window.");
+      const err = new TypeError("Invalid input. Expected a DOM Window.");
+      return Promise.reject(err);
    }
    return fetchManifest(aContent).then(response =>
-      processResponse(response, aContent)
+      processResponse(response, aContent, aOptions)
    );
  },
 };
@ -100,7 +103,7 @@ function isXULBrowser(aBrowser) {
 * @param {Window} aContentWindow The content window.
 * @return {Promise<Object>} The processed manifest.
 */
-async function processResponse(aResp, aContentWindow) {
+async function processResponse(aResp, aContentWindow, aOptions) {
  const badStatus = aResp.status < 200 || aResp.status >= 300;
  if (aResp.type === "error" || badStatus) {
    const msg = `Fetch error: ${aResp.status} - ${aResp.statusText} at ${
@ -114,7 +117,8 @@ async function processResponse(aResp, aContentWindow) {
    manifestURL: aResp.url,
    docURL: aContentWindow.location.href,
  };
-  const manifest = ManifestProcessor.process(args);
+  const processingOptions = Object.assign({}, args, aOptions);
+  const manifest = ManifestProcessor.process(processingOptions);
  return manifest;
 }

--- a/dom/manifest/ManifestProcessor.jsm
+++ b/dom/manifest/ManifestProcessor.jsm
@ -17,7 +17,6 @@
 *
 * TODO: The constructor should accept the UA's supported orientations.
 * TODO: The constructor should accept the UA's supported display modes.
- * TODO: hook up developer tools to console. (1086997).
 */
 /* globals Components, ValueExtractor, ImageObjectProcessor, ConsoleAPI*/
 "use strict";
@ -44,7 +43,6 @@ const orientationTypes = new Set([
 ]);
 const textDirections = new Set(["ltr", "rtl", "auto"]);

-const { ConsoleAPI } = ChromeUtils.import("resource://gre/modules/Console.jsm");
 const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
 // ValueExtractor is used by the various processors to get values
 // from the manifest and to report errors.
@ -80,11 +78,19 @@ var ManifestProcessor = {
  //  * jsonText: the JSON string to be processed.
  //  * manifestURL: the URL of the manifest, to resolve URLs.
  //  * docURL: the URL of the owner doc, for security checks
+  //  * checkConformance: boolean. If true, collects any conformance
+  //    errors into a "moz_validation" property on the returned manifest.
  process(aOptions) {
-    const { jsonText, manifestURL: aManifestURL, docURL: aDocURL } = aOptions;
-    const console = new ConsoleAPI({
-      prefix: "Web Manifest",
-    });
+    const {
+      jsonText,
+      manifestURL: aManifestURL,
+      docURL: aDocURL,
+      checkConformance,
+    } = aOptions;
+
+    // The errors get populated by the different process* functions.
+    const errors = [];
+
    let rawManifest = {};
    try {
      rawManifest = JSON.parse(jsonText);
@ -93,13 +99,18 @@ var ManifestProcessor = {
      return null;
    }
    if (typeof rawManifest !== "object") {
-      console.warn(domBundle.GetStringFromName("ManifestShouldBeObject"));
+      const warn = domBundle.GetStringFromName("ManifestShouldBeObject");
+      errors.push({ warn });
      rawManifest = {};
    }
    const manifestURL = new URL(aManifestURL);
    const docURL = new URL(aDocURL);
-    const extractor = new ValueExtractor(console, domBundle);
-    const imgObjProcessor = new ImageObjectProcessor(console, extractor);
+    const extractor = new ValueExtractor(errors, domBundle);
+    const imgObjProcessor = new ImageObjectProcessor(
+      errors,
+      extractor,
+      domBundle
+    );
    const processedManifest = {
      dir: processDirMember.call(this),
      lang: processLangMember(),
@ -113,6 +124,9 @@ var ManifestProcessor = {
      background_color: processBackgroundColorMember(),
    };
    processedManifest.scope = processScopeMember();
+    if (checkConformance) {
+      processedManifest.moz_validation = errors;
+    }
    return processedManifest;

    function processDirMember() {
@ -207,19 +221,22 @@ var ManifestProcessor = {
      try {
        scopeURL = new URL(value, manifestURL);
      } catch (e) {
-        console.warn(domBundle.GetStringFromName("ManifestScopeURLInvalid"));
+        const warn = domBundle.GetStringFromName("ManifestScopeURLInvalid");
+        errors.push({ warn });
        return undefined;
      }
      if (scopeURL.origin !== docURL.origin) {
-        console.warn(domBundle.GetStringFromName("ManifestScopeNotSameOrigin"));
+        const warn = domBundle.GetStringFromName("ManifestScopeNotSameOrigin");
+        errors.push({ warn });
        return undefined;
      }
      // If start URL is not within scope of scope URL:
      let isSameOrigin = startURL && startURL.origin !== scopeURL.origin;
      if (isSameOrigin || !startURL.pathname.startsWith(scopeURL.pathname)) {
-        console.warn(
-          domBundle.GetStringFromName("ManifestStartURLOutsideScope")
+        const warn = domBundle.GetStringFromName(
+          "ManifestStartURLOutsideScope"
        );
+        errors.push({ warn });
        return undefined;
      }
      return scopeURL.href;
@ -242,13 +259,15 @@ var ManifestProcessor = {
      try {
        potentialResult = new URL(value, manifestURL);
      } catch (e) {
-        console.warn(domBundle.GetStringFromName("ManifestStartURLInvalid"));
+        const warn = domBundle.GetStringFromName("ManifestStartURLInvalid");
+        errors.push({ warn });
        return result;
      }
      if (potentialResult.origin !== docURL.origin) {
-        console.warn(
-          domBundle.GetStringFromName("ManifestStartURLShouldBeSameOrigin")
+        const warn = domBundle.GetStringFromName(
+          "ManifestStartURLShouldBeSameOrigin"
        );
+        errors.push({ warn });
      } else {
        result = potentialResult.href;
      }
--- a/dom/manifest/ValueExtractor.jsm
+++ b/dom/manifest/ValueExtractor.jsm
@ -3,7 +3,7 @@
 * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 /*
 * Helper functions extract values from manifest members
- * and reports conformance violations.
+ * and reports conformance errors.
 */
 /* globals Components*/
 "use strict";
@ -14,8 +14,8 @@ const { XPCOMUtils } = ChromeUtils.import(

 XPCOMUtils.defineLazyGlobalGetters(this, ["InspectorUtils"]);

-function ValueExtractor(aConsole, aBundle) {
-  this.console = aConsole;
+function ValueExtractor(errors, aBundle) {
+  this.errors = errors;
  this.domBundle = aBundle;
 }

@ -35,13 +35,11 @@ ValueExtractor.prototype = {
    const type = isArray ? "array" : typeof value;
    if (type !== expectedType) {
      if (type !== "undefined") {
-        this.console.warn(
-          this.domBundle.formatStringFromName("ManifestInvalidType", [
-            objectName,
-            property,
-            expectedType,
-          ])
+        const warn = this.domBundle.formatStringFromName(
+          "ManifestInvalidType",
+          [objectName, property, expectedType]
        );
+        this.errors.push({ warn });
      }
      return undefined;
    }
@ -59,12 +57,11 @@ ValueExtractor.prototype = {
      const rgba = InspectorUtils.colorToRGBA(value);
      color = "#" + ((rgba.r << 16) | (rgba.g << 8) | rgba.b).toString(16);
    } else if (value) {
-      this.console.warn(
-        this.domBundle.formatStringFromName("ManifestInvalidCSSColor", [
-          spec.property,
-          value,
-        ])
+      const warn = this.domBundle.formatStringFromName(
+        "ManifestInvalidCSSColor",
+        [spec.property, value]
      );
+      this.errors.push({ warn });
    }
    return color;
  },
@ -75,12 +72,11 @@ ValueExtractor.prototype = {
      try {
        langTag = Intl.getCanonicalLocales(value)[0];
      } catch (err) {
-        console.warn(
-          this.domBundle.formatStringFromName("ManifestLangIsInvalid", [
-            spec.property,
-            value,
-          ])
+        const warn = this.domBundle.formatStringFromName(
+          "ManifestLangIsInvalid",
+          [spec.property, value]
        );
+        this.errors.push({ warn });
      }
    }
    return langTag;
--- a/dom/manifest/test/test_ManifestProcessor_warnings.html
+++ b/dom/manifest/test/test_ManifestProcessor_warnings.html
@ -11,76 +11,70 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1086997
  <script src="common.js"></script>
  <script>
 "use strict";
-
-const {
-  ConsoleAPI,
-} = SpecialPowers.Cu.import("resource://gre/modules/Console.jsm");
-
-var warning = null;
-
-var originalWarn = ConsoleAPI.prototype.warn;
-ConsoleAPI.prototype.warn = function(aWarning) {
-  warning = aWarning;
-};
-
+const options = {...data, checkConformance: true } ;
 [
  {
-    func: () => data.jsonText = JSON.stringify(1),
-    warning: "Manifest should be an object.",
+    func: () => options.jsonText = JSON.stringify(1),
+    warn: "Manifest should be an object.",
  },
  {
-    func: () => data.jsonText = JSON.stringify("a string"),
-    warning: "Manifest should be an object.",
+    func: () => options.jsonText = JSON.stringify("a string"),
+    warn: "Manifest should be an object.",
  },
  {
-    func: () => data.jsonText = JSON.stringify({
+    func: () => options.jsonText = JSON.stringify({
      scope: "https://www.mozilla.org",
    }),
-    warning: "The scope URL must be same origin as document.",
+    warn: "The scope URL must be same origin as document.",
  },
  {
-    func: () => data.jsonText = JSON.stringify({
+    func: () => options.jsonText = JSON.stringify({
      scope: "foo",
      start_url: "bar",
    }),
-    warning: "The start URL is outside the scope, so the scope is invalid.",
+    warn: "The start URL is outside the scope, so the scope is invalid.",
  },
  {
-    func: () => data.jsonText = JSON.stringify({
+    func: () => options.jsonText = JSON.stringify({
      start_url: "https://www.mozilla.org",
    }),
-    warning: "The start URL must be same origin as document.",
+    warn: "The start URL must be same origin as document.",
  },
  {
-    func: () => data.jsonText = JSON.stringify({
+    func: () => options.jsonText = JSON.stringify({
      start_url: 42,
    }),
-    warning: "Expected the manifest\u2019s start_url member to be a string.",
+    warn: "Expected the manifest\u2019s start_url member to be a string.",
  },
  {
-    func: () => data.jsonText = JSON.stringify({
+    func: () => options.jsonText = JSON.stringify({
      theme_color: "42",
    }),
-    warning: "theme_color: 42 is not a valid CSS color.",
+    warn: "theme_color: 42 is not a valid CSS color.",
  },
  {
-    func: () => data.jsonText = JSON.stringify({
+    func: () => options.jsonText = JSON.stringify({
      background_color: "42",
    }),
-    warning: "background_color: 42 is not a valid CSS color.",
+    warn: "background_color: 42 is not a valid CSS color.",
  },
-].forEach(function(test) {
+  {
+    func: () => options.jsonText = JSON.stringify({
+      icons: [
+        { "src": "http://exmaple.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",
+  },
+].forEach((test, index) => {
  test.func();
-
-  processor.process(data);
-
-  is(warning, test.warning, "Correct warning.");
-
-  warning = null;
-  data.manifestURL = manifestURL;
-  data.docURL = docURL;
+  const result = processor.process(options);
+  const [message] = result.moz_validation;
+  is(message.warn, test.warn, "Check warning.");
+  options.manifestURL = manifestURL;
+  options.docURL = docURL;
 });

-ConsoleAPI.prototype.warn = originalWarn;
  </script>
 </head>