Opened on 04/22/2016 at 09:43:07 AM
Closed on 05/04/2016 at 03:15:54 PM
#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): | |||
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.
Attachments (0)
Change History (10)
comment:2 in reply to: ↑ 1 Changed on 04/22/2016 at 11:02:56 AM 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 on 04/22/2016 at 11:18:32 AM 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 on 04/22/2016 at 11:48:44 AM 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 on 04/26/2016 at 09:48:51 AM 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 on 04/28/2016 at 05:55:00 PM 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 on 04/28/2016 at 08:06:48 PM by greiner
- Priority changed from Unknown to P3
- Ready set
comment:8 Changed on 04/29/2016 at 12:47:19 PM by kzar
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:9 Changed on 05/04/2016 at 03:13:39 PM by abpbot
A commit referencing this issue has landed:
https://hg.adblockplus.org/adblockplusui/rev/75534a4a1e0e
comment:10 Changed on 05/04/2016 at 03:15:54 PM by kzar
- Resolution set to fixed
- Status changed from reviewing to closed
Is this ticket merely about removing the call to Object.observe() or does it also include replacing the fallback code for Object.observe()?