Opened 22 months ago

Closed 22 months ago

Last modified 22 months ago

#5803 closed change (fixed)

Integrate new options page into adblockpluschrome firefox web extensions

Reported by: saroyanm Assignee: saroyanm
Priority: P2 Milestone: Adblock-Plus-3.0-for-Firefox
Module: Platform Keywords:
Cc: sebastian, kzar, mjethani, hfiguiere, greiner, fhd, trev, Shikitita Blocked By: #2825, #5587
Blocking: #5158 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29559679/
https://codereview.adblockplus.org/29565838/
https://codereview.adblockplus.org/29565858/
https://codereview.adblockplus.org/29565861/

Description (last modified by greiner)

Background

With #5158 we are preparing for the new options page launch for the beta release. As a final step for that we will need to integrate it into our firefox web extensions dev build.

What to change

New preferences

Add Privacy Friendly Acceptable Ads pref with the URL to the subscription:

subscriptions_exceptionsurl_privacy = "https://easylist-downloads.adblockplus.org/exceptionrules-privacy-friendly.txt";

Mappings

skin/abp-logo.svg = adblockplusui/skin/abp-logo.svg
skin/icons/tooltip.svg = adblockplusui/skin/icons/tooltip.svg
skin/icons/gear.svg = adblockplusui/skin/icons/gear.svg
skin/icons/toggle.svg = adblockplusui/skin/icons/toggle.svg
skin/icons/reload.svg = adblockplusui/skin/icons/reload.svg
skin/icons/globe.svg = adblockplusui/skin/icons/globe.svg
skin/icons/code.svg = adblockplusui/skin/icons/code.svg
skin/icons/trash.svg = adblockplusui/skin/icons/trash.svg
skin/icons/delete.svg = adblockplusui/skin/icons/delete.svg
skin/icons/attention.svg = adblockplusui/skin/icons/attention.svg
skin/icons/checkmark.svg = adblockplusui/skin/icons/checkmark.svg
skin/icons/checkbox.png = adblockplusui/skin/icons/checkbox.png
skin/fonts/SourceSansPro-Regular.woff = adblockplusui/skin/fonts/SourceSansPro-Regular.woff
skin/fonts/SourceSansPro-Light.woff = adblockplusui/skin/fonts/SourceSansPro-Light.woff
skin/fonts/SourceSansPro-bold.woff = adblockplusui/skin/fonts/SourceSansPro-bold.woff
skin/social/twitter.svg = adblockplusui/skin/social/twitter.svg
skin/social/facebook.svg = adblockplusui/skin/social/facebook.svg
skin/social/googleplus.svg = adblockplusui/skin/social/googleplus.svg

Import locales

adblockplusui/locale/*/new-options.json = =*
adblockplusui/locale/*/common.json = =*

Replace options page

  • Replace old(current) options page with "New options page"

Change History (35)

comment:1 Changed 22 months ago by saroyanm

  • Cc sebastian kzar added
  • Description modified (diff)

In case you think this will speed up process I can prepare patch for updating prefs.js and https://adblockplus.org/en/preferences after this ticket will be ready(tomorrow).

comment:2 follow-up: Changed 22 months ago by sebastian

I think these preferences should be added in the same change that integrates the new options page. There is no point in adding preferences, that are still unused, in advance.

comment:3 Changed 22 months ago by saroyanm

  • Description modified (diff)
  • Summary changed from Introduce subscriptions_exceptionsurl_privacy and ui_warn_tracking preferences to Integrate new options page into adblockpluschrome

comment:4 Changed 22 months ago by saroyanm

  • Component changed from Libadblockplus to Platform

comment:5 in reply to: ↑ 2 ; follow-up: Changed 22 months ago by saroyanm

Replying to sebastian:

I think these preferences should be added in the same change that integrates the new options page. There is no point in adding preferences, that are still unused, in advance.

I think I updated this ticket accordingly.

Question: Do you want me to remove all the resources that are still used in "Old/current" options page, or we want to still bundle them until it will make to the stable release ?

comment:6 Changed 22 months ago by kzar

I wonder if instead of having these mappings

new-options.html = adblockplusui/new-options.html
new-options.js = adblockplusui/new-options.js

and then this one (I assume the lack of file extension was a typo in the description)

options.html = new-options.html

We could just replace options.html and options.js with the new ones in adblockplusui?

Edit: Sorry accidentally stripped the end of my comment when I posted it.

Last edited 22 months ago by kzar (previous) (diff)

comment:7 in reply to: ↑ 5 Changed 22 months ago by kzar

Replying to saroyanm:

