Bug 1615436 - Allow capturing profiles even when profiling was started by another tool. r=gregtatum
authorMarkus Stange <mstange@themasta.com>
Tue, 18 Feb 2020 16:23:50 +0000
changeset 514455 df596657bebcb96b917d75ff452316bbe8140a1a
parent 514454 8dbb70090704721d93881f5a4d419a3722abac8f
child 514456 a169b2355d8d1e37b5bd76404155ddf72bc0b620
push id107600
push usermstange@themasta.com
push dateTue, 18 Feb 2020 18:02:52 +0000
treeherderautoland@df596657bebc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgregtatum
bugs1615436
milestone75.0a1
first release with
nightly linux32
df596657bebc / 75.0a1 / 20200218213359 / files
nightly linux64
df596657bebc / 75.0a1 / 20200218213359 / files
nightly mac
df596657bebc / 75.0a1 / 20200218213359 / files
nightly win32
df596657bebc / 75.0a1 / 20200218213359 / files
nightly win64
df596657bebc / 75.0a1 / 20200218213359 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1615436 - Allow capturing profiles even when profiling was started by another tool. r=gregtatum Differential Revision: https://phabricator.services.mozilla.com/D62825
devtools/client/performance-new/@types/perf.d.ts
devtools/client/performance-new/components/ProfilerEventHandling.js
devtools/client/performance-new/components/RecordingButton.js
devtools/client/performance-new/test/chrome/test_perf-state-02.html
--- a/devtools/client/performance-new/@types/perf.d.ts
+++ b/devtools/client/performance-new/@types/perf.d.ts
@@ -84,20 +84,18 @@ export type RecordingState =
   | "available-to-record"
   // An async request has been sent to start the profiler.
   | "request-to-start-recording"
   // An async request has been sent to get the profile and stop the profiler.
   | "request-to-get-profile-and-stop-profiler"
   // An async request has been sent to stop the profiler.
   | "request-to-stop-profiler"
   // The profiler notified us that our request to start it actually started
-  // it.
+  // it, or it was already started.
   | "recording"
-  // Some other code with access to the profiler started it.
-  | "other-is-recording"
   // Profiling is not available when in private browsing mode.
   | "locked-by-private-browsing";
 
 // We are currently migrating to a new UX workflow with about:profiling.
 // This type provides an easy way to change the implementation based
 // on context.
 export type PageContext = "popup" | "devtools" | "aboutprofiling";
 
--- a/devtools/client/performance-new/components/ProfilerEventHandling.js
+++ b/devtools/client/performance-new/components/ProfilerEventHandling.js
@@ -37,17 +37,16 @@
  */
 
 "use strict";
 
 const { PureComponent } = require("devtools/client/shared/vendor/react");
 const { connect } = require("devtools/client/shared/vendor/react-redux");
 const actions = require("devtools/client/performance-new/store/actions");
 const selectors = require("devtools/client/performance-new/store/selectors");
-const { UnhandledCaseError } = require("devtools/client/performance-new/utils");
 
 /**
  * This component state changes for the performance recording. e.g. If the profiler
  * suddenly becomes unavailable, it needs to react to those changes, and update the
  * recordingState in the store.
  *
  * @extends {React.PureComponent<Props>}
  */
