Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Get rid of IntentProcessor.matches() #6097

Closed
pocmo opened this issue Feb 27, 2020 · 0 comments
Closed

Get rid of IntentProcessor.matches() #6097

pocmo opened this issue Feb 27, 2020 · 0 comments
Assignees
Labels
⌨️ code Technical debt, code clean up, small API change .. E1 Estimation points: 1
Milestone

Comments

@pocmo
Copy link
Contributor

pocmo commented Feb 27, 2020

While debugging #5974 I noticed that there can be a mismatch where processIntent() fails to process and Intent but the simple check in matches() match. In that situation we may end up trying to route an intent even though we didn't process it.

After all the required checks ("Is that a matching intent?") are already performed when processing the Intent`. So there's no need to run over all processors again and ask them if they match - causing potentially wrong matches.

We can first refactor Fenix to only use process() and then we can get rid of matches in our IntentProcessor interface.

┆Issue is synchronized with this Jira Task

@pocmo pocmo added the ⌨️ code Technical debt, code clean up, small API change .. label Feb 27, 2020
@pocmo pocmo self-assigned this Feb 27, 2020
pocmo added a commit to pocmo/fenix that referenced this issue Feb 27, 2020
This patch removes the need to process() and then match() on every intent processor.
A successful process() means that we already matched. Doing this in two steps caused
issues when the process() failed but later the simple check in match() returned true.

After this patch lands I intent to remove the match() API from the IntentProcessor
interface in AC:
mozilla-mobile/android-components#6097
boek pushed a commit to mozilla-mobile/fenix that referenced this issue Feb 28, 2020
This patch removes the need to process() and then match() on every intent processor.
A successful process() means that we already matched. Doing this in two steps caused
issues when the process() failed but later the simple check in match() returned true.

After this patch lands I intent to remove the match() API from the IntentProcessor
interface in AC:
mozilla-mobile/android-components#6097
@pocmo pocmo added the E1 Estimation points: 1 label Mar 2, 2020
@jonalmeida jonalmeida added this to ⏳ Sprint Backlog in A-C: Android Components Sprint Planning Mar 2, 2020
@pocmo pocmo moved this from ⏳ Sprint Backlog to 🏃‍♀️ In Progress in A-C: Android Components Sprint Planning Mar 3, 2020
pocmo added a commit to pocmo/android-components that referenced this issue Mar 3, 2020
IntentProcessor.process() already checks whether the Intent matches and returns true or
false. Another call to matches() is not necessary. Fenix was already refactored and we
removed the matches() call.
@pocmo pocmo added this to the 35.0.0 ✨ milestone Mar 3, 2020
pocmo added a commit to pocmo/android-components that referenced this issue Mar 3, 2020
IntentProcessor.process() already checks whether the Intent matches and returns true or
false. Another call to matches() is not necessary. Fenix was already refactored and we
removed the matches() call.
bors bot pushed a commit that referenced this issue Mar 3, 2020
6159: Issue #6097: Remove IntentProcessor.matches(). r=psymoon a=pocmo

IntentProcessor.process() already checks whether the Intent matches and returns true or
false. Another call to matches() is not necessary. Fenix was already refactored and we
removed the matches() call.



Co-authored-by: Sebastian Kaspari <s.kaspari@gmail.com>
boek pushed a commit to boek/fenix that referenced this issue Mar 3, 2020
This patch removes the need to process() and then match() on every intent processor.
A successful process() means that we already matched. Doing this in two steps caused
issues when the process() failed but later the simple check in match() returned true.

After this patch lands I intent to remove the match() API from the IntentProcessor
interface in AC:
mozilla-mobile/android-components#6097
@pocmo pocmo closed this as completed Mar 4, 2020
A-C: Android Components Sprint Planning automation moved this from 🏃‍♀️ In Progress to 🏁 Done Mar 4, 2020
jonalmeida pushed a commit to jonalmeida/android-components that referenced this issue Mar 4, 2020
IntentProcessor.process() already checks whether the Intent matches and returns true or
false. Another call to matches() is not necessary. Fenix was already refactored and we
removed the matches() call.
gmierz pushed a commit to gmierz/fenix that referenced this issue Mar 5, 2020
This patch removes the need to process() and then match() on every intent processor.
A successful process() means that we already matched. Doing this in two steps caused
issues when the process() failed but later the simple check in match() returned true.

After this patch lands I intent to remove the match() API from the IntentProcessor
interface in AC:
mozilla-mobile/android-components#6097
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⌨️ code Technical debt, code clean up, small API change .. E1 Estimation points: 1
Projects
No open projects
Development

No branches or pull requests

1 participant