Put the BrowserConnectionStatus into redux state #3781

mstange · 2021-12-31T11:30:34Z

This makes it easily accessible anywhere we need it, and is an acceptable solution to the conundrum I encountered in #3656.

codecov · 2021-12-31T11:36:32Z

Codecov Report

Merging #3781 (cfaaf64) into main (ccabe8e) will increase coverage by 0.03%.
The diff coverage is 78.12%.

❗ Current head cfaaf64 differs from pull request most recent head 10a80c9. Consider uploading reports for the commit 10a80c9 to get more accurate results

@@            Coverage Diff             @@
##             main    #3781      +/-   ##
==========================================
+ Coverage   87.85%   87.88%   +0.03%     
==========================================
  Files         269      269              
  Lines       22364    22434      +70     
  Branches     5737     5745       +8     
==========================================
+ Hits        19647    19716      +69     
- Misses       2506     2507       +1     
  Partials      211      211

Impacted Files	Coverage Δ
src/app-logic/browser-connection.js	`86.66% <ø> (ø)`
src/components/app/ProfileLoader.js	`73.68% <ø> (+4.45%)`	⬆️
src/actions/receive-profile.js	`86.28% <33.33%> (-0.21%)`	⬇️
src/components/app/Home.js	`71.11% <50.00%> (-0.66%)`	⬇️
src/actions/app.js	`88.54% <100.00%> (+0.24%)`	⬆️
src/components/app/DragAndDrop.js	`98.46% <100.00%> (ø)`
src/components/app/UrlManager.js	`84.05% <100.00%> (+0.47%)`	⬆️
src/reducers/app.js	`99.37% <100.00%> (+0.03%)`	⬆️
src/selectors/app.js	`57.04% <100.00%> (+1.18%)`	⬆️
src/selectors/cpu.js	`100.00% <0.00%> (ø)`
... and 5 more

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 ccabe8e...10a80c9. Read the comment docs.

julienw

Thanks for the change!

If I'd done this, I'd have implemented a singleton in browser-connection (which is a global effectively) for more simplicity. Indeed this will never change after the app is loaded.
But I agree that this might be more painful in tests. So I don't mind using redux for this.

julienw · 2022-01-03T11:34:22Z

src/components/app/DragAndDrop.js