@@ -61,17 +60,17 @@ class ProfilerEventHandling extends Pure
       this
     );
     this.handlePrivateBrowsingEnding = this.handlePrivateBrowsingEnding.bind(
       this
     );
   }
 
   componentDidMount() {
-    const { perfFront, reportProfilerReady, pageContext } = this.props;
+    const { perfFront, reportProfilerReady } = this.props;
 
     // Ask for the initial state of the profiler.
     Promise.all([
       perfFront.isActive(),
       perfFront.isSupportedPlatform(),
       perfFront.isLockedForPrivateBrowsing(),
     ]).then(results => {
       const [
@@ -82,32 +81,17 @@ class ProfilerEventHandling extends Pure
 
       let recordingState = this.props.recordingState;
       // It's theoretically possible we got an event that already let us know about
       // the current state of the profiler.
       if (recordingState === "not-yet-known" && isSupportedPlatform) {
         if (isLockedForPrivateBrowsing) {
           recordingState = "locked-by-private-browsing";
         } else if (isActive) {
-          switch (pageContext) {
-            case "popup":
-            case "aboutprofiling":
-              // These page contexts are in global control of the profiler, so allow it
-              // to take control of recording.
-              recordingState = "recording";
-              break;
-            case "devtools":
-              // The DevTools performance recording shouldn't take control of others
-              // use of the Gecko Profiler, since it's more interested in the current
-              // page.
-              recordingState = "other-is-recording";
-              break;
-            default:
-              throw new UnhandledCaseError(pageContext, "PageContext");
-          }
+          recordingState = "recording";
         } else {
           recordingState = "available-to-record";
         }
       }
       reportProfilerReady(isSupportedPlatform, recordingState);
 
       // If this component is inside the popup, then report it being ready so that
       // it will show. This defers the initial visibility of the popup until the
@@ -137,67 +121,50 @@ class ProfilerEventHandling extends Pure
 
   componentWillUnmount() {
     switch (this.props.recordingState) {
       case "not-yet-known":
       case "available-to-record":
       case "request-to-stop-profiler":
       case "request-to-get-profile-and-stop-profiler":
       case "locked-by-private-browsing":
-      case "other-is-recording":
         // Do nothing for these states.
         break;
 
       case "recording":
       case "request-to-start-recording":
         this.props.perfFront.stopProfilerAndDiscardProfile();
         break;
 
       default:
         throw new Error("Unhandled recording state.");
     }
   }
 
   handleProfilerStarting() {
-    const { changeRecordingState, recordingState, pageContext } = this.props;
+    const { changeRecordingState, recordingState } = this.props;
     switch (recordingState) {
       case "not-yet-known":
       // We couldn't have started it yet, so it must have been someone
       // else. (fallthrough)
       case "available-to-record":
       // We aren't recording, someone else started it up. (fallthrough)
       case "request-to-stop-profiler":
       // We requested to stop the profiler, but someone else already started
       // it up. (fallthrough)
       case "request-to-get-profile-and-stop-profiler":
-        switch (pageContext) {
-          case "popup":
-          case "aboutprofiling":
-            // These page contexts are in global control of the profiler, so allow it
-            // to take control of recording.
-            changeRecordingState("recording");
-            break;
-          case "devtools":
-            // The DevTools performance recording shouldn't take control of others
-            // use of the Gecko Profiler, since it's more interested in the current
-            // page.
-            changeRecordingState("other-is-recording");
-            break;
-          default:
-            throw new UnhandledCaseError(pageContext, "PageContext");
-        }
+        changeRecordingState("recording");
         break;
 
       case "request-to-start-recording":
         // Wait for the profiler to tell us that it has started.
         changeRecordingState("recording");
         break;
 
       case "locked-by-private-browsing":
-      case "other-is-recording":
       case "recording":
         // These state cases don't make sense to happen, and means we have a logical
         // fallacy somewhere.
         throw new Error(
           "The profiler started recording, when it shouldn't have " +
             `been able to. Current state: "${recordingState}"`
         );
       default:
@@ -206,17 +173,16 @@ class ProfilerEventHandling extends Pure
   }
 
   handleProfilerStopping() {
     const { changeRecordingState, recordingState } = this.props;
     switch (recordingState) {
       case "not-yet-known":
       case "request-to-get-profile-and-stop-profiler":
       case "request-to-stop-profiler":
-      case "other-is-recording":
         changeRecordingState("available-to-record");
         break;
 
       case "request-to-start-recording":
       // Highly unlikely, but someone stopped the recorder, this is fine.
       // Do nothing (fallthrough).
       case "locked-by-private-browsing":
         // The profiler is already locked, so we know about this already.
@@ -241,17 +207,16 @@ class ProfilerEventHandling extends Pure
     const { recordingState, changeRecordingState } = this.props;
 
     switch (recordingState) {
       case "request-to-get-profile-and-stop-profiler":
       // This one is a tricky case. Go ahead and act like nothing went wrong, maybe
       // it will resolve correctly? (fallthrough)
       case "request-to-stop-profiler":
       case "available-to-record":
-      case "other-is-recording":
       case "not-yet-known":
         changeRecordingState("locked-by-private-browsing");
         break;
 
       case "request-to-start-recording":
       case "recording":
         changeRecordingState("locked-by-private-browsing", {
           didRecordingUnexpectedlyStopped: false,
--- a/devtools/client/performance-new/components/RecordingButton.js
+++ b/devtools/client/performance-new/components/RecordingButton.js
@@ -180,23 +180,16 @@ class RecordingButton extends PureCompon
           onClick: this._getProfileAndStopProfiler,
           disabled: recordingState === "request-to-start-recording",
           additionalButton: {
             label: "Cancel recording",
             onClick: stopProfilerAndDiscardProfile,
           },
         });
 
-      case "other-is-recording":
-        return this.renderButton({
-          label: "Stop and discard the other recording",
-          onClick: stopProfilerAndDiscardProfile,
-          additionalMessage: "Another tool is currently recording.",
-        });
-
       case "locked-by-private-browsing":
         return this.renderButton({
           label: span(
             null,
             img({
               className: "perf-button-image",
               src: "chrome://devtools/skin/images/tool-profiler.svg",
             }),
--- a/devtools/client/performance-new/test/chrome/test_perf-state-02.html
+++ b/devtools/client/performance-new/test/chrome/test_perf-state-02.html
@@ -35,22 +35,23 @@
         await perfFrontMock._flushAsyncQueue();
 
         mountComponent();
 
         is(getRecordingState(), "not-yet-known",
           "The component at first is in an unknown state.");
 
         await perfFrontMock._flushAsyncQueue();
-        is(getRecordingState(), "other-is-recording",
-          "The profiler is not available to record.");
+        is(getRecordingState(), "recording",
+          "The profiler is reflecting the recording state.");
 
-        const button = container.querySelector("button");
-        ok(button, "Selected a button on the component");
-        button.click();
+        const [captureButton, stopButton] = container.querySelectorAll("button");
+        ok(captureButton, "Selected capture button on the component");
+        ok(stopButton, "Selected stop button on the component");
+        stopButton.click();
         is(getRecordingState(), "request-to-stop-profiler",
           "We can request to stop the profiler.");
 
         await perfFrontMock._flushAsyncQueue();
         is(getRecordingState(), "available-to-record",
           "The profiler is now available to record.");
       });
     </script>