From 1118e89526716c1c1f9bdc62cbe246fbe9287621 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcos=20C=C3=A1ceres?= Date: Tue, 9 Jul 2019 02:33:39 +0000 Subject: [PATCH] Bug 1563460 - Collect console messages generated by processing manifest for Dev Tools r=baku MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- dom/locales/en-US/chrome/dom/dom.properties | 2 + dom/manifest/ImageObjectProcessor.jsm | 21 ++++-- dom/manifest/ManifestObtainer.jsm | 16 +++-- dom/manifest/ManifestProcessor.jsm | 51 +++++++++----- dom/manifest/ValueExtractor.jsm | 34 ++++----- .../test/test_ManifestProcessor_warnings.html | 70 +++++++++---------- 6 files changed, 108 insertions(+), 86 deletions(-) diff --git a/dom/locales/en-US/chrome/dom/dom.properties b/dom/locales/en-US/chrome/dom/dom.properties index aa7e88f683d0..c6fa4b06641c 100644 --- 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 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’). diff --git a/dom/manifest/ImageObjectProcessor.jsm b/dom/manifest/ImageObjectProcessor.jsm index 6428885f9087..b9b0961987fb 100644 --- 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; } diff --git a/dom/manifest/ManifestObtainer.jsm b/dom/manifest/ManifestObtainer.jsm index 794dc2102623..edcc9b01acca 100644 --- 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} 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} 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; } diff --git a/dom/manifest/ManifestProcessor.jsm b/dom/manifest/ManifestProcessor.jsm index d830b114cbcd..fcfd6b4a5764 100644 --- 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; } diff --git a/dom/manifest/ValueExtractor.jsm b/dom/manifest/ValueExtractor.jsm index 60eb4b87a4f2..04f311bc5130 100644 --- 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; diff --git a/dom/manifest/test/test_ManifestProcessor_warnings.html b/dom/manifest/test/test_ManifestProcessor_warnings.html index c39e2a67875c..da6943237ec1 100644 --- 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