Last Comment Bug 699978 - Tools should notify the highlighter when they've modified a node
: Tools should notify the highlighter when they've modified a node
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: 10 Branch
: x86 Mac OS X
: -- normal (vote)
: Firefox 10
Assigned To: Dave Camp (:dcamp)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-04 17:29 PDT by Dave Camp (:dcamp)
Modified: 2011-11-07 07:15 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP 1 (5.29 KB, patch)
2011-11-04 17:29 PDT, Dave Camp (:dcamp)
rcampbell: feedback+
Details | Diff | Splinter Review
final v1 (17.14 KB, patch)
2011-11-05 17:36 PDT, Dave Camp (:dcamp)
rcampbell: review+
Details | Diff | Splinter Review
v2 (17.58 KB, patch)
2011-11-06 19:02 PST, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review

Description Dave Camp (:dcamp) 2011-11-04 17:29:42 PDT
Created attachment 572135 [details] [diff] [review]
WIP 1

... to let the highlighter and other tools know to update themselves after a change.
Comment 1 Rob Campbell [:rc] (:robcee) 2011-11-05 07:40:50 PDT
Comment on attachment 572135 [details] [diff] [review]
WIP 1

looks good. We'll need to add the onUpdate references and methods to each of the registration objects and tools (I think the only one that'll really need it is the style inspector).
Comment 2 Dave Camp (:dcamp) 2011-11-05 09:18:31 PDT
* Style inspector needs it (and should be easy, it already has refresh code)
* Rule View needs it (and will take a bit of code, in progress.  Will be needed if someone edits a style attribute in html view)
* HTML view needs it (when someone edits a style attribute in rule view).

Was planning to do each of these as followups, but we could do them all here.
Comment 3 Rob Campbell [:rc] (:robcee) 2011-11-05 12:44:29 PDT
your call, depending on the workload. Style inspector should be easy. just a matter of telling it to reselect the node.

Ruleview is baby, so you should know what needs doing.

HTML view is the trickiest of the bunch because of the ridiculous way the tree panel is constructed. Talked a bit about this in IRC, am willing to leave this as a followup.
Comment 4 Dave Camp (:dcamp) 2011-11-05 17:36:16 PDT
Created attachment 572242 [details] [diff] [review]
final v1

OK, this version makes sure the highlighter rectangle and the style inspector are updated on changes from the html panel or the rule view.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-11-06 13:39:21 PST
Comment on attachment 572242 [details] [diff] [review]
final v1

   /**
+   * Notify register tools of changes to the highlighted element.
+   * @param object aUpdater

probably should read "notify registered tools". That is a pretty nitty comment.

CssRuleView.jsm

+  /**
    * Creates editor UI for each of the rules in _elementStyle.
    */
   _createEditors: function CssRuleView_createEditors()
@@ -1003,10 +1034,12 @@ TextPropertyEditor.prototype = {
    */
   _parseValue: function TextPropertyEditor_parseValue(aValue)

should probably document aValue and @return

ok!
Comment 6 Dave Camp (:dcamp) 2011-11-06 18:36:28 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #5)
> Comment on attachment 572242 [details] [diff] [review] [diff] [details] [review]
> final v1
> 
>    /**
> +   * Notify register tools of changes to the highlighted element.
> +   * @param object aUpdater
> 
> probably should read "notify registered tools". That is a pretty nitty
> comment.
> 

Oops, updated.

>     */
>    _parseValue: function TextPropertyEditor_parseValue(aValue)
> 
> should probably document aValue and @return

Bad context, those are documented before the diff cutoff.
Comment 7 Dave Camp (:dcamp) 2011-11-06 19:02:33 PST
Created attachment 572376 [details] [diff] [review]
v2

Fixed review comments and a minor test mistake.
Comment 9 Rob Campbell [:rc] (:robcee) 2011-11-07 07:15:58 PST
https://hg.mozilla.org/mozilla-central/rev/815234795765

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