Closed Bug 198342 Opened 21 years ago Closed 6 years ago

Selected element should flash in a different color, according to the element name.

Categories

(Other Applications :: DOM Inspector, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: romain.guy, Assigned: romain.guy)

Details

Attachments

(1 file, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312

When selecting an element in DOM Inspector, it is always flashing red. I'm
adding a feature to make it flash a different color according to the selected
element.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Romain volunteered to take this bug.
Assignee: caillon → romain.guy
Status: UNCONFIRMED → NEW
Ever confirmed: true
I did not manage to add this file to the patch. This is a new file in the tree
which goes into extensions/inspector/resources/content/res/userDOMInspector.css
. The new Makefile.in adds it into dist/bin/res/inspector. You're not obliged
to have this file for the patch to work (it will just do nothing). Yet, it will
avoid make errors.
The enhancement has been achieved. As C. Aillon suggested, it supports
namespaces and makes use of a CSS file.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Attachment #118174 - Flags: review?(caillon)
Attachment #118175 - Flags: review?(caillon)
since the patches haven't been reviewed yet, I'm assuming you marked it fixed on
accident...

it's not fixed until it's checked in.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
A few comments:

(1) The license is out-of-date.  See bug 112775; I made the same mistake.
http://www.mozilla.org/MPL/

(2) Copyright date shouldn't be 1998.

(3) Your diff patch adds the CSS file to the makefile.in; perhaps it should go
in the jar.mn file instead.

(4) Newlines at the ends of files, please.  This is something else I was chided
on while hacking on bug 112775.

(5) Are we really certain we want to base the styling on the element name? 
Consider:

<?xml version="1.0" encoding="UTF-8" ?>
<root>
  <img src="foo"/>
</root>
(1) Fixed

(2) Fixed

(3) The CSS file is not meant to be put in the JAR file

(4) Fixed

(5) Sure. The CSS is basically used as a king of properties storage file
(caillon pointed out that my former use of JS properties was bloating the prefs
registry and we came to that). This patch just loads the CSS, browse every rule
and add some stuffs in a hashtable. The CSS itself is *never* applied to the
document
Attachment #118174 - Attachment is obsolete: true
Attachment #118175 - Attachment is obsolete: true
Fixes the issues raised by last comment.
Attachment #118380 - Flags: superreview?(bzbarsky)
Attachment #118380 - Flags: review?(caillon)
Attachment #118380 - Attachment is obsolete: true
Attached patch Full patch. (obsolete) — Splinter Review
I changed the "color" in the CSS to "border-color". More consistent with CSS
standards.
Attachment #118382 - Flags: superreview?(bzbarsky)
Attachment #118382 - Flags: review?(caillon)
Attachment #118382 - Attachment is obsolete: true
Attached patch Full patch.Splinter Review
The latest patch used a shorthand properties to look up in the CSS file. This
is not allowed. Here is the fix (it also fixes a bad comment in CSS file). This
one should be the last :-)
Attachment #118383 - Flags: superreview?(bzbarsky)
Attachment #118383 - Flags: review?(caillon)
Attachment #118382 - Flags: superreview?(bzbarsky)
Attachment #118382 - Flags: review?(caillon)
Attachment #118380 - Flags: superreview?(bzbarsky)
Attachment #118380 - Flags: review?(caillon)
I just gave it a skim... My first reaction is, does this build on MacOSX?  We
had some issues with that before -- see nsIInspectorCSSUtils -- not sure whether
they're a problem here.

