Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#653 closed change (fixed)

Object.defineProperty instead of defineGetter / defineSetter

Reported by: tschuster Assignee: tschuster
Priority: Unknown Milestone: Adblock-Plus-2.6.4-for-Firefox
Module: Adblock-Plus-for-Firefox Keywords:
Cc: trev Blocked By:
Blocking: #312 Platform: Firefox
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/5055554716172288/

Description (last modified by tschuster)

We should use the standard Object.defineProperty function. It also allows us to override getters with plain value property, which is something we often want for some of the "lazy caching" getters that we use.

Change History (11)

comment:1 Changed 6 years ago by tschuster

  • Description modified (diff)

comment:2 Changed 6 years ago by philll

I trained SpamBayed to let this through now.

comment:3 Changed 6 years ago by tschuster

  • Description modified (diff)
  • Owner set to tschuster
  • Review URL(s) modified (diff)

comment:4 Changed 6 years ago by tschuster

  • Blocking 312 added

comment:5 Changed 5 years ago by tschuster

  • Platform set to Unknown

I just committed this: https://hg.adblockplus.org/adblockplus/rev/69871d10cf2a.

Testing:
I am sorry to say that this is patch doesn't have any specific target area, so you should make sure Adblockplus still works probably after this patch.

comment:6 Changed 5 years ago by tschuster

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

comment:7 Changed 5 years ago by tschuster

  • Resolution fixed deleted
  • Status changed from closed to reopened

Backout:
https://hg.adblockplus.org/adblockplus/rev/17869a1b0f95

We actually try to modify FilterStorage.sourceFile in our tests: https://hg.adblockplus.org/adblockplustests/file/a22b38cb9212/chrome/content/common.js#l115

Easiest solution is to use {configurable: true} when defining it.

comment:8 Changed 5 years ago by trev

  • Cc trev added
  • Ready set

Fine with me.

comment:9 Changed 5 years ago by tschuster

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

Relanded with configurable sourceFile:
https://hg.adblockplus.org/adblockplus/rev/309d8a2b3949
Fixed test suite to treat it as a normal property:
https://hg.adblockplus.org/adblockplustests/rev/1ef3d5a601d4

comment:10 Changed 5 years ago by trev

  • Component changed from Unknown to Adblock-Plus-for-Firefox
  • Milestone set to Adblock-Plus-for-Firefox-next
  • Platform changed from Unknown to Firefox/Firefox Mobile

comment:11 Changed 5 years ago by philll

  • Platform changed from Firefox/Firefox Mobile to Firefox

Made Firefox and Firefox mobile available as seperate platforms.

Note: See TracTickets for help on using tickets.