From 4fe2ec1188cfcc0523d0e9475396068061c0fc13 Mon Sep 17 00:00:00 2001
From: Marc Leclair <mleclair@mozilla.com>
Date: Tue, 24 May 2022 18:28:25 -0400
Subject: [PATCH] [fenix] Fixed PR review

---
 .../fenix/perf/ProfilerStartDialogFragment.kt |  4 ++-
 .../fenix/perf/ProfilerStopDialogFragment.kt  | 11 +++-----
 .../org/mozilla/fenix/perf/ProfilerUtils.kt   | 16 +++++++----
 .../mozilla/fenix/perf/ProfilerViewModel.kt   | 16 +++++++++--
 .../fenix/settings/SettingsFragment.kt        |  2 +-
 app/src/main/res/values/static_strings.xml    | 28 +++++++------------
 .../fenix/settings/SettingsFragmentTest.kt    |  3 +-
 7 files changed, 44 insertions(+), 36 deletions(-)

diff --git a/app/src/main/java/org/mozilla/fenix/perf/ProfilerStartDialogFragment.kt b/app/src/main/java/org/mozilla/fenix/perf/ProfilerStartDialogFragment.kt
index 248bc6d5f..fdcbd7c33 100644
--- a/app/src/main/java/org/mozilla/fenix/perf/ProfilerStartDialogFragment.kt
+++ b/app/src/main/java/org/mozilla/fenix/perf/ProfilerStartDialogFragment.kt
@@ -105,7 +105,9 @@ class ProfilerStartDialogFragment : AppCompatDialogFragment() {
         viewStateObserver: MutableState<CardState>,
         startProfiler: (Array<String>, Array<String>) -> Unit
     ) {
-        val featureAndThreadsObserver = remember { mutableStateOf("") }
+        val featureAndThreadsObserver = remember {
+            mutableStateOf(requireContext().resources.getString(R.string.profiler_filter_firefox))
+        }
         ProfilerDialogueCard {
             Column(modifier = Modifier.padding(8.dp)) {
                 Text(
diff --git a/app/src/main/java/org/mozilla/fenix/perf/ProfilerStopDialogFragment.kt b/app/src/main/java/org/mozilla/fenix/perf/ProfilerStopDialogFragment.kt
index cc2dda57f..fc37a84c5 100644
--- a/app/src/main/java/org/mozilla/fenix/perf/ProfilerStopDialogFragment.kt
+++ b/app/src/main/java/org/mozilla/fenix/perf/ProfilerStopDialogFragment.kt
@@ -71,10 +71,6 @@ class ProfilerStopDialogFragment : DialogFragment() {
                 // since the user needs to wait for the profiler data to be ready and we don't want to handle
                 // the process in the background.
                 if (viewStateObserver.value != CardState.WaitForProfilerState) {
-                    profilerViewModel.setProfilerState(
-                        requireContext()
-                            .components.core.engine.profiler!!.isProfilerActive()
-                    )
                     this@ProfilerStopDialogFragment.dismiss()
                 }
             }
@@ -116,7 +112,8 @@ class ProfilerStopDialogFragment : DialogFragment() {
                 ) {
                     TextButton(
                         onClick = {
-                            displayToastAndDismiss(R.string.profile_stop_dialogue_cancel_save)
+                            requireContext().components.core.engine.profiler?.stopProfiler({}, {})
+                            dismiss()
                         }
                     ) {
                         Text(stringResource(R.string.profiler_start_cancel))
@@ -151,9 +148,9 @@ class ProfilerStopDialogFragment : DialogFragment() {
             },
             onError = {
                 error ->
-                error.message?.let {
+                if (error.message != null) {
                     displayToastAndDismiss(R.string.profiler_error, " error: $error")
-                }?.let {
+                } else {
                     displayToastAndDismiss(R.string.profiler_error)
                 }
             }
diff --git a/app/src/main/java/org/mozilla/fenix/perf/ProfilerUtils.kt b/app/src/main/java/org/mozilla/fenix/perf/ProfilerUtils.kt
index faa3e1706..c81e9b514 100644
--- a/app/src/main/java/org/mozilla/fenix/perf/ProfilerUtils.kt
+++ b/app/src/main/java/org/mozilla/fenix/perf/ProfilerUtils.kt
@@ -74,14 +74,18 @@ enum class ProfilerSettings(val threads: Array<String>, val features: Array<Stri
  */
 object ProfilerUtils {
 
-    private fun saveProfileUrlToClipboardAndToast(profileResult: ByteArray, context: Context): String {
+    private fun saveProfileUrlToClipboard(profileResult: ByteArray, context: Context): String {
         // The profile is saved to a temporary file since our fetch API takes a file or a string.
         // Converting the ByteArray to a String would hurt the encoding, which we need to preserve.
+        // The file may potentially contain sensitive data and should be handled carefully.
         val outputFile = createTemporaryFile(profileResult, context)
-        val response = networkCallToProfilerServer(outputFile, context)
-        val profileToken = decodeProfileToken(response)
-        outputFile.delete()
-        return PROFILER_DATA_URL + profileToken
+        try {
+            val response = networkCallToProfilerServer(outputFile, context)
+            val profileToken = decodeProfileToken(response)
+            return PROFILER_DATA_URL + profileToken
+        } finally {
+            outputFile.delete()
+        }
     }
 
     private fun finishProfileSave(context: Context, url: String, onUrlFinish: (Int) -> Unit) {
@@ -138,7 +142,7 @@ object ProfilerUtils {
         onUrlFinish: (Int) -> Unit
     ) {
         try {
-            val url = saveProfileUrlToClipboardAndToast(profile, context)
+            val url = saveProfileUrlToClipboard(profile, context)
             finishProfileSave(context, url, onUrlFinish)
         } catch (e: IOException) {
             onUrlFinish(R.string.profiler_io_error)
diff --git a/app/src/main/java/org/mozilla/fenix/perf/ProfilerViewModel.kt b/app/src/main/java/org/mozilla/fenix/perf/ProfilerViewModel.kt
index 87f3b0598..6a8910c60 100644
--- a/app/src/main/java/org/mozilla/fenix/perf/ProfilerViewModel.kt
+++ b/app/src/main/java/org/mozilla/fenix/perf/ProfilerViewModel.kt
@@ -4,20 +4,32 @@
 
 package org.mozilla.fenix.perf
 
+import android.app.Application
+import androidx.lifecycle.AndroidViewModel
 import androidx.lifecycle.LiveData
 import androidx.lifecycle.MutableLiveData
 import androidx.lifecycle.ViewModel
+import org.mozilla.fenix.FenixApplication
+import org.mozilla.fenix.ext.components
 
 /**
  * [ViewModel] to keep track of the profiler state
  */
-class ProfilerViewModel : ViewModel() {
-    var isProfilerActive: MutableLiveData<Boolean> = MutableLiveData()
+class ProfilerViewModel(application: Application) : AndroidViewModel(application) {
+    val isProfilerActive: MutableLiveData<Boolean> = MutableLiveData(
+        application.components.core.engine.profiler?.isProfilerActive() ?: false
+    )
 
     /**
      * @return profiler status
      */
     fun getProfilerState(): LiveData<Boolean> {
+        // We check here since this can be polled from anywhere in Fenix.
+        getApplication<FenixApplication>().components.core.engine.profiler?.let {
+            check(it.isProfilerActive() == isProfilerActive.value) {
+                "The Profiler state from Gecko is out of sync with the LiveData profiler state."
+            }
+        }
         return isProfilerActive
     }
 
diff --git a/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt b/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt
index 23a165459..45a68c324 100644
--- a/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt
+++ b/app/src/main/java/org/mozilla/fenix/settings/SettingsFragment.kt
@@ -419,7 +419,7 @@ class SettingsFragment : PreferenceFragmentCompat() {
                 null
             }
             resources.getString(R.string.pref_key_start_profiler) -> {
-                if (requireContext().components.core.engine.profiler?.isProfilerActive()!!) {
+                if (profilerViewModel.getProfilerState().value == true) {
                     SettingsFragmentDirections.actionSettingsFragmentToStopProfilerDialog()
                 } else {
                     SettingsFragmentDirections.actionSettingsFragmentToStartProfilerDialog()
diff --git a/app/src/main/res/values/static_strings.xml b/app/src/main/res/values/static_strings.xml
index 715d9214e..a3fda09ce 100644
--- a/app/src/main/res/values/static_strings.xml
+++ b/app/src/main/res/values/static_strings.xml
@@ -57,6 +57,7 @@
     <!-- Profiler settings -->
     <string name="preferences_start_profiler">Start Profiler</string>
     <string name="profiler_stop">Stop Profiler</string>
+
     <string name="profiler_settings_title">Profiler Settings</string>
     <string name="profiler_filter_firefox">Firefox</string>
     <string name="profiler_running">Profiler is currently running</string>
@@ -67,30 +68,21 @@
     <string name="profiler_filter_media_explain">Preset for investigating audio and video bugs in Firefox</string>
     <string name="profiler_filter_networking">Networking</string>
     <string name="profiler_filter_networking_explain">Preset for investigating networking bugs in Firefox</string>
+
     <string name="profiler_start_dialog_started">Profiler started</string>
-    <string name="profiler_save_method">Would you like to save your profile as:</string>
-    <string name="profiler_save_method_url">URL</string>
-    <string name="profiler_save_method_url_explained">The profile will upload automatically with all the information gathered and provide you a link from profiler.firefox.com</string>
-    <string name="profiler_save_method_file">File</string>
-    <string name="profiler_save_method_file_explained">The file will be saved locally on your android device. To share it, go to profiler.firefox.com on your computer or mobile device</string>
+
     <string name="profiler_start_cancel">Cancel</string>
+
     <string name="profiler_gathering">Gathering the profile</string>
-    <string name="profiler_waiting_start">Waiting for Profiler to start</string>
-    <string name="profiler_url_warning">Saving to URL warning</string>
-    <string name="profiler_url_warning_explained">"Saving to URL could potentially show private information such as " +
-                        "urls and screenshots. If you want to keep these" +
-                        "privates, save your profile as a file instead and head on over to profiler.firefox.com" +
-                        "where you can select what to share and not share."</string>
-    <string name="profiler_file_save">Save as File</string>
-    <string name="profiler_as_url">Get Profile URL</string>
     <string name="profiler_no_info">No information was gathered</string>
+
+    <string name="profiler_waiting_start">Waiting for Profiler to start</string>
+
+    <string name="profiler_url_warning">⚠️ Warning: Profile Upload</string>
+    <string name="profiler_url_warning_explained">Performance profiles may contain sensitive information such as websites visited and screenshots of them. If you continue, your profile will be uploaded and made accessible to anyone with the link. Do you wish to continue?</string>
+    <string name="profiler_as_url">Upload profile</string>
     <string name="profiler_error">Something went wrong with the profiler</string>
     <string name="profiler_io_error">Something went wrong contacting the Profiler server.</string>
-    <string name="profiler_file_error">Something went wrong writing the profile data to the file</string>
-    <string name="profiler_error_saving">Something went wrong saving the profile</string>
 
-    <string name="profiler_stop_dialogue_file_saved">The profile was saved locally</string>
-    <string name="profiler_stop_dialogue_file_saved_failed">Saving profile to file failed, please try again</string>
     <string name="profiler_uploaded_url_to_clipboard">URL copied to clipboard successfully</string>
-    <string name="profile_stop_dialogue_cancel_save">Profile save operation canceled</string>
 </resources>
diff --git a/app/src/test/java/org/mozilla/fenix/settings/SettingsFragmentTest.kt b/app/src/test/java/org/mozilla/fenix/settings/SettingsFragmentTest.kt
index a66ea9c91..1563c0d8a 100644
--- a/app/src/test/java/org/mozilla/fenix/settings/SettingsFragmentTest.kt
+++ b/app/src/test/java/org/mozilla/fenix/settings/SettingsFragmentTest.kt
@@ -42,15 +42,16 @@ class SettingsFragmentTest {
 
     @Before
     fun setup() {
+
         // Mock client for fetching account avatar
         val client = mockk<Client>()
         every { client.fetch(any()) } throws IOException("test")
 
+        every { testContext.components.core.engine.profiler } returns mockk(relaxed = true)
         every { testContext.components.core.client } returns client
         every { testContext.components.settings } returns mockk(relaxed = true)
         every { testContext.components.analytics } returns mockk(relaxed = true)
         every { testContext.components.backgroundServices } returns mockk(relaxed = true)
-
         mockkObject(Config)
         every { Config.channel } returns ReleaseChannel.Nightly