Issue 29833597: Issue 6784 - Upgrade adblockpluscore to 658f0229baa1 and adblockplusui to 9a652397b9af (Closed)
|
Created:
July 18, 2018, 5:59 p.m. by hub Modified:
July 20, 2018, 4:44 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 6784 - Upgrade adblockpluscore to 658f0229baa1 and adblockplusui to 9a652397b9af
Patch Set 1 #Patch Set 2 : Update revision #Patch Set 3 : Bump revisions #MessagesTotal messages: 20
On 2018/07/18 17:59:44, hub wrote: Aren't there still changes which need to land in adblockpluscore still? For example https://codereview.adblockplus.org/29761612/
Right. Let's revisit this review when the rest has landed (I'll update). Albeit we'll need it for the WebExt snippet change. Also we'll still be missing the UI change for the anti-circumvention subscription.
On 2018/07/18 21:58:12, hub wrote: > Also we'll still be missing the UI change for the anti-circumvention > subscription. Does this mean we have to wait for one more change in adblockplusui? If not, we can update core to 8e466288a0c3 and UI to whatever version you have set in this patch. We're good to go from the core side now.
On 2018/07/18 23:33:41, Manish Jethani wrote: > On 2018/07/18 21:58:12, hub wrote: > > > Also we'll still be missing the UI change for the anti-circumvention > > subscription. > > Does this mean we have to wait for one more change in adblockplusui? The adblockplusui release process is different, the changes for the anti-circumvention are in the branch that will be merged for the next release, and therefore we'll update at that time. This upgrade here for adblockplusui is an exception agreed upon because it is a breaking change. So no, we won't block on it. > If not, we can update core to 8e466288a0c3 and UI to whatever version you have > set in this patch. We're good to go from the core side now. Excellent ! I'll update the patch.
patch updated
LGTM But let's wait for the go-ahead from Dave or Sebastian.
Please could you update the issue description? It doesn't include any testing information for example. On 2018/07/19 10:37:41, Manish Jethani wrote: > But let's wait for the go-ahead from Dave or Sebastian. Yes, that's a requirement for adblockpluschrome.
On 2018/07/19 12:31:29, kzar wrote: > Please could you update the issue description? It doesn't include any testing > information for example. Added some testing information. I probably did miss some.
On 2018/07/19 15:46:53, hub wrote: > On 2018/07/19 12:31:29, kzar wrote: > > Please could you update the issue description? It doesn't include any testing > > information for example. > > Added some testing information. Ace, thanks. I've added a few more details as well. > I probably did miss some. Please could you go through all the included changes, and note specifically which functionality needs to be re-tested and how it can be tested? I know it's a pain, but otherwise we might miss regressions.
> Issue 6784 - Upgrade adblockpluscore to 8e466288a0c3 Also, please could you adjust the commit message? We're also updating the adblockplusui dependency.
While we're still not done with this, can we also include the hide-if-shadow-contains snippet? https://hg.adblockplus.org/adblockpluscore/rev/658f0229baa1 Hubert, I'll let you update the issue and the commit message (and the patch, of course) for this.
On 2018/07/19 17:03:45, Manish Jethani wrote: > While we're still not done with this, can we also include the > hide-if-shadow-contains snippet? > > https://hg.adblockplus.org/adblockpluscore/rev/658f0229baa1 > > Hubert, I'll let you update the issue and the commit message (and the patch, of > course) for this. Updated patch now.
On 2018/07/19 17:28:07, hub wrote: > On 2018/07/19 17:03:45, Manish Jethani wrote: > > While we're still not done with this, can we also include the > > hide-if-shadow-contains snippet? > > > > https://hg.adblockplus.org/adblockpluscore/rev/658f0229baa1 > > > > Hubert, I'll let you update the issue and the commit message (and the patch, > of > > course) for this. > > Updated patch now. Shouldn't we also update the commit message here to include the adblockplusui dependency version?
On 2018/07/19 19:47:10, Manish Jethani wrote: > Shouldn't we also update the commit message here to include the adblockplusui > dependency version? Done
LGTM from my end
(This change is still blocked by the issue description. Especially testing hints and confirmation that all the included changes have been checked through.)
On 2018/07/20 09:23:42, kzar wrote: > (This change is still blocked by the issue description. Especially testing hints > and confirmation that all the included changes have been checked through.) Should the testing information be in the individual issues or in the dependency update issue?
On 2018/07/20 11:25:50, Manish Jethani wrote: > On 2018/07/20 09:23:42, kzar wrote: > > (This change is still blocked by the issue description. Especially testing > hints > > and confirmation that all the included changes have been checked through.) > > Should the testing information be in the individual issues or in the dependency > update issue? Well, if there are good testing instructions in an issue then it's fine to refer to those in the dependency update issue. Otherwise, the instructions should be in the dependency update issue.
LGTM 
|