Question: Do you want me to remove all the resources that are still used in "Old/current" options page, or we want to still bundle them until it will make to the stable release ?

Looks like we had the same thought :p

Well in my opinion we should completely remove the old options page and any related files or metadata when switching over to the new one.

comment:8 Changed 22 months ago by kzar

  • Cc mjethani hfiguiere greiner fhd trev added
  • Priority changed from Unknown to P2

comment:9 Changed 22 months ago by saroyanm

  • Blocked By 2825 added

comment:10 Changed 22 months ago by mjethani

Note that we're making options.html a wrapper around the actual options page, which is selected based on the platform (desktop or mobile). This change is under review. If it goes through, we'll just keep the options page as options.html and edit the new options.js to load new-options.html instead of desktop-options.html, if I understand this correctly.

comment:11 follow-up: Changed 22 months ago by sebastian

  • Blocked By 5587 added

Yeah, I would very much like to first land #5587, as it is already under review and more urgent anyway. I marked it as blocking. The issue description still needs to be updated.

Also it seems we have to setup the new subscription types (#2825) first, for the new options page to behave as expected, before we integrate it, anyway.

Then what is about translations? I don't even see them uploaded to the translation project on CrowdIn yet. I suppose this isn't necessarily a blocker for the integration of the new options page, assuming this wouldn't be the last dependency update before the release, and we can start testing without. But we shouldn't wait until the last minute for the translations either. If we are going to release the new options in about a month from now, I'd expect translations (for all languages the old options page is available in!) to complete and being used, soon.

Furthermore, I noticed usage of -{webkit|moz}-{margin|border}-{start|end} in the new options page. This obviously won't work on Microsoft Edge. Not sure how large the impact will be though. But assuming nobody tested the new options page on Microsoft Edge yet, there might be more compatibility issues. Since Microsoft Edge is still released out of a branch, this doesn't necessarily block the initial integration of the new options page, but I'd still expect compatibility with Microsoft Edge being addressed very soon, as we cannot diverge the edge and master branch for too long.

Also I agree, that this change should remove any legacy code and assets of the old options page.

Last edited 22 months ago by sebastian (previous) (diff)

comment:12 Changed 22 months ago by saroyanm

  • Description modified (diff)

Updated accordingly to #5807

comment:13 in reply to: ↑ 11 ; follow-up: Changed 22 months ago by saroyanm

  • Cc Shikitita added

Replying to sebastian:

Then what is about translations? I don't even see them uploaded to the translation project on CrowdIn yet. I suppose this isn't necessarily a blocker for the integration of the new options page, assuming this wouldn't be the last dependency update before the release, and we can start testing without. But we shouldn't wait until the last minute for the translations either. If we are going to release the new options in about a month from now, I'd expect translations (for all languages the old options page is available in!) to complete and being used, soon.

Maybe @Tamara can give us a heads-up here.

Furthermore, I noticed usage of -{webkit|moz}-{margin|border}-{start|end} in the new options page. This obviously won't work on Microsoft Edge. Not sure how large the impact will be though. But assuming nobody tested the new options page on Microsoft Edge yet, there might be more compatibility issues. Since Microsoft Edge is still released out of a branch, this doesn't necessarily block the initial integration of the new options page, but I'd still expect compatibility with Microsoft Edge being addressed very soon, as we cannot diverge the edge and master branch for too long.

Yes edge wasn't a target during the development, I created an issue, to fix that.

Also I agree, that this change should remove any legacy code and assets of the old options page.

Acknowledged, I'll update the ticket accordingly.

comment:14 Changed 22 months ago by saroyanm

  • Description modified (diff)

Updated ticket description according to the discussion in the comments

comment:15 Changed 22 months ago by saroyanm

  • Owner set to saroyanm

comment:16 Changed 22 months ago by saroyanm

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:17 Changed 22 months ago by abpbot

A commit referencing this issue has landed:
Issue 5803 - rename new-options to options

comment:18 Changed 22 months ago by sebastian

  • Status changed from reviewing to reopened

Removing reviewing state. The only patch has been submitted was to rename new-options.* to options.* inside adblockplusui. While this change might be reasonable, and has some implication on the changes to integrate the new options, it still is quite a stretch to consider it part of this effort.

comment:19 in reply to: ↑ 13 ; follow-up: Changed 22 months ago by Shikitita

Replying to saroyanm:

Replying to sebastian:

Then what is about translations? I don't even see them uploaded to the translation project on CrowdIn yet. I suppose this isn't necessarily a blocker for the integration of the new options page, assuming this wouldn't be the last dependency update before the release, and we can start testing without. But we shouldn't wait until the last minute for the translations either. If we are going to release the new options in about a month from now, I'd expect translations (for all languages the old options page is available in!) to complete and being used, soon.

Maybe @Tamara can give us a heads-up here.

Translations are being handled already since end of August and am still reviewing them. Once they are final they can be added to the project.

In the meantime, I would really appreciate it if the project on Crowdin wasn't updated without checking with me first. Therefore, @sebastian, as I already asked in #adblockplus, could you please revert the changes you've done to the project? I will let you know when the translations are ready to be uploaded.

Thanks.

comment:20 in reply to: ↑ 19 Changed 22 months ago by Shikitita

In the meantime, I would really appreciate it if the project on Crowdin wasn't updated without checking with me first. Therefore, @sebastian, as I already asked in #adblockplus, could you please revert the changes you've done to the project? I will let you know when the translations are ready to be uploaded.

I've deleted the four new files added. Unfortunately, Crowdin doesn't allow me to revert the "firstRun" file to the previous version.

@sebastian: could you please look into this and let me know if you're successful doing it? Please note that I am actually off today and most probably won't check until Wednesday.

comment:21 Changed 22 months ago by sebastian

As discussed with Felix and Winsley, I updated the source translation on Crowdin again. All strings in target translations that appear in my name have been imported from the Translation Memory (TM), meaning these are strings that have been merely moved from our other translation projects preserving their original translation.

As soon as you have the translations from the agency ready and reviewed, we can import them and if we wish to have them override any community-contributed suggestion. If you have trouble merging the translations, I'm happy to help.

comment:22 follow-up: Changed 22 months ago by sebastian

As discussed with Felix and Winsley, at the current stage, we only want to show the new options page to Firefox users. This should be possible to achieve by adding the assets to metadta.gecko-webext (instead of metadata.common), overwriting the old options page, in the Firefox builds only.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 22 months ago by saroyanm

  • Description modified (diff)
  • Summary changed from Integrate new options page into adblockpluschrome to Integrate new options page into adblockpluschrome firefox web extensions

Replying to sebastian:

As discussed with Felix and Winsley, at the current stage, we only want to show the new options page to Firefox users. This should be possible to achieve by adding the assets to metadta.gecko-webext (instead of metadata.common), overwriting the old options page, in the Firefox builds only.

Thanks for the heads-up. Updated the ticket accordingly.

Missing dependency: The subscriptionlist is updated, but seems like the hgbot didn't updated the subscription.xml yet, my assumption is that we need to trigger the update manually. Whom I can ask to do that ?

comment:24 Changed 22 months ago by greiner

  • Description modified (diff)

Fixed preference name in ticket description

comment:25 Changed 22 months ago by mjethani

#5587 is resolved now.

comment:26 Changed 22 months ago by saroyanm

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

comment:27 Changed 22 months ago by saroyanm

  • Review URL(s) modified (diff)

comment:28 Changed 22 months ago by saroyanm

  • Review URL(s) modified (diff)

comment:29 in reply to: ↑ 23 ; follow-up: Changed 22 months ago by trev

Replying to saroyanm:

Missing dependency: The subscriptionlist is updated, but seems like the hgbot didn't updated the subscription.xml yet, my assumption is that we need to trigger the update manually.

There is no hgbot, it died when we migrated the Mercurial server and wasn't reanimated. So I've run python -m sitescripts.subscriptions.bin.processTemplate recommendations manually and updated this file: https://hg.adblockplus.org/adblockpluscore/rev/a1b481e7d728

comment:30 Changed 22 months ago by abpbot

A commit referencing this issue has landed:
Issue 5803 - rename desktop options page to desktop-options.*

comment:31 in reply to: ↑ 29 Changed 22 months ago by saroyanm

Replying to trev:

Replying to saroyanm:

Missing dependency: The subscriptionlist is updated, but seems like the hgbot didn't updated the subscription.xml yet, my assumption is that we need to trigger the update manually.

There is no hgbot, it died when we migrated the Mercurial server and wasn't reanimated. So I've run python -m sitescripts.subscriptions.bin.processTemplate recommendations manually and updated this file: https://hg.adblockplus.org/adblockpluscore/rev/a1b481e7d728

Thanks Wladimir, I've updated the review accordingly.

comment:32 Changed 22 months ago by abpbot

A commit referencing this issue has landed:
Issue 5803 - Integrate new options page into web extensions build

comment:33 Changed 22 months ago by abpbot

A commit referencing this issue has landed:
Issue 5803 - add privacy friendly acceptable ads preference

comment:34 Changed 22 months ago by saroyanm

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:35 Changed 22 months ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-next
Note: See TracTickets for help on using tickets.