Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29573083: Issue 5028 - Use browser namespace (Closed)

Created:
Oct. 10, 2017, 10:24 p.m. by Manish Jethani
Modified:
Oct. 17, 2017, 1:01 p.m.
CC:
kzar
Base URL:
https://hg.adblockplus.org/adblockplusui/
Visibility:
Public.

Description

Issue 5028 - Use browser namespace Related: https://codereview.adblockplus.org/29570614/

Patch Set 1 #

Total comments: 6

Patch Set 2 : Move i18n code into polyfill.js #

Total comments: 1

Patch Set 3 : Remove check for window.browser #

Total comments: 6

Patch Set 4 : Rebase #

Total comments: 10

Patch Set 5 : Make more changes from chrome.runtime to browser.runtime #

Total comments: 4

Patch Set 6 : Add polyfill.js to README.md #

Unified diffs Side-by-side diffs Delta from patch set Stats (+197 lines, -171 lines) Patch
M README.md View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M background.html View 1 1 chunk +1 line, -0 lines 0 comments Download
M background.js View 1 chunk +1 line, -1 line 0 comments Download
M common.js View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M desktop-options.html View 1 chunk +1 line, -0 lines 0 comments Download
M desktop-options.js View 1 2 3 11 chunks +26 lines, -26 lines 0 comments Download
M devtools-panel.html View 1 chunk +1 line, -0 lines 0 comments Download
M devtools-panel.js View 1 2 3 4 3 chunks +3 lines, -3 lines 0 comments Download
M ext/common.js View 1 2 3 2 chunks +0 lines, -120 lines 0 comments Download
M ext/content.js View 1 2 3 4 1 chunk +3 lines, -3 lines 0 comments Download
M firstRun.html View 1 chunk +1 line, -0 lines 0 comments Download
M firstRun.js View 1 2 3 4 3 chunks +3 lines, -3 lines 0 comments Download
M i18n.js View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M lib/antiadblockInit.js View 1 chunk +2 lines, -2 lines 0 comments Download
M mobile-options.html View 1 chunk +1 line, -0 lines 0 comments Download
M mobile-options.js View 1 2 3 4 5 chunks +9 lines, -9 lines 0 comments Download
A polyfill.js View 1 2 3 1 chunk +140 lines, -0 lines 0 comments Download

Messages

