Opened 4 years ago

Closed 4 years ago

#3971 closed change (fixed)

Remove use of deprecated Object.observe from the new options page

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

https://codereview.adblockplus.org/29340967/

Description (last modified by kzar)

Background

The new options page for Adblock Plus that is under development makes use of Object.observe to track when subscriptions have been changed. Unfortunately that API has since been deprecated.

What to change

Remove the observeSubscription function completely including the Object.observe call it contains. Improve the onSubscriptionMessage hook to perform the same work that onObjectChanged used to.

Change History (10)

comment:1 follow-up: Changed 4 years ago by greiner

Is this ticket merely about removing the call to Object.observe() or does it also include replacing the fallback code for Object.observe()?

comment:2 in reply to: ↑ 1 Changed 4 years ago by kzar

Replying to greiner:

Is this ticket merely about removing the call to Object.observe() or does it also include replacing the fallback code for Object.observe()?

Well I'm just having a play with the code to figure out the best approach.

So far I think it might be best to remove observeSubscription completely and then modify onSubscriptionMessage to directly perform the same work that onObjectChanged did before. That would remove a whole bunch of apparently pointless indirection. (After all the old options page uses asynchronous messaging without doing this dance!)

Perhaps I'm wrong though, I'll update the issue when I've tried it for real. At worst we can simply remove the call to Object.observe like you pointed out.

comment:3 Changed 4 years ago by sebastian

Yeah, that is the same I thought when I last touched that code. The Object.observe() code and it's fallback seem to be a redundant layer of abstraction, making further changes rather complicated. As you indicated, all subscription changes are already handled by onSubscriptionMessage and IMO should be propagated directly to the UI from there.

comment:4 Changed 4 years ago by greiner

I'm also in favor of overhauling the existing approach to have a more streamlined workflow so I'm looking forward to your assessment of the situation.

comment:5 Changed 4 years ago by greiner

  • Description modified (diff)
  • Summary changed from Remove use of depreciated Object.observe from the new options page to Remove use of deprecated Object.observe from the new options page

comment:6 Changed 4 years ago by kzar

  • Description modified (diff)

Sorry for the delay, I reinstalled my laptop which was a distraction and also this code was very confusing to refactor.

Anyway I've finally managed to refactor onSubscriptionMessage away without breaking anything. I've updated the issue description accordingly and I will upload my changes for review (once tidied) tomorrow.

comment:7 Changed 4 years ago by greiner

  • Priority changed from Unknown to P3
  • Ready set

comment:8 Changed 4 years ago by kzar

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

comment:9 Changed 4 years ago by abpbot

A commit referencing this issue has landed:
https://hg.adblockplus.org/adblockplusui/rev/75534a4a1e0e

comment:10 Changed 4 years ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.