For #15362: Remove swipe to delete for bookmarks

[kleinph]

Closes #15362

Pull Request checklist

• Tests: This PR includes thorough tests or an explanation of why it does not
  This PR does remove a feature and its tests.
• Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.
  A feature was removed, so accessibility should not change.

To download an APK when reviewing a PR:

1. click on Show All Checks,
2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
3. click on the "Fenix - assemble" task, then click "Run Artifacts".
4. the APK links should be on the left side of the screen, named for each CPU architecture

[firefoxci-taskcluster]

No Taskcluster jobs started for this pull request

The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

[Mugurell]

Thank you for helping with this @kleinph!
There is still a little bit of cleanup to do around

• the onStartSwipingItem, onStopSwipingItem methods from BookmarkView/Interactor/Controller
  • also the tests for these methods
• and the code related to isSwipeToRefreshEnabled in BookmarkFragmentStore.
  • also the related tests

[kleinph]

@Mugurell should I do the missing pieces in extra commits or can I amend the original commit?
Thanks for reviewing!

[Mugurell]

@Mugurell should I do the missing pieces in extra commits or can I amend the original commit?
Thanks for reviewing!

To me a single commit is fine - I see all changes related and all for code cleanup.
So I'd be for amending the original one.

[Mugurell]

Thank you @kleinph for the prompt changes!

After deleting the handleStartSwipingItem & handleStopSwipingItem methods from BookmarkController there are now 2 tests failing in BookmarkControllerTest as they tried testing these 2 methods. These tests probably needs to be removed also.

Looking at the implementation of these two Controller methods I see that their intent was to enable / disable the pull down to refresh functionality while the user does a horizontal swipe, something that was implemented in #12831.
Since after removing this swipe to delete functionality there is no need for having this code to disable pull to refresh anymore (& there are no other methods that would emit that SwipeRefreshAvailabilityChanged Action) we should also cleanup BookmarkFragmentStore and BookmarkFragmentStoreTest of now dead code.
Maybe for these changes a separate commit would make them easier to follow?

If you need any help with this please say. I understand it can be a bit more complex than it appeared at a first look.
You can also use the changes from #12831 as a guide in what to remove.

[kleinph]

Thank you @kleinph for the prompt changes!

After deleting the handleStartSwipingItem & handleStopSwipingItem methods from BookmarkController there are now 2 tests failing in BookmarkControllerTest as they tried testing these 2 methods. These tests probably needs to be removed also.

Sorry, I missed the part about building and the pre push hooks in the Readme, because I followed the link to the contributing guide and never returned. Will fix this and run tests for all commits locally from now on.

Looking at the implementation of these two Controller methods I see that their intent was to enable / disable the pull down to refresh functionality while the user does a horizontal swipe, something that was implemented in #12831.
Since after removing this swipe to delete functionality there is no need for having this code to disable pull to refresh anymore (& there are no other methods that would emit that SwipeRefreshAvailabilityChanged Action) we should also cleanup BookmarkFragmentStore and BookmarkFragmentStoreTest of now dead code.
Maybe for these changes a separate commit would make them easier to follow?

Yes, I also think so.

If you need any help with this please say. I understand it can be a bit more complex than it appeared at a first look.

That's no problem, it just makes it more interesting :-)

You can also use the changes from #12831 as a guide in what to remove.

I now fixed the failing tests and also removed an obsolete import the linter spotted. Cleanup of dead code will follow soon.

[Mugurell]

This looks awesome!
Thank you!

[Mugurell]

bors try

[bors]

try

Build succeeded:

[Mugurell]

bors r+

[Mugurell]

Sorry that this took so long @kleinph .
The patches are now merged.
Thank you for helping with this!