Bug 1529427 - Refactor sources reducer so that all source actors are included.
☠☠ backed out by 74006e58ea46 ☠ ☠
authorJason Laster <jlaster@mozilla.com>
Thu, 21 Feb 2019 22:43:03 +0000
changeset 460636 0bf221eea0496d0619684e073605032e30c76191
parent 460635 b0b1558a0b84132aa631e01e4b930138bb220d61
child 460637 1b39e25001ca7a9c20934c5f1fa37b08c5d4ab6a
push id78783
push userjlaster@mozilla.com
push dateFri, 22 Feb 2019 17:32:55 +0000
treeherderautoland@0bf221eea049 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1529427
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1529427 - Refactor sources reducer so that all source actors are included. Differential Revision: https://phabricator.services.mozilla.com/D20592
devtools/client/debugger/new/src/actions/pause/commands.js
devtools/client/debugger/new/src/actions/sources/newSources.js
devtools/client/debugger/new/src/reducers/sources.js
devtools/client/debugger/new/src/utils/source-queue.js
devtools/client/debugger/new/test/mochitest/browser_dbg-windowless-workers.js
devtools/client/debugger/new/test/mochitest/examples/doc-windowless-workers.html
devtools/client/debugger/new/test/mochitest/examples/simple-worker.js
--- a/devtools/client/debugger/new/src/actions/pause/commands.js
+++ b/devtools/client/debugger/new/src/actions/pause/commands.js
@@ -17,22 +17,17 @@ import { addHiddenBreakpoint } from "../
 import { features } from "../../utils/prefs";
 import { recordEvent } from "../../utils/telemetry";
 
 import type { Source } from "../../types";
 import type { ThunkArgs } from "../types";
 import type { Command } from "../../reducers/types";
 
 export function selectThread(thread: string) {
-  return async ({ dispatch, client }: ThunkArgs) => {
-    return dispatch({
-      type: "SELECT_THREAD",
-      thread
-    });
-  };
+  return { type: "SELECT_THREAD", thread }
 }
 
 /**
  * Debugger commands like stepOver, stepIn, stepUp
  *
  * @param string $0.type
  * @memberof actions/pause
  * @static
--- a/devtools/client/debugger/new/src/actions/sources/newSources.js
+++ b/devtools/client/debugger/new/src/actions/sources/newSources.js
@@ -204,26 +204,18 @@ function restoreBlackBoxedSources(source
  * @static
  */
 export function newSource(source: Source) {
   return async ({ dispatch }: ThunkArgs) => {
     await dispatch(newSources([source]));
   };
 }
 
