-
Notifications
You must be signed in to change notification settings - Fork 711
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small nits but this LGTM.
If you have time to add some tests for the new code would be great!
r+ for the code.
app/src/main/java/org/mozilla/gecko/search/SearchWidgetProvider.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/mozilla/gecko/search/SearchWidgetProvider.kt
Outdated
Show resolved
Hide resolved
ff375ed
to
1a41f99
Compare
Request for data collection review formAll questions are mandatory. You must receive review from a data steward peer on your responses to these questions before shipping new data collection.
|
Please make sure you answer question 5 for the data review request. Thanks |
Done @rocketsroger |
Data Review
Yes, through the metrics.yaml file and the Glean Dictionary
Yes, through the "Send Usage Data" preference in the application settings
N/A, collection set to end or be renewed by version 119
Category 2, Interaction data
default-on
No
Yes
No Resultdata-review+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks and functions nicely 👍 ✨
app/src/main/AndroidManifest.xml
Outdated
<activity | ||
android:name=".searchwidget.VoiceSearchActivity" | ||
android:theme="@style/Theme.AppCompat.Translucent" /> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous version of the patch contained an important bug fix related to the search activity which was implemented in https://github.com/mozilla-mobile/fenix/pull/24296/files. Do we need that back?
<activity | |
android:name=".searchwidget.VoiceSearchActivity" | |
android:theme="@style/Theme.AppCompat.Translucent" /> | |
<activity | |
android:name=".searchwidget.VoiceSearchActivity" | |
android:excludeFromRecents="true" | |
android:taskAffinity="" | |
android:theme="@style/Theme.AppCompat.Translucent" /> | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
startActivity(intent) | ||
} | ||
|
||
companion object { | ||
const val SEARCH_WIDGET = "search_widget" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It's not clear what this property represents and when to be used. Can you update the naming and/or add documentation?
I see other such properties in the app using the EXTRA
prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if (intent.getBooleanExtra(SEARCH_WIDGET, false)) { | ||
SearchWidget.newTabButton.record(NoExtras()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be moved to MainActivity
?
Just to have a better separation of responsabilities. This would be the first time IntentReceiverActivity
records telemetry while MainActivity
already does this, even for AppOpened
events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
11f0a0c
to
4e1a431
Compare
Pull Request checklist
QA
To download an APK when reviewing a PR:
Show All Checks
,Details
next tobuild-focus-debug
orbuild-klar-debug
for changes targeting Klar,View task in Taskcluster
,Artifacts
row,GitHub Automation
Fixes #7249