Issue 29516569: Issue 5347 - Check for windows API support (Closed)
|
Created:
Aug. 15, 2017, 2:08 p.m. by Manish Jethani Modified:
Aug. 24, 2017, 11:27 a.m. Reviewers:
Sebastian Noack CC:
Wladimir Palant Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 5347 - Check for windows API support
Patch Set 1 #
Total comments: 6
Patch Set 2 : Remove check for ext.windows #Patch Set 3 : Rebase #MessagesTotal messages: 9
We need to add these checks for Firefox for Android. In addition to this, the code in composer.postload.js shouldn't be loaded at all. I'm wondering if there should be a new gecko-mobile-webext build target.
On 2017/08/15 14:11:16, Manish Jethani wrote: > In addition to this, the code in composer.postload.js shouldn't be loaded at > all. I'm wondering if there should be a new gecko-mobile-webext build target. I wonder whether it is even possible to publish different builds for Android and desktop on AMO? However, even if it is, I think it would be preferable, to have one unified build. Can't you detect whether we are running in Firefox on Android, and then disable the composer, at run time? https://codereview.adblockplus.org/29516569/diff/29516570/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29516569/diff/29516570/ext/background.js#o... ext/background.js:699: // Edge does not yet support runtime.openOptionsPage (tested version 38) This comment should be updated, also mentioning Firefox on Android, in addition to Microsoft Edge. https://codereview.adblockplus.org/29516569/diff/29516570/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29516569/diff/29516570/ext/background.js#n... ext/background.js:751: ext.windows = { What is about the code using ext.windows? I suppose it will just fail with a different error now?
On 2017/08/15 14:40:19, Sebastian Noack wrote: > On 2017/08/15 14:11:16, Manish Jethani wrote: > > In addition to this, the code in composer.postload.js shouldn't be loaded at > > all. I'm wondering if there should be a new gecko-mobile-webext build target. > > I wonder whether it is even possible to publish different builds for Android and > desktop on AMO? However, even if it is, I think it would be preferable, to have > one unified build. Can't you detect whether we are running in Firefox on > Android, and then disable the composer, at run time? If this lands: https://codereview.adblockplus.org/29516656/ Then yes, we can detect at the time of initialization and avoid calling a lot of that code.
https://codereview.adblockplus.org/29516569/diff/29516570/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29516569/diff/29516570/ext/background.js#o... ext/background.js:699: // Edge does not yet support runtime.openOptionsPage (tested version 38) On 2017/08/15 14:40:19, Sebastian Noack wrote: > This comment should be updated, also mentioning Firefox on Android, in addition > to Microsoft Edge. I'm going to make another change for runtime.openOptionsPage, because even though Firefox for Android has this function, it does nothing. We have to use the info module here. I'll update the comment as part of that change. https://codereview.adblockplus.org/29516569/diff/29516570/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29516569/diff/29516570/ext/background.js#n... ext/background.js:751: ext.windows = { On 2017/08/15 14:40:19, Sebastian Noack wrote: > What is about the code using ext.windows? I suppose it will just fail with a > different error now? ext.windows is only used from lib/filterComposer.js. Since we're going to disable the composer on mobile, this shouldn't be a problem. If any other code wants to use ext.windows in the future, it should check if it is supported and do the right thing accordingly.
https://codereview.adblockplus.org/29516569/diff/29516570/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29516569/diff/29516570/ext/background.js#n... ext/background.js:751: ext.windows = { On 2017/08/15 16:24:12, Manish Jethani wrote: > On 2017/08/15 14:40:19, Sebastian Noack wrote: > > What is about the code using ext.windows? I suppose it will just fail with a > > different error now? > > ext.windows is only used from lib/filterComposer.js. Since we're going to > disable the composer on mobile, this shouldn't be a problem. > > If any other code wants to use ext.windows in the future, it should check if it > is supported and do the right thing accordingly. I guess we can just leave the code here unchanged then, and just disable the calling code on Android, as we have to anyway. And if ext.windows is used there it shouldn't matter whether it fails because it doesn't exist or because the underlying API it wraps doesn't exist.
Patch Set 2: Remove check for ext.windows https://codereview.adblockplus.org/29516569/diff/29516570/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29516569/diff/29516570/ext/background.js#n... ext/background.js:751: ext.windows = { On 2017/08/18 10:25:21, Sebastian Noack wrote: > On 2017/08/15 16:24:12, Manish Jethani wrote: > > On 2017/08/15 14:40:19, Sebastian Noack wrote: > > > What is about the code using ext.windows? I suppose it will just fail with a > > > different error now? > > > > ext.windows is only used from lib/filterComposer.js. Since we're going to > > disable the composer on mobile, this shouldn't be a problem. > > > > If any other code wants to use ext.windows in the future, it should check if > it > > is supported and do the right thing accordingly. > > I guess we can just leave the code here unchanged then, and just disable the > calling code on Android, as we have to anyway. And if ext.windows is used there > it shouldn't matter whether it fails because it doesn't exist or because the > underlying API it wraps doesn't exist. I just checked and the only way ext.windows can be called on Android is if the context menu item is actually displayed. We're not going to display it, so I think it's OK to just remove this check here.
Patch Set 3: Rebase
LGTM 
|
