[cr137][Android] Fix tablet toolbar click event handling #29087
Conversation
This regression happened here ac28907 Only `onLongClick` part was supposed to be removed.
samartnik
added
CI/skip-macos-x64
| forward(); | ||
| RecordUserAction.record("MobileToolbarForward"); | ||
| } | ||
| + BraveToolbarLayout.class.cast(this).onClickImpl(v); |
There was a problem hiding this comment.
shouldn't we subclass ToolbarTablet?
There was a problem hiding this comment.
we definitely need to start creating unit tests for these overrides so this kind of thing doesn't happen
There was a problem hiding this comment.
This one in particular is just a revert of the change that wasn't meant to be done during rebase.
Tests would be great, but we don't run them on tablets so this issue probably wouldn't be caught. Robolectric tests would help, but we don't have them running in Brave at the moment.
As for subclassing ToolbarTablet and ToolbarPhone, we can do it, but then we will have to patch (or likely copy over) corresponding xml files, which is also not great, so we have to choose between 2 not great options.
There was a problem hiding this comment.
I think we can do this pretty cleanly if we change the name of the original class to ToolbarLayoutChromiumImpl and then rename our subclass to to ToolbarLayout so it properly inflates as our class instead of theirs?
There was a problem hiding this comment.
I'm pretty sure we have some robolectric tests, but if not we need to start adding them because we need more tests on android.
…ression [cr137][Android] Fix tablet toolbar click event handling
|
Released in v1.80.75 |
This regression happened here ac28907
Only
onLongClickpart was supposed to be removed.Resolves brave/brave-browser#46110