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)
Other Applications
DOM Inspector
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
People
(Reporter: romain.guy, Assigned: romain.guy)
Details
Attachments
(1 file, 4 obsolete files)
15.14 KB,
patch
|
caillon
:
review-
|
Details | Diff | Splinter Review |
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.
Comment 1•21 years ago
|
||
Romain volunteered to take this bug.
Assignee: caillon → romain.guy
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 2•21 years ago
|
||
Assignee | ||
Comment 3•21 years ago
|
||
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.
Assignee | ||
Comment 4•21 years ago
|
||
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
Assignee | ||
Updated•21 years ago
|
Attachment #118174 -
Flags: review?(caillon)
Assignee | ||
Updated•21 years ago
|
Attachment #118175 -
Flags: review?(caillon)
Comment 5•21 years ago
|
||
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 → ---
Comment 6•21 years ago
|
||
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>
Assignee | ||
Comment 7•21 years ago
|
||
(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
Assignee | ||
Updated•21 years ago
|
Attachment #118174 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #118175 -
Attachment is obsolete: true
Assignee | ||
Comment 8•21 years ago
|
||
Fixes the issues raised by last comment.
Assignee | ||
Updated•21 years ago
|
Attachment #118380 -
Flags: superreview?(bzbarsky)
Attachment #118380 -
Flags: review?(caillon)
Assignee | ||
Updated•21 years ago
|
Attachment #118380 -
Attachment is obsolete: true
Assignee | ||
Comment 9•21 years ago
|
||
I changed the "color" in the CSS to "border-color". More consistent with CSS standards.
Assignee | ||
Updated•21 years ago
|
Attachment #118382 -
Flags: superreview?(bzbarsky)
Attachment #118382 -
Flags: review?(caillon)
Assignee | ||
Updated•21 years ago
|
Attachment #118382 -
Attachment is obsolete: true
Assignee | ||
Comment 10•21 years ago
|
||
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 :-)
Assignee | ||
Updated•21 years ago
|
Attachment #118383 -
Flags: superreview?(bzbarsky)
Attachment #118383 -
Flags: review?(caillon)
Updated•21 years ago
|
Attachment #118382 -
Flags: superreview?(bzbarsky)
Attachment #118382 -
Flags: review?(caillon)
Updated•21 years ago
|
Attachment #118380 -
Flags: superreview?(bzbarsky)
Attachment #118380 -
Flags: review?(caillon)
Comment 11•21 years ago
|
||
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....
Comment 12•21 years ago
|
||
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...
Comment 13•21 years ago
|
||
I've not gotten the module owner's approval to r= anything. Sorry.
Comment 14•21 years ago
|
||
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 15•21 years ago
|
||
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-
Updated•21 years ago
|
Attachment #118174 -
Flags: review?(caillon)
Updated•21 years ago
|
Attachment #118175 -
Flags: review?(caillon)
Updated•20 years ago
|
Product: Core → Other Applications
Comment 16•20 years ago
|
||
romain: Any progress?
Assignee | ||
Comment 17•20 years ago
|
||
> 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 :) -- ?
Assignee | ||
Comment 18•20 years ago
|
||
> 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.
Comment 19•20 years ago
|
||
> 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...
Assignee | ||
Comment 20•20 years ago
|
||
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.
Comment 21•18 years ago
|
||
do we still want this?
Updated•17 years ago
|
QA Contact: timeless → dom-inspector
Comment 22•6 years ago
|
||
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 ago → 6 years ago
Resolution: --- → INCOMPLETE
Comment 23•6 years ago
|
||
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.
Description
•