757253 - Implement real update in the rule view

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Dave Camp (:dcamp)]

Attachment: v1

The rule view doesn't have a real refresh method.  When we refresh for pseudo-class lock it's a complete destroy + rebuild, which can lose a small amount of user data (disabled properties, doubled-up properties, etc) as well as being a bit inefficient.

Attached patch implements a real refresh method for the rule view.

Asking for review from Rob because he did the initial rule view review, but if paul wants to steal the review I wouldn't complain.

Comment 1

[Paul Rouget [:paul]]

(I think you forgot to remove a couple of dump calls)

Comment 2

[Dave Camp (:dcamp)]

Indeed, will get rid of them in the next version of the patch.

Comment 3

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 625796 [details] [diff] [review]
v1

-    let rule = new Rule(this, aOptions);
+    let rule = null;
+
+    // If we're refreshing and the rule previously existed, reuse the
+    // Rule object.
+    for each (let r in (this._refreshRules || [])) {
+      if (r.matches(aOptions)) {
+        rule = r;
+        rule.refresh();
+        break;
+      }
+    }
+
+    // If this is a new rule, create its Rule object.
+    if (!rule) {
+      rule = new Rule(this, aOptions);
+    }

I might rework this a bit.

let rule = null;
for each (rule in (this._refreshRules || [])) {
  if (rule.matches(aOptions)) {
     rule.refresh();
     break;
  }
}

gets rid of an extra assignment, I think.

   /**
+   * Returns true if the rule matches the creation options
+   * specified.
+   */
+  matches: function Rule_matches(aOptions)
+  {
+    return (this.style === (aOptions.style || aOptions.domRule.style));
+  },

Could use an @param on your javadoc.

NIT in _getTextProperties:

in for each loop:

if(!matches || !matches[2])
         continue;
 
missing space.


+  _updateTextProperty: function Rule__updateTextProperty(aNewProp) {

could use an @param in your javadoc.

+      let rank = 1;

maybe add a comment // 1 for matching name

+      if (prop.enabled) {
+        rank += 1;
+      }

that's only worth 1? :)

After reading through this, I might score the rankings in your method comment:

+   *   Name, value, and priority match, enabled. (6)
+   *   Name, value, and priority match, disabled. (5)
+   *   Name and value match, enabled (4)
+   *   Name and value match, disabled (3)
+   *   Name matches, enabled (2)
+   *   Name matches, disabled. (1)

not sure if that helps or not.

in nodeChanged:
+    dump("Refresh took " + (end - start) + "ms\n");

one of the missed dumps, I presume. don't forget the start and end times.

Comment 4

[Dão Gottwald [:dao]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #3)
> Comment on attachment 625796 [details] [diff] [review]
> v1
> 
> -    let rule = new Rule(this, aOptions);
> +    let rule = null;
> +
> +    // If we're refreshing and the rule previously existed, reuse the
> +    // Rule object.
> +    for each (let r in (this._refreshRules || [])) {
> +      if (r.matches(aOptions)) {
> +        rule = r;
> +        rule.refresh();
> +        break;
> +      }
> +    }
> +
> +    // If this is a new rule, create its Rule object.
> +    if (!rule) {
> +      rule = new Rule(this, aOptions);
> +    }
> 
> I might rework this a bit.
> 
> let rule = null;
> for each (rule in (this._refreshRules || [])) {

Make this a for...of loop:
for (foo of (this._refreshRules || [])) {

Comment 5

[Dave Camp (:dcamp)]

Attachment: v2

Comment 6

[Dave Camp (:dcamp)]

(In reply to Rob Campbell [:rc] (:robcee) from comment #3)
> I might rework this a bit.
> 
> let rule = null;
> for each (rule in (this._refreshRules || [])) {
>   if (rule.matches(aOptions)) {
>      rule.refresh();
>      break;
>   }
> }
> 
> gets rid of an extra assignment, I think.

Yeah, but it leaves rule assigned if rule didn't match any rule.

I didn't come up with a more compact way to do this.

> +  matches: function Rule_matches(aOptions)
> Could use an @param on your javadoc.

Done.

> NIT in _getTextProperties:
> missing space.

Done.

> +  _updateTextProperty: function Rule__updateTextProperty(aNewProp) {
> could use an @param in your javadoc.

Done.

> +      let rank = 1;
> 
> maybe add a comment // 1 for matching name
> 
> +      if (prop.enabled) {
> +        rank += 1;
> +      }
> 
> that's only worth 1? :)

I beefed up inline comments about the ranking system.

> 
> After reading through this, I might score the rankings in your method
> comment:
> not sure if that helps or not.

I think it did, I added those.

> in nodeChanged:
> +    dump("Refresh took " + (end - start) + "ms\n");
> 
> one of the missed dumps, I presume. don't forget the start and end times.

Stripped out that dump and associated timers.

(In reply to Dão Gottwald [:dao] from comment #4)
> Make this a for...of loop:
> for (foo of (this._refreshRules || [])) {

Indeed, I did this throughout the patch.

Comment 7

[Dave Camp (:dcamp)]

https://tbpl.mozilla.org/?tree=Try&rev=2e9df75cda67

Comment 8

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/40462fc79a4b

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/40462fc79a4b