Bug 1778647 - [devtools] Store source's framework only once, in symbols. r=bomsy

We are currently duplicated source's framework in symbols and tabs reducers.
This is uncessary and confusing. And this possibly lead to more state/component updates.

This is mostly used to computed the source icon.
I extented test coverage to better cover the behavior of SourceIcon,
while simplifying a few things around this component by unifying the various places
where we compute the final icon, so that now it is only done by getSourceClassnames
and modifier.

Differential Revision: https://phabricator.services.mozilla.com/D151358

devtools/client/debugger/src/actions/sources/symbols.js

@@ -5,7 +5,6 @@
 import { getSymbols } from "../../selectors";
 
 import { PROMISE } from "../utils/middleware/promise";
-import { updateTab } from "../tabs";
 import { loadSourceText } from "./loadSourceText";
 
 import { memoizeableAction } from "../../utils/memoizableAction";
@@ -22,11 +21,6 @@ async function doSetSymbols(cx, source, { dispatch, getState, parser }) {
     sourceId,
     [PROMISE]: parser.getSymbols(sourceId),
   });
-
-  const symbols = getSymbols(getState(), source);
-  if (symbols && symbols.framework) {
-    dispatch(updateTab(source, symbols.framework));
-  }
 }
 
 export const setSymbols = memoizeableAction("setSymbols", {

devtools/client/debugger/src/actions/tabs.js

@@ -16,19 +16,10 @@ import {
   getNewSelectedSourceId,
 } from "../selectors";
 
-export function updateTab(source, framework) {
-  return {
-    type: "UPDATE_TAB",
-    source,
-    framework,
-  };
-}
-
 export function addTab(source) {
   return {
     type: "ADD_TAB",
     source,
-    framework: null,
   };
 }
 

devtools/client/debugger/src/components/PrimaryPanes/SourcesTreeItem.js

@@ -13,7 +13,6 @@ import AccessibleImage from "../shared/AccessibleImage";
 
 import {
   getGeneratedSourceByURL,
-  hasPrettyTab as checkHasPrettyTab,
   getContext,
   getSourceContent,
 } from "../../selectors";
@@ -43,7 +42,6 @@ class SourceTreeItem extends Component {
       focused: PropTypes.bool.isRequired,
       getBlackBoxSourcesGroups: PropTypes.func.isRequired,
       hasMatchingGeneratedSource: PropTypes.bool.isRequired,
-      hasPrettyTab: PropTypes.bool.isRequired,
       item: PropTypes.object.isRequired,
       loadSourceText: PropTypes.func.isRequired,
       projectRoot: PropTypes.string.isRequired,
@@ -270,7 +268,7 @@ class SourceTreeItem extends Component {
   }
 
   renderIcon(item, depth) {
-    const { projectRoot, source, hasPrettyTab, threads } = this.props;
+    const { projectRoot, source, threads } = this.props;
 
     if (item.name === "Webpack") {
       return <AccessibleImage className="webpack" />;
@@ -319,14 +317,6 @@ class SourceTreeItem extends Component {
       return <AccessibleImage className="folder" />;
     }
 
-    if (this.props.isSourceBlackBoxed) {
-      return <AccessibleImage className="blackBox" />;
-    }
-
-    if (hasPrettyTab) {
-      return <AccessibleImage className="prettyPrint" />;
-    }
-
     if (source) {
       return (
         <SourceIcon
@@ -413,7 +403,6 @@ const mapStateToProps = (state, props) => {
   return {
     cx: getContext(state),
     hasMatchingGeneratedSource: getHasMatchingGeneratedSource(state, source),
-    hasPrettyTab: source ? checkHasPrettyTab(state, source.url) : false,
     sourceContent: source ? getSourceContentValue(state, source) : null,
   };
 };

devtools/client/debugger/src/components/shared/SourceIcon.js

@@ -9,33 +9,23 @@ import { connect } from "../../utils/connect";
 
 import AccessibleImage from "./AccessibleImage";
 
-import { getSourceClassnames, isPretty } from "../../utils/source";
-import { getFramework } from "../../utils/tabs";
-import { getSymbols, getTabs, isSourceBlackBoxed } from "../../selectors";
+import { getSourceClassnames } from "../../utils/source";
+import { getSymbols, isSourceBlackBoxed, hasPrettyTab } from "../../selectors";
 
 import "./SourceIcon.css";
 
 class SourceIcon extends PureComponent {
   static get propTypes() {
     return {
-      framework: PropTypes.string.isRequired,
       modifier: PropTypes.func.isRequired,
       source: PropTypes.object.isRequired,
-      symbols: PropTypes.object,
+      iconClass: PropTypes.string,
     };
   }
 
   render() {
-    const { modifier, source, symbols, framework, isBlackBoxed } = this.props;
-    let iconClass = "";
-
-    if (isPretty(source)) {
-      iconClass = "prettyPrint";
-    } else {
-      iconClass = framework
-        ? framework.toLowerCase()
-        : getSourceClassnames(source, symbols, isBlackBoxed);
-    }
+    const { modifier } = this.props;
+    let { iconClass } = this.props;
 
     if (modifier) {
       const modified = modifier(iconClass);
@@ -49,8 +39,22 @@ class SourceIcon extends PureComponent {
   }
 }
 
-export default connect((state, props) => ({
-  symbols: getSymbols(state, props.source),
-  isBlackBoxed: props.source ? isSourceBlackBoxed(state, props.source) : null,
-  framework: getFramework(getTabs(state), props.source.url),
-}))(SourceIcon);
+export default connect((state, props) => {
+  const { source } = props;
+  const symbols = getSymbols(state, source);
+  const isBlackBoxed = isSourceBlackBoxed(state, source);
+  const hasMatchingPrettyTab = hasPrettyTab(state, source.url);
+
+  // This is the key function that will compute the icon type,
+  // In addition to the "modifier" implemented by each callsite.
+  const iconClass = getSourceClassnames(
+    source,
+    symbols,
+    isBlackBoxed,
+    hasMatchingPrettyTab
+  );
+
+  return {
+    iconClass,
+  };
+})(SourceIcon);

devtools/client/debugger/src/reducers/tabs.js

@@ -23,8 +23,7 @@ function resetTabState(state) {
 function update(state = initialTabState(), action) {
   switch (action.type) {
     case "ADD_TAB":
-    case "UPDATE_TAB":
-      return updateTabList(state, action.source, action.framework);
+      return updateTabList(state, action.source);
 
     case "MOVE_TAB":
       return moveTabInList(state, action);
@@ -71,7 +70,7 @@ function addSelectedSource(state, source) {
     return state;
   }
 
-  return updateTabList(state, source, null);
+  return updateTabList(state, source);
 }
 
 function addVisibleTabs(state, sources) {
@@ -122,10 +121,9 @@ function resetTabsForThread(state, threadActorID) {
 }
 
 /**
- * Adds the new source to the tab list if it is not already there,
- * only update its "framework" attribute if it already exists.
+ * Adds the new source to the tab list if it is not already there.
  */
-function updateTabList(state, source, framework) {
+function updateTabList(state, source) {
   const { url } = source;
   const isOriginal = isOriginalId(source.id);
 
@@ -139,16 +137,12 @@ function updateTabList(state, source, framework) {
   if (currentIndex === -1) {
     const newTab = {
       url,
-      framework,
       sourceId: source.id,
       isOriginal,
       threadActorID: source.thread,
     };
     // New tabs are added first in the list
     tabs = [newTab, ...tabs];
-  } else if (framework && tabs[currentIndex].framework != framework) {
-    tabs = Array.from(tabs);
-    tabs[currentIndex].framework = framework;
   } else {
     return state;
   }

devtools/client/debugger/src/utils/source.js

@@ -485,7 +485,27 @@ export function getTextAtPosition(sourceId, asyncContent, location) {
   return lineText.slice(column, column + 100).trim();
 }
 
-export function getSourceClassnames(source, symbols, isBlackBoxed) {
+/**
+ * Compute the CSS classname string to use for the icon of a given source.
+ *
+ * @param {Object} source
+ *        The reducer source object.
+ * @param {Object} symbols
+ *        The reducer symbol object for the given source.
+ * @param {Boolean} isBlackBoxed
+ *        To be set to true, when the given source is blackboxed.
+ * @param {Boolean} hasPrettyTab
+ *        To be set to true, if the given source isn't the pretty printed one,
+ *        but another tab for that source is opened pretty printed.
+ * @return String
+ *        The classname to use.
+ */
+export function getSourceClassnames(
+  source,
+  symbols,
+  isBlackBoxed,
+  hasPrettyTab = false
+) {
   // Conditionals should be ordered by priority of icon!
   const defaultClassName = "file";
 
@@ -493,7 +513,10 @@ export function getSourceClassnames(source, symbols, isBlackBoxed) {
     return defaultClassName;
   }
 
-  if (isPretty(source)) {
+  // In the SourceTree, we don't show the pretty printed sources,
+  // but still want to show the pretty print icon when a pretty printed tab
+  // for the current source is opened.
+  if (isPretty(source) || hasPrettyTab) {
     return "prettyPrint";
   }
 

devtools/client/debugger/src/utils/tabs.js

@@ -32,11 +32,6 @@ export function getHiddenTabs(sourceTabs, sourceTabEls) {
   });
 }
 
-export function getFramework(tabs, url) {
-  const tab = tabs.find(t => t.url === url);
-  return tab?.framework ?? "";
-}
-
 export function getTabMenuItems() {
   return {
     closeTab: {

devtools/client/debugger/test/mochitest/browser_dbg-features-source-tree.js

@@ -248,8 +248,10 @@ add_task(async function testSourceTreeOnTheIntegrationTestPage() {
     "named-eval.js"
   );
 
+  info("Verify source tree content");
   await waitForSourcesInSourceTree(dbg, INTEGRATION_TEST_PAGE_SOURCES);
 
+  info("Verify Thread Source Items");
   const mainThreadItem = findSourceTreeThreadByName(dbg, "Main Thread");
   ok(mainThreadItem, "Found the thread item for the main thread");
   ok(
@@ -310,6 +312,20 @@ add_task(async function testSourceTreeOnTheIntegrationTestPage() {
     "The thread has the worker icon"
   );
 
+  info("Verify source icons");
+  assertSourceIcon(dbg, "index.html", "file");
+  assertSourceIcon(dbg, "script.js", "javascript");
+  assertSourceIcon(dbg, "query.js?x=1", "javascript");
+  assertSourceIcon(dbg, "original.js", "javascript");
+  info("Verify blackbox source icon");
+  await selectSource(dbg, "script.js");
+  await clickElement(dbg, "blackbox");
+  await waitForDispatch(dbg.store, "BLACKBOX");
+  assertSourceIcon(dbg, "script.js", "blackBox");
+  await clickElement(dbg, "blackbox");
+  await waitForDispatch(dbg.store, "BLACKBOX");
+  assertSourceIcon(dbg, "script.js", "javascript");
+
   info("Assert the content of the named eval");
   await selectSource(dbg, "named-eval.js");
   assertTextContentOnLine(dbg, 3, `console.log("named-eval");`);
@@ -336,6 +352,7 @@ add_task(async function testSourceTreeOnTheIntegrationTestPage() {
   clickElement(dbg, "prettyPrintButton");
   await waitForSource(dbg, "query.js?x=1:formatted");
   await waitForSelectedSource(dbg, "query.js?x=1:formatted");
+  assertSourceIcon(dbg, "query.js?x=1", "prettyPrint");
 
   const prettyTab = findElement(dbg, "activeTab");
   is(prettyTab.innerText, "query.js?x=1", "Tab label is query.js?x=1");
@@ -397,6 +414,7 @@ add_task(async function testSourceTreeWithWebExtensionContentScript() {
     contentScriptGroupItem.querySelector("span.img.extension"),
     "The group has the extension icon"
   );
+  assertSourceIcon(dbg, "content_script.js", "javascript");
 
   for (let i = 1; i < 3; i++) {
     info(

devtools/client/debugger/test/mochitest/browser_dbg-react-app.js

@@ -30,4 +30,5 @@ add_task(async function() {
     sourceTab.querySelector(".source-icon.react"),
     "Source tab has a React icon"
   );
+  assertSourceIcon(dbg, "App.js", "react");
 });

devtools/client/debugger/test/mochitest/shared-head.js

@@ -1078,6 +1078,25 @@ function findSourceNodeWithText(dbg, text) {
   });
 }
 
+/**
+ * Assert the icon type used in the SourceTree for a given source
+ *
+ * @param {Object} dbg
+ * @param {String} sourceName
+ *        Name of the source displayed in the source tree
+ * @param {String} icon
+ *        Expected icon CSS classname
+ */
+function assertSourceIcon(dbg, sourceName, icon) {
+  const sourceItem = findSourceNodeWithText(dbg, sourceName);
+  ok(sourceItem, `Found the source item for ${sourceName}`);
+  is(
+    sourceItem.querySelector(".source-icon").className,
+    `img source-icon ${icon}`,
+    `The icon for ${sourceName} is correct`
+  );
+}
+
 async function expandAllSourceNodes(dbg, treeNode) {
   const onContextMenu = waitForContextMenu(dbg);
   rightClickEl(dbg, treeNode);