-  _handleProfileDrop = (event: DragEvent) => {
+  _handleProfileDrop = async (event: DragEvent) => {


This seems to be a leftover? Or did you want to await retrieveProfileFromFile too?

Oh yes, leftover indeed.

julienw · 2022-01-03T12:45:07Z

src/test/store/receive-profile.test.js

+      const browserConnection =
+        browserConnectionStatus.status === 'ESTABLISHED'
+          ? browserConnectionStatus.browserConnection
+          : null;


It's not clear to me what is expected here. Because we're in a test it would be good to better assert what result we have in browserConnection after the call to createBrowserConnection.

Ok, I converted this to an if + throw. An expect(...).toBe(...) is not enough to convince flow.

Oh, nevermind, this test always ended up with browserConnectionStatus.status === 'DENIED', because it mocks a WebChannel which always says "No such channel".
So I was able to simplify this whole setup: We no longer need to mock a WebChannel, we can just pass a null browserConnection. And we don't need to call createBrowserConnection.

So this review comment was definitely warranted. I was confused about what the test was expecting!

julienw · 2022-01-03T13:22:47Z

src/components/app/UrlManager.js

@@ -101,7 +107,9 @@ class UrlManagerImpl extends React.PureComponent<Props> {
    let browserConnectionStatus;
    const route = window.location.pathname.split('/').filter((s) => s)[0];
    if (['from-browser', 'from-addon', 'from-file'].includes(route)) {


Looking at this now, I wonder why it's not happening for from-url or even compare? What happens when the datasource changes (eg moving from 'none' to 'from-file')?

update: I see that when loading from a file, we create a browser connection at that moment. Note that there shouldn't be a from-file here because we can't load "from-file" at load time.

I think I'd create the connection for all datasources always but I'm fine if it's in a follow-up.

Yes, that's definitely planned for a follow-up. The current behavior is not what we want, but it's equivalent to what we had before.

julienw · 2022-01-03T13:28:42Z

src/components/app/UrlManager.js

@@ -101,7 +107,9 @@ class UrlManagerImpl extends React.PureComponent<Props> {
    let browserConnectionStatus;
    const route = window.location.pathname.split('/').filter((s) => s)[0];
    if (['from-browser', 'from-addon', 'from-file'].includes(route)) {
+      updateBrowserConnectionStatus({ status: 'WAITING' });
      browserConnectionStatus = await createBrowserConnection('Firefox/123.0');


I don't remember why we override the user agent here? Is that only so that this passes in tests?

Tests, and because doing so didn't change behavior. This definitely needs to be addressed at some point, for example to fix #3638.

julienw · 2022-01-03T13:39:20Z

src/components/app/Home.js

-  >,
-  +triggerLoadingFromUrl: typeof triggerLoadingFromUrl,
+  +onLoadProfileFromFileRequested: (file: File) => void,
+  +onLoadProfileFromUrlRequested: (url: string) => void,
 |};

 type ActionButtonsState = {
  isLoadFromUrlPressed: boolean,
 };

 type LoadFromUrlProps = {


nit: you can make this an exact object now

julienw · 2022-01-03T14:47:01Z

src/components/app/Home.js

+    createBrowserConnection('Firefox/123.0').then((browserConnectionStatus) => {
+      const browserConnection =
+        browserConnectionStatus.status === 'ESTABLISHED'
+          ? browserConnectionStatus.browserConnection
+          : undefined;


If we create a browser connection for all data sources like suggested, then I believe we can retrieve it here instead of creating it.

Yes, that's the plan. Just trying to keep the set of behavior changes minimal here.

src/components/app/Home.js

mstange · 2022-01-05T11:59:53Z

If I'd done this, I'd have implemented a singleton in browser-connection (which is a global effectively) for more simplicity. Indeed this will never change after the app is loaded. But I agree that this might be more painful in tests.

Exactly, and painful tests mean painful code changes. I've struggled enough with existing tests recently that I'm now strongly leaning towards avoiding globals wherever possible.

…le and into the callers. This introduces some code duplication, while making no changes to behavior. A future commit in this series removes the code duplication again.

…ction call further outside.

- Recommend gecko_profile_generator.py for Android (PR #3763 - Replace fetchSource thunk action with a <SourceFetcher>. (PR #3765) - ⬆️ Update @fluent/bundle to version 0.17.1 (PR #3774) - ⬆️ Update @fluent/langneg to version 0.6.1 (PR #3775) - ⬆️ Update @fluent/react to version 0.14.1 (PR #3776) - Update all development Yarn dependencies (2022-01-03) (PR #3773) - Remove the prop-types dependency (PR #3784) - Check all threads for threadCPUDelta. (PR #3785) - Limit default visible threads to 15. (PR #3743) - Put the BrowserConnectionStatus into redux state (PR #3781) - Add tests for source fetching. (PR #3780) - Add support for fetching source code from local symbol servers (PR #3777) - Add support for fetching source code from crates.io. (PR #3778) - Add support for fetching source code from the browser, for local Firefox builds (PR #3786) - When the user sanitizes a fake global track, we need to sanitize its real children tracks (PR #3788) - Fix a few styles of the timeline header (PR #3792)

mstange requested a review from julienw December 31, 2021 11:30

mstange self-assigned this Dec 31, 2021

mstange mentioned this pull request Jan 3, 2022

Add support for fetching source code from the browser, for local Firefox builds #3786

Merged

julienw approved these changes Jan 3, 2022

View reviewed changes

mstange added 4 commits January 5, 2022 14:12

Move the call to createBrowserConnection out of retrieveProfileFromFi…

ce8d0db

…le and into the callers. This introduces some code duplication, while making no changes to behavior. A future commit in this series removes the code duplication again.

Rename some callback props in Home.js and move the createBrowserConne…

4a40d3b

…ction call further outside.

Store BrowserConnectionStatus in the redux state.

ae39514

Pass null instead of undefined in various places.

10a80c9

mstange force-pushed the browserconnection-in-redux-state branch from cfaaf64 to 10a80c9 Compare January 5, 2022 13:15

mstange enabled auto-merge January 5, 2022 13:16

mstange merged commit 4d921af into firefox-devtools:main Jan 5, 2022

mstange mentioned this pull request Jan 6, 2022

Deploy January 6, 2022 #3796

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put the BrowserConnectionStatus into redux state #3781

Put the BrowserConnectionStatus into redux state #3781

mstange commented Dec 31, 2021

codecov bot commented Dec 31, 2021 •

edited

Loading

julienw left a comment •

edited

Loading

julienw Jan 3, 2022

mstange Jan 5, 2022

julienw Jan 3, 2022

mstange Jan 5, 2022

mstange Jan 5, 2022

mstange Jan 5, 2022

julienw Jan 3, 2022

mstange Jan 5, 2022

julienw Jan 3, 2022

mstange Jan 5, 2022

julienw Jan 3, 2022

mstange Jan 5, 2022

julienw Jan 3, 2022

mstange Jan 5, 2022

mstange commented Jan 5, 2022

		_handleProfileDrop = (event: DragEvent) => {
		_handleProfileDrop = async (event: DragEvent) => {

Put the BrowserConnectionStatus into redux state #3781

Put the BrowserConnectionStatus into redux state #3781

Conversation

mstange commented Dec 31, 2021

codecov bot commented Dec 31, 2021 • edited Loading

Codecov Report

julienw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstange commented Jan 5, 2022

codecov bot commented Dec 31, 2021 •

edited

Loading

julienw left a comment •

edited

Loading