Closed Bug 1363097 Opened 7 years ago Closed 7 years ago

Convert font inspector to React/Redux

Categories

(DevTools :: Inspector, enhancement, P3)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: mpark, Assigned: mpark)

Details

Attachments

(1 file, 1 obsolete file)

1363097.patch
38.12 KB, patch
gl
: review+
Details | Diff | Splinter Review 
      No description provided.
Assignee: nobody → mikeparkms
Comment on attachment 8866460 [details]
Bug 1363097 - Convert font inspector to React/Redux.

https://reviewboard.mozilla.org/r/138090/#review141706

::: devtools/client/inspector/fonts/actions/index.js:11
(Diff revision 1)
> +
> +const { createEnum } = require("devtools/client/shared/enum");
> +
> +createEnum([
> +
> +  // Update the list of fonts

s/fonts/fonts.

::: devtools/client/inspector/fonts/components/App.js:25
(Diff revision 1)
> +const App = createClass({
> +
> +  displayName: "App",
> +
> +  propTypes: {
> +    object: PropTypes.any,

Instead of passing in the object, pass in the functions that you will be calling. Usually this is in the form of onX where X is a handler event.

::: devtools/client/inspector/fonts/components/App.js:34
(Diff revision 1)
> +  mixins: [ addons.PureRenderMixin ],
> +
> +  componentDidMount() {
> +    let searchInput = findDOMNode(this).querySelector(".devtools-textinput");
> +    searchInput.addEventListener("contextmenu",
> +     this.props.object.inspector.onTextBoxContextMenu);

We should just pass onTextBoxContextMenu into the propTypes instead of object. You would destructure for { onTextBoxContextMenu } = this.props;

::: devtools/client/inspector/fonts/components/App.js:43
(Diff revision 1)
> +    let searchInput = findDOMNode(this).querySelector(".devtools-textinput");
> +    searchInput.removeEventListener("contextmenu",
> +     this.props.object.inspector.onTextBoxContextMenu);
> +  },
> +
> +  filterFonts(value) {

s/filterFonts/onFilterFonts

::: devtools/client/inspector/fonts/components/App.js:48
(Diff revision 1)
> +  filterFonts(value) {
> +    let { object } = this.props;
> +    object.update(object._lastUpdateShowedAllFonts, value);
> +  },
> +
> +  showAll() {

s/showAll/onShowAllFontClick

::: devtools/client/inspector/fonts/components/Font.js:23
(Diff revision 1)
> +  mixins: [ addons.PureRenderMixin ],
> +
> +  getSectionClasses() {
> +    let classes = ["font"];
> +    classes.push((this.props.URI) ? "is-remote" : "is-local");
> +    if (this.props.rule) {

Add a new line before and after the if statement to separate the block from the rest.

::: devtools/client/inspector/fonts/fonts.js:95
(Diff revision 1)
> -  },
> -
> -  /**
> -   * Preview input 'input' event handler.
>     */
> -  previewTextChanged: function () {
> +  isActive: function () {

s/isActive/isPanelVisible

::: devtools/client/inspector/fonts/fonts.js:95
(Diff revision 1)
> -  },
> -
> -  /**
> -   * Preview input 'input' event handler.
>     */
> -  previewTextChanged: function () {
> +  isActive: function () {

We should be consistent and use the function shorthand 

isPanelActive() { .. }

::: devtools/client/inspector/fonts/fonts.js:96
(Diff revision 1)
> -
> -  /**
> -   * Preview input 'input' event handler.
>     */
> -  previewTextChanged: function () {
> -    if (this._previewUpdateTimeout) {
> +  isActive: function () {
> +    return this.inspector.sidebar &&

Add checks for this.inspector.toolbox && this.inspector.toolbox.currentToolId === "inspector"

::: devtools/client/inspector/fonts/fonts.js:110
(Diff revision 1)
>    },
>  
>    /**
>     * Hide the font list. No node are selected.
>     */
>    dim: function () {

I don't think we need a dim, undim and clear function when we do this with react. We should call an update each time the sidebar panel is selected and update the view. The react components should naturally know there is nothing to display if the fonts prop is just an empty array and an appropriate no fonts available message should be displayed.

::: devtools/client/inspector/fonts/fonts.js:127
(Diff revision 1)
>    },
>  
>    /**
> -   * Clears the font list.
> +   * Selection 'new-node' event handler.
>     */
> -  clear: function () {
> +  onNewNode: function () {

Use function shorthand: 

onNewNode() { .. }

::: devtools/client/inspector/fonts/fonts.js:128
(Diff revision 1)
>  
>    /**
> -   * Clears the font list.
> +   * Selection 'new-node' event handler.
>     */
> -  clear: function () {
> -    this.chromeDoc.querySelector("#all-fonts").innerHTML = "";
> +  onNewNode: function () {
> +    if (this.isActive() &&

This should simply be 
if (this.isPanelVisible()) {
 this.update(....)
}

::: devtools/client/inspector/fonts/fonts.js:131
(Diff revision 1)
>     */
> -  clear: function () {
> -    this.chromeDoc.querySelector("#all-fonts").innerHTML = "";
> +  onNewNode: function () {
> +    if (this.isActive() &&
> +        this.inspector.selection.isConnected() &&
> +        this.inspector.selection.isElementNode()) {
> +      this.undim();

I would remove undim and dim since this is unneccessary. Simply, call a single update and let the react view handle everything.

::: devtools/client/inspector/fonts/fonts.js:139
(Diff revision 1)
> +      this.dim();
> +    }
>    },
>  
> - /**
> +  /**
> -  * Retrieve all the font info for the selected node and display it.
> +   * Callback for the theme-switched event.

s/Callback/Handler

::: devtools/client/inspector/fonts/fonts.js:139
(Diff revision 1)
> +      this.dim();
> +    }
>    },
>  
> - /**
> +  /**
> -  * Retrieve all the font info for the selected node and display it.
> +   * Callback for the theme-switched event.

s/theme-switched/"theme-switched"

::: devtools/client/inspector/fonts/fonts.js:141
(Diff revision 1)
>    },
>  
> - /**
> +  /**
> -  * Retrieve all the font info for the selected node and display it.
> +   * Callback for the theme-switched event.
> -  */
> +   */
> -  update: Task.async(function* (showAllFonts) {
> +  onThemeChanged: function (event, frame) {

onThemeChanged(event, frame) { .. }

::: devtools/client/inspector/fonts/fonts.js:149
(Diff revision 1)
> +    }
> +  },
> +
> +  update: Task.async(function* (showAllFonts, previewText) {
>      let node = this.inspector.selection.nodeFront;
> -    let panel = this.chromeDoc.getElementById("sidebar-panel-fontinspector");
> +    let panel = this.document.getElementById("sidebar-panel-fontinspector");

Remove this.

::: devtools/client/inspector/fonts/fonts.js:155
(Diff revision 1)
>  
>      if (!node ||
>          !this.isActive() ||
>          !this.inspector.selection.isConnected() ||
>          !this.inspector.selection.isElementNode() ||
>          panel.classList.contains("dim")) {

Remove this check for dim. Also, remove the css for dim/undim

::: devtools/client/inspector/fonts/fonts.js:166
(Diff revision 1)
> +    }
> +    this._lastPreviewText = previewText;
>  
>      let options = {
>        includePreviews: true,
> -      previewText: this.getPreviewText(),
> +      previewText: (!previewText || previewText === "") ?

previewText === "" is accounted for with !previewText.

::: devtools/client/inspector/fonts/fonts.js:182
(Diff revision 1)
>                        .then(null, console.error);
>      }
>  
>      if (!fonts || !fonts.length) {
>        // No fonts to display. Clear the previously shown fonts.
> -      this.clear();
> +      this.store.dispatch(updateFonts([]));

you can simply pass in fonts

::: devtools/client/inspector/fonts/moz.build:10
(Diff revision 1)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +DIRS += [
> +    'actions',
> +    'components',
> +    'utils',

Keep this list alphabetically ordered. Move utils below reducers. You will find that new unit tests that are added in /tests/ are also added in a browser.ini file, and there is something that enforces that the list of files are alphabetically ordered.

::: devtools/client/inspector/fonts/test/head.js:65
(Diff revision 1)
>   */
>  function* updatePreviewText(view, text) {
>    info(`Changing the preview text to '${text}'`);
>  
> -  let doc = view.chromeDoc;
> -  let input = doc.getElementById("font-preview-text-input");
> +  let doc = view.document;
> +  let input = doc.querySelectorAll("#sidebar-panel-fontinspector .devtools-textinput")[0];

We should probably just do querySelector() since we only expect one textinput for the font inspector panel.

::: devtools/client/inspector/inspector.js:638
(Diff revision 1)
>          defaultTab == "animationinspector");
>      }
>  
>      if (Services.prefs.getBoolPref("devtools.fontinspector.enabled") &&
>          this.canGetUsedFontFaces) {
> -      this.sidebar.addExistingTab(
> +      // this.sidebar.addExistingTab(

Remove this commented out code.

::: devtools/client/inspector/inspector.xhtml:148
(Diff revision 1)
>              <div id="computedview-no-results" hidden="" data-localization="content=inspector.noProperties"></div>
>            </div>
>          </div>
>        </div>
>  
>        <div id="sidebar-panel-fontinspector" class="devtools-monospace theme-sidebar inspector-tabpanel"

The whole sidebar-panel for the fontinspector can be removed since addSidebarTab does this for us.

::: devtools/client/inspector/reducers.js:13
(Diff revision 1)
>  // settings.
>  
>  exports.boxModel = require("devtools/client/inspector/boxmodel/reducers/box-model");
>  exports.grids = require("devtools/client/inspector/grids/reducers/grids");
>  exports.highlighterSettings = require("devtools/client/inspector/grids/reducers/highlighter-settings");
> +exports.fonts = require("devtools/client/inspector/fonts/reducers/fonts");

Move this before exports.grids to keep this alphabetically ordered.
Attachment #8866460 - Flags: review?(gl)
Comment on attachment 8866460 [details]
Bug 1363097 - Convert font inspector to React/Redux.

https://reviewboard.mozilla.org/r/138090/#review142604

::: devtools/client/inspector/fonts/fonts.js:87
(Diff revision 1)
>     */
> -  destroy: function () {
> -    this.chromeDoc = null;
> -    this.inspector.sidebar.off("fontinspector-selected", this.onNewNode);
> +  destroy() {
> +    this.document = null;
> +    this.inspector = null;
> +    this.store = null;
>      this.inspector.selection.off("new-node-front", this.onNewNode);

Let's move these event listener removal before setting the variables to null.
Comment on attachment 8866460 [details]
Bug 1363097 - Convert font inspector to React/Redux.

https://reviewboard.mozilla.org/r/138090/#review142732

::: devtools/client/inspector/fonts/components/App.js:24
(Diff revision 2)
> +
> +const App = createClass({
> +
> +  displayName: "App",
> +
> +  propTypes: {

Keep the propTypes alphabetically ordered, but with the handler (onX) functions at the bottom.

::: devtools/client/inspector/fonts/components/App.js:35
(Diff revision 2)
> +  mixins: [ addons.PureRenderMixin ],
> +
> +  componentDidMount() {
> +    let searchInput = findDOMNode(this).querySelector(".devtools-textinput");
> +    searchInput.addEventListener("contextmenu",
> +     this.props.onTextBoxContextMenu);

We can make the addEventListener into one line if we destruct let { onTextBoxContextMenu } = this.props

::: devtools/client/inspector/fonts/components/App.js:40
(Diff revision 2)
> +     this.props.onTextBoxContextMenu);
> +  },
> +
> +  componentWillUnmount() {
> +    let searchInput = findDOMNode(this).querySelector(".devtools-textinput");
> +    searchInput.removeEventListener("contextmenu",

Same as above.

::: devtools/client/inspector/fonts/components/App.js:44
(Diff revision 2)
> +    let searchInput = findDOMNode(this).querySelector(".devtools-textinput");
> +    searchInput.removeEventListener("contextmenu",
> +     this.props.onTextBoxContextMenu);
> +  },
> +
> +  onFilterFonts(value) {

s/onFilterFonts/onPreviewFonts since we never actually filter font.

::: devtools/client/inspector/fonts/components/Font.js:28
(Diff revision 2)
> +
> +    if (this.props.rule) {
> +      classes.push("has-code");
> +    }
> +
> +    return classes;

Do .join here.

::: devtools/client/inspector/fonts/components/Font.js:42
(Diff revision 2)
> +      rule,
> +      ruleText,
> +      previewUrl,
> +    } = this.props;
> +
> +    return dom.section(

It would be good to break the render of individual components down a bit more to make this a lot more readable. For instance, we can have renderFontFormatURL, renderFontCSS, etc.

::: devtools/client/inspector/fonts/components/Font.js:44
(Diff revision 2)
> +      previewUrl,
> +    } = this.props;
> +
> +    return dom.section(
> +      {
> +        className: this.getSectionClasses().join(" "),

Remove the join b/c of above.

::: devtools/client/inspector/fonts/components/Font.js:71
(Diff revision 2)
> +        ),
> +        dom.span(
> +          {
> +            className: "font-is-local",
> +          },
> +          getStr("fontinspector.system")

" " + getStr

::: devtools/client/inspector/fonts/components/Font.js:78
(Diff revision 2)
> +        dom.span(
> +          {
> +            className: "font-is-remote",
> +          },
> +          " ",
> +          getStr("fontinspector.remote")

" " + getStr.

::: devtools/client/inspector/fonts/components/FontList.js:36
(Diff revision 2)
> +      dom.ul(
> +        {
> +          id: "all-fonts"
> +        },
> +        fonts.map(font => Font({
> +          name: font.name,

We can destructure would lessen some of the for us after line 24.

let { name, CSSFamilyName: cssFamilyName, .. } = font;

::: devtools/client/inspector/fonts/fonts.js
(Diff revision 2)
>  function FontInspector(inspector, window) {
> +  this.document = window.document;
>    this.inspector = inspector;
>    this.pageStyle = this.inspector.pageStyle;
> -  this.chromeDoc = window.document;
> +  this.store = inspector.store;
> -  this.init();

Go back to calling this.init() here, but add a new line after the reference declarations prior to calling this.init().

::: devtools/client/inspector/fonts/fonts.js:45
(Diff revision 2)
> +  init() {
> +    if (!this.inspector) {
> +      return;
> +    }
> +
>      this.update = this.update.bind(this);

Move all these bindings in a block after the reference declarations in the constructor. Again add a new line after this.store and a new line after all these bindings to really just separate it to logical blocks.

::: devtools/client/inspector/fonts/fonts.js:52
(Diff revision 2)
>      this.onThemeChanged = this.onThemeChanged.bind(this);
> +
> +    let app = App({
> +      update: this.update,
> +      onTextBoxContextMenu: this.inspector.onTextBoxContextMenu,
> +      fonts: []

Remove fonts. We shouldn't have to pass in the fonts since the fonts is part of the store, which is connected to the App.

::: devtools/client/inspector/fonts/fonts.js:84
(Diff revision 2)
>  
>    /**
> -   * Is the fontinspector visible in the sidebar?
> +   * Destruction function called when the inspector is destroyed. Removes event listeners
> +   * and cleans up references.
>     */
> -  isActive: function () {
> +  destroy() {

We need to make sure we call this.fontinspector.destroy() in inspector.js#destroy()

::: devtools/client/inspector/fonts/fonts.js:87
(Diff revision 2)
> -    this.showAllLink.removeEventListener("click", this.showAll);
> +    this.inspector.sidebar.off("fontinspector-selected", this.onNewNode);
> -    this.previewInput.removeEventListener("input", this.previewTextChanged);
> -    this.previewInput.removeEventListener("contextmenu",
> -      this.inspector.onTextBoxContextMenu);
> -
>      gDevTools.off("theme-switched", this.onThemeChanged);

Add a new line to separate the logical blocks (event listener removal and variables being set to null).

::: devtools/client/inspector/fonts/fonts.js:88
(Diff revision 2)
> -    this.previewInput.removeEventListener("input", this.previewTextChanged);
> -    this.previewInput.removeEventListener("contextmenu",
> -      this.inspector.onTextBoxContextMenu);
> -
>      gDevTools.off("theme-switched", this.onThemeChanged);
> -
> +    this.document = null;

We need to add this.pageStyle = null.

::: devtools/client/inspector/fonts/fonts.js:94
(Diff revision 2)
> -      clearTimeout(this._previewUpdateTimeout);
> +    this.store = null;
> -    }
>    },
>  
>    /**
> -   * Selection 'new-node' event handler.
> +   * Is the fontinspector visible in the sidebar?

Returns true if the font inspector panel is visible, and false otherwise.

::: devtools/client/inspector/fonts/fonts.js:98
(Diff revision 2)
>    /**
> -   * Selection 'new-node' event handler.
> +   * Is the fontinspector visible in the sidebar?
>     */
> -  onNewNode: function () {
> -    if (this.isActive() &&
> -        this.inspector.selection.isConnected() &&
> +  isPanelVisible() {
> +    return this.inspector.sidebar &&
> +           this.inspector.sidebar.getCurrentTabID() == "fontinspector" &&

s/==/===

::: devtools/client/inspector/fonts/fonts.js:106
(Diff revision 2)
>    /**
> -   * The text to use for previews. Returns either the value user has typed to
> +   * Clears the font list.
> -   * the preview input or DEFAULT_PREVIEW_TEXT if the input is empty or contains
> -   * only whitespace.
>     */
> -  getPreviewText: function () {
> +  clear() {

This isn't used anymore. So, let's remove it.

::: devtools/client/inspector/fonts/fonts.js:122
(Diff revision 2)
>    },
>  
>    /**
> -   * Callback for the theme-switched event.
> +   * Handler for the "theme-switched" event.
>     */
> -  onThemeChanged: function (event, frame) {
> +  onThemeChanged(event, frame) {

I have found in my testing that we don't really have the theme-switched problem from commenting out this code. I believe we can remove all of this onThemeChanged code that was added as a result of https://bugzilla.mozilla.org/show_bug.cgi?id=1119149.

::: devtools/client/inspector/fonts/fonts.js:128
(Diff revision 2)
> -    if (frame === this.chromeDoc.defaultView) {
> -      this.update(this._lastUpdateShowedAllFonts);
> +    if (frame === this.document.defaultView) {
> +      this.update(this._lastUpdateShowedAllFonts, this._lastPreviewText);
>      }
>    },
>  
> -  /**
> +  update: Task.async(function* (showAllFonts, previewText) {

We should move the previewText and showAllFonts into the font redux store, and change how we do updates. Ideally, when we update the preview text in the react component, we call some like onPreviewTextChange, which will dispatch an action to update the store's previewText (string) or showAllFonts (boolean). We want to offload this to the redux store because we want to maintain the state in these stores.

::: devtools/client/inspector/inspector.js:636
(Diff revision 2)
>      if (Services.prefs.getBoolPref("devtools.fontinspector.enabled") &&
>          this.canGetUsedFontFaces) {
> -      this.sidebar.addExistingTab(
> -        "fontinspector",
> -        INSPECTOR_L10N.getStr("inspector.sidebar.fontInspectorTitle"),
> +      const FontInspector = this.browserRequire("devtools/client/inspector/fonts/fonts");
> +      this.fontinspector = new FontInspector(this, this.panelWin);
> +      this.fontinspector.init();
> -        defaultTab == "fontinspector");
>  
>        this.sidebar.toggleTab(true, "fontinspector");

We actually need to move this before the animationinspector is added to maintain the original tab order.
Attachment #8866460 - Flags: review?(gl)
Comment on attachment 8866460 [details]
Bug 1363097 - Convert font inspector to React/Redux.

https://reviewboard.mozilla.org/r/138090/#review142732

> I have found in my testing that we don't really have the theme-switched problem from commenting out this code. I believe we can remove all of this onThemeChanged code that was added as a result of https://bugzilla.mozilla.org/show_bug.cgi?id=1119149.

I found in my testing that the theme-switched problem does still exist (when you switch from light to dark theme, the previews aren't visible), and that the change to isPanelVisible() also broke onThemeChanged. It makes the update halt if you're not on the inspector, but to change the theme you need to be on the settings panel, so the colour of the preview doesn't update. If reverting the isPanelVisible change is OK, I can fix it.
Comment on attachment 8866460 [details]
Bug 1363097 - Convert font inspector to React/Redux.

https://reviewboard.mozilla.org/r/138090/#review142732

> Go back to calling this.init() here, but add a new line after the reference declarations prior to calling this.init().

Calling this.init() here causes an "Invariant Violation" React error. I thought it might have been because we also call init() in inspector.js, but the error still occurs with that commented out.
Comment on attachment 8866460 [details]
Bug 1363097 - Convert font inspector to React/Redux.

https://reviewboard.mozilla.org/r/138090/#review146520

::: devtools/client/inspector/fonts/components/App.js:38
(Diff revisions 2 - 3)
>    mixins: [ addons.PureRenderMixin ],
>  
>    componentDidMount() {
> +    let { onTextBoxContextMenu, inspector } = this.props;
> +
> +    inspector.selection.on("new-node-front", this.onNewNode);

I get this change, but I don't think we should be passing in the inspector object as a prop to the react components, and listening to these events in the component. Was there any particular problem that we encounter that required this changed?

::: devtools/client/inspector/fonts/components/App.js:56
(Diff revisions 2 - 3)
> -    searchInput.removeEventListener("contextmenu",
> +    searchInput.removeEventListener("contextmenu", onTextBoxContextMenu);
> -     this.props.onTextBoxContextMenu);
>    },
>  
> -  onFilterFonts(value) {
> -    let { update } = this.props;
> +  onPreviewFonts(value) {
> +    let { update, showAllFonts } = this.props;

Create a new function (onPreviewFonts) in fonts.js and pass it in as a prop and call this.props.onPreviewFonts(value). In onPreviewFonts we should call dispatch and update(). In update, we can fetch the state from the store annd not worry about passing all these props around.

::: devtools/client/inspector/fonts/components/App.js:61
(Diff revisions 2 - 3)
> -    let { update } = this.props;
> -    update(null, value);
> +    let { update, showAllFonts } = this.props;
> +    update(showAllFonts, value);
>    },
>  
>    onShowAllFontClick() {
> -    let { update } = this.props;
> +    let { update, previewText } = this.props;

Same as above.

::: devtools/client/inspector/fonts/fonts.js:110
(Diff revisions 2 - 3)
> -      this.update(this._lastUpdateShowedAllFonts, this._lastPreviewText);
> +      this.update(showAllFonts, previewText);
>      }
>    },
>  
>    update: Task.async(function* (showAllFonts, previewText) {
>      let node = this.inspector.selection.nodeFront;

We should just grab showAllFonts and previewText from the store here.

::: devtools/client/inspector/inspector.js:628
(Diff revision 3)
>  
>        const LayoutView = this.browserRequire("devtools/client/inspector/layout/layout");
>        this.layoutview = new LayoutView(this, this.panelWin);
>      }
>  
> +    if (Services.prefs.getBoolPref("devtools.fontinspector.enabled") &&

I think I messed up here and was looking at the wrong thing, but the font inspector should be last panel (after the animation inspector).
Attachment #8866460 - Flags: review?(gl)
Comment on attachment 8866460 [details]
Bug 1363097 - Convert font inspector to React/Redux.

https://reviewboard.mozilla.org/r/138090/#review162156
Attachment #8866460 - Flags: review?(gl) → review+
Attached patch 1363097.patch — — Splinter Review 

        Attachment #8866460 -
        Attachment is obsolete: true

        Attachment #8886286 -
        Flags: review+

    
    

    
  
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/010ba1c14577
Convert font inspector to React/Redux. r=gl

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/010ba1c14577
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago

          status-firefox56:
          --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

    
  
Product: Firefox → DevTools
Component: Inspector: Fonts → Inspector

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: