Issue 29516679: Issue 5347 - Do not show composer menu item on Firefox for Android (Closed)
|
Created:
Aug. 15, 2017, 11:40 p.m. by Manish Jethani Modified:
Aug. 24, 2017, 11:23 a.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 5347 - Do not show composer menu item on Firefox for Android
Patch Set 1 #
Total comments: 2
Patch Set 2 : Check for chrome.windows instead #
Total comments: 6
Patch Set 3 : Check for "Fennec" explicitly #MessagesTotal messages: 11
Patch Set 1 https://codereview.adblockplus.org/29516679/diff/29516680/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29516679/diff/29516680/lib/filterComposer.... lib/filterComposer.js:190: // Disable the composer if the browser is unknown or if it's We have to cover null and "unknown" as well, because on Gecko that's the value until the actual name has been fetched. https://codereview.adblockplus.org/29516679/diff/29516680/lib/filterComposer.... lib/filterComposer.js:270: if (shouldDisable()) If we simply cut off the communication between composer.postload.js and this file in these two places, it does the trick of "disabling" the composer. Firefox for Android for one doesn't have a popup, nor does it have context menus, therefore the UI for "Block element" will never be shown.
Patch Set 2: Check for chrome.windows instead As discussed, we can just check for chrome.windows, because that's the API we need for the composer to work anyway. The rest of the checks were unnecessary.
https://codereview.adblockplus.org/29516679/diff/29517759/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29516679/diff/29517759/lib/filterComposer.... lib/filterComposer.js:185: // required for the composer to work. Not really, we could fall back to creating a new tab - something that makes more sense on Android anyway.
https://codereview.adblockplus.org/29516679/diff/29517759/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29516679/diff/29517759/lib/filterComposer.... lib/filterComposer.js:185: // required for the composer to work. On 2017/08/17 12:22:49, Wladimir Palant wrote: > Not really, we could fall back to creating a new tab - something that makes more > sense on Android anyway. Do you think it makes sense to make the composer work using tabs? Maybe the answer is different for phone and tablet. Anyway, the composer opens a dialog, I'm not sure it would be a good user experience to use a tab instead on mobile.
https://codereview.adblockplus.org/29516679/diff/29517759/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29516679/diff/29517759/lib/filterComposer.... lib/filterComposer.js:185: // required for the composer to work. On 2017/08/17 13:25:19, Manish Jethani wrote: > On 2017/08/17 12:22:49, Wladimir Palant wrote: > > Not really, we could fall back to creating a new tab - something that makes > more > > sense on Android anyway. > > Do you think it makes sense to make the composer work using tabs? > > Maybe the answer is different for phone and tablet. > > Anyway, the composer opens a dialog, I'm not sure it would be a good user > experience to use a tab instead on mobile. Also, the user experience when selecting elements isn't great without a mouse, since elements to be blocked are highlighted on hover so that you know what would be blocked before you click. This obviously doesn't work on touch devices. IMO, there is no point to make the composer work there.
LGTM https://codereview.adblockplus.org/29516679/diff/29517759/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29516679/diff/29517759/lib/filterComposer.... lib/filterComposer.js:185: // required for the composer to work. On 2017/08/18 09:23:01, Sebastian Noack wrote: > IMO, there is no point to make the composer work there. Ok, fine with me.
https://codereview.adblockplus.org/29516679/diff/29517759/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29516679/diff/29517759/lib/filterComposer.... lib/filterComposer.js:185: // required for the composer to work. On 2017/08/18 20:52:32, Wladimir Palant wrote: > On 2017/08/18 09:23:01, Sebastian Noack wrote: > > IMO, there is no point to make the composer work there. > > Ok, fine with me. Perhaps we should rather check for `info.application != "fennec"` here, since we seem to agree that the composer doesn't make much sense on mobile, regardless of the lack of support of the windows API.
Patch Set 3: Check for "Fennec" explicitly https://codereview.adblockplus.org/29516679/diff/29517759/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29516679/diff/29517759/lib/filterComposer.... lib/filterComposer.js:185: // required for the composer to work. On 2017/08/23 12:00:35, Sebastian Noack wrote: > On 2017/08/18 20:52:32, Wladimir Palant wrote: > > On 2017/08/18 09:23:01, Sebastian Noack wrote: > > > IMO, there is no point to make the composer work there. > > > > Ok, fine with me. > > Perhaps we should rather check for `info.application != "fennec"` here, since we > seem to agree that the composer doesn't make much sense on mobile, regardless of > the lack of support of the windows API. Done.
LGTM
LGTM 
|