-export function newSources(foundSources: Source[]) {
+export function newSources(sources: Source[]) {
   return async ({ dispatch, getState }: ThunkArgs) => {
-    const sources = foundSources.filter(
-      source => !getSource(getState(), source.id)
-    );
-
-    if (sources.length == 0) {
-      return;
-    }
-
     dispatch({ type: "ADD_SOURCES", sources });
 
     for (const source of sources) {
       dispatch(checkSelectedSource(source.id));
     }
 
     // We would like to restore the blackboxed state
     // after loading all states to make sure the correctness.
--- a/devtools/client/debugger/new/src/reducers/sources.js
+++ b/devtools/client/debugger/new/src/reducers/sources.js
@@ -100,26 +100,26 @@ export function createSource(state: Sour
 function update(
   state: SourcesState = initialSourcesState(),
   action: Action
 ): SourcesState {
   let location = null;
 
   switch (action.type) {
     case "UPDATE_SOURCE":
-      return updateSources(state, [action.source]);
+      return updateSource(state, action.source);
 
     case "ADD_SOURCE":
-      return updateSources(state, [action.source]);
+      return addSources(state, [action.source]);
 
     case "ADD_SOURCES":
-      return updateSources(state, action.sources);
+      return addSources(state, action.sources);
 
     case "SET_WORKERS":
-      return updateWorkers(state, action.workers, action.mainThread);
+      return updateWorkers(state, action);
 
     case "SET_SELECTED_LOCATION":
       location = {
         ...action.location,
         url: action.source.url
       };
 
       if (action.source.url) {
@@ -150,179 +150,181 @@ function update(
         url: action.url,
         line: action.line
       };
 
       prefs.pendingSelectedLocation = location;
       return { ...state, pendingSelectedLocation: location };
 
     case "LOAD_SOURCE_TEXT":
-      return setSourceTextProps(state, action);
+      return updateLoadedState(state, action);
 
     case "BLACKBOX":
       if (action.status === "done") {
         const { id, url } = action.source;
         const { isBlackBoxed } = ((action: any): DonePromiseAction).value;
         updateBlackBoxList(url, isBlackBoxed);
-        return updateSources(state, [{ id, isBlackBoxed }]);
+        return updateSource(state, { id, isBlackBoxed });
       }
       break;
 
     case "SET_PROJECT_DIRECTORY_ROOT":
       return updateProjectDirectoryRoot(state, action.url);
 
     case "NAVIGATE":
       return initialSourcesState();
 
     case "SET_FOCUSED_SOURCE_ITEM":
       return { ...state, focusedItem: action.item };
   }
 
   return state;
 }
 
-function getTextPropsFromAction(action) {
-  const { sourceId } = action;
-
-  if (action.status === "start") {
-    return { id: sourceId, loadedState: "loading" };
-  } else if (action.status === "error") {
-    return { id: sourceId, error: action.error, loadedState: "loaded" };
-  }
-
-  if (!action.value) {
-    return null;
-  }
-
+/*
+ * Update a source when its state changes
+ * e.g. the text was loaded, it was blackboxed
+ */
+function updateSource(state: SourcesState, source: Object) {
+  const existingSource = state.sources[source.id];
   return {
-    id: sourceId,
-    text: action.value.text,
-    contentType: action.value.contentType,
-    loadedState: "loaded"
+    ...state, 
+    sources: {
+      ...state.sources, 
+      [source.id]: { ...existingSource, ...source }
+    }
   };
 }
 
-// TODO: Action is coerced to `any` unfortunately because how we type
-// asynchronous actions is wrong. The `value` may be null for the
-// "start" and "error" states but we don't type it like that. We need
-// to rethink how we type async actions.
-function setSourceTextProps(state, action: LoadSourceAction): SourcesState {
-  const source = getTextPropsFromAction(action);
-  if (!source) {
-    return state;
-  }
-  return updateSources(state, [source]);
+/*
+ * Update all of the sources when an event occurs.
+ * e.g. workers are updated, project directory root changes
+ */
+function updateAllSources(
+  state: SourcesState,
+  callback: any
+) {
+  const updatedSources = Object.values(state.sources).map(source => ({
+    ...source,
+    ...callback(source)
+  }))
+
+  return addSources({ ...state, ...emptySources }, updatedSources);
 }
 
-function updateSources(state, sources: Object[]) {
-  const displayed = { ...state.displayed };
-  for (const thread in displayed) {
-    displayed[thread] = { ...displayed[thread] };
-  }
-
+/*
+ * Add sources to the sources store
+ * - Add the source to the sources store
+ * - Add the source URL to the urls map
+ * - Add the source ID to the thread displayed map
+ */
+function addSources(state: SourcesState, sources: Source[]) {
   state = {
     ...state,
     sources: { ...state.sources },
     urls: { ...state.urls },
-    displayed
+    displayed: { ...state.displayed }
   };
 
-  sources.forEach(source => updateSource(state, source));
+  for (const source of sources) {
+    const existingSource = state.sources[source.id];
+    let updatedSource = existingSource || source;
+
+    // Merge the source actor list
+    if (existingSource && source.actors) {
+      const actors = uniqBy(
+        [...existingSource.actors, ...source.actors],
+        ({ actor }) => actor
+      );
+      updatedSource = { ...updatedSource, actors }
+    }
+
+    // 1. Add the source to the sources map
+    state.sources[source.id] = updatedSource;
+
+    // 2. Update the source url map
+    const existing = state.urls[source.url] || [];
+    if (!existing.includes(source.id)) {
+      state.urls[source.url] = [...existing, source.id];
+    }
+
+    // 3. Update the displayed actor map
+    if (
+      underRoot(source, state.projectDirectoryRoot) &&
+      (!source.isExtension || getChromeAndExtenstionsEnabled({ sources: state }))
+    ) {
+      for (const actor of getSourceActors(state, source)) {
+        if (!state.displayed[actor.thread]) {
+          state.displayed[actor.thread] = {}
+        }
+        state.displayed[actor.thread][source.id] = true;
+      }
+    }
+  }
+
   return state;
 }
 
-function updateSourceUrl(state: SourcesState, source: Source) {
-  const existing = state.urls[source.url] || [];
-  if (!existing.includes(source.id)) {
-    state.urls[source.url] = [...existing, source.id];
-  }
-}
-
-function updateSource(state: SourcesState, source: Object) {
-  if (!source.id) {
-    return;
-  }
+/*
+ * Update sources when the worker list changes.
+ * - filter source actor lists so that missing threads no longer appear
+ * - NOTE: we do not remove sources for destroyed threads
+ */
+function updateWorkers(state: SourcesState, action: Object) {
+  const threads = [
+    action.mainThread,
+    ...action.workers.map(({ actor }) => actor)
+  ];
 
-  const existingSource = state.sources[source.id];
-  const updatedSource = existingSource
-    ? { ...existingSource, ...source }
-    : createSource(state, source);
-
-  // Any actors in the source are added to the existing ones.
-  if (existingSource && source.actors) {
-    updatedSource.actors = uniqBy(
-      [...existingSource.actors, ...updatedSource.actors],
-      ({ actor }) => actor
-    );
-  }
-
-  state.sources[source.id] = updatedSource;
-
-  updateSourceUrl(state, updatedSource);
-  updateDisplayedSource(state, updatedSource);
+  return updateAllSources(state, source => ({
+    actors: source.actors.filter(({ thread }) => threads.includes(thread))
+  }));
 }
 
-function updateDisplayedSource(state: SourcesState, source: Source) {
-  const root = state.projectDirectoryRoot;
-
-  if (
-    !underRoot(source, root) ||
-    (!getChromeAndExtenstionsEnabled({ sources: state }) && source.isExtension)
-  ) {
-    return;
-  }
-
-  let actors = source.actors;
-
-  // Original sources do not have actors, so use the generated source.
-  if (isOriginalSource(source)) {
-    const generatedSource = state.sources[originalToGeneratedId(source.id)];
-    actors = generatedSource ? generatedSource.actors : [];
-  }
-
-  actors.forEach(({ thread }) => {
-    if (!state.displayed[thread]) {
-      state.displayed[thread] = {};
-    }
-    state.displayed[thread][source.id] = true;
-  });
-}
-
-function updateWorkers(
-  state: SourcesState,
-  workers: WorkerList,
-  mainThread: ThreadId
-) {
-  // Filter out source actors for all removed threads.
-  const threads = [mainThread, ...workers.map(({ actor }) => actor)];
-  const updateActors = source =>
-    source.actors.filter(({ thread }) => threads.includes(thread));
-
-  const sources: Source[] = Object.values(state.sources)
-    .map((source: any) => ({ ...source, actors: updateActors(source) }))
-
-  // Regenerate derived information from the updated sources.
-  return updateSources({ ...state, ...emptySources }, sources);
-}
-
+/*
+ * Update sources when the project directory root changes
+ */
 function updateProjectDirectoryRoot(state: SourcesState, root: string) {
   prefs.projectDirectoryRoot = root;
 
-  const sources: Source[] = Object.values(state.sources).map((source: any) => {
-    return {
-      ...source,
-      relativeUrl: getRelativeUrl(source, root)
+  return updateAllSources(
+    { ...state, projectDirectoryRoot: root },
+    source => ({ relativeUrl: getRelativeUrl(source, root) })
+  );
+}
+
+/*
+ * Update a source's loaded state fields
+ * i.e. loadedState, text, error
+ */
+function updateLoadedState(state, action: LoadSourceAction): SourcesState {
+  const { sourceId } = action;
+  let source;
+
+  if (action.status === "start") {
+    source = { id: sourceId, loadedState: "loading" };
+
+  } else if (action.status === "error") {
+    source = { id: sourceId, error: action.error, loadedState: "loaded" };
+
+  } else {
+
+    if (!action.value) {
+      return state;
+    }
+
+    source = {
+      id: sourceId,
+      text: action.value.text,
+      contentType: action.value.contentType,
+      loadedState: "loaded"
     };
-  });
+  }
 
-  // Regenerate derived information from the updated sources.
-  return updateSources(
-    { ...state, projectDirectoryRoot: root, ...emptySources },
-    sources
-  );
+  return updateSource(state, source);
 }
 
 function updateBlackBoxList(url, isBlackBoxed) {
   const tabs = getBlackBoxList();
   const i = tabs.indexOf(url);
   if (i >= 0) {
     if (!isBlackBoxed) {
       tabs.splice(i, 1);
@@ -345,16 +347,26 @@ export function getBlackBoxList() {
 // module for the UI, and all of those selectors should take the
 // top-level app state, so we'd have to "wrap" them to automatically
 // pick off the piece of state we're interested in. It's impossible
 // (right now) to type those wrapped functions.
 type OuterState = { sources: SourcesState };
 
 const getSourcesState = (state: OuterState) => state.sources;
 
+function getSourceActors(state, source) {
+  if (isGenerated(source)) {
+    return source.actors;
+  }
+
+  // Original sources do not have actors, so use the generated source.
+  const generatedSource = state.sources[originalToGeneratedId(source.id)];
+  return generatedSource ? generatedSource.actors : [];
+}
+
 export function getSourceInSources(sources: SourcesMap, id: string): ?Source {
   return sources[id];
 }
 
 export function getSource(state: OuterState, id: SourceId): ?Source {
   return getSourceInSources(getSources(state), id);
 }
 
--- a/devtools/client/debugger/new/src/utils/source-queue.js
+++ b/devtools/client/debugger/new/src/utils/source-queue.js
@@ -24,15 +24,17 @@ export default {
     newSources = actions.newSources;
     queuedSources = [];
   },
   queue: (source: Source) => {
     queuedSources.push(source);
     queue();
   },
   queueSources: (sources: Source[]) => {
-    queuedSources = queuedSources.concat(sources);
-    queue();
+    if (sources.length > 0) {
+      queuedSources = queuedSources.concat(sources);
+      queue();
+    }
   },
 
   flush: () => Promise.all([queue.flush(), currentWork]),
   clear: () => queue.cancel()
 };
--- a/devtools/client/debugger/new/test/mochitest/browser_dbg-windowless-workers.js
+++ b/devtools/client/debugger/new/test/mochitest/browser_dbg-windowless-workers.js
@@ -20,46 +20,69 @@ function threadIsSelected(dbg, index) {
 // separately controlled from the same debugger.
 add_task(async function() {
   await pushPref("devtools.debugger.features.windowless-workers", true);
 
   const dbg = await initDebugger("doc-windowless-workers.html");
   const mainThread = dbg.toolbox.threadClient.actor;
 
   const workers = await getWorkers(dbg);
-  ok(workers.length == 1, "Got one worker");
-  const workerThread = workers[0].actor;
+  ok(workers.length == 2, "Got two workers");
+  const worker1Thread = workers[0].actor;
+  const worker2Thread = workers[1].actor;
 
   const mainThreadSource = findSource(dbg, "doc-windowless-workers.html");
   const workerSource = findSource(dbg, "simple-worker.js");
 
+  info("Test pausing in the main thread");
   assertNotPaused(dbg);
-
   await dbg.actions.breakOnNext();
   await waitForPaused(dbg, "doc-windowless-workers.html");
-
-  // We should be paused at the timer in doc-windowless-workers.html
-  assertPausedAtSourceAndLine(dbg, mainThreadSource.id, 9);
+  assertPausedAtSourceAndLine(dbg, mainThreadSource.id, 10);
 
-  await dbg.actions.selectThread(workerThread);
+  info("Test pausing in a worker");
+  await dbg.actions.selectThread(worker1Thread);
   assertNotPaused(dbg);
-
   await dbg.actions.breakOnNext();
   await waitForPaused(dbg, "simple-worker.js");
-
-  // We should be paused at the timer in simple-worker.js
   assertPausedAtSourceAndLine(dbg, workerSource.id, 3);
 
+  info("Test stepping in a worker");
   await stepOver(dbg);
   assertPausedAtSourceAndLine(dbg, workerSource.id, 4);
 
-  await dbg.actions.selectThread(mainThread);
+  info("Test resuming in a worker");
+  await resume(dbg);
+  assertNotPaused(dbg);
+
+  info("Test stepping in the main thread");
+  dbg.actions.selectThread(mainThread);
+  await stepOver(dbg);
+  assertPausedAtSourceAndLine(dbg, mainThreadSource.id, 11);
+
+  info("Test resuming in the mainThread");
+  await resume(dbg);
+  assertNotPaused(dbg);
 
+  info("Test pausing in both workers");
+  await addBreakpoint(dbg, "simple-worker", 10);
+  invokeInTab("sayHello");
+  dbg.actions.selectThread(worker1Thread);
+  await waitForPaused(dbg);
+  assertPausedAtSourceAndLine(dbg, workerSource.id, 10);
+
+  dbg.actions.selectThread(worker2Thread);
+  await waitForPaused(dbg);
+  assertPausedAtSourceAndLine(dbg, workerSource.id, 10);
+
+  info("Test stepping in second worker and not the first");
   await stepOver(dbg);
-  assertPausedAtSourceAndLine(dbg, mainThreadSource.id, 10);
+  assertPausedAtSourceAndLine(dbg, workerSource.id, 11);
+  dbg.actions.selectThread(worker1Thread);
+  assertPausedAtSourceAndLine(dbg, workerSource.id, 10);
 
   is(
     findElement(dbg, "threadSourceTreeHeader", 2).innerText,
     "simple-worker.js",
     "simple-worker.js - worker is shown in the source tree"
   );
 
   clickElement(dbg, "threadSourceTreeSourceNode", 2, 2);
--- a/devtools/client/debugger/new/test/mochitest/examples/doc-windowless-workers.html
+++ b/devtools/client/debugger/new/test/mochitest/examples/doc-windowless-workers.html
@@ -1,19 +1,25 @@
 <!DOCTYPE HTML>
 <html>
 
 <script>
-var worker = new Worker("simple-worker.js");
+var worker1 = new Worker("simple-worker.js");
+var worker2 = new Worker("simple-worker.js");
 
 var count = 0;
 function timer() {
   var n = ++count;
   console.log("MAIN THREAD SAYS HELLO! " + n);
 }
 
 setInterval(timer, 1000);
+
+window.sayHello = () => {
+  worker1.postMessage({yo: 'yo'})
+  worker2.postMessage({yo: 'yo'})
+}
 </script>
 
 <body>
 Hello World!
 </body>
 </html>
--- a/devtools/client/debugger/new/test/mochitest/examples/simple-worker.js
+++ b/devtools/client/debugger/new/test/mochitest/examples/simple-worker.js
@@ -1,7 +1,11 @@
 var count = 0;
 function timer() {
   var n = ++count;
   console.log("WORKER SAYS HELLO! " + n);
 }
 
 setInterval(timer, 1000);
+
+self.onmessage = () => {
+  console.log('hi')
+}
\ No newline at end of file