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

Issue 6273189772525568: Fixed regression: Clickhide functionality broken (Closed)

Created:
Nov. 19, 2013, 12:02 p.m. by Thomas Greiner
Modified:
Nov. 19, 2013, 2:37 p.m.
Visibility:
Public.

Description

The clickhide feature was broken due to the recent Safari related changes. This has already been addressed in http://codereview.adblockplus.org/5762529972191232/ but not completely fixed yet. To fix the issue, usage of sendMessage/onMessage had to be reverted to sendRequest/onRequest due to differing behaviors (as reported at https://code.google.com/p/chromium/issues/detail?id=321074).

Patch Set 1 #

Patch Set 2 : Made it work with sendMessage/onMessage #

Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -2 lines) Patch
M background.js View 1 1 chunk +7 lines, -1 line 0 comments Download
M chrome/common.js View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 4
Thomas Greiner
Nov. 19, 2013, 12:10 p.m. (2013-11-19 12:10:21 UTC) #1
Thomas Greiner
Apparently, this is not a bug but a feature. :D Adapted changes to make it ...
Nov. 19, 2013, 12:23 p.m. (2013-11-19 12:23:26 UTC) #2
Sebastian Noack
I wonder whether it would be better to always return true from the event listener, ...
Nov. 19, 2013, 1:25 p.m. (2013-11-19 13:25:31 UTC) #3
Wladimir Palant
Nov. 19, 2013, 1:48 p.m. (2013-11-19 13:48:25 UTC) #4
On 2013/11/19 13:25:31, sebastian wrote:
> I wonder whether it would be better to always return true from the event
> listener, in order to have a consistent behavior between Chrome and Safari.

No, that would introduce memory leaks - we don't always call the callback.
Properly indicating that the message will be handled asynchronously is actually
necessary, even if it will be ignored in Safari.

LGTM

Powered by Google App Engine
This is Rietveld