Update adblockplusui dependencies to ead38c2013b5

[saroyanm]

Background

After previous dependency updates (#6403), some UI related issues were noticed.
This update fixes those issues and makes some other changes.

What to change

• Update adblockplusui dependency to ead38c2013b5
• Update lib/options.js to use a long-lived connection to listen to "app.listen" message (see ​reference implementation)

Changes since last adblockplusui dependency update

Ticket	Summary	Component
#6488	Dependency update fails due to missing dependencies	User-Interface
#6487	IOElement as base component for ABP UI	User-Interface
#6451	Adjust devtools panel to support $csp filters	User-Interface
#6440	High CPU/memory usage when adding large number of custom filters	User-Interface
#6432	Regression: Hide remove button for additional filter lists	User-Interface
#6420	Whitelist filter notification shown on page load	User-Interface
#6310	Start using JavaScript modularization tool in adblockplusui	User-Interface
#6309	Start using SASS in adblockplusui	User-Interface
#6292	Make issue reporter compatible with test server	User-Interface

Hints for testers

(Also see test instructions for #6403 and #6511.)

• Test that modifying large amounts of custom filters (like 15,000) doesn't crash the extension / run out of memory (#6440, #6309).
• Perform a general of the extension, e.g. that blocking works and the options page opens, since there have been some changes to the way adblockplusui is built (#6488, ​410b982c7a9c, #6309, #6310).
• Smoke test the issue reporter since a change was made to that (#6292).
• Test that subscriptions defined by administrators in the additional_subscriptions can't be removed (#6432).
• Test that the "example.com has been whitelisted" message is not shown every time a whitelisted page loads (#6420). #6432 was fixed
• Test that subscription links open the options page properly (#6499).

[saroyanm]

• User custom filter lists now are more optimized, so they now should handle huge amount(15000) of filters as good or even better than old options page.

@Thomas is this hint accurate enough ?

[greiner]

I modified it a bit because the aim was that the extension shouldn't break anymore. I also added a reference to the hints in #6440.

[saroyanm]

@Dave: As soon this ticket is ready, I can also upload the patch.

[greiner]

Updated ticket description to reflect necessary changes mentioned in #6499.

[kzar]

Note we are currently in feature freeze for the upcoming release.

[sebastian]

Yes, we are, but patches to be landed in next are still welcome. ;)

[saroyanm]

I think would be great to have this dependency update for the current release if possible, the way I understand last two comments, is that we intend to make this dependency update for the next release ?

I just would like to clarify that I understood the comments correctly.

There are couple of issues that can become annoying for users, especially : #6432, #6420.
Also maybe: #6440

But if you say we can't make this one into current release, would be great to prepare for the next release soon if possible.

[sebastian]

These certainly aren't the last issues we will find with the new options page. If it would have been up to me, I'd happily waited until the new options page got more mature before rolling it out on Chrome. But you guys pushed to have the new options page shipped with this release.

If we'd abandon code freeze now to land this dependency update before releasing Adblock Plus 3.0.3, this will delay the release by 1-2 more weeks, and by that time we will probably have found new issues, and we keep delaying this release indefinitely.

[greiner]

Those are fair points. I hope that we can find a better approach for that in the future though because the UI team can't do any real QA before a dependency update. Maybe that wasn't clearly communicated. At least it sounds like that during the QA phase we're only addressing issues in adblockpluschrome but none of its dependencies.

Unfortunately, I don't have a solution for this at this point either and if we are satisfied with the current release as-is that's fine with me.

[sebastian]

The UI is tested as part of our release QA process, even though those particular issues were identified by developers and users (not testers). However, QA not catching every single bug during manual testing isn't entirely surprising, in particular when replacing the whole thing (i.e. the options page in this case) with a new implementation. But one thing you can do, to improve QA in the future, is putting a "Hints for Testers" section in your issues with sufficient information for testers to cover all test cases.

Normally a regression would mean that we will fix it before the release and delay the release if necessary. But this approach is mutually exclusive if we want to release the new options on Chrome/Opera any time soon, which wasn't our call but yours. Moreover, we have other improvements and bug fixes waiting to be released already for too long. So if anything, the conclusion would be to not replace the options page on Chrome/Opera yet, but move forward with the other changes in the release, but it's a bit late for that.

[kzar]

Replying to greiner:

I hope that we can find a better approach for that in the future though because the UI team can't do any real QA before a dependency update.

How about we just merge the adblockpluschrome and adblockplusui repositories? That way after code-freeze commits would land to adblockpluschrome's next branch no matter if they are UI changes or other changes. We could then also cherry-pick commits needed for the release after feature freeze from the next branch.

[sebastian]

It turned out we now have to update the dependency, before releasing Adblock Plus 3.0.3, due to the (non options page related) regressions #6510 and #6512.

I tend to agree with kzar about merging the adblockpluschrome and adblockplusui repositories. But this should probably be discussed somewhere else. However, regardless whether we use separate repositories or merge them, it would be great if the UI team would respect code freeze. So that if regressions are found in the UI, we can fix them without pulling in unrelated changes.

[kzar]

Please can you also include ​12c59fe0fa0a with this update, since that is a blocker for #6511 which is a regression since the previous Chrome release.

[kzar]

Replying to sebastian:

However, regardless whether we use separate repositories or merge them, it would be
great if the UI team would respect code freeze. So that if regressions are found in the
UI, we can fix them without pulling in unrelated changes.

Yea, since for example now we're going to end up including #6487 in the release to get the fix for #6511 in! :/

[agiammarchi]

Replying to kzar:

Yea, since for example now we're going to end up including #6487 in the release to get the fix for #6511 in! :/

That's used nowhere so far, hence not bundled, which means it won't be part of the desktop-options.js file which is all you care about when you ./build.py, right?

[saroyanm]

I prepared a patch, but this still depends on #6510 which I think will be ready soon.

[saroyanm]

Replying to sebastian:

I tend to agree with kzar about merging the adblockpluschrome and adblockplusui repositories. But this should probably be discussed somewhere else. However, regardless whether we use separate repositories or merge them, it would be great if the UI team would respect code freeze. So that if regressions are found in the UI, we can fix them without pulling in unrelated changes.

Ideally would be great to have a release branch for ABPUI, while still keeping it separate if possible, but I agree to discuss this separately and see what are the options.

Most of the changes here suppose to be bugfixes we considered important for the release(P1, like regressions from old options page), or changes that do not affect the pages functionality, but I agree that from now on we shouldn't introduce non critical bugfixes until current release.

[sebastian]

I prepared a patch

Thanks.

Ideally would be great to have a release branch for ABPUI

FWIW, in adblockpluschrome we create a next branch for changes, landing after code freeze, to be released after the next release. Then after the release we merge these changes back into master. Ideally, you would adapt the same process in adblockplusui, moving development to next after the last planned dependency update before the release.

Most of the changes here suppose to be bugfixes we considered important for the release(P1, like regressions from old options page), or changes that do not affect the pages functionality

Only few of the changes here are critical to the release. Even if you disagree with my line of argument above regarding the issues with the new options page, at very least the changes to generate the JS and CSS still would be completely inappropriate for a bugfix update. Just because a change isn't supposed to have any visible effects, it can still cause new bugs.

[agiammarchi]

Replying to sebastian:

FWIW, in adblockpluschrome we create a next branch for changes, landing after code freeze, to be released after the next release. Then after the release we merge these changes back into master.

I'm sorry for the merge at this point. I swear I've asked yesterday in apb-ui channel if it was OK during code freeze to merge but I guess I fully misunderstood the answer.

Regardless, my changes in #6487 are really production/release safe because do not touch the bundled file ABP Chrome uses at all.

The ticket about dependencies update mechanism is a story a part thought, and the ticket to fix that is needed, and not really related to my changes. I am quite happy it goes in this time because sooner or later we had to validate our bundling process even for releases.

However, if I have to be honest, I've worked in small to very big companies and this release process compared to other places, seems upside down to me.

Usually one creates a release branch so that nothing, not even by accident, goes in such branch.

Then patches are carefully cherry picked from master and, if some patch needs unrelated changes, we can create an emergency patch that drops all unrelated changes.

Blocking another repository master, our only source of truth/progress, due ABP Chrome code freeze, and having a branch for the next release, but keeping master free to receive commits, is a non sense to me, but I guess there were reasons to do it this way, although it feels very error prone and a coordination hell.

I wonder if we can improve here: is this something related to Mercurial and how it works? Why isn't adblockplusui just a sumbodule of adblockpluschrome that, as such, can be pinned to a specifc version in a branch a part?

Thinking out loudly, I have the impression if we were on git everything would be simpler, or at lest that's been my experience so far with any other project.

[kzar]

Note I'm going to need to update the adblockplusui dependency again for #6511 since some other changes are required to adblockpluschrome to do that. Then we're going to need to update the dependency again for #6510 as well.

Ross I recommend testing #6403, this and #6511 at the same time. Sorry for the mess :/

[saroyanm]

Updated with latest changes that we have included.

[abpbot]

A commit referencing this issue has landed:
​Issue 6476 - Update adblockplusui dependency to revision ead38c2013b5

[sebastian]

Replying to agiammarchi:

However, if I have to be honest, I've worked in small to very big companies and this release process compared to other places, seems upside down to me.

Usually one creates a release branch so that nothing, not even by accident, goes in such branch.

The main problem with this approach is that our development builds are created from the master branch. So if we use a different branch for the code to be released, we won't have automated builds for testing the release candidate. The other way around, if we'd generate the the development builds from the release branch instead, the development builds won't be updated before we start planning a release, and we won't have continues builds with the latest changes anymore.

Our current release process seems to match (more or less) what other open source projects of the same and smaller size do, just that they might not create a next branch, but either call it a feature branch, or just don't land new changes during code freeze at all. The latter is what we did before, but as more people started to work on the code base, it became quite unwieldy to go through all the LGTM'd reviews after the release to find changes that can land now, to resolve merge conflicts after review already passed, and to work on changes that depend on other changes that are blocked by code freeze. The common solution here is using a feature branch, which our next branch essentially is.

Larger open source projects (like Firefox and Chromium) would go with multiple pre-release channels, one with the latest (untested) changes built from the main development branch, and then at least one branch that is forked off and stabilized as part of the release process, with separate automated builds for each pre-release channel. But our development efforts don't seem large enough to justify this overhead, and we don't have enough development build users that splitting up the development builds into multiple channels would do any good. Once we move closer to a release, we want people to test the release candidate, otherwise we want them to test the latest changes that landed. Currently, at any given time, what is in our master branch, and therefore in our development builds, is what we want people to try out. If we split those into multiple channels, people would only test either for one or the other case.

Then patches are carefully cherry picked from master and, if some patch needs unrelated changes, we can create an emergency patch that drops all unrelated changes.

A complete diverge from release and main development branch seems rather error prone. Changes accidentally not being released is as much a concern as immature changes being released too early. Not to mention occasional bugs caused by non-obvious side effects of combining changes in a different set than they have been engineered for, which might or might not involve post-review merge conflicts. A feature branch might be a better approach. But if you go with a release branch then at least make sure to start over with a fresh fork of the main development branch at the beginning of each release cycle.

Blocking another repository master, our only source of truth/progress, due ABP Chrome code freeze, and having a branch for the next release, but keeping master free to receive commits, is a non sense to me, but I guess there were reasons to do it this way, although it feels very error prone and a coordination hell.

It seems the process would be more streamlined if we merge the adblockpluschrome and adblockplusui repositories. But as long as we don't, we have to sync the release process manually.

Why isn't adblockplusui just a sumbodule of adblockpluschrome that, as such, can be pinned to a specifc version in a branch a part?

This is exactly how it works, just that we use ensure_depdencies.py as dependency mechanism, which lets you pin versions in the same way. We used Mercurial Subrepositories before, but these cannot be synced properly to the GitHub mirror. The problem here is not Mercurial itself, but that the URL of the external repository has to refer to the respective mirror when synced to a foreign VCS.

Anyway, the problem here is not that we cannot pin a specifc revision of adblockplusui (because this is exactly what we do), but that we cannot pull in only regression fixes during code freeze, if there is no revision in adblockplusui to point to that wouldn't pull in unrelated features as well.

I wonder if we can improve here: is this something related to Mercurial and how it works?
[...]
Thinking out loudly, I have the impression if we were on git everything would be simpler, or at lest that's been my experience so far with any other project.

Nothing of this is specific to Mercurial or would be any different if all of us would use only Git.

[agiammarchi]

Hi Sebastian, I appreciate your time to write this exhaustive explanation, which surely deserves some expansion from my post too.

Larger open source projects (like Firefox and Chromium) would go with multiple pre-release channels

You basically went from we release from master to multiple pre-release channels are not what we want.

What I had in mind instead, is a basic (pardon my git) git checkout -b release-march in chrome repository, having adblockplusui as submodule, hence pinned it with its current version.

That is the code freeze branch for the release.

Nothing goes in by accident, everything can be tested at any time, any ticket can be cherry picked, anyone can use its hash as base to create patches: everyone is happy.

That's not the beginning of multiple channel like Chrome or Firefox, that's simply our source of truth for the release.

A complete diverge from release and main development branch seems rather error prone.

Yes and no, and it's not necessarily a complete one. A code freeze is a code freeze, master wouldn't really care much what goes into that branch, but I am also talking about cherry picking changes where, if there is something that absolutely should not go out in the current release, we can create a new ticket, bring in only changes that are welcome, and drop the rest. It's fine, that release branch will be tested to validate emergency patches.

This is different from a complete diversion: master keeps going and the relase tests in place to ensure everything works will simply keep working next release (unless new bugs are introduced, of course).

I see though we need to automate testing way more than we do now. I've recently introduced smoke tests for UI components, and I've read with interest your comments about testing an extension via Selenium. I'm sure that won't work out of the box, but combining it with Docker might (I'm still digging through details).

we use ensure_depdencies.py as dependency mechanism ...

That's great, if we can easily improve that too.

AFAIK, branching the release is the most common technique, and I've personally witnessed it in Facebook, Twitter, and others bigs or little startups I've worked with.

That process never really disappointed anyone expectations: you have a release source of truth, and also a single entry point to eventually rollback if something goes really wrong.

As summary, I think having slightly higher friction to land patches in a release branch, is also more convenient than having a situation nobody really controls 'cause the tool keeps bringing in from master, which is also a notoriously unstable environment.

I don't think we need multiple release channels, and we could just try to branch out the release next time, and see how next code freeze will work.

From my perspective, cherry picking into release branch, with auto rejections on merge conflicts, is a good guard for release stability.

Nothing of this is specific to Mercurial or would be any different if all of us would use only Git.

All I know is that Git like branches in HG seems to be very different, while bookmarks seem to keep changes from other bookmarks (with the little experience I have with HG).

ABP UI is also not just a submodule like I would expect, and there's not even a way to whitelist files in .hgignore.

So maybe it's not Mercurial fault, but I honestly don't see any reason to not branch releases, something I've always done behind git.

Thanks for your patience in reading all this :-)

[agiammarchi]

I don't know what's going on, I am apparently unable to post my whole answer. I'll try one more time in here.

---

No, apparently I cannot post my answer.

[agiammarchi]

right, I've edited my previous answer. I've just realized if you post an emoji in trac, everything goes bananas.

Do not ever use CTR+CMD+SPACE to write an emoji in trac, sorry for the off topic.

[sebastian]

Whether to use a release branch, or release from master (and create a feature branch for changes landing during code freeze), generally doesn't matter much. However, in our case it is necessary to release from master in order to have a single meaningful pre-release channel, as I explained.

I don't see how the source of truth would be ambiguous. Currently, the master branch in adblockpluschrome, the latest development build, and what would be released if we'd release right now, is the same at any given point in time. So there is no uncertainty about what is going to be released.

Where necessary we can (and do) graft bug fixes from next to master (or land them in master in the first place and merge with next later). However, if the relevant bug fix is in a dependency, we cannot graft it but have to update the dependency (this would be the same with Git Submodules).

So the problem is as long as adblockplusui is a separate repository, and doesn't participate in code freeze, we might not be able to update the dependency without pulling in other unrelated changes, as it was the case here. Any other problem you try to address is not a problem we have.

Replying to agiammarchi:

All I know is that Git like branches in HG seems to be very different, while bookmarks seem to keep changes from other bookmarks (with the little experience I have with HG).

When I talked about branches above, I was essentially talking about bookmarks (in Mercurial terms) which work exactly the same (and are as easy to handle) as branches in Git.

[agiammarchi]

Replying to sebastian:

So the problem is as long as adblockplusui is a separate repository, and doesn't participate in code freeze

That's solved when you have a proper submodule and you create a branch in the parent, that's the problem I see here, the one I am trying to solve.

We don't need to participate code freeze as submodule, we act like a submodule and you can cherry pick from the release branch.

Alternatively, we, ABP UI, can have a release branch and you, parent folder but not parent module, can pull from that one instead.

We, ABP UI, will eventually cherry pick changes into that branch.

We move the branch release strategy in a submodule, that is not really a submodule, instead of having the branch strategy in the root folder, 'cause you prefer using master as release branch.

Would that work better? In a way or another, we don't want to be stuck behind other repositories code freeze and we need to discuss and find a solution that works for everyone.

If we need to create a next branch too when there is a code freeze we still need coordination, something my release branch proposal wouldn't, but then we'll adapt to make it work for everyone.

Thanks for extra hints or solutions.

P.S. it is still not fully clear to me why we're not just a submodule and we need 3rd parts scripts to make us look like one

[sebastian]

Replying to agiammarchi:

That's solved when you have a proper submodule and you create a branch in the parent, that's the problem I see here, the one I am trying to solve.

We don't need to participate code freeze as submodule, we act like a submodule and you can cherry pick from the release branch.

I don't see how that is supposed to work. Even if we'd use Git Submodules (instead of ensure_depdencies.py), we still need to pin a single revision of the external repository. We cannot simply say give me baseline 93b2850 plus 1669870 and 7411639, or can we?

If we need to create a next branch too when there is a code freeze we still need coordination, something my release branch proposal wouldn't, but then we'll adapt to make it work for everyone.

You (the UI team) already requests a dependency update, once you have changes ready to be released. At this point you could just create a next branch for further changes to land, and just before requesting the next (non-bugfix) dependency update you merge next back into master.

P.S. it is still not fully clear to me why we're not just a submodule and we need 3rd parts scripts to make us look like one

This is necessary to manage our dependencies (aka submodules/subrepos) transparently with both Git and Mercurial, as I explained before.

[agiammarchi]

Replying to sebastian:

I don't see how that is supposed to work. Even if we'd use Git Submodules (instead of ensure_depdencies.py), we still need to pin a single revision of the external repository. We cannot simply say give me baseline 93b2850 plus 1669870 and 7411639, or can we?

If we use a branch strategy we can define the code-freeze "day" and create such branch. At that point the submodule can point at such branch and we cherry pick changes, when needed, so that parent module has nothing to do if not update that branch.

In few words, in release day the build tools/process checksout realease-march (example name) and we keep working in master and cherry pick patches. Parent module would use the release-march as submodule, since you can specify branches, and be done.

We won't ship unrelated changes, parent module doesn't need to do anything else from that time on.

This is necessary to manage our dependencies (aka submodules/subrepos) transparently with both Git and Mercurial, as I explained before.

I guess my question is rather why we need to manage both mercurial and git instead of git only, but I know this is another story not worth discussing in here/now, while it'd be great to actually define the new process we want to follow for the next release.

Unless we just keep things the way these are, but it's quite clear to me the situation is not ideal.

[sebastian]

What you are saying is that you would essentially do code freeze in adblockplusui but instead of creating a feature branch to land changes not yet to be released, you would create a release branch. It's just somewhat inconsistent with what we do in adblockpluschrome, where I explained why we have to do it the other way around, but I guess for adblockplusui that could work. But I still don't see how using Git Submodules (instead of ensure_depdencies.py) would make any difference here.

[agiammarchi]

Replying to sebastian:

It's just somewhat inconsistent with what we do in adblockpluschrome, where I explained why we have to do it the other way around, but I guess for adblockplusui that could work.

The problem is that right now we're kinda stuck in a limbo and I won't push anything at this time because we don't have a proper process in place (so I work in local folders/branches).

Bookmarking a release looks to me the easiest way to move forward because we don't change our process if not when absolutely needed (patches/fixes) and we keep working in master.

Doing the other way around would require two major steps instead of one:

1. remembering to not work on master, which is our default way to work except in release/code-freeze days (error prone, IMO)
2. once the release is done merge back features with all possible conflicts we could have in master that meanwhile moved forward for, potentially, weeks

If we branch the release instead, the second step would never need to happen.

Patches for the release could land in master, release, or both, and we can "forget" about that branch once released.

I see this as a simpler approach, but if the parent folder does not use UI bookmark then it won't work.

I also would like to know Thomas and Manvel opinion on this.

But I still don't see how using Git Submodules (instead of ensure_depdencies.py) would make any difference here.

You are right. Done this way would need some coordination regardless. I was thinking about branching recursively in code-freeze day but if you need to be on master then it won't work, no matter if UI is submodule or not.

[sebastian]

Sounds good enough, for now.

(But again if we'd just merge the repositories, all changes would be subject to the same code freeze process and we don't have to worry about dependency updates.)

[Ross]

Done. Issue reporter looks okay. General testing is okay. Adding a huge number of filters is better. Whistlist notification is fixed. Subscription links work. Additional subscriptions work and cannot be removed.

ABP 3.0.2.2002
Firefox 51 / 58 / Windows 10
Chrome 49 / 65 / Windows 7
Opera 36 / 51 / Windows 7