(In reply to Razvan Caliman [:rcaliman] from comment #4) > Comment on attachment 9038039 [details] [diff] [review] > 1521013.patch [3.0] > > Review of attachment 9038039 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/inspector/rules/components/Rules.js > @@ +10,5 @@ > > const Rule = createFactory(require("./Rule")); > > > > const Types = require("../types"); > > > > class Rules extends PureComponent { > > Do you think we still need the standalone Rules component? It seems to only > iterate through the given array to return a list of individual Rule > components, forwarding them the callbacks as props. > > Both the Rules (wrapper) and Rule (individual) components are used in > RulesApp. It looks like that's the only place where they're used. > > If you don't foresee any logic that needs to live in the Rules component, it > would simplify the code quite a bit if we drop this wrapper and just iterate > the array and generate individual Rule components right in the RulesApp. > > Moving this logic up the stack reduces some of the prop drilling too. > I wrote the Rules component initially because we needed to pass a component to the Accordion items. I added a new getComponentProps method to help with the duplicate prop declarations in RulesApp component. I will file a new bug to investigate whether or not we can remove the Rules component somehow. > ::: devtools/client/inspector/rules/models/element-style.js > @@ +363,5 @@ > > + }); > > + > > + // Recompute the list of applied styles because editing a > > + // selector might cause this rule's position to change. > > + const appliedStyles = await this.pageStyle.getApplied(this.element, { > > I was very confused for a while why `getApplied()` would return an array > when the actor implementation, which passes down the result of > getAppliedProps(), returns an object with multiple arrays: > > ``` > return { > entries: entries, > rules: [...rules], > sheets: [...sheets], > }; > ``` > https://searchfox.org/mozilla-central/rev/ > 330daedbeac2bba296d663668e0e0cf248bc6823/devtools/server/actors/styles. > js#762-766 > > This was especially confusing since the spec defines the same structure: > https://searchfox.org/mozilla-central/rev/ > 330daedbeac2bba296d663668e0e0cf248bc6823/devtools/shared/specs/styles.js#45- > 49 > > It turns out that the front overwrites the `getApplied()` implementation and > discards everything in that object aside from the `entries` array which it > returns directly: > https://searchfox.org/mozilla-central/rev/ > 330daedbeac2bba296d663668e0e0cf248bc6823/devtools/shared/fronts/styles.js#66- > 76 > > This change was done way back in 2016. > > There don't seem to be any other consumers of the method on the server. On > the client, the only consumers, via the overwritten front, are the Rule > view, Computed view and a few tests: > https://searchfox.org/mozilla-central/search?q=getApplied%28&path= > > > Given this information, what do you think about refactoring the server > method to return consistently and perahps avoid doing needless extra work to > get that data that's not actually used? > > This should be a separate bug, of course, but I'd like to get your insight > on this. Perhaps there are other use cases I'm not familiar with. But having > the actor, spec and front respect the same return signature would definitely > help others in the future. > Yes, we should definitely refactor this. It's also confusing to me too. I will file a new bug for this. > @@ +394,5 @@ > > + } > > + > > + this._changed(); > > + } catch (err) { > > + promiseWarn(err); > > Shouldn't we return this rejected promise? > Since an async function returns a promise already, we don't need to return promiseWarn(err). It's also an eslint error to do this. We could await promiseWarn(err), but looking at what promiseWarn did. I simplified it to simply console.error(err) since await Promise.reject(new Error("") is equivalent to throw new Error(""). https://javascript.info/async-await#error-handling > ::: devtools/client/inspector/rules/new-rules.js > @@ +197,5 @@ > > + * The selector's span element to show the inplace editor. > > + * @param {String} ruleId > > + * The id of the Rule to be modified. > > + */ > > + onShowSelectorEditor(element, ruleId) { > > This method name is slightly misleading and it's passed as a prop through > many layers of components. > > On a cursory read, it seems to be a callback _in response_ to showing the > selector editor when, in fact, it's a request to set it up. Perhaps > showSelectorEditor or setupSelectorEditor is less ambiguous. Renamed to showSelectorEditor.
Bug 1521013 Comment 5 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
(In reply to Razvan Caliman [:rcaliman] from comment #4) > Comment on attachment 9038039 [details] [diff] [review] > 1521013.patch [3.0] > > Review of attachment 9038039 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/inspector/rules/components/Rules.js > @@ +10,5 @@ > > const Rule = createFactory(require("./Rule")); > > > > const Types = require("../types"); > > > > class Rules extends PureComponent { > > Do you think we still need the standalone Rules component? It seems to only > iterate through the given array to return a list of individual Rule > components, forwarding them the callbacks as props. > > Both the Rules (wrapper) and Rule (individual) components are used in > RulesApp. It looks like that's the only place where they're used. > > If you don't foresee any logic that needs to live in the Rules component, it > would simplify the code quite a bit if we drop this wrapper and just iterate > the array and generate individual Rule components right in the RulesApp. > > Moving this logic up the stack reduces some of the prop drilling too. > I wrote the Rules component initially because we needed to pass a component to the Accordion items. I added a new getRuleProps method to help with the duplicate prop declarations in RulesApp component. I will file a new bug to investigate whether or not we can remove the Rules component somehow. > ::: devtools/client/inspector/rules/models/element-style.js > @@ +363,5 @@ > > + }); > > + > > + // Recompute the list of applied styles because editing a > > + // selector might cause this rule's position to change. > > + const appliedStyles = await this.pageStyle.getApplied(this.element, { > > I was very confused for a while why `getApplied()` would return an array > when the actor implementation, which passes down the result of > getAppliedProps(), returns an object with multiple arrays: > > ``` > return { > entries: entries, > rules: [...rules], > sheets: [...sheets], > }; > ``` > https://searchfox.org/mozilla-central/rev/ > 330daedbeac2bba296d663668e0e0cf248bc6823/devtools/server/actors/styles. > js#762-766 > > This was especially confusing since the spec defines the same structure: > https://searchfox.org/mozilla-central/rev/ > 330daedbeac2bba296d663668e0e0cf248bc6823/devtools/shared/specs/styles.js#45- > 49 > > It turns out that the front overwrites the `getApplied()` implementation and > discards everything in that object aside from the `entries` array which it > returns directly: > https://searchfox.org/mozilla-central/rev/ > 330daedbeac2bba296d663668e0e0cf248bc6823/devtools/shared/fronts/styles.js#66- > 76 > > This change was done way back in 2016. > > There don't seem to be any other consumers of the method on the server. On > the client, the only consumers, via the overwritten front, are the Rule > view, Computed view and a few tests: > https://searchfox.org/mozilla-central/search?q=getApplied%28&path= > > > Given this information, what do you think about refactoring the server > method to return consistently and perahps avoid doing needless extra work to > get that data that's not actually used? > > This should be a separate bug, of course, but I'd like to get your insight > on this. Perhaps there are other use cases I'm not familiar with. But having > the actor, spec and front respect the same return signature would definitely > help others in the future. > Yes, we should definitely refactor this. It's also confusing to me too. I will file a new bug for this. > @@ +394,5 @@ > > + } > > + > > + this._changed(); > > + } catch (err) { > > + promiseWarn(err); > > Shouldn't we return this rejected promise? > Since an async function returns a promise already, we don't need to return promiseWarn(err). It's also an eslint error to do this. We could await promiseWarn(err), but looking at what promiseWarn did. I simplified it to simply console.error(err) since await Promise.reject(new Error("") is equivalent to throw new Error(""). https://javascript.info/async-await#error-handling > ::: devtools/client/inspector/rules/new-rules.js > @@ +197,5 @@ > > + * The selector's span element to show the inplace editor. > > + * @param {String} ruleId > > + * The id of the Rule to be modified. > > + */ > > + onShowSelectorEditor(element, ruleId) { > > This method name is slightly misleading and it's passed as a prop through > many layers of components. > > On a cursory read, it seems to be a callback _in response_ to showing the > selector editor when, in fact, it's a request to set it up. Perhaps > showSelectorEditor or setupSelectorEditor is less ambiguous. Renamed to showSelectorEditor.