Issue 29366966: [adblockpluscore] Issue 3672 - Moved antiadblockInit.js from adblockpluscore to adblockplusui (Closed)
|
Created:
Dec. 7, 2016, 7:47 a.m. by wspee Modified:
Dec. 22, 2016, 8:47 a.m. CC:
Felix Dahlke, Thomas Greiner Visibility:
Public. |
DescriptionIssue 3672 - Moved antiadblockInit.js from adblockpluscore to adblockplusui
Patch Set 1 #
Total comments: 1
MessagesTotal messages: 12
(Setting Wladimir as the reviewer since he is the module owner for Core[1] which the adblockpluscore repository is under. Adding Felix and Thomas as Cc since they are involved with the related review.) When submitting reviews where it is ambiguous which repository it is for please add a note in the title like "(adblockpluscore)". (Alternatively you can use Wladimir's hgreview.py[1] Mercurial module which submits that as an extra field so it shows along the side by reviewer names etc.) Otherwise it gets confusing for people who receive a lot of incoming reviews. The other thing to note with these changes is that the metadata files in adblockplus and adblockpluschrome will need to be updated once this file lives in adblockplusui and the adblockpluscore + adblockplusui dependendencies will need to be updated for both. The adblockplus and adblockpluschrome changes will need to be as separate issues, so you'll first need to file an issue for each of those. Have a look at this https://issues.adblockplus.org/ticket/4589 as an example, notice how Wladimir checked what other changes would be included to avoid regressions. To make things more confusing you currently can't update the adblockplusui dependency for adblockpluschrome since Felix has made a change for "element hide emulation" filters there but the correspoding change did not land in adblockpluschrome yet. (This is why when you do even a minor change and update a dependency you need to test the extension(s) still build and work properly.) In hindsight maybe the "goodfirstbug" tag was optimistic here, but on the positive side it's a good opportunity to learn about how we work with dependencies. [1] - http://adblockplus.org/modules [2] - https://github.com/adblockplus/codingtools
LGTM Please add some basic integration notes to the description in https://issues.adblockplus.org/ticket/3672 however - repositories depending on adblockpluscore (meaning adblockplus and adblockpluschrome) will need to update their build configuration.
On 2016/12/07 11:26:10, kzar wrote: > When submitting reviews where it is ambiguous which repository it is for please > add a note in the title like "(adblockpluscore)". (Alternatively you can use > Wladimir's hgreview.py[1] Mercurial module which submits that as an extra field > so it shows along the side by reviewer names etc.) Otherwise it gets confusing > for people who receive a lot of incoming reviews. Ok, done that (might perhaps make sense to always do it, regardless?) > The other thing to note with these changes is that the metadata files in > adblockplus and adblockpluschrome will need to be updated once this file lives > in adblockplusui and the adblockpluscore + adblockplusui dependendencies will > need to be updated for both. I have prepared commits to change the metadata files, but I wanted to update the dependencies in the same commit as both changes belong together. Should have mentioned that probably ;). > The adblockplus and adblockpluschrome changes will need to be as separate > issues, so you'll first need to file an issue for each of those. Have a look at > this https://issues.adblockplus.org/ticket/4589 as an example, notice how > Wladimir checked what other changes would be included to avoid regressions. Ok see #4715 and #4716 ... are they ok that way? > To make things more confusing you currently can't update the adblockplusui > dependency for adblockpluschrome since Felix has made a change for "element hide > emulation" filters there but the correspoding change did not land in > adblockpluschrome yet. (This is why when you do even a minor change and update a > dependency you need to test the extension(s) still build and work properly.) Is there a way map this in the issue tracker and/or codereview, is there a ticket i can #3672 and/or #4715 depend on. ty ;)
On 2016/12/07 12:22:07, Wladimir Palant wrote: > LGTM > > Please add some basic integration notes to the description in > https://issues.adblockplus.org/ticket/3672 however - repositories depending on > adblockpluscore (meaning adblockplus and adblockpluschrome) will need to update > their build configuration. Please see #4715 and #4716 (ok that way?).
On 2016/12/07 13:55:34, wspee wrote: > I have prepared commits to change the metadata files, but I wanted to update the > dependencies in the same commit as both changes belong together. Should have > mentioned that probably ;). Fair enough. (Sometimes I even upload the review with the dependency pointing at my local branch name, but I guess in this case it's probably not worth it. In any case that's fine so long as you leave an inline comment pointing out that the revision will need to be fixed.) > > > The adblockplus and adblockpluschrome changes will need to be as separate > > issues, so you'll first need to file an issue for each of those. Have a look > at > > this https://issues.adblockplus.org/ticket/4589 as an example, notice how > > Wladimir checked what other changes would be included to avoid regressions. > Ok see #4715 and #4716 ... are they ok that way? Yep, they look mostly OK. Some small nits: - Don't set the Priority or Ready flags unless you are the module owner or peer for a module. (Please could you unset the priority flags you set?) - Please add the module owner and peers as Cc to issues you've created. So for the adblockpluschrome issue me and Sebastian should be Cc, for the adblockplus issue Wladimir should be. That way your issues will get spotted and triaged sooner. Once the adblockpluscore and adblockplusui changes have landed and you have the revision you will need to of course update the issue descriptions. Less obviously you will also need to look through the revision history for both repositories to check if any other changes are included by the dependency updates. If so you will need to check those to see if anything relevant changed, that way we hope to avoid regressions. > Is there a way map this in the issue tracker and/or codereview, is there a > ticket i can #3672 and/or #4715 depend on. Yep, it's issue #4569 which is blocking #4715. You can add that as a blocker in Trac (our issue tracker).
On 2016/12/07 14:20:42, kzar wrote: > Fair enough. (Sometimes I even upload the review with the dependency pointing at > my local branch name, but I guess in this case it's probably not worth it. In > any case that's fine so long as you leave an inline comment pointing out that > the revision will need to be fixed.) You mean a comment in the ticket that causes the dependency update, in this case #3672, like [0]? > Yep, they look mostly OK. Some small nits: > > - Don't set the Priority or Ready flags unless you are the module owner or peer > for a module. (Please could you unset the priority flags you set?) > - Please add the module owner and peers as Cc to issues you've created. So for > the adblockpluschrome issue me and Sebastian should be Cc, for the adblockplus > issue Wladimir should be. That way your issues will get spotted and triaged > sooner. Ok done. > Once the adblockpluscore and adblockplusui changes have landed and you have the > revision you will need to of course update the issue descriptions. Less > obviously you will also need to look through the revision history for both > repositories to check if any other changes are included by the dependency > updates. If so you will need to check those to see if anything relevant changed, > that way we hope to avoid regressions. So currently it depends on A my change ends up being C I need to make sure B doesn't brake anything? Ok ... will do. > Yep, it's issue #4569 which is blocking #4715. You can add that as a blocker in > Trac (our issue tracker). Done Ty [0] https://issues.adblockplus.org/ticket/3672#comment:6
On 2016/12/07 14:20:42, kzar wrote: > Yep, it's issue #4569 which is blocking #4715. You can add that as a blocker in > Trac (our issue tracker). Damn I typed the issue number wrong, it's actually #4659!
On 2016/12/07 14:37:01, kzar wrote: > Damn I typed the issue number wrong, it's actually #4659! Changed to #4659 :)
https://codereview.adblockplus.org/29366966/diff/29366967/lib/antiadblockInit.js File lib/antiadblockInit.js (left): https://codereview.adblockplus.org/29366966/diff/29366967/lib/antiadblockInit... lib/antiadblockInit.js:33: title: Utils.getString("notification_antiadblock_title"), I suppose those translations should be removed from adblockplus and be moved to adblockplusui, now as well? (Yes, these strings have never been in adblockpluscore but in adblockplus).
On 2016/12/13 11:45:26, Sebastian Noack wrote: > I suppose those translations should be removed from adblockplus and be moved to > adblockplusui, now as well? (Yes, these strings have never been in > adblockpluscore but in adblockplus). Moved translations to adblockplusui see [0], [1] [0] https://codereview.adblockplus.org/29369232/ [1] https://codereview.adblockplus.org/29366969/
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
