Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(762)

Issue 29573892: Issue 4579 - Promisify APIs (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months ago by Manish Jethani
Modified:
1 month, 3 weeks ago
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Description

Issue 4579 - Promisify APIs

Patch Set 1 #

Total comments: 3

Patch Set 2 : Do not wrap APIs on Firefox #

Total comments: 5

Patch Set 3 : Use loop #

Total comments: 10

Patch Set 4 : Do not return promise if last argument is undefined #

Patch Set 5 : Revert patch set 4 #

Patch Set 6 : Remove browser check #

Patch Set 7 : Rebase #

Total comments: 9

Patch Set 8 : Use anonymous block #

Patch Set 9 : Use arrow functions #

Patch Set 10 : Revert patch set 9 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+100 lines, -12 lines) Patch
M polyfill.js View 1 2 3 4 5 6 7 9 1 chunk +100 lines, -12 lines 2 comments Download

Messages

Total messages: 24
Manish Jethani
2 months ago (2017-10-11 19:16:52 UTC) #1
Manish Jethani
Patch Set 1 I thought the proxy-based approach was too unnecessarily expensive, this is much ...
2 months ago (2017-10-11 19:23:51 UTC) #2
Manish Jethani
Patch Set 2: Do not wrap APIs on Firefox https://codereview.adblockplus.org/29573892/diff/29573903/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29573903/ext/common.js#newcode94 ext/common.js:94: ...
2 months ago (2017-10-11 21:33:55 UTC) #3
kzar
(Adding Wladimir to Cc since he has done something similar for EasyPasswords IIRC.)
2 months ago (2017-10-12 18:16:12 UTC) #4
Sebastian Noack
I think this should be landed after https://codereview.adblockplus.org/29570614/, and then go into polyfill.js, rather than ...
2 months ago (2017-10-13 04:04:19 UTC) #5
Manish Jethani
Patch Set 3: Use loop https://codereview.adblockplus.org/29573892/diff/29573903/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29573903/ext/common.js#newcode58 ext/common.js:58: let [root, ...subPath] = ...
2 months ago (2017-10-13 07:17:47 UTC) #6
Manish Jethani
On 2017/10/13 04:04:19, Sebastian Noack wrote: > I think this should be landed after > ...
2 months ago (2017-10-13 07:25:59 UTC) #7
Wladimir Palant
https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newcode78 ext/common.js:78: if (typeof args[args.length - 1] == "undefined") I don't ...
2 months ago (2017-10-13 09:53:14 UTC) #8
Manish Jethani
https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newcode78 ext/common.js:78: if (typeof args[args.length - 1] == "undefined") On 2017/10/13 ...
2 months ago (2017-10-13 15:15:16 UTC) #9
Sebastian Noack
https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newcode78 ext/common.js:78: if (typeof args[args.length - 1] == "undefined") On 2017/10/13 ...
2 months ago (2017-10-14 00:44:07 UTC) #10
Manish Jethani
Patch Set 4: Do not return promise if last argument is undefined https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js File ext/common.js ...
2 months ago (2017-10-14 01:07:30 UTC) #11
Manish Jethani
https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newcode97 ext/common.js:97: if (typeof browser == "undefined") On 2017/10/14 00:44:07, Sebastian ...
2 months ago (2017-10-14 01:13:02 UTC) #12
Sebastian Noack
https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newcode78 ext/common.js:78: if (typeof args[args.length - 1] == "undefined") On 2017/10/14 ...
2 months ago (2017-10-14 01:31:21 UTC) #13
Manish Jethani
Patch Set 6: Remove browser check https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573892/diff/29575555/ext/common.js#newcode78 ext/common.js:78: if (typeof args[args.length ...
2 months ago (2017-10-14 02:01:30 UTC) #14
Wladimir Palant
LGTM
2 months ago (2017-10-14 13:49:47 UTC) #15
Manish Jethani
Patch Set 7: Rebase https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js File polyfill.js (left): https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#oldcode20 polyfill.js:20: // Unlike Firefox and Microsoft ...
1 month, 4 weeks ago (2017-10-17 13:33:24 UTC) #16
Manish Jethani
Patch Set 8: Use anonymous block https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#newcode20 polyfill.js:20: (function() On 2017/10/17 ...
1 month, 3 weeks ago (2017-10-17 17:35:45 UTC) #17
kzar
https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#newcode20 polyfill.js:20: (function() On 2017/10/17 17:35:45, Manish Jethani wrote: > On ...
1 month, 3 weeks ago (2017-10-17 18:13:42 UTC) #18
Manish Jethani
Patch Set 9: Use arrow functions https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#newcode20 polyfill.js:20: (function() On 2017/10/17 ...
1 month, 3 weeks ago (2017-10-17 20:48:45 UTC) #19
Sebastian Noack
https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#newcode20 polyfill.js:20: (function() On 2017/10/17 20:48:45, Manish Jethani wrote: > On ...
1 month, 3 weeks ago (2017-10-17 23:31:03 UTC) #20
Manish Jethani
Patch Set 10: Revert patch set 9 https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573892/diff/29581792/polyfill.js#newcode20 polyfill.js:20: (function() On ...
1 month, 3 weeks ago (2017-10-18 00:05:33 UTC) #21
Sebastian Noack
LGTM
1 month, 3 weeks ago (2017-10-18 00:07:38 UTC) #22
Wladimir Palant
LGTM Addressing the nit below is optional. https://codereview.adblockplus.org/29573892/diff/29581917/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573892/diff/29581917/polyfill.js#newcode98 polyfill.js:98: return true; ...
1 month, 3 weeks ago (2017-10-18 08:59:12 UTC) #23
Manish Jethani
1 month, 3 weeks ago (2017-10-18 11:00:26 UTC) #24
Message was sent while issue was closed.
On 2017/10/18 08:59:12, Wladimir Palant wrote:
> LGTM

Thanks!

> Addressing the nit below is optional.

I've left it as it is for now. I'm seeing it more as a default return value when
all checks fail (we may have multiple checks in the future).

https://codereview.adblockplus.org/29573892/diff/29581917/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573892/diff/29581917/polyfill.js#newcode98
polyfill.js:98: return true;
On 2017/10/18 08:59:11, Wladimir Palant wrote:
> Nit: I would put that return inside the catch block which makes the logic
> clearer IMHO.

Acknowledged.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5