Opened 14 months ago

Closed 14 months ago

Last modified 13 months ago

#6825 closed change (fixed)

Update adblockpluscore dependency to hg:d9196c0de5f2

Reported by: mjethani Assignee: mjethani
Priority: P1 Milestone: Adblock-Plus-3.3-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: sebastian, kzar, hfiguiere, Ross, rscott Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29843592/

Description (last modified by 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.

Change History (28)

comment:1 Changed 14 months ago by mjethani

  • Review URL(s) modified (diff)

comment:2 Changed 14 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 14 months ago by sebastian

  • Blocking 6821 removed

comment:4 Changed 14 months ago by mjethani

  • Description modified (diff)

comment:5 follow-up: Changed 14 months ago by kzar

  • Cc Ross rscott added
  • Description modified (diff)
  • Milestone set to Adblock-Plus-3.3-for-Chrome-Opera-Firefox
  • Priority changed from Unknown to P1
  • Ready set

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.

comment:6 Changed 14 months ago by kzar

  • Description modified (diff)

comment:7 follow-up: Changed 14 months ago by mjethani

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

comment:8 in reply to: ↑ 5 Changed 14 months ago by 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.

comment:9 follow-up: Changed 14 months ago by 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.

comment:10 in reply to: ↑ 9 Changed 14 months ago by 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!

comment:11 in reply to: ↑ 7 ; follow-up: Changed 14 months ago by 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.

comment:12 Changed 14 months ago by mjethani

  • Description modified (diff)

comment:13 in reply to: ↑ 11 Changed 14 months ago by 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.

comment:14 Changed 14 months ago by mjethani

  • Description modified (diff)
  • Summary changed from Update adblockpluscore dependency to hg:c7c3b5e643f4 to Update adblockpluscore dependency to hg:d9196c0de5f2

comment:15 Changed 14 months ago by 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.

comment:16 follow-up: Changed 14 months ago by 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.

comment:17 follow-up: Changed 14 months ago by 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 ::.
Last edited 14 months ago by mjethani (previous) (diff)

comment:18 in reply to: ↑ 16 Changed 14 months ago by 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.

comment:19 in reply to: ↑ 17 ; follow-ups: Changed 14 months ago by 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.

Last edited 14 months ago by kzar (previous) (diff)

comment:20 in reply to: ↑ 19 ; follow-up: Changed 14 months ago by 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.

Last edited 14 months ago by kzar (previous) (diff)

comment:21 in reply to: ↑ 19 Changed 14 months ago by 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"

comment:22 in reply to: ↑ 20 ; follow-up: Changed 14 months ago by 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.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 14 months ago by 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.

Last edited 14 months ago by kzar (previous) (diff)

comment:24 Changed 14 months ago by abpbot

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

comment:25 in reply to: ↑ 23 Changed 14 months ago by 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.

comment:26 Changed 14 months ago by mjethani

  • Owner set to mjethani

comment:27 Changed 14 months ago by mjethani

  • Resolution set to fixed
  • Status changed from new to closed

comment:28 Changed 13 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

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

Note: See TracTickets for help on using tickets.