Issue 29565838: Issue 5803 - Integrate new options page into adblockpluschrome (Closed)
|
Created:
Oct. 5, 2017, 9:02 p.m. by saroyanm Modified:
Oct. 9, 2017, 4:34 p.m. Reviewers:
Sebastian Noack CC:
Thomas Greiner, Manish Jethani Visibility:
Public. |
DescriptionCOLLABORATOR=sebastian@adblockplus.org
COLLABORATOR=thomas@adblockplus.org
Patch Set 1 : #
Total comments: 10
Patch Set 2 : #
MessagesTotal messages: 8
This is ready for the review. Note: I'll be available tomorrow after 2pm. In case of urgency feel free to upload additional patch and collaborate. https://codereview.adblockplus.org/29565838/diff/29565851/metadata.gecko-webext File metadata.gecko-webext (right): https://codereview.adblockplus.org/29565838/diff/29565851/metadata.gecko-webe... metadata.gecko-webext:12: [mapping] Note: On the time of creation of this review the subscriptions.xml were not updated yet. So currently the EasyPrivacy and fanboy-social are not visible in Privacy and Security section on General tab. As soon it's updated I'll update dependencies accordingly. https://codereview.adblockplus.org/29565838/diff/29565851/metadata.gecko-webe... metadata.gecko-webext:15: options.html = adblockplusui/options.html As far as I understood, the iframe issue is only relevant to the android mobile. So seems like we are safe overriding options.html, Rather than pointing everything to desktop-options.* ?
https://codereview.adblockplus.org/29565838/diff/29565851/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29565838/diff/29565851/lib/prefs.js#newcode93 lib/prefs.js:93: * @see https://adblockplus.org/en/preferences#subscriptions_exceptionsurl_privacy I'll create separate review to update websites.
https://codereview.adblockplus.org/29565838/diff/29565851/metadata.gecko-webext File metadata.gecko-webext (right): https://codereview.adblockplus.org/29565838/diff/29565851/metadata.gecko-webe... metadata.gecko-webext:15: options.html = adblockplusui/options.html On 2017/10/05 21:56:58, saroyanm wrote: > As far as I understood, the iframe issue is only relevant to the android mobile. > > So seems like we are safe overriding options.html, Rather than pointing > everything to desktop-options.* ? But then it is also overridden on Android. We use the same build for Firefox on desktop and Firefox Mobile. Otherwise, the iframe workaround wouldn't be necessary in the first place.
https://codereview.adblockplus.org/29565838/diff/29565851/metadata.gecko-webext File metadata.gecko-webext (right): https://codereview.adblockplus.org/29565838/diff/29565851/metadata.gecko-webe... metadata.gecko-webext:15: options.html = adblockplusui/options.html On 2017/10/05 22:12:34, Sebastian Noack wrote: > On 2017/10/05 21:56:58, saroyanm wrote: > > As far as I understood, the iframe issue is only relevant to the android > mobile. > > > > So seems like we are safe overriding options.html, Rather than pointing > > everything to desktop-options.* ? > > But then it is also overridden on Android. We use the same build for Firefox on > desktop and Firefox Mobile. Otherwise, the iframe workaround wouldn't be > necessary in the first place. Noted: -> https://codereview.adblockplus.org/29565861/
https://codereview.adblockplus.org/29565838/diff/29565851/metadata.gecko-webext File metadata.gecko-webext (right): https://codereview.adblockplus.org/29565838/diff/29565851/metadata.gecko-webe... metadata.gecko-webext:15: options.html = adblockplusui/options.html On 2017/10/05 22:37:57, saroyanm wrote: > On 2017/10/05 22:12:34, Sebastian Noack wrote: > > On 2017/10/05 21:56:58, saroyanm wrote: > > > As far as I understood, the iframe issue is only relevant to the android > > mobile. > > > > > > So seems like we are safe overriding options.html, Rather than pointing > > > everything to desktop-options.* ? > > > > But then it is also overridden on Android. We use the same build for Firefox > on > > desktop and Firefox Mobile. Otherwise, the iframe workaround wouldn't be > > necessary in the first place. > > Noted: -> https://codereview.adblockplus.org/29565861/ This is not the issue because we could still map it as desktop-options.html = adblockplusui/options.html skin/options.css = adblockplusui/options.css which is why we went forward with renaming it from new-options.html to options.html in the first place. However, there seems to be one collision we can't avoid with the mapping which is options.js since both adblockpluschrome/options.html as well as adblockplusui/options.html refer to it. Therefore the rename to desktop-options.html will be necessary after all, unfortunately.
https://codereview.adblockplus.org/29565838/diff/29565851/lib/prefs.js File lib/prefs.js (right): https://codereview.adblockplus.org/29565838/diff/29565851/lib/prefs.js#newcode93 lib/prefs.js:93: * @see https://adblockplus.org/en/preferences#subscriptions_exceptionsurl_privacy On 2017/10/05 21:58:13, saroyanm wrote: > I'll create separate review to update websites. See -> https://codereview.adblockplus.org/29565858/ https://codereview.adblockplus.org/29565838/diff/29565851/metadata.gecko-webext File metadata.gecko-webext (right): https://codereview.adblockplus.org/29565838/diff/29565851/metadata.gecko-webe... metadata.gecko-webext:15: options.html = adblockplusui/options.html On 2017/10/06 12:16:55, Thomas Greiner wrote: > On 2017/10/05 22:37:57, saroyanm wrote: > > On 2017/10/05 22:12:34, Sebastian Noack wrote: > > > On 2017/10/05 21:56:58, saroyanm wrote: > > > > As far as I understood, the iframe issue is only relevant to the android > > > mobile. > > > > > > > > So seems like we are safe overriding options.html, Rather than pointing > > > > everything to desktop-options.* ? > > > > > > But then it is also overridden on Android. We use the same build for Firefox > > on > > > desktop and Firefox Mobile. Otherwise, the iframe workaround wouldn't be > > > necessary in the first place. > > > > Noted: -> https://codereview.adblockplus.org/29565861/ > > This is not the issue because we could still map it as > > desktop-options.html = adblockplusui/options.html > skin/options.css = adblockplusui/options.css > > which is why we went forward with renaming it from new-options.html to > options.html in the first place. > > However, there seems to be one collision we can't avoid with the mapping which > is options.js since both adblockpluschrome/options.html as well as > adblockplusui/options.html refer to it. Therefore the rename to > desktop-options.html will be necessary after all, unfortunately. Acknowledged.
New patch uploaded. https://codereview.adblockplus.org/29565838/diff/29565851/metadata.gecko-webext File metadata.gecko-webext (right): https://codereview.adblockplus.org/29565838/diff/29565851/metadata.gecko-webe... metadata.gecko-webext:12: [mapping] On 2017/10/05 21:56:58, saroyanm wrote: > Note: On the time of creation of this review the subscriptions.xml were not > updated yet. > > So currently the EasyPrivacy and fanboy-social are not visible in Privacy and > Security section on General tab. > > As soon it's updated I'll update dependencies accordingly. Subscriptions.xml in the adblockpluscore are updated. Dependencies are updated accordingly. https://codereview.adblockplus.org/29565838/diff/29565851/metadata.gecko-webe... metadata.gecko-webext:15: options.html = adblockplusui/options.html On 2017/10/05 22:12:34, Sebastian Noack wrote: > On 2017/10/05 21:56:58, saroyanm wrote: > > As far as I understood, the iframe issue is only relevant to the android > mobile. > > > > So seems like we are safe overriding options.html, Rather than pointing > > everything to desktop-options.* ? > > But then it is also overridden on Android. We use the same build for Firefox on > desktop and Firefox Mobile. Otherwise, the iframe workaround wouldn't be > necessary in the first place. Done.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
