Closed Bug 1083410 Opened 10 years ago Closed 10 years ago

Obtain a web manifest

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: marcosc, Assigned: marcosc)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(1 file, 19 obsolete files)

added a bit more wait time, just to be safe.
22.12 KB, patch
Details | Diff | Splinter Review
Now that we can process manifests, we now need a way of obtaining them form the network in a way that conforms to the W3C spec. Specifically, we need to respect same origin and CSP rules.
Assignee: nobody → mcaceres
For review if anyone is interested.
So, this can't really be implemented using XHR because of CSP. Need to implement this as part of link element itself.
Summary: Implement means to obtain a W3C manifest → Obtain a web manifest
Depends on: 1124638
Attached file Manifest obtainer, using Fetch API. (obsolete) —
Here is a rewrite that uses fetch instead. Works in Nightly.
@baku, you can probably ignore ManifestProcessor.jsm for now. I refactored it and just made mostly cosmetic changes - though, if you have any feedback, I'll take it!:) If you can focus on ManifestObtainer.jsm and the tests (browser_ManifestObtainer_obtain.js and resource.sjs) that would be great. I got @annevk to have a quick look also at the CSP stuff in ManifestObtainer and he said it looks ok. In resource.sjs, I don't yet use the HTTPStatus map - but will probably make use of it once the manifest CSP stuff lands.
Attachment #8517085 - Attachment is obsolete: true
Attachment #8553062 - Attachment is obsolete: true
Attachment #8557880 - Flags: review?(amarchesini)
Comment on attachment 8557880 [details] [diff] [review] 0001-Refactored-ManifestProcessor.patch Review of attachment 8557880 [details] [diff] [review]: ----------------------------------------------------------------- I would like to read it again with this comments fixed. Thanks. ::: dom/manifest/ManifestObtainer.jsm @@ +20,5 @@ > +const imports = {}; > +Components.utils.importGlobalProperties(['URL']); > +Components.utils.import('resource://gre/modules/XPCOMUtils.jsm', imports); > +imports.XPCOMUtils.defineLazyModuleGetter(imports, 'ManifestProcessor', > + 'resource://gre/modules/ManifestProcessor.jsm'); indentation @@ +28,5 @@ > +function ManifestObtainer() { > + const manifestQuery = 'link[rel~="manifest"]', > + processor = new imports.ManifestProcessor(); > + //expose public method `.obtainManifest()` > + Object.defineProperty(this, 'obtainManifest', { It's not the same thing: function ManifestObtainer() {} ManifestObtainer.prototype = { obtainManifest: function(contentWindow) { .. } }; @@ +32,5 @@ > + Object.defineProperty(this, 'obtainManifest', { > + value: obtainManifest > + }); > + > + function obtainManifest(contentWindow) { What about: return new Promise(function(resolve, reject) { ... reject(new Error(msg)); ... resolve(foobar); }; @@ +46,5 @@ > + let manifestURL = ''; > + //Will throw on "about:blank" and possibly other invalid URIs. > + try { > + manifestURL = new URL(elem.href, elem.baseURI).toString(); > + } catch (e) { if you do what I suggest, any exception is converted in a reject() @@ +71,5 @@ > + > + function processResponse(response) { > + return new Promise((resolve, reject) => { > + if (response.type === 'error' || response.status < 200 || > + response.status >= 300) { indentation ::: dom/manifest/ManifestProcessor.jsm @@ +55,3 @@ > * process method: processes json text into a clean manifest > * that conforms with the W3C specification. > + * @param json - the JSON string to be processd. it's called jsonText. @@ +72,5 @@ > + property, > + expectedType, > + dontTrim > + }) { > + const value = object[property], why const? and not let? @@ +110,5 @@ > object: manifest, > property: 'short_name', > expectedType: 'string' > }; > + return extractValue(spec); no trim? @@ +133,5 @@ > object: manifest, > property: 'display', > expectedType: 'string' > }; > + let value = extractValue(spec); why no trim is needed? @@ +222,5 @@ > + value.filter( > + item => !!processSrcMember(item, baseURL) > + ) > + .map(toIconObject) > + .forEach(icon => icons.push(icon)); the indentation here is correct, but up-to-you, think about this: value.filter(item => !!processSrcMember(item,baseURL)) .map(toIconObject) .forEach(icon => icon.push(icon)); @@ +320,5 @@ > function processIconsMember(manifest, manifestURL) { > const iconsProcessor = new IconsProcessor(); > return iconsProcessor.processIcons(manifest, manifestURL); > } > + extra spaces. ::: dom/manifest/moz.build @@ +11,5 @@ > > MOCHITEST_MANIFESTS += ['test/mochitest.ini'] > +BROWSER_CHROME_MANIFESTS += ['test/browser.ini'] > + > + no extra lines. ::: dom/manifest/test/browser_ManifestObtainer_obtain.js @@ +1,1 @@ > +/*global gBrowser, Components, todo, ise, test, finish, dump, waitForExplicitFinish*/ what is this line? @@ +1,4 @@ > +/*global gBrowser, Components, todo, ise, test, finish, dump, waitForExplicitFinish*/ > +'use strict'; > +const imports = {}; > +Components.utils.import('resource://gre/modules//ManifestObtainer.jsm', imports); extra / before ManifestObtainer @@ +36,5 @@ > + }, > + testData: `<link rel="manifest" href='http://mochi.test:8888/tests/dom/manifest/test/resource.sjs?body={"name":"pass"}'>` > + }, > + > + // CORS extra space @@ +47,5 @@ > + get testData() { > + const origin = 'http://mochi.test:8888/tests/dom/manifest/test/resource.sjs'; > + const body = 'body={"name": "pass"}'; > + const CORS = `Access-Control-Allow-Origin=` + new URL(this.tabURL).origin; > + const link = `<link many extra spaces in this file. ::: modules/libpref/init/all.js @@ +793,1 @@ > #ifdef MOZ_DEV_EDITION remove this ifdef completely. @@ +793,4 @@ > #ifdef MOZ_DEV_EDITION > pref("devtools.chrome.enabled", true); > #else > +pref("devtools.chrome.enabled", true); why do you need this? @@ +803,4 @@ > #ifdef MOZ_DEV_EDITION > pref("devtools.debugger.remote-enabled", true); > #else > +pref("devtools.debugger.remote-enabled", true); same here @@ +4415,4 @@ > pref("camera.control.face_detection.enabled", true); > > // Fetch API. > +pref("dom.fetch.enabled", true); I don't think we want to do this...
Attachment #8557880 - Flags: review?(amarchesini) → review-
fixed indentation + extra spaces - sorry, still getting familiar with Moz conventions! Refactored ManifestObtainer.obtainManifest to be on prototype. Rewrote obtainManifest to immediately return an unresolved promise, as suggested. That was a good suggestion, wrt the try/catch blocks! Fixed "it's called jsonText." in ManifestProcessor.jsm ... code comments :) > why const? and not let? Although the object in this case is not immutable, the variable is not supposed to be replaced. > no trim? The extractValue trims by default. Where no trim is needed, there is a {dontTrim: true} required. See, for instance, processStartURLMember. > what is this line? JSLint things - I've added a comment to explain what it is. > extra / before ManifestObtainer Fixed > many extra spaces in this file. Should be all fixed. > modules/libpref/init/all.js Sorry about this one. I forgot to remove this from the patch. I was using it for testing. Will send new patch soon.
Updated patch. This one includes fixes from prev. review + code to do CSP check. However, I have the CSP `manifest-src` implementation coming separate patch (bug 1089255). It was suggested there I land this first, then we land the other one. Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=709a429f585c
Attachment #8557880 - Attachment is obsolete: true
Attachment #8561915 - Flags: review?(amarchesini)
Blocks: 1089255
Depends on: 1130924
No longer blocks: 1089255
Depends on: 1089255
Comment on attachment 8561915 [details] [diff] [review] 0001-Bug-1083410-Obtain-a-web-manifest.patch Review of attachment 8561915 [details] [diff] [review]: ----------------------------------------------------------------- ah! sorry! I did the review and I forgot to submit it... it was hidden in one of my (too many) tabs. The main concern is about the style guideline. If you can adjust this and send me the patch for a new review, would be great. Thanks! ::: dom/manifest/ManifestObtainer.jsm @@ +17,5 @@ > + * Following line is used the JSHint. > + */ > + /*global Components*/ > +'use strict'; > +const imports = {}; any particular reason why you use this variable? @@ +46,5 @@ > + return reject(new Error(msg)); > + } > + // Will throw on "about:blank" and possibly other invalid URIs. > + const manifestURL = new URL(elem.href, elem.baseURI); > + // Wil throw if we can't do the check. Will @@ +50,5 @@ > + // Wil throw if we can't do the check. > + const contentPolicy = Cc['@mozilla.org/layout/content-policy;1'] > + .getService(Ci.nsIContentPolicy); > + if (contentPolicy.shouldLoad(Ci.nsIContentPolicy.TYPE_MANIFEST, > + toNsURI(elem), elem.ownerDocument.documentURIObject, elem, elem.type, if (contentPolicy.sholdLoad(Ci.nsIContentPolicy.TYPE_MANIFEST, toNsURI(elem), elem.ownerDocument.documentURIObject, elem, elem.type, null) != Ci.nsIContentPolicy.ACCEPT) { or something similar... @@ +70,5 @@ > + break; > + } > + const req = new contentWindow.Request(manifestURL, reqInit); > + req.setContext('manifest'); > + contentWindow.fetch(req).then(processResponse).then(resolve,reject); resolve,<space>reject @@ +77,5 @@ > + function processResponse(response) { > + return new Promise((resolve, reject) => { > + if (response.type === 'error' || response.status < 200 || > + response.status >= 300) { > + let msg = `Fetch error: ${response.status} - ${response.statusText}`; wow... never seen this js syntax. Is it ruby? :) @@ +84,5 @@ > + response.json().then((json) => { > + const args = { > + jsonText: JSON.stringify(json), > + manifestURL: response.url, > + docURL: contentWindow.location.href It's not clear to understand that contentWindow is in this scope. I see 2 options: 1. move this function just before line 74 into the Promise() block. 2. 2 params, response and the contentWindow. What do you think? ::: dom/manifest/ManifestProcessor.jsm @@ +1,4 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. > + * Revert this change. It's a common pattern to have the header in a independent comment block. @@ +19,2 @@ > /*exported EXPORTED_SYMBOLS */ > +/*JSLint options in comment below :*/ below: */ @@ +43,5 @@ > + 'portrait-primary', > + 'portrait-secondary', > + 'landscape-primary', > + 'landscape-secondary' > + ]); All of these changes are not strictly needed. right? Plus: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_practices It seems we don't like this pattern :) @@ +83,1 @@ > issueDeveloperWarning(msg); About this issue, what about if process() receives the resolve and reject functions? Then when something bad happens, you can reject(msg). Can it work? @@ +158,5 @@ > let msg = 'Scope needs to be same-origin as Document.'; > issueDeveloperWarning(msg); > return undefined; > } > + // If start URL is not within scope of scope URL: why this? @@ +177,5 @@ > + dontTrim: true > + }, > + value = extractValue(spec); > + let result = new URL(docURL), > + potentialResult; change these. @@ +239,5 @@ > expectedType: 'string' > }; > + let value = extractValue(spec); > + if (value) { > + value = netutil.parseContentType(value, charset, hadCharset); 2 spaces.
Attachment #8561915 - Flags: review?(amarchesini) → review-
(Just two small questions below... will prepare another patch) (In reply to Andrea Marchesini (:baku) from comment #8) > Comment on attachment 8561915 [details] [diff] [review] > 0001-Bug-1083410-Obtain-a-web-manifest.patch > > Review of attachment 8561915 [details] [diff] [review]: > ----------------------------------------------------------------- > > ah! sorry! I did the review and I forgot to submit it... it was hidden in > one of my (too many) tabs. Heh, I was starting to feel very unloved :) It's ok, I'm still blocked on other dependencies, so this review actually came at a good time. Really appreciate the detailed feedback. > The main concern is about the style guideline. If you can adjust this and > send me the patch for a new review, would be great. > Thanks! I'll try to adjust. Do people have particular tools they are using for styling code? Doing it manually doesn't seem to scale. > ::: dom/manifest/ManifestObtainer.jsm > @@ +17,5 @@ > > + * Following line is used the JSHint. > > + */ > > + /*global Components*/ > > +'use strict'; > > +const imports = {}; > > any particular reason why you use this variable? Yes, it prevents leakage onto the BackstagePass class. I've seen some pretty ugly abuses of people pulling things from backstage pass where they shouldn't be. Keeps everything clean. > @@ +46,5 @@ > > + return reject(new Error(msg)); > > + } > > + // Will throw on "about:blank" and possibly other invalid URIs. > > + const manifestURL = new URL(elem.href, elem.baseURI); > > + // Wil throw if we can't do the check. > Fixed. > > @@ +50,5 @@ > > + // Wil throw if we can't do the check. > > + const contentPolicy = Cc['@mozilla.org/layout/content-policy;1'] > > + .getService(Ci.nsIContentPolicy); > > + if (contentPolicy.shouldLoad(Ci.nsIContentPolicy.TYPE_MANIFEST, > > + toNsURI(elem), elem.ownerDocument.documentURIObject, elem, elem.type, > > if (contentPolicy.sholdLoad(Ci.nsIContentPolicy.TYPE_MANIFEST, > toNsURI(elem), > elem.ownerDocument.documentURIObject, > elem, elem.type, null) != > Ci.nsIContentPolicy.ACCEPT) { > > or something similar... Yeah, think I'm going to clean all that up. It's so ugly. > @@ +70,5 @@ > > + break; > > + } > > + const req = new contentWindow.Request(manifestURL, reqInit); > > + req.setContext('manifest'); > > + contentWindow.fetch(req).then(processResponse).then(resolve,reject); > > resolve,<space>reject Think I'm going to go with: contentWindow.fetch(req).then(processResponse).then(resolve).catch(reject); WDYT? > @@ +77,5 @@ > > + function processResponse(response) { > > + return new Promise((resolve, reject) => { > > + if (response.type === 'error' || response.status < 200 || > > + response.status >= 300) { > > + let msg = `Fetch error: ${response.status} - ${response.statusText}`; > > wow... never seen this js syntax. Is it ruby? :) Template strings, my friend... They are the new hot in JS (after promises, of course ;)). > @@ +84,5 @@ > > + response.json().then((json) => { > > + const args = { > > + jsonText: JSON.stringify(json), > > + manifestURL: response.url, > > + docURL: contentWindow.location.href > > It's not clear to understand that contentWindow is in this scope. > I see 2 options: > > 1. move this function just before line 74 into the Promise() block. > 2. 2 params, response and the contentWindow. > > What do you think? Went with 2. > ::: dom/manifest/ManifestProcessor.jsm > @@ +1,4 @@ > > /* This Source Code Form is subject to the terms of the Mozilla Public > > * License, v. 2.0. If a copy of the MPL was not distributed with this > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + * > > Revert this change. It's a common pattern to have the header in a > independent comment block. > > @@ +19,2 @@ > > /*exported EXPORTED_SYMBOLS */ > > +/*JSLint options in comment below :*/ > > below: */ Fixed. > @@ +43,5 @@ > > + 'portrait-primary', > > + 'portrait-secondary', > > + 'landscape-primary', > > + 'landscape-secondary' > > + ]); > > All of these changes are not strictly needed. right? > > Plus: > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ > Coding_Style#JavaScript_practices > > It seems we don't like this pattern :) The guidelines apply to Arrays, not Sets. The Set constructor doesn't have the ambiguity of the Array constructor - note also that array within the set is constructed correctly. > @@ +83,1 @@ > > issueDeveloperWarning(msg); > > About this issue, what about if process() receives the resolve and reject > functions? > Then when something bad happens, you can reject(msg). Can it work? The developer warnings are just warnings, the processor is "liberal" hence can't ever fail (or reject). However, I need to rewrite how error reporting will be done, but will do that separately. > @@ +158,5 @@ > > let msg = 'Scope needs to be same-origin as Document.'; > > issueDeveloperWarning(msg); > > return undefined; > > } > > + // If start URL is not within scope of scope URL: > > why this? Sorry, I'm not sure what you are asking here? If you are asking why "start URL" must be in scope, its because it wouldn't make sense to open the first page of an application if it's not in the scope of the application. Like: foo.com/myapp <- scope foo.com/somewhere/else/ <- Start URL. That would lead to sadness. > @@ +177,5 @@ > > + dontTrim: true > > + }, > > + value = extractValue(spec); > > + let result = new URL(docURL), > > + potentialResult; > > change these. fixed. > @@ +239,5 @@ > > expectedType: 'string' > > }; > > + let value = extractValue(spec); > > + if (value) { > > + value = netutil.parseContentType(value, charset, hadCharset); > > 2 spaces. Fixed.
I've included the changes you suggested - see the previous comment. I've also included test for CORS and CSP (marked as todo, as they waiting on a dependent bug). Let me know if you want me to break the patch up - with the tests, etc. it's getting a bit large.
Attachment #8570378 - Flags: review?(amarchesini)
Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67150af10fc9 I'm seeing that "nsMixedContentBlocker::ShouldLoad" is crashing, but not sure what to do about that?
Would be good to start landing some of this, so I can start making incremental improvements based on spec discussions. This just keeps growing locally ATM... maybe that's ok. Dunno.
Attachment #8561915 - Attachment is obsolete: true
Attachment #8570378 - Attachment is obsolete: true
Attachment #8570378 - Flags: review?(amarchesini)
Attachment #8576275 - Flags: feedback?(ehsan)
Andrea has looked at this before. Can he continue to review the next versions?
Flags: needinfo?(mcaceres)
oh no! I had reviewed 80% of the previous patch yesterday.
Attachment #8576275 - Flags: feedback?(ehsan) → feedback?(amarchesini)
Comment on attachment 8576275 [details] [diff] [review] 0001-Bug-1083410-Obtain-a-web-manifest.patch Review of attachment 8576275 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/manifest/ManifestObtainer.jsm @@ +33,5 @@ > +class ManifestObtainer { > + constructor() { > + throw new Error('Static use only.'); > + } > + static obtainManifest(contentWindow) { NIT: aContentWindow @@ +46,5 @@ > + let msg = 'No manifest to fetch.'; > + return reject(new Error(msg)); > + } > + // Will throw on "about:blank" and possibly other invalid URIs. > + const manifestURL = new URL(elem.href, elem.baseURI); actually you can do: const manifestURL = new aContentWindow.URL(elm.href, elem.baseURI); and remove : Cu.importGlobalProperties(['URL']); @@ +53,5 @@ > + let msg = 'Blocked by CSP.'; > + return reject(new Error(msg)); > + } > + const reqInit = { > + mode: 'no-cors' credentials: "same-origin" ? @@ +73,5 @@ > + .then(resolve) > + .catch(reject); > + }); > + > + function cspCanLoadManifest(elem) { NIT: aElem @@ +85,5 @@ > + elemURI, docURI, elem, elem.type, null); > + return shouldLoad === Ci.nsIContentPolicy.ACCEPT; > + } > + > + function processResponse(resp, contentWindow) { NIT: aResp, aContentWindow @@ +87,5 @@ > + } > + > + function processResponse(resp, contentWindow) { > + return new Promise((resolve, reject) => { > + const badStatus = resp.status < 200 || resp.status >= 300; extra space. ::: dom/manifest/ManifestProcessor.jsm @@ +42,5 @@ > + expectedType, > + dontTrim > +}, console) { > + const value = object[property], > + type = (Array.isArray(value)) ? 'array' : typeof value; 2 lines please: const value = object[property]; const type = (Array.isArray(value)) ? 'array' : typeof value; @@ +43,5 @@ > + dontTrim > +}, console) { > + const value = object[property], > + type = (Array.isArray(value)) ? 'array' : typeof value; > + let result; I guess you don't need this variable. @@ +44,5 @@ > +}, console) { > + const value = object[property], > + type = (Array.isArray(value)) ? 'array' : typeof value; > + let result; > + // We need to special-case "array", as it's not a JS primitive. this comment should go to line 46, right? @@ +51,5 @@ > + let msg = `Expected the ${objectName}'s ${property} `; > + msg += `member to a be a ${expectedType}.`; > + console.log(msg); > + } > + return result; return undefined; @@ +55,5 @@ > + return result; > + } > + // Trim string and returned undefined if the empty string. > + result = (expectedType === 'string' && value && !dontTrim) ? value.trim() || > + undefined : value; if (expectedType === 'string' && value && !dontTrim) { return alue.trim() || undefined; } return value; @@ +63,1 @@ > 'browser' indent this under 'fullscreen' @@ +63,4 @@ > 'browser' > ]); > +const orientationTypes = new Set(['any', 'natural', 'landscape', 'portrait', > + 'portrait-primary', 'portrait-secondary', 'landscape-primary', same here. @@ +79,3 @@ > } > + static get orientationTypes() { > + return orientationTypes; 2 spaces indentation @@ +79,4 @@ > } > + static get orientationTypes() { > + return orientationTypes; > + } same here. @@ +86,5 @@ > + * expecting the following dictionary items: > + * jsonText: the JSON string to be processd. > + * manifestURL: the URL of the manifest, to resolve URLs. > + * docURL: the URL of the owner doc, for security checks. > + */ and here. @@ +97,2 @@ > > + function processNameMember(manifest) { NIT: aManifest @@ +106,3 @@ > } > > + function processShortNameMember(manifest) { aManifest @@ +116,3 @@ > } > > + function processOrientationMember(manifest) { aManifest @@ +121,5 @@ > + object: manifest, > + property: 'orientation', > + expectedType: 'string', > + }, > + value = extractValue(spec, console), Split this in: const spec = { ...}; const value = extractValue(spec, console); if (ManifestProcessor.orientatioNType.has(value)) { return value; } return ''; @@ +131,2 @@ > > + function processDisplayMember(manifest) { NIT: aManifest @@ +136,5 @@ > + property: 'display', > + expectedType: 'string' > + }; > + let value = extractValue(spec, console); > + return (ManifestProcessor.displayModes.has(value)) ? value : What about: return (ManifestProcessor.displayModes.has(value)) ? value : ManifestProcessor.defaultDispayMode; @@ +142,3 @@ > } > > + function processScopeMember(manifest, manifestURL, docURL, startURL) { NIT: aManifest, AManifestURL, aDocURL, aStartURL @@ +148,5 @@ > + property: 'scope', > + expectedType: 'string', > + dontTrim: true > + }, > + value = extractValue(spec, console); const spec = { .. } const value = expectValue(spec, console); @@ +163,5 @@ > + console.warn(msg); > + return undefined; > + } > + // If start URL is not within scope of scope URL: > + if (startURL && startURL.origin !== scopeURL.origin || !startURL.pathname if (startURL && (startURL.origin !== scopeURL.origin || !startURL.pathName.startWith(scopeURL.pathName)) { ... @@ +175,4 @@ > } > + > + function processStartURLMember(manifest, manifestURL, docURL) { > + const spec = { 1. 2 spaces indentation. 2. NIT: aManifestURL, ... @@ +181,5 @@ > + property: 'start_url', > + expectedType: 'string', > + dontTrim: true > + }, > + value = extractValue(spec, console); split const spec = { ... }; const value = extractValue(spec, console); @@ +183,5 @@ > + dontTrim: true > + }, > + value = extractValue(spec, console); > + let result = new URL(docURL); > + let potentialResult; move these lines only when you need them: line 190? @@ +201,5 @@ > + result = potentialResult; > + } > + return result; > + } > + // Processing starts here! The indentation is wrong... @@ +219,5 @@ > + name: processNameMember(rawManifest), > + icons: IconsProcessor.process(rawManifest, manifestURL, console), > + short_name: processShortNameMember(rawManifest), > + }; > + processedManifest.scope = processScopeMember(rawManifest, manifestURL, processedManifest.scope = processScopeMember(rawManifest, manifestURL, docURL, processedManifest.start_url); @@ +226,3 @@ > } > +} > +class IconsProcessor { an empty line here and there will make this code more readable :) @@ +234,5 @@ > + } > + static get anyRegEx() { > + return new RegExp('any', 'i'); > + } > + static process(manifest, baseURL, console) { NIT: aManifest, ... @@ +241,5 @@ > object: manifest, > property: 'icons', > expectedType: 'array' > }, > + icons = [], 1. extra spaces. 2. split the consts: const spec = { ... }; const icons = []; const value = extractValue(spec, console); @@ +252,5 @@ > } > return icons; > > + function toIconObject(iconData) { > + const icon = { return { src: processSrcMember(iconData, baseURL), ... }; @@ +280,5 @@ > > function processDensityMember(icon) { > + const value = parseFloat(icon.density); > + const result = (Number.isNaN(value) || value === +Infinity || value <= 0) ? > + 1.0 : value; Indent this after the beginning of the if() condition. @@ +285,4 @@ > return result; > } > > function processSrcMember(icon, baseURL) { aIcon, aBaseURL @@ +295,2 @@ > }, > + value = extractValue(spec, console); split this :) @@ +305,2 @@ > > function processSizesMember(icon) { aIcon ? @@ +310,5 @@ > object: icon, > property: 'sizes', > expectedType: 'string' > + }, > + value = extractValue(spec, console); same here. @@ +325,5 @@ > return true; > } > size = size.toLowerCase(); > + const hasX = size.contains('x'); > + if (!hasX || size.indexOf('x') !== size.lastIndexOf('x')) { well, you don't use hasX right? we don't need this change. @@ +330,5 @@ > return false; > } > + // Split left of x for width, after x for height. > + const width = size.substring(0, size.indexOf('x')), > + height = size.substring(size.indexOf('x') + 1, size.length), can you split them? ::: dom/manifest/test/browser_ManifestObtainer_obtain.js @@ +28,5 @@ > + testData: ` > + <link rel="manifesto" href='${defaultURL}?body={"name":"fail"}'> > + <link rel="foo bar manifest bar test" href='${defaultURL}?body={"name":"pass"}'> > + <link rel="manifest" href='${defaultURL}?body={"name":"fail"}'>` > + }, { split this. OTherwise it's very hard to understand that this is another const. ::: dom/manifest/test/resource.sjs @@ +85,5 @@ > + response.setHeader(key, queryMap.get(key)); > + } > + > + function createQueryMap(request) { > + const queryMap = new Map(); 2 spaces indentation.
Attachment #8576275 - Flags: feedback?(amarchesini) → review+
Flags: needinfo?(mcaceres)
(In reply to Andrea Marchesini (back the 23rd or March) from comment #15) > Comment on attachment 8576275 [details] [diff] [review] > 0001-Bug-1083410-Obtain-a-web-manifest.patch > > Review of attachment 8576275 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/manifest/ManifestObtainer.jsm > @@ +33,5 @@ > > +class ManifestObtainer { > > + constructor() { > > + throw new Error('Static use only.'); > > + } > > + static obtainManifest(contentWindow) { > > NIT: aContentWindow Fixed > @@ +46,5 @@ > > + let msg = 'No manifest to fetch.'; > > + return reject(new Error(msg)); > > + } > > + // Will throw on "about:blank" and possibly other invalid URIs. > > + const manifestURL = new URL(elem.href, elem.baseURI); > > actually you can do: > > const manifestURL = new aContentWindow.URL(elm.href, elem.baseURI); > > and remove : Cu.importGlobalProperties(['URL']); Fixed > @@ +53,5 @@ > > + let msg = 'Blocked by CSP.'; > > + return reject(new Error(msg)); > > + } > > + const reqInit = { > > + mode: 'no-cors' > > credentials: "same-origin" ? Added. > @@ +73,5 @@ > > + .then(resolve) > > + .catch(reject); > > + }); > > + > > + function cspCanLoadManifest(elem) { > > NIT: aElem Fixed. > @@ +85,5 @@ > > + elemURI, docURI, elem, elem.type, null); > > + return shouldLoad === Ci.nsIContentPolicy.ACCEPT; > > + } > > + > > + function processResponse(resp, contentWindow) { > > NIT: aResp, aContentWindow Fixed > @@ +87,5 @@ > > + } > > + > > + function processResponse(resp, contentWindow) { > > + return new Promise((resolve, reject) => { > > + const badStatus = resp.status < 200 || resp.status >= 300; > > extra space. Fixed. > ::: dom/manifest/ManifestProcessor.jsm > @@ +42,5 @@ > > + expectedType, > > + dontTrim > > +}, console) { > > + const value = object[property], > > + type = (Array.isArray(value)) ? 'array' : typeof value; > > 2 lines please: > > const value = object[property]; > const type = (Array.isArray(value)) ? 'array' : typeof value; Fixed. > @@ +43,5 @@ > > + dontTrim > > +}, console) { > > + const value = object[property], > > + type = (Array.isArray(value)) ? 'array' : typeof value; > > + let result; > > I guess you don't need this variable. Moved it. > @@ +44,5 @@ > > +}, console) { > > + const value = object[property], > > + type = (Array.isArray(value)) ? 'array' : typeof value; > > + let result; > > + // We need to special-case "array", as it's not a JS primitive. > > this comment should go to line 46, right? Fixed. > @@ +51,5 @@ > > + let msg = `Expected the ${objectName}'s ${property} `; > > + msg += `member to a be a ${expectedType}.`; > > + console.log(msg); > > + } > > + return result; > > return undefined; Fixed. > @@ +55,5 @@ > > + return result; > > + } > > + // Trim string and returned undefined if the empty string. > > + result = (expectedType === 'string' && value && !dontTrim) ? value.trim() || > > + undefined : value; > > if (expectedType === 'string' && value && !dontTrim) { > return alue.trim() || undefined; > } > > return value; Fixed > @@ +63,1 @@ > > 'browser' > > indent this under 'fullscreen' Maybe need to find a better beautifier. > @@ +63,4 @@ > > 'browser' > > ]); > > +const orientationTypes = new Set(['any', 'natural', 'landscape', 'portrait', > > + 'portrait-primary', 'portrait-secondary', 'landscape-primary', > > same here. As above. If change any whitespace, it just gets trashed by the beautifier. > @@ +79,3 @@ > > } > > + static get orientationTypes() { > > + return orientationTypes; > > 2 spaces indentation Fixed. > @@ +79,4 @@ > > } > > + static get orientationTypes() { > > + return orientationTypes; > > + } > > same here. Fixed. > > @@ +86,5 @@ > > + * expecting the following dictionary items: > > + * jsonText: the JSON string to be processd. > > + * manifestURL: the URL of the manifest, to resolve URLs. > > + * docURL: the URL of the owner doc, for security checks. > > + */ > > and here. Fixed. > > @@ +97,2 @@ > > > > + function processNameMember(manifest) { > > NIT: aManifest fixed > @@ +106,3 @@ > > } > > > > + function processShortNameMember(manifest) { > > aManifest Fixed. > @@ +116,3 @@ > > } > > > > + function processOrientationMember(manifest) { > > aManifest fixed > @@ +121,5 @@ > > + object: manifest, > > + property: 'orientation', > > + expectedType: 'string', > > + }, > > + value = extractValue(spec, console), > > Split this in: > > const spec = { ...}; > > const value = extractValue(spec, console); > if (ManifestProcessor.orientatioNType.has(value)) { > return value; > } > > return ''; Fixed. > @@ +131,2 @@ > > > > + function processDisplayMember(manifest) { > > NIT: aManifest > > @@ +136,5 @@ > > + property: 'display', > > + expectedType: 'string' > > + }; > > + let value = extractValue(spec, console); > > + return (ManifestProcessor.displayModes.has(value)) ? value : > > What about: > > return (ManifestProcessor.displayModes.has(value)) ? > value : ManifestProcessor.defaultDispayMode; Fixed to match previous. > @@ +142,3 @@ > > } > > > > + function processScopeMember(manifest, manifestURL, docURL, startURL) { > > NIT: aManifest, AManifestURL, aDocURL, aStartURL Fixed. > @@ +148,5 @@ > > + property: 'scope', > > + expectedType: 'string', > > + dontTrim: true > > + }, > > + value = extractValue(spec, console); > > const spec = { .. } > const value = expectValue(spec, console); Fixed. > @@ +163,5 @@ > > + console.warn(msg); > > + return undefined; > > + } > > + // If start URL is not within scope of scope URL: > > + if (startURL && startURL.origin !== scopeURL.origin || !startURL.pathname > > if (startURL && > (startURL.origin !== scopeURL.origin || > !startURL.pathName.startWith(scopeURL.pathName)) { > ... Cleaned it up using a variable. > @@ +175,4 @@ > > } > > + > > + function processStartURLMember(manifest, manifestURL, docURL) { > > + const spec = { > > 1. 2 spaces indentation. > 2. NIT: aManifestURL, ... Fixed. > @@ +181,5 @@ > > + property: 'start_url', > > + expectedType: 'string', > > + dontTrim: true > > + }, > > + value = extractValue(spec, console); > > split const spec = { ... }; > const value = extractValue(spec, console); Fixed. > @@ +183,5 @@ > > + dontTrim: true > > + }, > > + value = extractValue(spec, console); > > + let result = new URL(docURL); > > + let potentialResult; > > move these lines only when you need them: line 190? fixed. > @@ +201,5 @@ > > + result = potentialResult; > > + } > > + return result; > > + } > > + // Processing starts here! > > The indentation is wrong... Fixed. > @@ +219,5 @@ > > + name: processNameMember(rawManifest), > > + icons: IconsProcessor.process(rawManifest, manifestURL, console), > > + short_name: processShortNameMember(rawManifest), > > + }; > > + processedManifest.scope = processScopeMember(rawManifest, manifestURL, > > processedManifest.scope = processScopeMember(rawManifest, manifestURL, > docURL, > processedManifest.start_url); Beautification is automatic. > @@ +226,3 @@ > > } > > +} > > +class IconsProcessor { > > an empty line here and there will make this code more readable :) Fixed. > @@ +234,5 @@ > > + } > > + static get anyRegEx() { > > + return new RegExp('any', 'i'); > > + } > > + static process(manifest, baseURL, console) { > > NIT: aManifest, ... fixed > @@ +241,5 @@ > > object: manifest, > > property: 'icons', > > expectedType: 'array' > > }, > > + icons = [], > > 1. extra spaces. > 2. split the consts: > const spec = { ... }; > const icons = []; > const value = extractValue(spec, console); fixed > @@ +252,5 @@ > > } > > return icons; > > > > + function toIconObject(iconData) { > > + const icon = { > > return { > src: processSrcMember(iconData, baseURL), > ... > }; Fixed. > @@ +280,5 @@ > > > > function processDensityMember(icon) { > > + const value = parseFloat(icon.density); > > + const result = (Number.isNaN(value) || value === +Infinity || value <= 0) ? > > + 1.0 : value; > > Indent this after the beginning of the if() condition. > > @@ +285,4 @@ > > return result; > > } > > > > function processSrcMember(icon, baseURL) { > > aIcon, aBaseURL Fixed > @@ +295,2 @@ > > }, > > + value = extractValue(spec, console); > > split this :) > > @@ +305,2 @@ > > > > function processSizesMember(icon) { > > aIcon ? Fixed. > @@ +310,5 @@ > > object: icon, > > property: 'sizes', > > expectedType: 'string' > > + }, > > + value = extractValue(spec, console); > > same here. Fixed. > @@ +325,5 @@ > > return true; > > } > > size = size.toLowerCase(); > > + const hasX = size.contains('x'); > > + if (!hasX || size.indexOf('x') !== size.lastIndexOf('x')) { > > well, you don't use hasX right? we don't need this change. fixed > @@ +330,5 @@ > > return false; > > } > > + // Split left of x for width, after x for height. > > + const width = size.substring(0, size.indexOf('x')), > > + height = size.substring(size.indexOf('x') + 1, size.length), > > can you split them? Fixed. > ::: dom/manifest/test/browser_ManifestObtainer_obtain.js > @@ +28,5 @@ > > + testData: ` > > + > > + > > + ` > > + }, { > > split this. OTherwise it's very hard to understand that this is another const. It's not another const, it's another test in the tests array :) > ::: dom/manifest/test/resource.sjs > @@ +85,5 @@ > > + response.setHeader(key, queryMap.get(key)); > > + } > > + > > + function createQueryMap(request) { > > + const queryMap = new Map(); > > 2 spaces indentation. Fixed.
Status: NEW → ASSIGNED
Version: unspecified → Trunk
Fixes based on baku's review.
Attachment #8576275 - Attachment is obsolete: true
It breaks a couple of browser tests. If the fix is small and not logic, just land the new patch when fully green on try. Otherwise I can take a look next week.
Depends on: 1143201
@baku, it's failing because it depends on bug 1089255. I'll see if I can get that finished by the time you are back.
Removed CSP-related stuff and tests, as it obviously won't work yet :P I was testing locally against my implementation of CSP-manifest, hence it was working locally but of course it will fail on try. I'll send the CSP based stuff as part of 1089255. It will make it easier for the people involved in 1089255 to apply/review the CSP code. Resent to try (hopefully should go green now): https://treeherder.mozilla.org/#/jobs?repo=try&revision=56c1d2de7623
Attachment #8577453 - Attachment is obsolete: true
Blocks: 1143898
After talking to Mike Conley and Ehsan, we (they?) concluded that Manifest obtainer won't work as currently coded with e10s, so needs as rewrite to access all contentWindow stuff async. Will need to rewrite it.
As I need to rewrite the Obtainer class to work better with e10s, I'm sending the Processor class for integration first. Try results (currently running): https://treeherder.mozilla.org/#/jobs?repo=try&revision=15e940175655 Will set the keywords for checkin once the above finishes.
Attachment #8578297 - Attachment is obsolete: true
Fixed a bug caused by the way these B2G handles .jsm's. Was getting: `ReferenceError: can’t access lexical declaration ‘ManifestProcessor’ before initialization` Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7170adbb0cd1
Attachment #8579647 - Attachment is obsolete: true
Keywords: checkin-needed, leave-open
A more e10s friendly refactor. Q: where should manifestMessages.js go?
Attachment #8580168 - Attachment is obsolete: true
Attachment #8582781 - Flags: feedback?(mconley)
So I guess for me, the big question is whether or not we are OK adding yet another framescript to be loaded in browser.js. Like, I see multiple choices here: 1) Load the framescript separately, as you're currently doing. If / when B2G ever needs that stuff, the framescript will be available for it to load as well. 2) Have content.js import the script via Services.scriptloader.loadSubScript. That has the advantage of putting all of the multi/single-process framescript stuff into a single place. 3) Or, instead of content.js, put it into browser-content.js in toolkit/, so that other XUL apps can get it as well. Again, you could use loadSubScript there to bring in the code. 4) Directly add the code to content.js / browser-content.js, and worry about B2G when the time comes. 5) Add some _new_ shared framescript that is shared between B2G, XUL apps, and browser, and put the code in there. I'm leaning towards 2 because that keeps Marco's code compartmentalized, but also lets it be imported by B2G easily if/when it needs it. It also means not adding another loadFrameScript call. But that's just my opinion. I don't know who else to ask about this though... I guess I'm going to pick on Mossop to see if he has an opinion.
Flags: needinfo?(dtownsend)
Fabrice, Ted, your input would be appreciated re: b2g aspects. See Mike's comment above.
Flags: needinfo?(tclancy)
Flags: needinfo?(fabrice)
I don't see a big problem with adding another loadFrameScript call so if this is something that will be shared across multiple products then that is probably the easiest choice.
Flags: needinfo?(dtownsend)
Depends on: 1147193
In b2g we load these kind of scripts here: http://mxr.mozilla.org/mozilla-central/source/dom/ipc/preload.js#90 So if you don't do 4) this should be trivial on b2g.
Flags: needinfo?(fabrice)
Let's go with (1) then.
Flags: needinfo?(tclancy)
Attachment #8582781 - Flags: feedback?(mconley) → review?(mconley)
Depends on: 1147714
Comment on attachment 8582781 [details] [diff] [review] 0001-Bug-1083410-Obtain-a-web-manifest.patch Review of attachment 8582781 [details] [diff] [review]: ----------------------------------------------------------------- So I haven't looked this through all the way yet, but just a forewarning - I just talked to the fine folks in #jsapi, and ES6 classes are very much Nightly-only, and are not yet ready for production use. So we'll have to use the old prototype syntax. I'll keep digging at this patch, but this is an r- just for that (since this code would immediately break on Aurora uplift).
Attachment #8582781 - Flags: review?(mconley) → review-
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #35) > Comment on attachment 8582781 [details] [diff] [review] > 0001-Bug-1083410-Obtain-a-web-manifest.patch > > Review of attachment 8582781 [details] [diff] [review]: > ----------------------------------------------------------------- > > So I haven't looked this through all the way yet, but just a forewarning - I > just talked to the fine folks in #jsapi, and ES6 classes are very much > Nightly-only, and are not yet ready for production use. So we'll have to use > the old prototype syntax. No probs, just pretend class says function. Swapping back to pre-classes code should only take a few minutes. > I'll keep digging at this patch, but this is an r- just for that (since this > code would immediately break on Aurora uplift). Glad I had a chance to play with class tho :)
Comment on attachment 8582781 [details] [diff] [review] 0001-Bug-1083410-Obtain-a-web-manifest.patch Review of attachment 8582781 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +940,5 @@ > let mm = window.getGroupMessageManager("browsers"); > mm.loadFrameScript("chrome://browser/content/content.js", true); > mm.loadFrameScript("chrome://browser/content/content-UITour.js", true); > + mm.loadFrameScript("chrome://global/content/manifestMessages.js", true); > + Nit - trailing ws ::: dom/ipc/jar.mn @@ +10,4 @@ > content/global/BrowserElementPanning.js (../browser-element/BrowserElementPanning.js) > * content/global/BrowserElementPanningAPZDisabled.js (../browser-element/BrowserElementPanningAPZDisabled.js) > content/global/preload.js (preload.js) > + content/global/manifestMessages.js (manifestMessages.js) Nit - Might as well keep the preload scripts closer together. Let's just move this to before preload.js ::: dom/ipc/manifestMessages.js @@ +11,5 @@ > + * > + * BUG: https://bugzilla.mozilla.org/show_bug.cgi?id=1083410 > + * exported ManifestObtainer > + * > + * Following line is used the JSHint. "is used the JSHint" -> "is used by JSHint"? I'm not even sure that line is necessary, tbh. @@ +15,5 @@ > + * Following line is used the JSHint. > + */ > +/*globals content, sendAsyncMessage, addMessageListener, Components*/ > +'use strict'; > +const imports = {}; I don't think you need imports at all, tbh. You should feel free to import things to this. @@ +92,5 @@ > + }); > +} > + > +/* > +function cspCanLoadManifest(aElem) { Please remove dead code, or re-enable it. @@ +106,5 @@ > + return shouldLoad === Ci.nsIContentPolicy.ACCEPT; > +} > +*/ > + > +function processResponse(aResp, aContentWindow) { Why does this need to be Promise-based? I don't see this needing to be async at all. @@ +124,5 @@ > + docURL: aContentWindow.location.href > + }; > + const processor = new imports.ManifestProcessor(); > + const manifest = processor.process(args); > + resolve(JSON.stringify(manifest)); Not necessary to stringify here or above, I believe. ::: dom/manifest/ManifestObtainer.jsm @@ +8,5 @@ > + * Exposes public method `.obtainManifest(browserWindow)`, which returns > + * a promise. If successful, you get back a manifest (string). > + * > + * For e10s compat, this JSM relies on the following to do > + * the nessesary IPC: Multiple trailing whitespace in this header. @@ +49,5 @@ > + return Promise.reject(err); > + } > + const { > + messageManager > + } = aBrowserWindow; This is kind of a weird construct. I know deconstruction is cool, but I think this is easier to read for others: const messageManager = aBrowserWindow.messageManager; Or, more commonly: const mm = aBrowserWindow.messageManager; @@ +52,5 @@ > + messageManager > + } = aBrowserWindow; > + > + return new Promise((resolve, reject) => { > + const msgId = UUIDGenerator.generateUUID().number; This is pretty heavy-handed. An int counter is probably fine. @@ +82,5 @@ > + if (!registeredBrowsers.has(aBrowserWindow)) { > + registeredBrowsers.set(aBrowserWindow, new Map()); > + } > + const browserMap = registeredBrowsers.get(aBrowserWindow); > + const listenerMap = browserMap.get(aMsgId) || browserMap.set(aMsgId, new Map()).get(aMsgId); This is pretty terse. How about: if (!browserMap.has(aMsgId)) { browserMap.set(aMsgId, new Map()); } const listenerMap = browserMap.get(aMsgId); @@ +89,5 @@ > + aBrowserWindow.messageManager.addMessageListener(aMsgKey, handler); > + } > + > + function toError(aErrorClone) { > + const error = new Error(); Don't know how possible it is, but it'd be good if Error was also automatically serialized and deserialized like this by the message managers. We do something similar with nsIPrincipal's. Might be worth filing a bug for. @@ +97,5 @@ > + } > + > + function deregisterMessageListeners(aMsgId, aBrowserWindow) { > + const listenerMap = registeredBrowsers.get(aBrowserWindow); > + const listeners = listenerMap.get(aMsgId); Should probably ensure that listenerMap and listeners are defined. @@ +112,5 @@ > + } > + } > +} > + > +this.EXPORTED_SYMBOLS = ['ManifestObtainer']; Convention has us put this near the top of the file. @@ +113,5 @@ > + } > +} > + > +this.EXPORTED_SYMBOLS = ['ManifestObtainer']; > +this.ManifestObtainer = ManifestObtainer; This is redundant. ::: dom/manifest/test/browser_ManifestObtainer_obtain.js @@ +1,4 @@ > +//Used by JSHint: > +/*global SpecialPowers, todo_is, gBrowser, Components, Assert, finish, dump, waitForExplicitFinish*/ > +'use strict'; > +const imports = {}; I believe you can just import these globally instead of in imports. Also, "Cu" should be available for you globally, as a shortcut to Components.utils. @@ +106,5 @@ > + href='${remoteURL}?${body}&${CORS}'>`; > + return link; > + } > + }, > + //CSP tests Commented out code should be removed. @@ +197,5 @@ > + ]; > + for (let test of tests) yield test; > +} > + > +function* runTestInTab(test) { We have a tool for this already in BrowserTestUtils: https://hg.mozilla.org/mozilla-central/file/8af276ab8636/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#l53 @@ +290,5 @@ > + }, err => dump('Test went bad!!! :( ' + err.message)); > + }); > +} > + > +function test() { We should be using tasks instead of test / waitForExplicitFinish. Mochitest-browser tests support: add_task(function* someGenerator() { // yield promises in here to temporarily halt execution until // promise resolves. Will show test failure and nice trace if // promise rejects. }); ::: dom/manifest/test/manifestLoader.html @@ +1,3 @@ > +<!doctype html> > +<meta charset=utf-8> > +<!-- Nit - trailing whitespace ::: dom/manifest/test/resource.sjs @@ +77,5 @@ > + let body = queryMap.get('body') || ''; > + queryMap.delete('body'); > + response.write(body); > + } > + for (let key of queryMap.keys()) { for (let [key, value] of queryMap) { ... }
Updated manifest processor to not use classes. Just threw everything on the prototype.
Can someone please check-in attachment 8587402 [details] [diff] [review] for me?
Keywords: checkin-needed
(In reply to Marcos Caceres [:marcosc] from comment #39) > Can someone please check-in attachment 8587402 [details] [diff] [review] for > me? Is that the same patch that has already been r+ somewhere? Asking because I see things that reviewers asked you to fix in a previous patch (like not coalescing the license comment with your own. I didn't look further).
Also, why do we even want that on aurora given that this is dead code with no users?
Keywords: checkin-needed
(In reply to Fabrice Desré [:fabrice] from comment #41) > (In reply to Marcos Caceres [:marcosc] from comment #39) > Is that the same patch that has already been r+ somewhere? It's the same that Baku reviewed. > Asking because I > see things that reviewers asked you to fix in a previous patch (like not > coalescing the license comment with your own. I didn't look further). Might have misunderstood the license comment. Can fix that quickly. Will see if there is anything else that is relevant, but last I checked it was ok.
(In reply to Fabrice Desré [:fabrice] from comment #42) > Also, why do we even want that on aurora given that this is dead code with > no users? It's valid that this doesn't need to be in Aurora. It's also a chicken/egg problem. People can't use code that is not there, if the code is there, people will use it in products.
Attached patch Addressed :mconley's feedback. (obsolete) — — Splinter Review
Sent to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f7a9c902ad8
Attachment #8582781 - Attachment is obsolete: true
Attachment #8587472 - Flags: review?(mconley)
(In reply to Marcos Caceres [:marcosc] from comment #44) > (In reply to Fabrice Desré [:fabrice] from comment #42) > > Also, why do we even want that on aurora given that this is dead code with > > no users? > > It's valid that this doesn't need to be in Aurora. It's also a chicken/egg > problem. People can't use code that is not there, if the code is there, > people will use it in products. On aurora there will never be users of this chrome code, it's too late.
(In reply to Fabrice Desré [:fabrice] from comment #46) > On aurora there will never be users of this chrome code, it's too late. Can you point me to an example of how to exclude this from Aurora? I've not seen any documentation explaining how that works (something about NIGHTLY_BUILD somewhere?).
(In reply to Marcos Caceres [:marcosc] from comment #47) > (In reply to Fabrice Desré [:fabrice] from comment #46) > > On aurora there will never be users of this chrome code, it's too late. > > Can you point me to an example of how to exclude this from Aurora? I've not > seen any documentation explaining how that works (something about > NIGHTLY_BUILD somewhere?). It's not in the aurora build right now, right? So just don't try to land it there.
> It's not in the aurora build right now, right? Correct. It was backed out, but with the side-effect that it was also backed out of master. > So just don't try to land it there. I'm not. I'm just trying to get this merged into master.
Attachment #8587402 - Attachment is obsolete: true
Fixed the combined license in attachment 8587548 [details] [diff] [review].
Can someone check in attachment 8587548 [details] [diff] [review] for me?
Keywords: checkin-needed
The review history is really confusing. Who reviewed that attachment?
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #53) > The review history is really confusing. Who reviewed that attachment? The original code (which used classes) was reviewed by baku, and the small changes that remove ES classes were reviewed by Ehsan.
Flags: needinfo?(ryanvm)
* Using checkin-needed is preferred over ni? a specific person for getting your patches landed in a timely manner. * Why do you still have a pending review on one of the patches?
Flags: needinfo?(ryanvm)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #55) > * Using checkin-needed is preferred over ni? a specific person for getting > your patches landed in a timely manner. Np. I was responding to your questions. > * Why do you still have a pending review on one of the patches? Sorry for the confusion. It's because that patch is still waiting for r+ from mconley. As it usually takes about 1-2 month for anything to be reviewed and land, I figured it would have been simpler if attachment 8587548 [details] [diff] [review] landed first while I wait for review of the other code.
Keywords: checkin-needed
Comment on attachment 8587548 [details] [diff] [review] 0001-Bug-1083410-Refactor-manifest-processor-for-Aurora-c.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/f9585613bd1a
Attachment #8587548 - Flags: checkin+
Comment on attachment 8587472 [details] [diff] [review] Addressed :mconley's feedback. Review of attachment 8587472 [details] [diff] [review]: ----------------------------------------------------------------- Just a few more questions, Marcos, before I feel like I can give an r+. ::: browser/base/content/browser.js @@ +946,1 @@ > window.messageManager.addMessageListener("Browser:LoadURI", RedirectLoad); Nit: Let's keep the newline between these. ::: dom/ipc/manifestMessages.js @@ +66,5 @@ > + }; > + switch (elem.crossOrigin) { > + case 'use-credentials': > + reqInit.credentials = 'include'; > + reqInit.mode = 'cors'; So reqInit.mode will always be cors? Why is it set to no-cors when initiall defined then? ::: dom/manifest/ManifestObtainer.jsm @@ +30,5 @@ > +function ManifestObtainer() {} > + > +// public static msgKeys > +const msgkeys = Object.freeze({ > + 'success': 'dom:manifestobtainer:success', Let's case these like: DOM:ManifestObtainer:Success That's more in tune with how we name the rest of our messages. @@ +56,5 @@ > + // register error handler > + registerOneShotListener(msgId, msgkeys.error, aBrowserWindow, (msg) => { > + reject(toError(msg.data)); > + }); > + aBrowserWindow.messageManager.sendAsyncMessage(msgkeys.obtain); Shouldn't we send the msgId down with the message as well? That way, when we get a response, we know we're calling the expected handler? @@ +62,5 @@ > + > + function makeHandlerClosure(aMsgId, aAction) { > + return function(msg) { > + const { > + target: browserWindow This is kind of an awkward construct. Why not just use msg.target down in deregisterMessageListeners? @@ +87,5 @@ > + > + function toError(aErrorClone) { > + const error = new Error(); > + Object.getOwnPropertyNames(aErrorClone) > + .map(key => error[key] = aErrorClone[key]); I think this is kind of an abuse of .map, since .map is supposed to result in a returned iterable with the function applied to each element... here, you're setting something on an external for each element. This is probably simpler: for (let name of Object.getOwnPropertyNames(aErrorClone)) { error[key] = aErrorClone[key]; } @@ +105,5 @@ > + for (let [msgKey, listener] of listeners.entries()) { > + aBrowserWindow.messageManager.removeMessageListener(msgKey, listener); > + } > + listenerMap.delete(aMsgId); > + // If there are no other depedent listeners, remove the browser Nit "dependent". @@ +106,5 @@ > + aBrowserWindow.messageManager.removeMessageListener(msgKey, listener); > + } > + listenerMap.delete(aMsgId); > + // If there are no other depedent listeners, remove the browser > + if (listenerMap.size === 0) { if (!listenerMap.size) ::: dom/manifest/test/browser_ManifestObtainer_obtain.js @@ +1,4 @@ > +//Used by JSHint: > +/*global Cu, add_task, SpecialPowers, todo_is, gBrowser, Assert, dump*/ > +'use strict'; > +const imports = {}; Why imports still? @@ +116,5 @@ > + ]; > + for (let test of tests) yield test; > +} > + > +function* runTestInTab(test) { Nothing calls this. I think this function is no longer needed. @@ +172,5 @@ > + * Open a bunch of tabs and load manifests > + * in each tab. They should all return pass. > + */ > +add_task(function* () { > + yield new Promise((resolve) => { Instead of yielding this Promise that has a bunch of Promises inside it then'ing, you should just yield the Promises you're then'ing in this function. Example: add_task(function* () { const defaultPath = ... // ... let browsers = [ gBrowser.addTab(tabURL1, { skipAnimation: true }), gBrowser.addTab(tabURL2, { skipAnimation: true }), gBrowser.addTab(tabURL3, { skipAnimation: true }), ]; let loads = browsers.map((browser) => { return BrowserTestUtils.browserLoaded(browser); }); yield Promise.all(loads); const requests = [ // ... ]; let results = yield Promise.all(requests); // Do checks and tear down }); @@ +189,5 @@ > + gBrowser.addTab(tabURL3, tabOptions).linkedBrowser > + ]; > + > + // Collect promises that the pages have loaded. > + let pageShowPromises = browsers.map(browser => { You should be able to use BrowserTestUtils.browserLoaded: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm?from=BrowserTestUtils.jsm#80 @@ +204,5 @@ > + Promise.all(pageShowPromises).then(() => { > + > + // Collect promises to obtain manifests > + const requests = [ > + obtainer.obtainManifest(browsers[0]), Any particular reasoning for this order? @@ +222,5 @@ > + Assert.strictEqual(results.length, requests.length, expected); > + > + expected = 'Expect every manifest to have name equal to `pass`.'; > + const everyManifestPass = results > + //.map(manifest => JSON.parse(manifest)) Please remove dead code. @@ +232,5 @@ > + .map(browser => gBrowser.getTabForBrowser(browser)) > + .forEach(tab => gBrowser.removeTab(tab)); > + resolve(); > + }); > + }, err => dump('Test went bad!!! :( ' + err.message)); I don't think this error message adds much. If you reject a promise inside an add_task, you get the message in the rejection. @@ +235,5 @@ > + }); > + }, err => dump('Test went bad!!! :( ' + err.message)); > + }); > +}); > + Too many blank lines down here.
Attachment #8587472 - Flags: review?(mconley) → review-
Depends on: 1154464
Depends on: 1153958
* simplified manifest obtainer design and message passing. * works nice with e10s now.
Attachment #8587472 - Attachment is obsolete: true
Attachment #8592956 - Flags: review?(mconley)
Comment on attachment 8592956 [details] [diff] [review] Addressed :mconley's second round feedback. Review of attachment 8592956 [details] [diff] [review]: ----------------------------------------------------------------- If you can answer these questions, I can probably give you an r+ today - though I want to point out that I'm not a DOM peer, just an e10s guy who knows messaging. You might want to double-check with ehsan that my review is sufficient to land this stuff. ::: dom/ipc/manifestMessages.js @@ +49,5 @@ > + 'stack': String(aError.stack), > + 'message': String(aError.message), > + 'name': String(aError.name) > + }; > + return Components.utils.cloneInto(clone, content); I'm curious - why are we using cloneInto here if we've already serialized the Error into an object of strings? @@ +104,5 @@ > + aResp > + .json() > + .then((json) => { > + const args = { > + jsonText: JSON.stringify(json), Seems a bit odd to convert to an Object from JSON, and then stringify again, but I guess this ensures that it's valid JSON. @@ +110,5 @@ > + docURL: aContentWindow.location.href > + }; > + const processor = new ManifestProcessor(); > + const manifest = processor.process(args); > + resolve(Components.utils.cloneInto(manifest, content)); I'm reasonably certain you don't need to use cloneInto here... am I wrong? ::: dom/manifest/ManifestObtainer.jsm @@ +22,5 @@ > + */ > +'use strict'; > +this.EXPORTED_SYMBOLS = ['ManifestObtainer']; > + > +const _msgKey = 'DOM:ManifestObtainer:Obtain'; Global consts, as I understand it, are generally not prefixed with _, but are either ALL_CAPS or prefixed with a k. Let's just call this MSG_KEY. @@ +36,5 @@ > + return new Promise((resolve, reject) => { > + const mm = aBrowserWindow.messageManager; > + mm.sendAsyncMessage(_msgKey); > + const onMessage = (msg) => { > + mm.removeMessageListener(_msgKey, onMessage); So we're not using the ID technique here anymore. Do we care if the Promises might not be resolved in order? Like, if the caller calls this function twice on some web property, two Promises are now waiting to resolve. As soon as the first message comes back, both Promises will resolve with that result. Do we care about that?
Attached patch Added msgIDs again. (obsolete) — — Splinter Review
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #62) > Comment on attachment 8592956 [details] [diff] [review] > Addressed :mconley's second round feedback. > > Review of attachment 8592956 [details] [diff] [review]: > ----------------------------------------------------------------- > > If you can answer these questions, I can probably give you an r+ today - > though I want to point out that I'm not a DOM peer, just an e10s guy who > knows messaging. You might want to double-check with ehsan that my review is > sufficient to land this stuff. > > ::: dom/ipc/manifestMessages.js > @@ +49,5 @@ > > + 'stack': String(aError.stack), > > + 'message': String(aError.message), > > + 'name': String(aError.name) > > + }; > > + return Components.utils.cloneInto(clone, content); > > I'm curious - why are we using cloneInto here if we've already serialized > the Error into an object of strings? Correct, I don't need it here. However, below (for manifest) it seems so. > @@ +104,5 @@ > > + aResp > > + .json() > > + .then((json) => { > > + const args = { > > + jsonText: JSON.stringify(json), > > Seems a bit odd to convert to an Object from JSON, and then stringify again, > but I guess this ensures that it's valid JSON. Agree. Switching back to aResp.text(). The manifest processor has built-in error recovery for malformed json. > @@ +110,5 @@ > > + docURL: aContentWindow.location.href > > + }; > > + const processor = new ManifestProcessor(); > > + const manifest = processor.process(args); > > + resolve(Components.utils.cloneInto(manifest, content)); > > I'm reasonably certain you don't need to use cloneInto here... am I wrong? It gets upset if I remove that: [JavaScript Warning: "Security wrapper denied access to property then on privileged Javascript object. Support for exposing privileged objects to untrusted content via __exposedProps__ is being gradually removed - use WebIDL bindings or Components.utils.cloneInto instead. Note that only the first denied property access from a given global object will be reported."] > ::: dom/manifest/ManifestObtainer.jsm > @@ +22,5 @@ > > + */ > > +'use strict'; > > +this.EXPORTED_SYMBOLS = ['ManifestObtainer']; > > + > > +const _msgKey = 'DOM:ManifestObtainer:Obtain'; > > Global consts, as I understand it, are generally not prefixed with _, but > are either ALL_CAPS or prefixed with a k. Let's just call this MSG_KEY. fixed. > @@ +36,5 @@ > > + return new Promise((resolve, reject) => { > > + const mm = aBrowserWindow.messageManager; > > + mm.sendAsyncMessage(_msgKey); > > + const onMessage = (msg) => { > > + mm.removeMessageListener(_msgKey, onMessage); > > So we're not using the ID technique here anymore. > > Do we care if the Promises might not be resolved in order? No. However, we obviously care if a caller was to get the wrong manifest. > Like, if the > caller calls this function twice on some web property, two Promises are now > waiting to resolve. As soon as the first message comes back, both Promises > will resolve with that result. Do we care about that? Not really about that case, (as you would get the same result). But generally, don't want stuff to get out of sync. I've added the ids again once.
Attachment #8594157 - Flags: review?(mconley)
Attachment 8594157 [details] [diff] obsoletes 8592956. Sorry, forgot to check the box.
Attachment #8592956 - Attachment is obsolete: true
Attachment #8592956 - Flags: review?(mconley)
Comment on attachment 8594157 [details] [diff] [review] Added msgIDs again. Review of attachment 8594157 [details] [diff] [review]: ----------------------------------------------------------------- This looks OK to me - but you'll probably want to check with a DOM peer to see if my review is enough to land. Great work! ::: dom/ipc/manifestMessages.js @@ +112,5 @@ > + docURL: aContentWindow.location.href > + }; > + const processor = new ManifestProcessor(); > + const manifest = processor.process(args); > + resolve(Components.utils.cloneInto(manifest, content)); Components.utils -> Cu ::: dom/manifest/ManifestObtainer.jsm @@ +74,5 @@ > + // If we we've processed all messages, > + // stop listening. > + if (!browsersMap.get(browserWindow).size) { > + browsersMap.delete(browserWindow); > + this.removeMessageListener(MSG_KEY, onMessage); Let's just use mm from the closure here instead of "this" - I think that's clearer.
Attachment #8594157 - Flags: review?(mconley) → review+
Attached patch Final manifest obtainer... (obsolete) — — Splinter Review
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #65) > Comment on attachment 8594157 [details] [diff] [review] > ::: dom/ipc/manifestMessages.js > @@ +112,5 @@ > > + docURL: aContentWindow.location.href > > + }; > > + const processor = new ManifestProcessor(); > > + const manifest = processor.process(args); > > + resolve(Components.utils.cloneInto(manifest, content)); > > Components.utils -> Cu fixed. > ::: dom/manifest/ManifestObtainer.jsm > @@ +74,5 @@ > > + // If we we've processed all messages, > > + // stop listening. > > + if (!browsersMap.get(browserWindow).size) { > > + browsersMap.delete(browserWindow); > > + this.removeMessageListener(MSG_KEY, onMessage); > > Let's just use mm from the closure here instead of "this" - I think that's > clearer. Fixed.
Attachment #8587548 - Attachment is obsolete: true
Attachment #8594157 - Attachment is obsolete: true
Blocks: 1089255
No longer depends on: 1089255
No longer depends on: 1154464
Try: https://hg.mozilla.org/try/rev/95f768f0dd2e (if all green will set to checkin needed)
Attachment #8594275 - Attachment is obsolete: true
Keywords: leave-open
Tests are timing out, so I'm reducing number of iterations one of the tests does (from 1000 to 100): https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dc27ea20b96
Attachment #8596015 - Attachment is obsolete: true
Seems all good now on the slow ones with requestLongerTimeout(4): https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e48d7fe8b75
Attachment #8596231 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b90ed17f245d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Blocks: 1162729
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: