Last Comment Bug 757253 - Implement real update in the rule view
: Implement real update in the rule view
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: 13 Branch
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Dave Camp (:dcamp)
:
Mentors:
Depends on:
Blocks: 740543
  Show dependency treegraph
 
Reported: 2012-05-21 15:41 PDT by Dave Camp (:dcamp)
Modified: 2012-05-31 01:58 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (29.61 KB, patch)
2012-05-21 15:41 PDT, Dave Camp (:dcamp)
rcampbell: review+
Details | Diff | Review
v2 (30.07 KB, patch)
2012-05-30 13:32 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Review

Description Dave Camp (:dcamp) 2012-05-21 15:41:34 PDT
Created attachment 625796 [details] [diff] [review]
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] 2012-05-22 07:17:34 PDT
(I think you forgot to remove a couple of dump calls)
Comment 2 Dave Camp (:dcamp) 2012-05-22 08:15:07 PDT
Indeed, will get rid of them in the next version of the patch.
Comment 3 Rob Campbell [:rc] (:robcee) 2012-05-29 05:52:58 PDT
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] 2012-05-29 05:56:36 PDT
(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) 2012-05-30 13:32:14 PDT
Created attachment 628446 [details] [diff] [review]
v2
Comment 6 Dave Camp (:dcamp) 2012-05-30 13:35:42 PDT
(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) 2012-05-30 13:41:14 PDT
https://tbpl.mozilla.org/?tree=Try&rev=2e9df75cda67
Comment 9 Tim Taubert [:ttaubert] 2012-05-31 01:58:49 PDT
https://hg.mozilla.org/mozilla-central/rev/40462fc79a4b

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