Total messages: 17
Manish Jethani
Oct. 10, 2017, 10:24 p.m. (2017-10-10 22:24:58 UTC) #1
Manish Jethani
Patch Set 1
Oct. 10, 2017, 10:25 p.m. (2017-10-10 22:25:45 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29573083/diff/29573084/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573083/diff/29573084/ext/common.js#newcode161 ext/common.js:161: browser.i18n = { I think this code should be ...
Oct. 10, 2017, 11:23 p.m. (2017-10-10 23:23:08 UTC) #3
Manish Jethani
Patch Set 2: Move i18n code into polyfill.js https://codereview.adblockplus.org/29573083/diff/29573084/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29573083/diff/29573084/ext/common.js#newcode161 ext/common.js:161: browser.i18n ...
Oct. 11, 2017, 11:29 a.m. (2017-10-11 11:29:06 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29573083/diff/29573084/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573083/diff/29573084/polyfill.js#newcode20 polyfill.js:20: if (typeof browser == "undefined") On 2017/10/11 11:29:06, Manish ...
Oct. 13, 2017, 2:48 a.m. (2017-10-13 02:48:17 UTC) #5
Manish Jethani
Patch Set 3: Remove check for window.browser https://codereview.adblockplus.org/29573083/diff/29573084/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573083/diff/29573084/polyfill.js#newcode20 polyfill.js:20: if (typeof ...
Oct. 13, 2017, 5 a.m. (2017-10-13 05:00:34 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29573083/diff/29574722/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573083/diff/29574722/polyfill.js#newcode22 polyfill.js:22: window.browser = {}; Nit: Perhaps we should just define ...
Oct. 13, 2017, 9:39 p.m. (2017-10-13 21:39:24 UTC) #7
Manish Jethani
https://codereview.adblockplus.org/29573083/diff/29574722/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573083/diff/29574722/polyfill.js#newcode22 polyfill.js:22: window.browser = {}; On 2017/10/13 21:39:23, Sebastian Noack wrote: ...
Oct. 13, 2017, 10:30 p.m. (2017-10-13 22:30:24 UTC) #8
Sebastian Noack
LGTM
Oct. 13, 2017, 10:36 p.m. (2017-10-13 22:36:01 UTC) #9
saroyanm
Seems like we are still using chrome.runtime.sendMessage, which probably also needs to be updated. There ...
Oct. 16, 2017, 7:24 p.m. (2017-10-16 19:24:21 UTC) #10
Manish Jethani
Patch Set 4: Rebase https://codereview.adblockplus.org/29573083/diff/29574722/desktop-options.js File desktop-options.js (right): https://codereview.adblockplus.org/29573083/diff/29574722/desktop-options.js#newcode546 desktop-options.js:546: ext.backgroundPage.sendMessage(message, (errors) => On 2017/10/16 ...
Oct. 16, 2017, 9:35 p.m. (2017-10-16 21:35:22 UTC) #11
saroyanm
We still have some occurrences of chrome.runtime.sendMessage https://codereview.adblockplus.org/29573083/diff/29580675/devtools-panel.js File devtools-panel.js (right): https://codereview.adblockplus.org/29573083/diff/29580675/devtools-panel.js#newcode22 devtools-panel.js:22: chrome.runtime.sendMessage({type: "types.get"}, ...
Oct. 17, 2017, 11:09 a.m. (2017-10-17 11:09:52 UTC) #12
Manish Jethani
Patch Set 5: Make more changes from chrome.runtime to browser.runtime https://codereview.adblockplus.org/29573083/diff/29580675/devtools-panel.js File devtools-panel.js (right): https://codereview.adblockplus.org/29573083/diff/29580675/devtools-panel.js#newcode22 ...
Oct. 17, 2017, 11:35 a.m. (2017-10-17 11:35:35 UTC) #13
saroyanm
Just a small notice. https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js#newcode18 polyfill.js:18: "use strict"; While this polyfill ...
Oct. 17, 2017, 12:13 p.m. (2017-10-17 12:13:59 UTC) #14
Manish Jethani
https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js#newcode18 polyfill.js:18: "use strict"; On 2017/10/17 12:13:58, saroyanm wrote: > While ...
Oct. 17, 2017, 12:19 p.m. (2017-10-17 12:19:50 UTC) #15
Manish Jethani
Patch Set 6: Add polyfill.js to README.md https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js#newcode18 polyfill.js:18: "use strict"; ...
Oct. 17, 2017, 12:38 p.m. (2017-10-17 12:38:17 UTC) #16
saroyanm
Oct. 17, 2017, 12:41 p.m. (2017-10-17 12:41:12 UTC) #17
LGTM

https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29573083/diff/29581567/polyfill.js#newcode18
polyfill.js:18: "use strict";
On 2017/10/17 12:38:17, Manish Jethani wrote:
> On 2017/10/17 12:19:50, Manish Jethani wrote:
> > On 2017/10/17 12:13:58, saroyanm wrote:
> > > While this polyfill file will be only used in the test environment and we
> want
> > > to overwrite this file during the build, I'll suggest to move it under
> > > ext/polyfill.js as it's described in the Readme See ->
> > > https://hg.adblockplus.org/adblockplusui/file/tip/README.md#l29 otherwise
we
> > > might want to update Readme accordingly.
> > > 
> > > As far as I can see pollyfill.js in adblockpluschrome repository is being
> used
> > > in the root directory instead of "ext" ->
> > > https://codereview.adblockplus.org/29570614/ I'm not sure whether it was
> > > intentional or not, so depending on that we might want to move pollyfill
in
> > both
> > > repositories into "ext" folder to make it consistent with other
> > implementations
> > > "ext/background.js", "ext/common.js" and etc. or we might want to make a
> note
> > > about pollyfill.js in the Readme.
> > > 
> > > In case I'm not missing something.
> > 
> > Hmm ... ext/ was supposed to have the ext namespace, while we're moving away
> > from it to use the standard browser extension APIs. At the same time, it
does
> > make sense to separate out files that will be overwritten by
product-specific
> > versions. We could make an exception for polyfill.js here, considering it to
> be
> > a special file, and update the README accordingly, or we could move it to
ext/
> > 
> > Sebastian, what do you think?
> 
> So I see that there are a few top-level files that are also not to be
imported,
> like background.js, subscriptions.xml, and so on. In that case the decision is
> easier. Let's keep pollfill.js where it is and just update the README file, in
> my opinion.
> 
> Done.

I agree

Powered by Google App Engine
This is Rietveld