Opened 3 years ago

Closed 3 years ago

Last modified 2 years ago

#5083 closed change (fixed)

Get rid of the "chrome" subdirectory and "metadata.common"

Reported by: sebastian Assignee: jsonesen
Priority: P3 Milestone: Adblock-Plus-1.13.3-for-Chrome-Opera
Module: Platform Keywords: goodfirstbug
Cc: kzar, jsonesen, greiner Blocked By: #5113
Blocking: #5084 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29409555/

Description (last modified by sebastian)

Background

With #4577 we dropped Safari support from the master branch, removing the Safari backend (i.e. safari/ext/*) and other legacy code. However, one relic that still remains is the chrome subdirectory, which is used to separate the Chrome-specific code and assets. But since we now use these on all supported platforms, keeping them in a separate folder doesn't make any sense anymore. In particular the separation between ext/* and chrome/ext/* is holding back some further cleanup (#5084).

Also there is metadata.common, which was used to be the common baseline of Chrome and Safari, but now it merely exists so that metadata.chrome can inherit from it. These two files can be merged as well now.

What to change

Merge the contents of the chrome subdirectory into the top-level file structure. The files ext/common.js and ext/background.js will conflict, so insert the code from the respective files in the (to be removed) chrome subdirectory, at the end of the IIFE. Adjust the paths pointing to the chrome subdirectory in metadata.chrome accordingly, and finally merge metadata.common into metadata.chrome.

Change History (11)

comment:1 Changed 3 years ago by sebastian

  • Blocking 5084 added

comment:2 Changed 3 years ago by sebastian

  • Description modified (diff)

comment:3 Changed 3 years ago by greiner

  • Cc greiner added

comment:4 Changed 3 years ago by jsonesen

  • Owner set to jsonesen

comment:5 Changed 3 years ago by jsonesen

  • Blocked By 5113 added

comment:6 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
Issue 5083 - Merges chrome subdirectory into top level directory

comment:7 Changed 3 years ago by kzar

As mentioned in IRC this change seems to have caused a regression relating to filter storage. I see exceptions in the background console when the extension starts and I don't have any filters or subscriptions.

adblockplus.js:820 Uncaught TypeError: Cannot read property 'get' of undefined
    at loadFile (adblockplus.js:820)
    at Object.statFile (adblockplus.js:904)
    at Object.loadFromDisk (adblockplus.js:6880)
    at init (adblockplus.js:6080)
    at Object.require.modules.filterListener (adblockplus.js:6090)
    at require (adblockplus.js:8)
    at adblockplus.js:10512
loadFile @ adblockplus.js:820
statFile @ adblockplus.js:904
loadFromDisk @ adblockplus.js:6880
init @ adblockplus.js:6080
require.modules.filterListener @ adblockplus.js:6090
require @ adblockplus.js:8
(anonymous) @ adblockplus.js:10512
messageResponder.js:80 Uncaught TypeError: ext.PageMap is not a constructor
    at messageResponder.js:80
    at messageResponder.js:431
(anonymous) @ messageResponder.js:80
(anonymous) @ messageResponder.js:431
adblockplus.js:1603 Uncaught (in promise) TypeError: Cannot read property 'get' of undefined
    at resolve (adblockplus.js:1603)
    at Promise (<anonymous>)
    at init (adblockplus.js:1601)
    at Object.require.modules.prefs (adblockplus.js:1660)
    at require (adblockplus.js:8)
    at Object.require.modules.filterStorage (adblockplus.js:6409)
    at require (adblockplus.js:8)
    at Object.require.modules.filterListener (adblockplus.js:5990)
    at require (adblockplus.js:8)
    at adblockplus.js:10512

comment:8 Changed 3 years ago by jsonesen

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

comment:9 Changed 3 years ago by abpbot

comment:10 Changed 3 years ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:11 Changed 2 years ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Does not appear to have caused any regressions (apart from the fixed one above).

ABP 1.13.2.1785
Chrome 49 / 59 / Windows 7
Opera 36 / 45 / Windows 7

Note: See TracTickets for help on using tickets.