Issue 2080 - Removed duplicated code from clickHide_deactivate

Issue 2080 - Removed duplicated code from clickHide_deactivate

[kzar, 2015-03-04 10:20:21]

http://codereview.adblockplus.org/6008293193416704/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/6008293193416704/diff/5629499534213120/incl...
include.postload.js:274: clickHide_rulesPending();
Nit: Seems like weird mix of camel case with snake case.

http://codereview.adblockplus.org/6008293193416704/diff/5629499534213120/incl...
include.postload.js:350: // Turn off click-to-hide
I don't think this comment really adds anything.

[Sebastian Noack, 2015-03-04 10:47:08]

http://codereview.adblockplus.org/6008293193416704/diff/5629499534213120/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/6008293193416704/diff/5629499534213120/incl...
include.postload.js:274: clickHide_rulesPending();
On 2015/03/04 10:20:21, kzar wrote:
> Nit: Seems like weird mix of camel case with snake case.

Yes, function names should be properly camelcase'd. However, this pattern is
currently consistently used in this file, and that particular function isn't
new. Changing the naming of all functions would clearly be out ouf scope of this
issue.

http://codereview.adblockplus.org/6008293193416704/diff/5629499534213120/incl...
include.postload.js:350: // Turn off click-to-hide
On 2015/03/04 10:20:21, kzar wrote:
> I don't think this comment really adds anything.

Also not really matter of this change, but fair enough.

[kzar, 2015-03-04 10:48:41]

LGTM

[Wladimir Palant, 2015-03-04 12:47:16]

http://codereview.adblockplus.org/6008293193416704/diff/5724160613416960/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/6008293193416704/diff/5724160613416960/incl...
include.postload.js:274: clickHide_rulesPending();
What if we go here because clickHideFiltersDialog is set?
clickHide_rulesPending() won't remove the existing dialog. We either need to
handle that case separately or stop checking for clickHideFiltersDialog if it
simply cannot be set here.

http://codereview.adblockplus.org/6008293193416704/diff/5724160613416960/incl...
include.postload.js:363: if (currentElement) {
Nit: Opening bracket should go on the next line, we should fix it if we touch
that code.

[Sebastian Noack, 2015-03-04 13:36:37]

Other changes due to rebase

http://codereview.adblockplus.org/6008293193416704/diff/5724160613416960/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/6008293193416704/diff/5724160613416960/incl...
include.postload.js:274: clickHide_rulesPending();
On 2015/03/04 12:47:16, Wladimir Palant wrote:
> What if we go here because clickHideFiltersDialog is set?
> clickHide_rulesPending() won't remove the existing dialog. We either need to
> handle that case separately or stop checking for clickHideFiltersDialog if it
> simply cannot be set here.

You are right, checking for clickHideFiltersDialog is unneeded. This function
must not be called when there is already a dialog. Calling
clickHide_rulesPending() prevents that by removing the event listeners. However,
the check where also unneeded before this change. But it's probably a good time
to get rid of it.

http://codereview.adblockplus.org/6008293193416704/diff/5724160613416960/incl...
include.postload.js:363: if (currentElement) {
On 2015/03/04 12:47:16, Wladimir Palant wrote:
> Nit: Opening bracket should go on the next line, we should fix it if we touch
> that code.

Done.

[Wladimir Palant, 2015-03-04 13:48:07]

LGTM but I think a proper testing round is in order here.