Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5816 +/- ##
==========================================
- Coverage 85.56% 85.55% -0.02%
==========================================
Files 320 320
Lines 31419 31430 +11
Branches 8649 8651 +2
==========================================
+ Hits 26884 26890 +6
- Misses 4104 4109 +5
Partials 431 431 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
canova
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I see that the first paragraph is saying that the profiling requires Firefox desktop, but in the second paragraph we tell them that they can also use the on device profiling. I think it would confuse our users. The old wording from the issue was actually suggested before we supported on device profiling, so it's not up-to-date anymore. See also my suggestions below.
| Recording performance profiles currently requires{' '} | ||
| <a href="https://www.firefox.com/en-US/browsers/desktop/"> | ||
| Firefox for Desktop | ||
| </a> | ||
| . However, existing profiles can be viewed in any modern desktop |
There was a problem hiding this comment.
Ah, it looks like we have 2 different instructions now that kinda say the opposite things. Since we have a Localized component below that says it's possible to profile on an Android device, "Recording performance profiles currently requires Firefox for Desktop" is no longer true. Can we move the android on device profiling above, and add the remote profiling instruction below?
For the wording of remote profiling, maybe something like:
You can also profile Firefox for Android remotely from a desktop
computer. For more information, please consult this documentation:{' '}
<a>Profiling Firefox for Android remotely</a>.
| <a>Profiling Firefox for Android directly on device</a>. | ||
| </p> | ||
| </Localized> | ||
| </div> |
There was a problem hiding this comment.
This wording is more suitable for the desktop message, since it says "You can also profile Firefox for Android.". Instead, I would prefer to have a new ftl key with a better wording suited specifically for Firefox for Android.
Maybe something like:
Firefox for Android can be profiled directly on this device. For
more information, please consult this documentation:{' '}
<a>Profiling Firefox for Android directly on device</a>.
Closes #2654.
Before
After