Allows to use a microphone for Brave Talk in the background on Android #27016
Conversation
SergeyZhukovsky
added
CI/skip-macos-x64
SergeyZhukovsky
requested review from
simoarpe,
deeppandya,
AlexeyBarabash and
samartnik
| public void startForeground( | ||
| Service service, int id, Notification notification, int foregroundServiceType) { | ||
| // Check for a service that is dedicated to Brave Talk | ||
| if (service.getClass().getSimpleName().equals(sBraveTalkServiceClassName) |
There was a problem hiding this comment.
nit: service.getClass().getSimpleName() is referred twice, can be a variable
| WebContents webContents = | ||
| (WebContents) | ||
| BraveReflectionUtil.getField( | ||
| MediaSessionHelper.class, "mWebContents", this); | ||
|
|
||
| return isBraveTalk(webContents); | ||
| } |
There was a problem hiding this comment.
Reflective operations in Java are usually extremely slow. They can slow down the computation by 10x.
What I would normally do is caching them in a static field so we don't access the reflection APIs multiple time.
So in this case it would be something like this:
if (sWebContents == null) {
sWebContents = (WebContents) BraveReflectionUtil.getField(MediaSessionHelper.class, "mWebContents", this);
}
return isBraveTalk(sWebContents);There was a problem hiding this comment.
thanks, that's a good point. However I'm going to save it is a non static class member as that helper is get created different for each tab.
There was a problem hiding this comment.
actually, I see that upstream class has a set method for it https://github.com/chromium/chromium/blob/main/components/browser_ui/media/android/java/src/org/chromium/components/browser_ui/media/MediaSessionHelper.java#L288 and I don't think we are 100% confident that it's not updated somewhere in the middle of our reflection calls to it. I would pick a safe approach and fetch it, we have 2 calls there ideally on a media page load, shouldn't slow down at all. It wouldn't be good if it calls it tens-hundreds times, but 2 calls per media page I think is ok.
| } catch (RuntimeException e) { | ||
| } |
There was a problem hiding this comment.
nit: when possible I usually prefer to log exceptions. When not possible it's helpful to add a comment that explains why
| } catch (RuntimeException e) { | |
| } | |
| } catch (RuntimeException e) { | |
| Log.e(TAG, "Can't start Brave service", e); | |
| } |
android/java/org/chromium/chrome/browser/media/ui/BraveMediaNotificationControllerDelegate.java
Show resolved
Hide resolved
SergeyZhukovsky
force-pushed
the
android_brave_talk_mic
branch
from
44ce554 to
1c166f1
Compare
|
Released in v1.75.104 |
Resolves brave/brave-browser#42897
Security review https://github.com/brave/reviews/issues/1821
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
It is described in the issue.