Skip to content

Added doc for local profiling on android with screenshots#4145

Merged
julienw merged 4 commits into
firefox-devtools:mainfrom
Malclir:doc_local_profiling_android
Jul 19, 2022
Merged

Added doc for local profiling on android with screenshots#4145
julienw merged 4 commits into
firefox-devtools:mainfrom
Malclir:doc_local_profiling_android

Conversation

@Malclir

@Malclir Malclir commented Jul 18, 2022

Copy link
Copy Markdown
Contributor

With the addition of the Firefox profiler on android, we wanted to add some doc to help users navigate it. I was going to add it to the performance wiki but this felt more centralized.

I wasn't sure where to add the doc so I created a new file, but I was also thinking it could live within the remote profiling doc and just rename it. I didn't want to make any decision regarding that so I decided to open a new PR and see what others have to say!

@julienw julienw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good to me!

Some comments:

  1. Can you make sure to run the png files through optipng or a similar tool to reduce the size of the png? On my computer I can reduce each file size by more than 30% using optipng.
  2. About where to put it, I feel like it's good in its own file, however we could have cross links (in both directions) between this file and the remote profiling page.
    Maybe you could add an Firefox for Android section in the sidebar with both pages Local Profiling and Remote Profiling.

I'll be waiting for these changes before approving :)

- Choose one of the four options that matches the closest to what you're trying to profile.
- Click "Start Profiler" and a toast should appear with the "Profiler started" message.

### To stop the prfoiler

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not:

Suggested change
### To stop the prfoiler
### To stop the profiler

@@ -0,0 +1,34 @@
# Local Profiling Firefox for Android

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When reading Local Profiling I initially thought about profiling with an emulator on a desktop machine.
I wonder if we can find a a better title... What about Profiling Firefox for Android directly on the device? Is it too long?

- Go back to the Settings screen
- Scroll to the bottom and you should see a "Stop profiler" option has replaced the "Start Profiler" one.
- After you click it, you should see a dialogue with a warning regarding the information contained in the profile.
- Once stopped, the URL for the profile that has just been taken will be copied to your clipboard which you can then use to share.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our test checker alex errors on the word just here, even though that's an appropriate use here. I believe that you can add <!--alex ignore just--> before the list to ignore it.
You can run yarn test-alex to run the tool locally.

@Malclir Malclir force-pushed the doc_local_profiling_android branch from a92b265 to eea6395 Compare July 19, 2022 06:24
@Malclir

Malclir commented Jul 19, 2022

Copy link
Copy Markdown
Contributor Author

@julienw I ran it through optipng, not sure if what I did was right though. I simply did optipn [insert file name here] and pushed that.

As for the side bar with two sub sections, I like that idea. Again, not sure how the markdown functions for that one so I based myself on what was there.

@codecov

codecov Bot commented Jul 19, 2022

Copy link
Copy Markdown

Codecov Report

Merging #4145 (aedc1ac) into main (0161eac) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head aedc1ac differs from pull request most recent head 94ee177. Consider uploading reports for the commit 94ee177 to get more accurate results

@@            Coverage Diff             @@
##             main    #4145      +/-   ##
==========================================
- Coverage   88.28%   88.27%   -0.01%     
==========================================
  Files         280      280              
  Lines       24414    24388      -26     
  Branches     6490     6477      -13     
==========================================
- Hits        21554    21529      -25     
+ Misses       2656     2655       -1     
  Partials      204      204              
Impacted Files Coverage Δ
src/actions/profile-view.js 83.30% <0.00%> (ø)
src/app-logic/tabs-handling.js 100.00% <0.00%> (ø)
src/selectors/per-thread/composed.js 100.00% <0.00%> (ø)
src/components/shared/SampleTooltipContents.js 95.00% <0.00%> (+1.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0161eac...94ee177. Read the comment docs.

@julienw

julienw commented Jul 19, 2022

Copy link
Copy Markdown
Contributor

Hey @MarcLeclair, thanks for latest changes, this looks good to me!
I added a small patch to do some small changes, can you please have a quick look and tell me what you think?
=> aedc1ac

@Malclir

Malclir commented Jul 19, 2022

Copy link
Copy Markdown
Contributor Author

That looks good to me @julienw thanks for the edit! I wasn't too sure how to make a sub header.

@julienw

julienw commented Jul 19, 2022

Copy link
Copy Markdown
Contributor

That looks good to me @julienw thanks for the edit! I wasn't too sure how to make a sub header.

TBH I was thinking about a top level header, but what you did looks good to me too, so I kept it :-)
Thanks for the second look, I'll merge it then!

@julienw julienw enabled auto-merge July 19, 2022 15:02
@julienw julienw merged commit f03bf5d into firefox-devtools:main Jul 19, 2022
@canova canova mentioned this pull request Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants