Closed Bug 1235172 Opened 4 years ago Closed 4 years ago

Modularize rule-view.js

Categories

(DevTools :: Inspector: Rules, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: gl, Assigned: gl)

References

Details

Attachments

(4 files, 6 obsolete files)

15.43 KB, patch
Details | Diff | Splinter Review
43.81 KB, patch
Details | Diff | Splinter Review
38.34 KB, patch
pbro
: review+
Details | Diff | Splinter Review
94.12 KB, patch
pbro
: review+
Details | Diff | Splinter Review
No description provided.
Depends on: 1235181
Attachment #8702801 - Flags: review?(pbrosset)
Attachment #8702802 - Flags: review?(pbrosset)
Attachment #8702803 - Flags: review?(pbrosset)
Comment on attachment 8702801 [details] [diff] [review]
Part 1: Extract TextProperty to a separate module

Review of attachment 8702801 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/rulesinspector/models/text-property.js
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const {Cc, Ci, Cu} = require("chrome");

Cc doesn't seem to be used in this module, no need to define it here.

@@ +211,5 @@
> +XPCOMUtils.defineLazyGetter(this, "domUtils", function() {
> +  return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
> +});
> +
> +exports.TextProperty = TextProperty;

We don't do this consistently throughout the code, but I think we generally put the exports line right after the constructor definition, so between the end of the TextProperty function and the start of the TextProperty prototype.
Attachment #8702801 - Flags: review?(pbrosset) → review+
Comment on attachment 8702802 [details] [diff] [review]
Part 2: Extract Rule to a separate module

Review of attachment 8702802 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/rulesinspector/models/rule.js
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const {Cc, Ci, Cu} = require("chrome");

Cc isn't used here either. Not a big deal, but I guess eslint complains about this, so we might as well just declare the dependencies we really need.
Attachment #8702802 - Flags: review?(pbrosset) → review+
Comment on attachment 8702803 [details] [diff] [review]
Part 3: Extract ElementStyle to a separate module

Review of attachment 8702803 [details] [diff] [review]:
-----------------------------------------------------------------

I have one comment about the gDummyPromise/dummyDocument/dummyElement logic that we should address before R+

::: devtools/client/rulesinspector/models/element-style.js
@@ +77,5 @@
> +      return;
> +    }
> +    this.destroyed = true;
> +
> +    gDummyPromise = null;

The previous 2 patches were just moving code around, but this one has this extra change in it which changes the way the rule-view works a little bit.
If I'm not mistaken, this will cause the rule-view to re-create a dummy document everytime a new element is selected:
- createDummyDocument is called by ElementStyle.init which is called every time an element is selected in the inspector
- createDummyDocument tries to early return gDummyPromise if it exists
- but with this change, gDummyPromise is set to null when ElementStyle is destroyed, so createDummyDocument will never early return and always re-create the document.

One way to fix this would be to keep the whole createDummyDocument logic in the rule-inspector.js module and make the ElementStyle constructor (or its init method) accept a new argument that would be the dummyDocument. This way, we can keep on reusing the same one over and over again and only destroy it when the rule-view gets destroyed.

Hm, in fact, I just realized that the dummyElement used to detect styles is only used in TextProperty. So I don't even know why ElementStyle should be the one responsible for it. We should move this code to where it really belongs. 
Related: this code should not exist at all, it's used to get style information, but this information may vary depending on which engine the page is running on, so this should really be something the actor returns, not something the client figures out on its own (it's even worse when using Valence for instance).

::: devtools/client/rulesinspector/rules-inspector.js
@@ +22,1 @@
>  const {Rule} = require("devtools/client/rulesinspector/models/rule");

Are you sure Rule is required in this module anymore? I did a very quick search and couldn't find a usage outside of element-style, but I might have missed it.
Attachment #8702803 - Flags: review?(pbrosset)
Attachment #8702801 - Attachment is obsolete: true
Attachment #8704437 - Flags: review+
Attachment #8702802 - Attachment is obsolete: true
Attachment #8704439 - Flags: review+
Attachment #8704439 - Attachment is obsolete: true
Attachment #8704441 - Flags: review+
Keywords: leave-open
Attachment #8704437 - Flags: checkin+
Attachment #8704441 - Flags: checkin+
Blocks: 1237441
Attachment #8706056 - Flags: review?(pbrosset) → review+
Attachment #8706056 - Flags: checkin+
Attachment #8708806 - Attachment is obsolete: true
Attachment #8708806 - Flags: review?(pbrosset)
Attachment #8708904 - Flags: review?(pbrosset)
Comment on attachment 8708904 [details] [diff] [review]
Part 4: Extract RuleEditor and TextPropertyEditor to a view module [1.0]

Review of attachment 8708904 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mechanical enough for a quick R+.
I like the direction this is taking, and TRY will be much more thorough than me to pick up problems in this type of change.
Attachment #8708904 - Flags: review?(pbrosset) → review+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/e472b30b08a4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS				Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.