I'll try to review this within the next week....
I'm sorry I haven't gotten to this yet... real life has caught up to me.  :(

weirdal, timeless, caillon, if you could decide between the three of you who
wants to do the review and start on it, that would be great...
I've not gotten the module owner's approval to r= anything.  Sorry.
Comment on attachment 118383 [details] [diff] [review]
Full patch.

>Index: base/src/inFlasher.cpp
>@@ -58,11 +73,18 @@ inFlasher::inFlasher() :
>   mThickness(0),
>   mInvert(PR_FALSE)
> {
don't add a random space here:
>-  mCSSUtils = do_GetService(kInspectorCSSUtilsCID);
>+  mCSSUtils  = do_GetService(kInspectorCSSUtilsCID);
we now have nsTHashtable<nsStringHashKey, nscolor> which is probably better
>+  // Gfx : initialize Hashtable
>+  mColorsMap = new nsHashtable(PR_TRUE);
otherwise you need to check to see if new fails...
>+  LoadColorsMap();
> }

>@@ -226,8 +250,129 @@ inFlasher::ScrollElementIntoView(nsIDOME

>+nsresult inFlasher::LoadColorsMap()
>+{
>+  nsresult rv;
>+  
>+  nsCOMPtr<nsICSSLoader> CSSLoader;
>+  nsCOMPtr<nsICSSStyleSheet> aSheet = nsnull;
>+
you need to reference count this (use an nscomptr):
>+  nsIURI *uri = nsnull;
and check the return value:
>+  NS_NewURI(&uri, inFlasher::inFlasherCSS);
>+
and make sure CSSLoader isn't null (check rv):
>+  CSSLoader = do_CreateInstance(kCSSLoaderCID, &rv);
>+  rv = CSSLoader->LoadAgentSheet(uri, getter_AddRefs(aSheet));

can this fail:
>+  aSheet->GetNameSpace(*getter_AddRefs(sheetNS));

or this:
>+  aSheet->StyleRuleCount(rulesCount);

can this be null:
>+    nsCSSSelector* selector = cssRule->FirstSelector();
if so, you'd crash here:
>+    nsIAtom* elementName = selector->mTag;

>+    nsAutoString nameSpaceStr;
>+    sheetNS->FindNameSpacePrefix(selector->mNameSpace, *getter_AddRefs(nameSpace));
>+    if (nameSpace)
>+      nameSpace->ToString(nameSpaceStr);
this isn't needed:
>+    else
>+      nameSpaceStr = NS_LITERAL_STRING("");
it's the default value...

>+nsresult inFlasher::RegisterElementColor(const nsAString &aNameSpace,
>+                                         const nsAString &aElement,
>+                                         const nscolor aColor)
>+{
check to see if new fails:
>+  nscolor* elementColor = new nscolor;
otherwise you'd crash:
>+  *elementColor = aColor;

i think you could do nsAutoString aLoowerElement = aElement;
>+  nsAutoString aLowerElement;
>+  aLowerElement.Assign(aElement);

use .IsEmpty() instead of .Length()
>+  if (aNameSpace.Length() > 0)

>Index: base/src/inFlasher.h
following/subsequent, not 'next'
>+  // Gfx : two next declarations for element context color blinking
>+  // Gfx : two next lines for element context color blinking
Comment on attachment 118383 [details] [diff] [review]
Full patch.

Sorry it took so long for me to review this...	 Anyway....  Here are some
comments.  Please address them and attach a new patch.	I will try and be
quicker next time.


>Index: base/public/inIFlasher.idl
>===================================================================

>@@ -50,8 +50,8 @@ interface inIFlasher : nsISupports 
>   attribute boolean         invert;
>   attribute unsigned short  thickness;
> 
>+  void selectDrawingColor(in nsIDOMElement aElement);

I'm not sure I like this method being on this interface.  Rather than forcing
the callers to call this every time, can't we just add the necessary checks in
drawElementOutline?

>   void drawElementOutline(in nsIDOMElement aElement);
>   void repaintElement(in nsIDOMElement aElement);
>   void scrollElementIntoView(in nsIDOMElement aElement);
> };



>Index: base/src/inFlasher.cpp
>===================================================================

>@@ -47,10 +48,24 @@
> #include "nsIPresShell.h"
> #include "nsIFrame.h"
> 
>+// Gfx : interfaces includes


Comments like the above are IMO useless, please remove them.  Obviously they
are interface includes.  (And no idea where the "Gfx" portion is coming from. 
This is not gfx code).


>+#include "nsILocalFile.h"
>+#include "nsICSSLoader.h"
>+#include "nsICSSStyleSheet.h"
>+#include "nsICSSStyleRule.h"
>+#include "nsINameSpace.h"
>+// Gfx : utilities includes
>+#include "nsNetUtil.h"
>+#include "nsContentCID.h"
>+
> #include "prprf.h"
> 
>+static NS_DEFINE_CID(kCSSLoaderCID, NS_CSS_LOADER_CID);


Please avoid use of static CIDs if possible.  Just pass the contractid
(preferably) or CID directly to getService.


> static NS_DEFINE_CID(kInspectorCSSUtilsCID, NS_INSPECTORCSSUTILS_CID);
> 
>+// Gfx : URI to userDOMInspector.css
>+const char inFlasher::inFlasherCSS[] = "resource:///res/inspector/userDOMInspector.css";
>+
> ///////////////////////////////////////////////////////////////////////////////
> 
> inFlasher::inFlasher() :
>@@ -58,11 +73,18 @@ inFlasher::inFlasher() :
>   mThickness(0),
>   mInvert(PR_FALSE)
> {
>-  mCSSUtils = do_GetService(kInspectorCSSUtilsCID);
>+  mCSSUtils  = do_GetService(kInspectorCSSUtilsCID);
>+  // Gfx : initialize Hashtable
>+  mColorsMap = new nsHashtable(PR_TRUE);

nsHashtable is deprecated.  Please use one of the new hashtables (ask bsmedberg
on #mozilla).  Also, what happens if we're out of memory?

>+  LoadColorsMap();
> }
> 
>+// Gfx : added some cleanup for the color map


What's with the Gfx prefix?  Please get rid of that.  Also, you do not need to
comment every single change you make.  That is what cvs is for.


> inFlasher::~inFlasher()
> {
>+  // Gfx : XXX : goes through every single value
>+  // and delete it ?
>+  delete mColorsMap;
> }
> 
> NS_IMPL_ISUPPORTS1(inFlasher, inIFlasher);
>@@ -92,12 +114,14 @@ inFlasher::SetColor(const nsAString& aCo
> 
>   if (colorStr.CharAt(0) != '#') {
>     if (NS_ColorNameToRGB(colorStr, &mColor)) {
>+      inFlasher::mOriginalColor = mColor;


Just say |mOriginalColor = mColor|.  Same below.


>       return NS_OK;
>     }
>   }
>   else {
>     colorStr.Cut(0, 1);
>     if (NS_HexToRGB(colorStr, &mColor)) {
>+      inFlasher::mOriginalColor = mColor;
>       return NS_OK;
>     }
>   }
>@@ -226,8 +250,129 @@ inFlasher::ScrollElementIntoView(nsIDOME
>   return NS_OK;
> }
> 
>+// Gfx : loads the color map from a CSS file

Again, not sure about the Gfx but documentation about methods don't belong in
the .cpp.  They belong in either the .h or the .idl (use the former here since
this is not an interface method).

>+nsresult inFlasher::LoadColorsMap()

Please follow convention and put the return type on its own line, e.g:

nsresult
inFlasher::LoadColorsMap()


>+{
>+  nsresult rv;
>+  
>+  nsCOMPtr<nsICSSLoader> CSSLoader;
>+  nsCOMPtr<nsICSSStyleSheet> aSheet = nsnull;

nsCOMPtrs don't need to be initialized to null.

>+
>+  nsIURI *uri = nsnull;

Use an nsCOMPtr here.

>+  NS_NewURI(&uri, inFlasher::inFlasherCSS);
>+
>+  CSSLoader = do_CreateInstance(kCSSLoaderCID, &rv);

Naming convention would dictate this variable is named |cssLoader|.  Also why
bother getting the rv if you're not going to check it?	what if creating the
loader fails?  We'd crash on the next line.  please just check for |if
(cssLoader)|

>+  rv = CSSLoader->LoadAgentSheet(uri, getter_AddRefs(aSheet));
>+
>+  // if we can't find the resource file, just get outta there
>+  if (!aSheet)
>+    return NS_OK;
>+
>+  // Gfx : namespaces handling
>+  nsCOMPtr<nsINameSpace> sheetNS;
>+  aSheet->GetNameSpace(*getter_AddRefs(sheetNS));
>+  nsCOMPtr<nsIAtom> nameSpace;
>+
>+  nsICSSRule* aRule;
>+  PRInt32 rulesCount;
>+  aSheet->StyleRuleCount(rulesCount);
>+
>+  for (PRInt32 i = 0; i < rulesCount; i++)

PRUint32 (unsigned) and your iteration should be |++i|

>+  {
>+    // Gfx : get the rule
>+    aSheet->GetStyleRuleAt(i, aRule);
>+
>+    // Gfx : turn the rule into a usable form
>+    nsCOMPtr<nsICSSStyleRule> cssRule = do_QueryInterface(aRule);
>+    if (!cssRule) // Gfx : it might be a nsICSSNameSpaceRule

And poof like that, we leak an nsICSSRule.  In general, please use nsCOMPtr for
every interface.  It exists to avoid errors like this.

>+      continue;
>+
>+    // Gfx : get the tag name
>+    nsCSSSelector* selector = cssRule->FirstSelector();
>+    nsIAtom* elementName = selector->mTag;
>+    nsAutoString elementNameStr;
>+    elementName->ToString(elementNameStr);
>+
>+    // Gfx : get the namespace
>+    nsAutoString nameSpaceStr;
>+    sheetNS->FindNameSpacePrefix(selector->mNameSpace, *getter_AddRefs(nameSpace));
>+    if (nameSpace)
>+      nameSpace->ToString(nameSpaceStr);
>+    else
>+      nameSpaceStr = NS_LITERAL_STRING("");
>+
>+    // Gfx : get the color property value
>+    nsCSSProperty prop;
>+    mCSSUtils->LookupCSSProperty(NS_LITERAL_STRING("border-top-color"), &prop);
>+    nsCSSValue value;
>+    cssRule->GetValue(prop, value);
>+
>+    if (value.GetUnit() == eCSSUnit_Color)
>+      RegisterElementColor(nameSpaceStr, elementNameStr, value.GetColorValue());
>+  }
>+
>+  return NS_OK;
>+}
>+
>+// Gfx : registers a color for a particular element
>+nsresult inFlasher::RegisterElementColor(const nsAString &aNameSpace,
>+                                         const nsAString &aElement,
>+                                         const nscolor aColor)
>+{
>+  nscolor* elementColor = new nscolor;
>+  *elementColor = aColor;

Crash if OOM.  Not good.

>+
>+  // Gfx : the key is stored lower case, namespace:elementName
>+  nsAutoString aLowerElement;
>+  aLowerElement.Assign(aElement);
>+  ToLowerCase(aLowerElement);
>+
>+  // Gfx : if we have a namescape, key is namespace:elementName
>+  if (aNameSpace.Length() > 0)

check for !IsEmpty() instead.

>+  {
>+    nsAutoString aLowerNameSpace;
>+    aLowerNameSpace.Assign(aNameSpace);
>+    ToLowerCase(aLowerNameSpace);
>+
>+    nsStringKey key(aLowerNameSpace + NS_LITERAL_STRING(":") + aLowerElement);
>+    this->mColorsMap->Put(&key, elementColor);
>+
>+  // Gfx : if we have no namespace, key is elementName
>+  } else {
>+    nsStringKey key(aLowerElement);
>+    this->mColorsMap->Put(&key, elementColor);
>+  }
>+
>+  return NS_OK;
>+}
>+
> ///////////////////////////////////////////////////////////////////////////////
> // inFlasher
>+
>+// Gfx : selects the most appropriate color for the element to flash
>+NS_IMETHODIMP
>+inFlasher::SelectDrawingColor(nsIDOMElement* aElement)
>+{
>+  // Gfx : so far we have the element to be flashed tag name
>+  nsAutoString aElementTagName;
>+  aElement->GetTagName(aElementTagName);
>+
>+  ToLowerCase(aElementTagName);
>+  nsStringKey key(aElementTagName);
>+
>+  if (this->mColorsMap->Exists(&key))
>+  {
>+    nscolor* elementColor = NS_STATIC_CAST(nscolor*, this->mColorsMap->Get(&key));
>+
>+    // Gfx : we avoid going through SetColor
>+    this->mColor = *elementColor;
>+
>+  // Gfx : we restore the original color if no match is found in the colors map
>+  } else
>+    this->mColor = inFlasher::mOriginalColor;

I find this syntax terribly disgusting.  If you have an if() with braces, then
your else must have them also.


>+
>+  return NS_OK;
>+}
> 
> void
> inFlasher::DrawOutline(nscoord aX, nscoord aY, nscoord aWidth, nscoord aHeight,
>Index: base/src/inFlasher.h
>===================================================================

>@@ -62,7 +63,18 @@ public:
>   inFlasher();
>   virtual ~inFlasher();
> 
>+public:
>+  /// STATIC MEMBERS ///

Again, the above comment is redundant.	If someone is too stupid to figure out
that the members that have "static" in front of them are static, then they
don't need to be looking at this code.	But more importantly....

>+  // Gfx : URI to userDOMInspector.css
>+  static const char inFlasherCSS[];

Does this really need to be a member?  It doesn't appear that anyone will ever
need access to it from outside the .cpp.  Make it a static global there
perhaps?  Or at least make it a private member if you're concerned about the
global namespace.  (And for whatever reason, I prefer |char* foo| as opposed to
|char foo[]|.  Please appease me :-)



>+
> protected:
>+  // Gfx : two next declarations for element context color blinking
>+  nsresult LoadColorsMap();
>+  nsresult RegisterElementColor(const nsAString &aNameSpace,
>+                                const nsAString &aElement,
>+                                const nscolor elementColor);
>+
>   void DrawOutline(nscoord aX, nscoord aY, nscoord aWidth, nscoord aHeight,
>                    float aP2T, nsIRenderingContext* aRenderContext);
>   void DrawLine(nscoord aX, nscoord aY, nscoord aLength,
>@@ -72,6 +84,10 @@ protected:
>   nsCOMPtr<nsIInspectorCSSUtils> mCSSUtils;
> 
>   nscolor mColor;
>+
>+  // Gfx : two next lines for element context color blinking
>+  nscolor mOriginalColor;
>+  nsHashtable* mColorsMap;
> 
>   PRUint16 mThickness;
>   PRPackedBool mInvert;
>Index: resources/content/Flasher.js
>===================================================================

>@@ -119,6 +119,12 @@ Flasher.prototype = 
>     this.mRegistryId = length;
>   },
> 
>+  // Gfx : registers a color for a given element
>+  registerElementColor: function(aElement, aColor)
>+  {
>+    this.mShell.registerElementColor(aElement, aColor);

Errm, how does this work?  This is not on the interface....

>+  },
>+
>   start: function(aDuration, aSpeed, aHold)
>   {
>     this.mUDuration = aDuration ? aDuration*1000 : this.mDuration;
>@@ -126,6 +132,8 @@ Flasher.prototype = 
>     this.mHold = aHold;
>     this.mFlashes = 0;
>     this.mStartTime = new Date();
>+    // Gfx : selects the most appropriate color for current flashing element
>+    this.mShell.selectDrawingColor(this.mElement);
>     this.doFlash();
>   },
> 

>Index: resources/content/res/userDOMInspector.css
>===================================================================

>+/* end of file */

Obviously.  Please remove that comment.  It's useless.



General comments:


- Please use

if (foo) {
  bar;
}
else {
  baz;
}

everywhere as opposed to 

if (foo)
{
  bar;
}
else
{
  baz;
}
Attachment #118383 - Flags: superreview?(bz-bugspam)
Attachment #118383 - Flags: review?(caillon)
Attachment #118383 - Flags: review-
Product: Core → Other Applications
romain: Any progress?
> romain: Any progress?

I'm ashamed to say that I've postponed my work on this patch again and again. If
the patch is still of interest I can get back to it and address all the comments
that have been made. Could you just tell me how to check wether a "new"
instruction failed or not -- I rarely use C++ (that is clear I guess when one
looks at my code :) -- ?
> romain: Any progress?

I check my hard disk and I found that the changes are almost complete. I just
need to add some memory allocations tests and switch from nsHashtable to
nsTHashtable. I'll do this ASAP.
> Could you just tell me how to check wether a "new" instruction failed or not

just check if (your_var), like:

  foo* x = new foo;
  if (!x) {
    // handle this error...
  }

that only works on windows really and is not what the C++ standard says, but
that's a mozilla-wide problem...
I have a problem to complete the patch. My first version of the patch was using
this code to get the value of a nsCSSStyleRule retrieved from the stylesheet :

nsCSSProperty prop;
mCSSUtils->LookupCSSProperty(NS_LITERAL_STRING("border-top-color"), &prop);
nsCSSValue value;
cssRule->GetValue(prop, value);

Unfortunately this cannot work anymore since GetValue() was removed from
nsICSSStyleRule. I have been trying to get around this by doing this :

nsAutoString value;
nsCSSDeclaration* decl = cssRule->GetDeclaration();
if (decl)
  decl->GetValue(prop, value);

Unfortunately this cannot compile. The method GetValue() is said to be
undeclared. I've also tried with GetPropertyValue() but it did not work either.
Do you have any idea of how to achieve this ? I've changed the code to take all
the comments in account and I now just need this to commit a new version.

Thanks.
do we still want this?
QA Contact: timeless → dom-inspector
Bulk close. This component is no longer supported or maintained.

https://bugzilla.mozilla.org/show_bug.cgi?id=1499023
Status: REOPENED → RESOLVED
Closed: 21 years ago6 years ago
Resolution: --- → INCOMPLETE
Bulk close. This component is no longer supported or maintained.

https://bugzilla.mozilla.org/show_bug.cgi?id=1499023
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: