Update adblockpluscore dependency to hg:d9196c0de5f2

[mjethani]

Background

Ross noticed that the extension had stopped working in Chrome 49. It turned out to be due to the use of the "u" regular expression flag, which isn't supported until Chrome 50. See #6821 for more information.

We've now stopped using the "u" regular expression flag in the core code, but to include that change we need to update the adblockpluscore dependency.

What to change

Update the adblockpluscore dependency in the dependencies file to hg:d9196c0de5f2 git:16febf9.

Unfortunately, as well as the necessary change

• #6823 - Remove unnecessary usage of /u flag

the following unrelated changes will be included:

• #6809 - Implement hide-if-contains snippet

Hints for testers

• Ensure that the extension has begun working on Chrome 49 again. Smoke test all functionality, checking for exceptions in the console for the background page, options page and popup. Also make sure to test ​element hiding emulation filters, e.g. example.com#?#selector, especially with compound selectors in the document (see #6619).
• Test that whitespace in snippet filters works as specified in #6538 and in the testing hints in #6782. e.g. example.com#$#log Hello; log 'How are you?' should call the log snippet twice; the console should show "Hello" and "How are you?", each on its own line, in that order.
• Test the hide-if-contains snippet, see #6809.

[kzar]

Thanks for filing the issue. I've fixed it up, but two bits of feedback for next time:

• We already had an issue (#6821) which could/should have been used for the dependency update. Since this issue was created from scratch the background section needed a bit more information.
• The testing instructions "See the individual issues listed above." weren't enough. For example, take #6538, #6781 - Minimize access to ElemHideBase's selector property. Well, the issue description (of either issue) didn't give much clue what had changed in that commit. I had to find the commit and look through it myself, from there I could see that the only functionality it touches is element hiding.

[mjethani]

Shouldn't the tester focus on $domain rather than $sitekey?

[mjethani]

Replying to kzar:

Thanks for filing the issue. I've fixed it up, but two bits of feedback for next time:

Thanks.

• We already had an issue (#6821) which could/should have been used for the dependency update. Since this issue was created from scratch the background section needed a bit more information.

Isn't #6821 a different issue (about the bug Ross found)? This issue is about the dependency update, which is more than just that one bug. I thought it was appropriate to make this one separate and mark it as blocking #6821.

• The testing instructions "See the individual issues listed above." weren't enough. For example, take #6538, #6781 - Minimize access to ElemHideBase's selector property. Well, the issue description (of either issue) didn't give much clue what had changed in that commit. I had to find the commit and look through it myself, from there I could see that the only functionality it touches is element hiding.

So the "hints for testers" section should include testing hints for each commit? Isn't the tester going to be testing the entire issue? I don't see anything new in what you've added here, #6781 already mentions element hiding and element hiding emulation.

[kzar]

Isn't the tester going to be testing the entire issue?

Well yes, hopefully that whole issue will be tested. Thing is, since this change is landing after testing already started, it's helpful to give some more details about it. Smoke testing of element hiding is enough for that tiny commit IMO, even though the issue it's linked to is for the whole snippets feature. Supposing someone already started testing the snippets feature, it might save them time to mention that they don't need to start testing it all over again.

Isn't #6821 a different issue (about the bug Ross found)? This issue is about the dependency update, which is more than just that one bug.

Well no, #6821 is about the fact that Chrome 49 support was broken. The fix is to update the adblockpluscore dependency, so IMO it should have been the same issue. Either way, it's not too big a deal, it just meant I had to duplicate what I'd typed in #6821 here. (I do agree about having a separate issue for the adblockpluscore change however.)

The fact that other unrelated changes are included in the dependency update is an unfortunate side-effect, which you're right should be mentioned in the issue description with testing instructions etc. But that doesn't make it a separate issue.

So the "hints for testers" section should include testing hints for each commit?

Yes, as far as it's useful. As an example there's no need to duplicate your explanation of hide-if-contains here, easier to just link to the issue.

[mjethani]

Replying to kzar:

[...]

So the "hints for testers" section should include testing hints for each commit?

Yes, as far as it's useful. As an example there's no need to duplicate your explanation of hide-if-contains here, easier to just link to the issue.

I see, this makes sense. Thanks!

[kzar]

Replying to mjethani:

Shouldn't the tester focus on $domain rather than $sitekey?

Well I saw that the $sitekey related logic was touched when quickly looking through the changes, but if you think the $domain related logic needs re-testing too then please by all means add that to the description as well.

[mjethani]

Replying to kzar:

Replying to mjethani:

Shouldn't the tester focus on $domain rather than $sitekey?

Well I saw that the $sitekey related logic was touched when quickly looking through the changes, but if you think the $domain related logic needs re-testing too then please by all means add that to the description as well.

Thanks, that makes sense. Done.

[mjethani]

Dave, I have updated the dependency commit hashes here and also the testing hints. Let me know if you want me to change anything. I'll update the patch now.

[kzar]

Why? The unrelated changes that got included before were an unfortunate side-effect of the dependency update, but now you've added another one deliberately.

This issue is P1 since it's to fix a release-blocking regression since the previous release. You already got a LGTM, I don't know why you didn't already push it. The testers need to carry on testing in preparation for the release.

[mjethani]

You're misunderstanding, this is a ​separate bookmark called adblockpluschrome-3.3 and it only includes the two changes we agreed upon.

Try:

hg update adblockpluschrome-3.3
hg log -r ::.

[mjethani]

Replying to kzar:

You already got a LGTM, I don't know why you didn't already push it.

I didn't push it because we were still discussing whether we should include the unrelated changes. Now we have decided not to include them. I have created a separate bookmark and cherry-picked only the two changes we want to include (see the issue description).

The testers need to carry on testing in preparation for the release.

Yes, so now I'm waiting on LGTM.

[kzar]

Replying to mjethani:

You're misunderstanding, this is a ​separate bookmark called adblockpluschrome-3.3...

I see, you created a new bookmark. I had assumed the updated hash was to include another commit, sorry my mistake. FWIW I'm glad you did it this way, the less unrelated changes that are included so late before a release the better.

...and it only includes the two changes we agreed upon.

I have to wonder, if we're doing it this way, why not only include the one change required for this issue? The hide-if-contains snippet is unrelated to the Chrome 49 problem right?

Now we have decided not to include them.

Where did you discuss that out of interest? Perhaps I was missed from the email chain.

[kzar]

Replying to kzar:

I have to wonder, if we're doing it this way, why not only include the one change required for this issue?

Ah I see, since you branched off at the existing commit, instead of cherry picking the desired commits for the new branch. Well, that's good enough in this instance I guess, I'll take a look at the review now.

[mjethani]

Replying to kzar:

Replying to mjethani:

[...]

...and it only includes the two changes we agreed upon.

I have to wonder, if we're doing it this way, why not only include the one change required for this issue? The hide-if-contains snippet is unrelated to the Chrome 49 problem right?

Since Ross mentioned that we might as well include the snippet (see his email), I included it as well.

Now we have decided not to include them.

Where did you discuss that out of interest? Perhaps I was missed from the email chain.

The email thread with the subject "Adding more snippets before release"

[kzar]

Replying to kzar:

Replying to kzar:

I have to wonder, if we're doing it this way, why not only include the one change required for this issue?

Ah I see, since you branched off at the existing commit, instead of cherry picking the desired commits for the new branch.

Actually, no I was wrong there. You've cherry picked both commits for the branch.

Since Ross mentioned that we might as well include the snippet...

Fair enough, if Ross' happy then so am I.

The email thread with the subject "Adding more snippets before release"

Mind forwarding me a copy?

Either way, just give me a second to do the review now. This issue looks good.

[kzar]

Replying to kzar:

Mind forwarding me a copy?

Nevermind, found it! Thanks.

Edit: I see, that's where you discussed creating a branch too. Sorry, I missed that somehow.

[abpbot]

A commit referencing this issue has landed:
​Issue 6825 - Update adblockpluscore dependency to hg:d9196c0de5f2

[mjethani]

Replying to kzar:

Replying to kzar:

Mind forwarding me a copy?

Nevermind, found it! Thanks.

Edit: I see, that's where you discussed creating a branch too. Sorry, I missed that somehow.

No problem, thanks!

The dependency has been updated now.

[Ross]

Done. Extension works in Chrome 49 again. The snippet and selector changes also work as expected.

ABP 3.2.0.2103
Chrome 68 / 55 / 49 / Windows 10
Firefox 61 / 55 / 51 / Windows 10