Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 5768552254537728: Fixed: Popup in Safari won't open after closing it programmatically (Closed)

Created:
Jan. 16, 2014, 11:18 a.m. by Thomas Greiner
Modified:
Jan. 16, 2014, 4 p.m.
Visibility:
Public.

Description

Popup closes itself programmatically after five seconds when clickhide is being activated. However, unlike Chrome, Safari has only one instance of the popup running which window.close was closing. Therefore when clicking on the icon again, Safari was unable to open it and crashed. See https://s3.amazonaws.com/f.cl.ly/items/13361P1T1g3S1f1a1m2P/adp.mp4

Patch Set 1 #

Total comments: 2

Patch Set 2 : #

Total comments: 1

Patch Set 3 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+15 lines, -3 lines) Patch
M chrome/ext/popup.js View 1 1 chunk +7 lines, -1 line 0 comments Download
M popup.js View 1 2 1 chunk +1 line, -1 line 0 comments Download
M safari/ext/popup.js View 1 1 chunk +7 lines, -1 line 0 comments Download

Messages

Total messages: 5
Thomas Greiner
Jan. 16, 2014, 11:22 a.m. (2014-01-16 11:22:34 UTC) #1
Sebastian Noack
http://codereview.adblockplus.org/5768552254537728/diff/5629499534213120/safari/ext/popup.js File safari/ext/popup.js (right): http://codereview.adblockplus.org/5768552254537728/diff/5629499534213120/safari/ext/popup.js#newcode20 safari/ext/popup.js:20: window.closePopup = function() This should go into the ext ...
Jan. 16, 2014, 11:28 a.m. (2014-01-16 11:28:11 UTC) #2
Thomas Greiner
http://codereview.adblockplus.org/5768552254537728/diff/5629499534213120/safari/ext/popup.js File safari/ext/popup.js (right): http://codereview.adblockplus.org/5768552254537728/diff/5629499534213120/safari/ext/popup.js#newcode20 safari/ext/popup.js:20: window.closePopup = function() On 2014/01/16 11:28:11, Sebastian Noack wrote: ...
Jan. 16, 2014, 3:17 p.m. (2014-01-16 15:17:48 UTC) #3
Sebastian Noack
http://codereview.adblockplus.org/5768552254537728/diff/5649050225344512/popup.js File popup.js (right): http://codereview.adblockplus.org/5768552254537728/diff/5649050225344512/popup.js#newcode120 popup.js:120: activateClickHide.timeout = window.setTimeout(closePopup, 5000); This has to be changed ...
Jan. 16, 2014, 3:22 p.m. (2014-01-16 15:22:23 UTC) #4
Sebastian Noack
Jan. 16, 2014, 3:37 p.m. (2014-01-16 15:37:28 UTC) #5
LGTM

Powered by Google App Engine
This is Rietveld