Opened 3 years ago

Closed 2 years ago

#6321 closed change (duplicate)

Move popup.html and related files to adblockplusui

Reported by: greiner Assignee: agiammarchi
Priority: P3 Milestone:
Module: User-Interface Keywords:
Cc: kzar, sebastian Blocked By:
Blocking: #6322 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by greiner)


The icon popup belongs to the User Interface module but the code for it is still located in adblockpluschrome. Therefore we want to move all its code to adblockplusui to be able to offer a more streamlined interface with the Web Extension team.

However, it turns out that this also affects notification.js and stats.js which are not explicitly listed on but due to their strong ties to the UI, it'd make sense to move them as well.
Unfortunately, they're not using the messaging API yet which makes this separation a bit more tedious than hoped but this can be worked on separately.

Another question would be to which module the logo icons belong since we use both adblockpluschrome/icons/detailed/abp-64.png as well as adblockplusui/skin/abp-128.png in the popup which is not ideal.

What to change

  • Move the following files from adblockpluschrome to adblockplusui:
    • icons/detailed/abp-64.png
    • skin/popup.css
    • skin/popup.png
    • popup.html
    • popup.js
  • Create or extend the mock implementation of the following files/modules accordingly in adblockplusui:
    • ext/popup.js
    • filterClasses
    • filterNotifier
    • filterStorage
    • notification
    • notificationHelper
    • prefs
    • stats
    • url
    • utils
    • whitelisting

Change History (9)

comment:1 Changed 3 years ago by sebastian

Is this effort worth it? You currently redesign the popup anyway, right? So how about doing it the same way as we did with the options page, i.e. developing the new popup in adblokcplusui and then removing the legacy implemantion from adblockpluschrome once we integrate the new popup?

comment:2 Changed 3 years ago by greiner

  • Blocking 6322 added

That's another good approach. There are barely any details about how such a redesign would look like and should be done.

But fair enough. Let's assume we will write it from scratch. In that case we'd only need to worry about where the icons should belong to.

comment:3 Changed 2 years ago by greiner

See also #6738.

comment:4 Changed 2 years ago by greiner

  • Description modified (diff)

Updated ticket description to reflect that notification.js and stat.js have been merged into popup.js (see cd17039102e2).

comment:5 Changed 2 years ago by greiner

  • Owner set to agiammarchi
  • Priority changed from Unknown to P3
  • Ready set

comment:6 Changed 2 years ago by greiner

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

comment:7 Changed 2 years ago by greiner

@agiammarchi Since this change will be part of #6892, mind if we close this one as duplicate and tackle all the necessary changes for the dependency update there?

For reference, the corresponding changes to our adblockpluschrome fork have landed in this commit.

comment:8 Changed 2 years ago by agiammarchi

@greiner works for me

comment:9 Changed 2 years ago by greiner

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

Closing this ticket as duplicate of #6892.

Note: See TracTickets for help on using tickets.