Bug 1529427 - Refactor sources reducer so that all source actors are included.
authorJason Laster <jlaster@mozilla.com>
Fri, 22 Feb 2019 20:37:42 +0000
changeset 518538 901be6adcc2d3d3e24b1a2d70213cd12565e977e
parent 518537 d945b5bd54f03d378481a960ae8293f6ce375aeb
child 518539 4978133d62260c9405d07d049e01bda746dee9af
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [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,82 +20,62 @@ 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);
-
-  await stepOver(dbg);
-  assertPausedAtSourceAndLine(dbg, mainThreadSource.id, 10);
+  info("Test resuming in a worker");
+  await resume(dbg);
+  assertNotPaused(dbg);
 
-  is(
-    findElement(dbg, "threadSourceTreeHeader", 2).innerText,
-    "simple-worker.js",
-    "simple-worker.js - worker is shown in the source tree"
-  );
-
-  clickElement(dbg, "threadSourceTreeSourceNode", 2, 2);
+  info("Test stepping in the main thread");
+  dbg.actions.selectThread(mainThread);
+  await stepOver(dbg);
+  assertPausedAtSourceAndLine(dbg, mainThreadSource.id, 11);
 
-  await waitForElement(dbg, "threadSourceTreeSourceNode", 2, 3);
-
-  is(
-    findElement(dbg, "threadSourceTreeSourceNode", 2, 3).innerText,
-    "simple-worker.js",
-    "simple-worker.js - source is shown in the source tree"
-  );
+  info("Test resuming in the mainThread");
+  await resume(dbg);
+  assertNotPaused(dbg);
 
-  is(
-    findElement(dbg, "threadSourceTreeSourceNode", 2, 3).innerText,
-    "simple-worker.js",
-    "simple-worker.js - source is shown in the source tree"
-  );
-  // Threads Pane
-  is(
-    findElement(dbg, "threadsPaneItem", 1).innerText,
-    "Main Thread",
-    "Main Thread is shown in the threads pane"
-  );
+  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);
 
-  is(
-    findElement(dbg, "threadsPaneItem", 2).innerText,
-    "simple-worker.js",
-    "simple-worker.js is shown in the threads pane"
-  );
+  dbg.actions.selectThread(worker2Thread);
+  await waitForPaused(dbg);
+  assertPausedAtSourceAndLine(dbg, workerSource.id, 10);
 
-  ok(threadIsSelected(dbg, 1), "main thread is selected");
-  ok(threadIsPaused(dbg, 1), "main thread is paused");
-  ok(threadIsPaused(dbg, 2), "simple-worker.js is paused");
-  is(findAllElements(dbg, "frames").length, 1, "Main Thread has one frame");
-
-  clickElement(dbg, "threadsPaneItem", 2);
-  await waitFor(() => threadIsSelected(dbg, 2));
-  is(findAllElements(dbg, "frames").length, 1, "simple worker has one frame");
+  info("Test stepping in second worker and not the first");
+  await stepOver(dbg);
+  assertPausedAtSourceAndLine(dbg, workerSource.id, 11);
+  dbg.actions.selectThread(worker1Thread);
+  assertPausedAtSourceAndLine(dbg, workerSource.id, 10);
 });
--- 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