Opened 8 months ago

Closed 8 months ago

#4973 closed defect (fixed)

Exception thrown in popup window

Reported by: Ross Assignee: kzar
Priority: P1 Milestone: Adblock-Plus-1.13-for-Chrome-Opera
Module: Platform Keywords:
Cc: kzar, trev, rraceanu Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Ross Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29379779/

Description (last modified by Ross)

Environment

Adblock Plus 1.12.4.1739
Chrome 49 / 56 / Windows 10
Opera 37 / 41 / Windows 7

How to reproduce

  1. Install extension.
  2. Inspect the popup and view the Console.

Observed behaviour

Javascript error related to notifications:
    Uncaught SyntaxError: Identifier 'require' has already been declared
    at notification.js:1

Expected behaviour

JS error to not occur.

Notes

The error being thrown in the popup's console is a regression caused by Issue 4795 - Use modern JavaScript syntax.

Change History (9)

comment:1 Changed 8 months ago by kzar

  • Cc trev added
  • Component changed from Unknown to Platform

What did you do to test notifications? When I run this code in the background console:

require("notification").Notification.addNotification({
  id: new Date() | 0,
  type: "normal",
  title: "Notification title",
  message: "<a>Click</a> <a>Configure</a> <a>notification</a> <a>settings</a>",
  urlFilters: ["||example.com^$document"]
});

and then browse to http://example.com the notification is displayed. Also the button to open the options page from the notification worked for me.

I can reproduce the console error though, argh.

comment:2 Changed 8 months ago by kzar

  • Description modified (diff)

comment:3 Changed 8 months ago by kzar

  • Description modified (diff)

comment:4 Changed 8 months ago by kzar

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

comment:5 Changed 8 months ago by Ross

Ah okay, I was using the popup console instead of the background console. Notifications do work.

comment:6 Changed 8 months ago by Ross

  • Description modified (diff)
  • Summary changed from Notifications JS error / Notifications not working to Notifications JS error

comment:7 Changed 8 months ago by kzar

  • Milestone set to Adblock-Plus-1.13-for-Chrome-Opera
  • Owner set to kzar
  • Priority changed from Unknown to P1
  • Ready set
  • Summary changed from Notifications JS error to Exception thrown in popup window

Thanks, marking this as ready.

comment:8 Changed 8 months ago by abpbot

A commit referencing this issue has landed:
Issue 4973 - Avoid redeclaring require in the popup

comment:9 Changed 8 months ago by kzar

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