Issue 29716600: Issue 6292 - Make issue reporter compatible with test server (Closed)
|
Created:
March 7, 2018, 3:43 p.m. by saroyanm Modified:
March 13, 2018, 11:20 a.m. Reviewers:
a.giammarchi CC:
Thomas Greiner Visibility:
Public. |
DescriptionIssue 6292 - Make issue reporter compatible with test server
Patch Set 1 : #
Total comments: 18
Patch Set 2 : Rebased #Patch Set 3 : Addressed Andrea's comments #
Total comments: 2
Patch Set 4 : Fixed nits #
MessagesTotal messages: 11
This is ready for the review. https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.html File issue-reporter.html (right): https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.html... issue-reporter.html:104: <button id="showDataClose" class="i18n_cancel primary"></button> The buttons are still broken, because currently this is using general messages.json from the adblockpluschrome repo -> https://hg.adblockplus.org/adblockpluschrome/file/tip/_locales/en_US/messages... Make sense to fix that separately I think, because that requires also updating the missing translations from the original repo to common.json and updating them back to the crowdin. https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.js File issue-reporter.js (left): https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.js#o... issue-reporter.js:20: window.ext = {}; We are safe to remove this as the "original" common.js is taking care of setting the ext. -> https://hg.adblockplus.org/adblockpluschrome/file/tip/ext/common.js#l21
Overall OK but I've suggested a couple of improvements. Please let me know what you think, thanks. https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newc... ext/content.js:104: let result = Map.prototype.get.apply(browser.tabs, args); if you can use `Map.prototype.get.apply` with `browser.tabs` it means you expect `browser.tabs` to be an `instanceof Map`, right? If that's the case, the following is all you need. let result = browser.tabs.get(...args); https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newc... ext/content.js:105: return new Promise((resolve, reject) => If I understand correctly result is synchronous so there's no need to create a Promise and its callback. return result ? Promise.resolve(result) : Promise.reject(new Error("Tab cannot be found")); https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.html File issue-reporter.html (right): https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.html... issue-reporter.html:24: <script src="ext/content.js"></script> anything we need to do on build tools level to be sure these files will be available and the right one? https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.html... issue-reporter.html:104: <button id="showDataClose" class="i18n_cancel primary"></button> On 2018/03/07 16:16:59, saroyanm wrote: > The buttons are still broken, because currently this is using general > messages.json from the adblockpluschrome repo -> > https://hg.adblockplus.org/adblockpluschrome/file/tip/_locales/en_US/messages... > > Make sense to fix that separately I think, because that requires also updating > the missing translations from the original repo to common.json and updating them > back to the crowdin. agreed https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.js File issue-reporter.js (left): https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.js#o... issue-reporter.js:20: window.ext = {}; On 2018/03/07 16:16:59, saroyanm wrote: > We are safe to remove this as the "original" common.js is taking care of setting > the ext. -> > https://hg.adblockplus.org/adblockpluschrome/file/tip/ext/common.js#l21 Off topic side note: it'd be awesome to stop assuming globals here and there and use proper modules instead, even to bring in mocks about web extension, IMO
updated a comment https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newc... ext/content.js:104: let result = Map.prototype.get.apply(browser.tabs, args); On 2018/03/07 17:11:04, a.giammarchi wrote: > if you can use `Map.prototype.get.apply` with `browser.tabs` it means you expect > `browser.tabs` to be an `instanceof Map`, right? > > If that's the case, the following is all you need. > > let result = browser.tabs.get(...args); actually, on a second thought, I don't understand why this callback needs to be so vague. It needs just one single argument that should be the tabId so you can just have that as parameter and use it right away as `browser.tabs.get(id);`, right? I'm not too fond of generic `...arg`, uinless really needed, because it's just like a meaningless `arguments` in a modern fashion.
https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newc... ext/content.js:104: let result = Map.prototype.get.apply(browser.tabs, args); On 2018/03/07 17:17:49, a.giammarchi wrote: > On 2018/03/07 17:11:04, a.giammarchi wrote: > > if you can use `Map.prototype.get.apply` with `browser.tabs` it means you > expect > > `browser.tabs` to be an `instanceof Map`, right? > > > > If that's the case, the following is all you need. > > > > let result = browser.tabs.get(...args); > > actually, on a second thought, I don't understand why this callback needs to be > so vague. It needs just one single argument that should be the tabId so you can > just have that as parameter and use it right away as `browser.tabs.get(id);`, > right? > > I'm not too fond of generic `...arg`, uinless really needed, because it's just > like a meaningless `arguments` in a modern fashion. This implementation is just use uses prototype.get method of Map to avoid infinite loop and return promise, so it's merely extends get method for this particular Map object. https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newc... ext/content.js:105: return new Promise((resolve, reject) => On 2018/03/07 17:11:04, a.giammarchi wrote: > If I understand correctly result is synchronous so there's no need to create a > Promise and its callback. > > return result ? Promise.resolve(result) : Promise.reject(new Error("Tab cannot > be found")); This is the Mock implmenetation of tabs.get -> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/get So I do need to return Promise here, in order to mimic actual implementation for our test server. https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.html File issue-reporter.html (right): https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.html... issue-reporter.html:24: <script src="ext/content.js"></script> On 2018/03/07 17:11:04, a.giammarchi wrote: > anything we need to do on build tools level to be sure these files will be > available and the right one? That's a good point and probably true for the other pages we have in the UI repository, also I'm not sure if some checks exists currently, maybe @Thomas knows about them. Anyway I'd suggest to tackle that separately from the current review. https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.js File issue-reporter.js (left): https://codereview.adblockplus.org/29716600/diff/29716609/issue-reporter.js#o... issue-reporter.js:20: window.ext = {}; On 2018/03/07 17:11:04, a.giammarchi wrote: > On 2018/03/07 16:16:59, saroyanm wrote: > > We are safe to remove this as the "original" common.js is taking care of > setting > > the ext. -> > > https://hg.adblockplus.org/adblockpluschrome/file/tip/ext/common.js#l21 > > Off topic side note: it'd be awesome to stop assuming globals here and there and > use proper modules instead, even to bring in mocks about web extension, IMO That's true. Another important side note: this Page implementation didn't go through my or Thomas review.
answered to a couple of points. https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newc... ext/content.js:104: let result = Map.prototype.get.apply(browser.tabs, args); On 2018/03/08 15:14:27, saroyanm wrote: > On 2018/03/07 17:17:49, a.giammarchi wrote: > > On 2018/03/07 17:11:04, a.giammarchi wrote: > > > if you can use `Map.prototype.get.apply` with `browser.tabs` it means you > > expect > > > `browser.tabs` to be an `instanceof Map`, right? > > > > > > If that's the case, the following is all you need. > > > > > > let result = browser.tabs.get(...args); > > > > actually, on a second thought, I don't understand why this callback needs to > be > > so vague. It needs just one single argument that should be the tabId so you > can > > just have that as parameter and use it right away as `browser.tabs.get(id);`, > > right? > > > > I'm not too fond of generic `...arg`, uinless really needed, because it's just > > like a meaningless `arguments` in a modern fashion. > > This implementation is just use uses prototype.get method of Map to avoid > infinite loop it's not so clear from the code. Maybe a comment might help others asking themself what is this about? https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newc... ext/content.js:105: return new Promise((resolve, reject) => On 2018/03/08 15:14:27, saroyanm wrote: > On 2018/03/07 17:11:04, a.giammarchi wrote: > > If I understand correctly result is synchronous so there's no need to create a > > Promise and its callback. > > > > return result ? Promise.resolve(result) : Promise.reject(new Error("Tab cannot > > be found")); > > This is the Mock implmenetation of tabs.get -> > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/get > > So I do need to return Promise here, in order to mimic actual implementation for > our test server. when the result is already known, there is no difference whatsoever between: `new Promise(resolve => resolve(data));` and `Promise.resolve(data);` As you can guess the latter doesn't need to create callbacks, pass through new, etc. The result is exactly a promise. `Promise.resolve(123).then(console.log);` Same goes for `Promise.reject(error)`. My point is that you can simplify code and return a resolved or a rejected promise right away. You are doing this already, but through an unnecessary callback and a `new` operation. As summary: ``` new Promise((resolve, reject) => { if (data) resolve(data); else reject(error); }); ``` is the exact equivalent of: `data ? Promise.resolve(data) : Promise.reject(error);` I hope my previous comment is more clear now/
https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newc... ext/content.js:104: let result = Map.prototype.get.apply(browser.tabs, args); On 2018/03/08 17:27:34, a.giammarchi wrote: > On 2018/03/08 15:14:27, saroyanm wrote: > > On 2018/03/07 17:17:49, a.giammarchi wrote: > > > On 2018/03/07 17:11:04, a.giammarchi wrote: > > > > if you can use `Map.prototype.get.apply` with `browser.tabs` it means you > > > expect > > > > `browser.tabs` to be an `instanceof Map`, right? > > > > > > > > If that's the case, the following is all you need. > > > > > > > > let result = browser.tabs.get(...args); > > > > > > actually, on a second thought, I don't understand why this callback needs to > > be > > > so vague. It needs just one single argument that should be the tabId so you > > can > > > just have that as parameter and use it right away as > `browser.tabs.get(id);`, > > > right? > > > > > > I'm not too fond of generic `...arg`, uinless really needed, because it's > just > > > like a meaningless `arguments` in a modern fashion. > > > > This implementation is just use uses prototype.get method of Map to avoid > > infinite loop > > it's not so clear from the code. Maybe a comment might help others asking > themself what is this about? > Noted, I'll add one. https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newc... ext/content.js:105: return new Promise((resolve, reject) => On 2018/03/08 17:27:34, a.giammarchi wrote: > On 2018/03/08 15:14:27, saroyanm wrote: > > On 2018/03/07 17:11:04, a.giammarchi wrote: > > > If I understand correctly result is synchronous so there's no need to create > a > > > Promise and its callback. > > > > > > return result ? Promise.resolve(result) : Promise.reject(new Error("Tab > cannot > > > be found")); > > > > This is the Mock implmenetation of tabs.get -> > > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/get > > > > So I do need to return Promise here, in order to mimic actual implementation > for > > our test server. > > when the result is already known, there is no difference whatsoever between: > > `new Promise(resolve => resolve(data));` > > and > > `Promise.resolve(data);` > > As you can guess the latter doesn't need to create callbacks, pass through new, > etc. > > The result is exactly a promise. > > `Promise.resolve(123).then(console.log);` > > Same goes for `Promise.reject(error)`. > > My point is that you can simplify code and return a resolved or a rejected > promise right away. > > You are doing this already, but through an unnecessary callback and a `new` > operation. > > As summary: > > ``` > new Promise((resolve, reject) => { > if (data) resolve(data); > else reject(error); > }); > ``` > > is the exact equivalent of: > > `data ? Promise.resolve(data) : Promise.reject(error);` > > I hope my previous comment is more clear now/ Sorry I think I misread your initial I though you were asking why we are creating a promise because in chrome is only callbacks were used.. Thanks for elaborating, will update.
Thanks Andrea, ready for the review. https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newc... ext/content.js:104: let result = Map.prototype.get.apply(browser.tabs, args); On 2018/03/08 18:16:47, saroyanm wrote: > On 2018/03/08 17:27:34, a.giammarchi wrote: > > On 2018/03/08 15:14:27, saroyanm wrote: > > > On 2018/03/07 17:17:49, a.giammarchi wrote: > > > > On 2018/03/07 17:11:04, a.giammarchi wrote: > > > > > if you can use `Map.prototype.get.apply` with `browser.tabs` it means > you > > > > expect > > > > > `browser.tabs` to be an `instanceof Map`, right? > > > > > > > > > > If that's the case, the following is all you need. > > > > > > > > > > let result = browser.tabs.get(...args); > > > > > > > > actually, on a second thought, I don't understand why this callback needs > to > > > be > > > > so vague. It needs just one single argument that should be the tabId so > you > > > can > > > > just have that as parameter and use it right away as > > `browser.tabs.get(id);`, > > > > right? > > > > > > > > I'm not too fond of generic `...arg`, uinless really needed, because it's > > just > > > > like a meaningless `arguments` in a modern fashion. > > > > > > This implementation is just use uses prototype.get method of Map to avoid > > > infinite loop > > > > it's not so clear from the code. Maybe a comment might help others asking > > themself what is this about? > > > > Noted, I'll add one. Done. https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newc... ext/content.js:105: return new Promise((resolve, reject) => On 2018/03/08 18:16:47, saroyanm wrote: > On 2018/03/08 17:27:34, a.giammarchi wrote: > > On 2018/03/08 15:14:27, saroyanm wrote: > > > On 2018/03/07 17:11:04, a.giammarchi wrote: > > > > If I understand correctly result is synchronous so there's no need to > create > > a > > > > Promise and its callback. > > > > > > > > return result ? Promise.resolve(result) : Promise.reject(new Error("Tab > > cannot > > > > be found")); > > > > > > This is the Mock implmenetation of tabs.get -> > > > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/get > > > > > > So I do need to return Promise here, in order to mimic actual implementation > > for > > > our test server. > > > > when the result is already known, there is no difference whatsoever between: > > > > `new Promise(resolve => resolve(data));` > > > > and > > > > `Promise.resolve(data);` > > > > As you can guess the latter doesn't need to create callbacks, pass through > new, > > etc. > > > > The result is exactly a promise. > > > > `Promise.resolve(123).then(console.log);` > > > > Same goes for `Promise.reject(error)`. > > > > My point is that you can simplify code and return a resolved or a rejected > > promise right away. > > > > You are doing this already, but through an unnecessary callback and a `new` > > operation. > > > > As summary: > > > > ``` > > new Promise((resolve, reject) => { > > if (data) resolve(data); > > else reject(error); > > }); > > ``` > > > > is the exact equivalent of: > > > > `data ? Promise.resolve(data) : Promise.reject(error);` > > > > I hope my previous comment is more clear now/ > > Sorry I think I misread your initial I though you were asking why we are > creating a promise because in chrome is only callbacks were used.. Thanks for > elaborating, will update. Done.
Thanks Andrea, ready for the review. https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newc... ext/content.js:104: let result = Map.prototype.get.apply(browser.tabs, args); On 2018/03/08 18:16:47, saroyanm wrote: > On 2018/03/08 17:27:34, a.giammarchi wrote: > > On 2018/03/08 15:14:27, saroyanm wrote: > > > On 2018/03/07 17:17:49, a.giammarchi wrote: > > > > On 2018/03/07 17:11:04, a.giammarchi wrote: > > > > > if you can use `Map.prototype.get.apply` with `browser.tabs` it means > you > > > > expect > > > > > `browser.tabs` to be an `instanceof Map`, right? > > > > > > > > > > If that's the case, the following is all you need. > > > > > > > > > > let result = browser.tabs.get(...args); > > > > > > > > actually, on a second thought, I don't understand why this callback needs > to > > > be > > > > so vague. It needs just one single argument that should be the tabId so > you > > > can > > > > just have that as parameter and use it right away as > > `browser.tabs.get(id);`, > > > > right? > > > > > > > > I'm not too fond of generic `...arg`, uinless really needed, because it's > > just > > > > like a meaningless `arguments` in a modern fashion. > > > > > > This implementation is just use uses prototype.get method of Map to avoid > > > infinite loop > > > > it's not so clear from the code. Maybe a comment might help others asking > > themself what is this about? > > > > Noted, I'll add one. Done. https://codereview.adblockplus.org/29716600/diff/29716609/ext/content.js#newc... ext/content.js:105: return new Promise((resolve, reject) => On 2018/03/08 18:16:47, saroyanm wrote: > On 2018/03/08 17:27:34, a.giammarchi wrote: > > On 2018/03/08 15:14:27, saroyanm wrote: > > > On 2018/03/07 17:11:04, a.giammarchi wrote: > > > > If I understand correctly result is synchronous so there's no need to > create > > a > > > > Promise and its callback. > > > > > > > > return result ? Promise.resolve(result) : Promise.reject(new Error("Tab > > cannot > > > > be found")); > > > > > > This is the Mock implmenetation of tabs.get -> > > > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/get > > > > > > So I do need to return Promise here, in order to mimic actual implementation > > for > > > our test server. > > > > when the result is already known, there is no difference whatsoever between: > > > > `new Promise(resolve => resolve(data));` > > > > and > > > > `Promise.resolve(data);` > > > > As you can guess the latter doesn't need to create callbacks, pass through > new, > > etc. > > > > The result is exactly a promise. > > > > `Promise.resolve(123).then(console.log);` > > > > Same goes for `Promise.reject(error)`. > > > > My point is that you can simplify code and return a resolved or a rejected > > promise right away. > > > > You are doing this already, but through an unnecessary callback and a `new` > > operation. > > > > As summary: > > > > ``` > > new Promise((resolve, reject) => { > > if (data) resolve(data); > > else reject(error); > > }); > > ``` > > > > is the exact equivalent of: > > > > `data ? Promise.resolve(data) : Promise.reject(error);` > > > > I hope my previous comment is more clear now/ > > Sorry I think I misread your initial I though you were asking why we are > creating a promise because in chrome is only callbacks were used.. Thanks for > elaborating, will update. Done.
LGTM but there's a nitpick you can ignore if you want. https://codereview.adblockplus.org/29716600/diff/29720616/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29720616/ext/content.js#newc... ext/content.js:106: const error = new Error("Tab cannot be found"); Nit: when there is a result, there's no actually need to create a new Error, involve the stack, etc. Also it's quite weird to find a let followed by const for two variables that won't change in scope. This is how I would've written this: ``` const result = Map.prototype.get.apply(browser.tabs, args); const error = result ? null : new Error("Tab cannot be found"); return result ? Promise.resolve(result) : Promise.reject(error); ``` `null` is always very cheap (no scope resolution like `undefined` might have) and it should never de-opt from engine optimizations.
Thanks, done. https://codereview.adblockplus.org/29716600/diff/29720616/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29716600/diff/29720616/ext/content.js#newc... ext/content.js:106: const error = new Error("Tab cannot be found"); On 2018/03/12 15:53:08, a.giammarchi wrote: > Nit: when there is a result, there's no actually need to create a new Error, > involve the stack, etc. > Also it's quite weird to find a let followed by const for two variables that > won't change in scope. > > This is how I would've written this: > > ``` > const result = Map.prototype.get.apply(browser.tabs, args); > const error = result ? null : new Error("Tab cannot be found"); > return result ? Promise.resolve(result) : Promise.reject(error); > ``` > > `null` is always very cheap (no scope resolution like `undefined` might have) > and it should never de-opt from engine optimizations. Done. I'm using one ternary operator to make this simpler.
On 2018/03/12 16:09:52, saroyanm wrote: > Done. I'm using one ternary operator to make this simpler. even better. LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
