Closed Bug 501178 Opened 15 years ago Closed 14 years ago

style sheets viewer doesn't handle HTMLStyleElements well and incorrectly implements nsITreeView::hasNextSibling

Categories

(Other Applications :: DOM Inspector, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: crussell, Assigned: crussell)

References

Details

Attachments

(1 file, 8 obsolete files)

27.20 KB, patch
crussell
: review+
crussell
: superreview+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11) Gecko/2009060310 Ubuntu/8.10 (intrepid) Firefox/3.0.11
Build Identifier: dom_inspector-2.0.3-fx+tb+sb+sm+fn

Two issues here:
1. If the style sheet originates from the content of a style element rather than a linked style sheet, the cell text denoting that file's URI is blank.

2. The the hasNextSibling method must take a parameter specifying the index after which to begin searching. Currently, it just walks down from the row index until it finds a row at the same depth or less:
> function(aRow) {
>   var baseLevel = this.mLevels[aRow];
>   for (var i = aRow+1; i < this.mRowCount; ++i) {
>     if (this.mLevels[i] < baseLevel)
>       break;
>     if (this.mLevels[i] == baseLevel)
>       return true;
>   }
>   return false;
> }

This will give incorrect return values for the following scenario and similar
> I
>    A  <-row
>    B
> II
> III   <-after index

Right now, there's only one place in native code that calls this method, and it's moot because it only happens when tree lines are drawn (a la treechildren::-moz-tree-line) <http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#3256>, but relying on this is akin to relying on undefined behavior.

Reproducible: Always
Switched anonymous functions to named functions. Fixed case differences between StylesheetsViewer and StyleSheetsView for consistency, then changed viewer name from Stylesheets to Style Sheets for continued consistency.
Attachment #385949 - Flags: review?(sdwilsh)
I'm not sure what the policy is on riders in patches, but considering my apparent intent for the scope of this bug was minor fixes to the Style Sheets viewer nsITreeView implementation, I don't feel bad about including this minor fix; I've already got my hands all over this file, and the overhead for a separate bug feels to me excessive.

This bug fixes the defective |toggleOpenState| behavior exhibited when @import rules follow @charset rules.[1] This results in the children being inserted past the known bounds of the treeview as determined by |rowCount|. The rationale, I suppose, was that any @import rules will appear at the beginning of the style sheet, which is generally the case. However, the spec allows for @charset rules to precede @import rules.[2][3]

[1] See http://craigmod.com/journal/font-face/ for the in-the-wild example that alerted me of this behavior.
[2] So says CSS 2.1: http://www.w3.org/TR/CSS21/cascade.html#at-import
[3] CSS 2 supports this allowance as well (and theoretically even allows @font-face, @page, and empty @media rules to precede any @imports): http://www.w3.org/TR/1998/REC-CSS2-19980512/syndata.html#at-rules
Attachment #385949 - Attachment is obsolete: true
Attachment #386837 - Flags: review?(sdwilsh)
Attachment #385949 - Flags: review?(sdwilsh)
Comment on attachment 386837 [details] [diff] [review]
fix other defective treeview behavior regarding @import rules

>-//////////// global variables /////////////////////
>+//////////// global variables ////////////////////////////////////////////////
If you are going to change these, I'd prefer to see them like this:

///////[continued up until you hit the 80 char limit]
//// Heading Name in Title Format

general nit: use let instead of var for for-loop variables

>-<!ENTITY styleLocation.label "Stylesheet">
>+<!ENTITY styleLocation.label "Style Sheet">
You need to change the entity name here so localizers know to update the string as well.

r=sdwilsh with these changes.  You'll have to get sr from Neil.
Attachment #386837 - Flags: review?(sdwilsh) → review+
Attached patch fixes for review (obsolete) — Splinter Review
Attachment #386837 - Attachment is obsolete: true
Attachment #390506 - Flags: review?(sdwilsh)
Attachment #390506 - Flags: review?(sdwilsh) → review+
Attachment #390506 - Flags: superreview?(neil)
Comment on attachment 390506 [details] [diff] [review]
fixes for review

>+* StyleSheetsViewer ------------------------------------------------------------
> *  The viewer for the stylesheets loaded by a document.
You changed all the other occurrences of style sheets to be two words...

>-  for (var i = 0; i < ss.length; ++i)
>+  for (let i = 0; i < ss.length; ++i)
[Normally I wouldn't bother changing unrelated lines]

> StyleSheetsView.prototype.shiftDataUp = 
>-function(aRow, aDiff)
> {
>-  for (var i = aRow; i < this.mRowCount; ++i) {
>-    this.mSheets[i] = this.mSheets[i+aDiff];
>-    this.mLevels[i] = this.mLevels[i+aDiff];
>-    this.mOpen[i] = this.mOpen[i+aDiff];
>-    this.mChildCount[i] = this.mChildCount[i+aDiff];
>   }
> }
[Eww, what ugly code. While I'd consider using splice to remove the unwanted data I'm not sure what the best way to insert multiple rows is]

> StyleSheetsView.prototype.hasNextSibling = 
>-function(aRow) 
>+function SSV_HasNextSibling(aRow, aAfter) 
> {
>   var baseLevel = this.mLevels[aRow];
>-  for (var i = aRow+1; i < this.mRowCount; ++i) {
>+  for (let i = aRow + 1; i < this.mRowCount; ++i) {
>     if (this.mLevels[i] < baseLevel)
>       break;
>-    if (this.mLevels[i] == baseLevel)
>+    if (i > aAfter && this.mLevels[i] == baseLevel)
>       return true;
>   }
>   return false;
The correct thing to do here is to start checking at aAfter + 1 because the contact guarantees that there is no sibling between aRow and aAfter.

>+        this.insertSheet(rules[i].styleSheet, this.mLevels[aRow] + 1,
>+                         aRow + changeCount + 1);
>+        ++changeCount;
[Could increment changeCount first to avoid adding 1]

>-  this.mTree.rowCountChanged(aRow+1, this.mRowCount - oldRowCount);
>+  this.mTree.rowCountChanged(aRow + 1, this.mRowCount - oldRowCount);
[I wonder why we bother tracking the old count when we know how many rows we're adding or removing]

>diff -r 51c6d483a4c1 resources/locale/en-US/viewers/stylesheets.dtd
You need to either fix the other locales or remove them from the Makefile.
Attachment #390506 - Flags: superreview?(neil) → superreview-
Comment on attachment 390506 [details] [diff] [review]
fixes for review

Oh, and some of the files you edited ended in spaces; it would be nice if you could remove them at the same time.
(In reply to comment #5)
> (From update of attachment 390506 [details] [diff] [review])
> ...
> You changed all the other occurrences of style sheets to be two words...
Done, and changed the other occurrence of "stylesheets"--the viewer's uid--necessitating renames.

> [Normally I wouldn't bother changing unrelated lines]

Was this just a general comment ("Normally I wouldn't..."), or did you want the patch changed as well? This means inconsistent code style.

> The correct thing to do here is to start checking at aAfter + 1 because the
> contact guarantees that there is no sibling between aRow and aAfter.
 
Ah, rereading the description, I see now that the scenario outlined in comment 1 is never possible, although it would be nice if that were made clearer in the idl.

> > StyleSheetsView.prototype.shiftDataUp = 
> >-function(aRow, aDiff)
> > {
> >-  for (var i = aRow; i < this.mRowCount; ++i) {
> >-    this.mSheets[i] = this.mSheets[i+aDiff];
> >-    this.mLevels[i] = this.mLevels[i+aDiff];
> >-    this.mOpen[i] = this.mOpen[i+aDiff];
> >-    this.mChildCount[i] = this.mChildCount[i+aDiff];
> >   }
> > }
> [Eww, what ugly code. While I'd consider using splice to remove the unwanted
> data I'm not sure what the best way to insert multiple rows is]

When working on bug 192841, this was my initial reaction as well and set out to change it. Then I ran into a problem when I realized splice takes each new element as a separate parameter, and won't accept an array. Now I feel like an idiot for not having thought of Array.prototype.apply.

If I'm reading Venkman's profiler correctly, splicing with native code results in a slight speedup. But this requires auxiliary space. It's not a big deal, since @import seems to be little-used, not to mention the scarcity of more than, say, half a dozen sibling imports. All in all, I don't think the alternative code is much nicer--redundantly initializing inserts's properties to the first arguments to splice feels especially gross.

> [Could increment changeCount first to avoid adding 1]
> ...
> [I wonder why we bother tracking the old count when we know how many rows we're
> adding or removing]

Heh, oops. Although this is made irrelevant by the changes to use splice.
Attachment #390506 - Attachment is obsolete: true
Attachment #390753 - Flags: superreview?(neil)
"Comment 1" in the above post should be read as "initial bug".
Comment on attachment 390753 [details] [diff] [review]
changes to meet initial superreview

>-  content/inspector/viewers/stylesheets/stylesheets.js                (resources/content/viewers/stylesheets/stylesheets.js)
>-  content/inspector/viewers/stylesheets/stylesheets.xul               (resources/content/viewers/stylesheets/stylesheets.xul)
>+  content/inspector/viewers/styleSheets/styleSheets.js                (resources/content/viewers/styleSheets/styleSheets.js)
>+  content/inspector/viewers/styleSheets/styleSheets.xul               (resources/content/viewers/styleSheets/styleSheets.xul)
Users of case-insensitive filesystems are going to hate you for this (including me, as I can't actually apply the patch locally...)
Comment on attachment 390753 [details] [diff] [review]
changes to meet initial superreview

>+    var inserts = { mSheets: [aRow + 1, 0],
>+                    mLevels: [aRow + 1, 0],
>+                    mOpen: [aRow + 1, 0],
>+                    mChildCount: [aRow + 1, 0],
>+                    mRowCount: 0 };
>+    for (let i = 0, j = 2; i < rules.length; ++i) {
>+      if (rules[i].type == CSSRule.IMPORT_RULE) {
>+        this.insertSheet(rules[i].styleSheet, level, j, inserts);
No, it isn't much nicer! How about splicing the new sheets in one at a time?

>-<!ENTITY styleLocation.label "Style Sheet">
>-<!ENTITY styleRules.label "Regeln">
>+<!ENTITY sheetLocation.label "Style Sheet">
>+<!ENTITY sheetRules.label "Regeln">
While some of your locale changes, such as this one, look unquestionably correct, I don't see how I can be sure that all of the others are...
(In reply to comment #9)
> (From update of attachment 390753 [details] [diff] [review])
> >-  content/inspector/viewers/stylesheets/stylesheets.js                (resources/content/viewers/stylesheets/stylesheets.js)
> >-  content/inspector/viewers/stylesheets/stylesheets.xul               (resources/content/viewers/stylesheets/stylesheets.xul)
> >+  content/inspector/viewers/styleSheets/styleSheets.js                (resources/content/viewers/styleSheets/styleSheets.js)
> >+  content/inspector/viewers/styleSheets/styleSheets.xul               (resources/content/viewers/styleSheets/styleSheets.xul)
> Users of case-insensitive filesystems are going to hate you for this (including
> me, as I can't actually apply the patch locally...)

So. . ? I used git diffing on this one for readability, otherwise, the changes to a file are overshadowed by the fact that it shows all lines as having been removed from the old file and all lines being inserted in the new file. git diffs show the file as renamed and still uses the old file for the context. I can submit git-style diffs for readability and a supplementary diff using hg's default diff, so long as I twiddle the sections to make sure the deletions come before the creations, I think, and that's the one that should be applied to the tree.

(In reply to comment #10)
> (From update of attachment 390753 [details] [diff] [review])
> >+    var inserts = { mSheets: [aRow + 1, 0],
> >+                    mLevels: [aRow + 1, 0],
> >+                    mOpen: [aRow + 1, 0],
> >+                    mChildCount: [aRow + 1, 0],
> >+                    mRowCount: 0 };
> >+    for (let i = 0, j = 2; i < rules.length; ++i) {
> >+      if (rules[i].type == CSSRule.IMPORT_RULE) {
> >+        this.insertSheet(rules[i].styleSheet, level, j, inserts);
> No, it isn't much nicer! How about splicing the new sheets in one at a time?

That would mean a ridiculous amount of data movements, something like O(n^2).

I'd be for keeping the for loop used today, although only for row additions. We should inline the shiftDataDown function since toggleOpenState is the only caller and I favor small call stacks. The for loop approach still has the least number of data movements and doesn't require the auxiliary space splice does; granted, splice uses native code, although with TraceMonkey this should matter less, and I think initializing inserts with splice arguments is incredibly ugly.

> >-<!ENTITY styleLocation.label "Style Sheet">
> >-<!ENTITY styleRules.label "Regeln">
> >+<!ENTITY sheetLocation.label "Style Sheet">
> >+<!ENTITY sheetRules.label "Regeln">
> While some of your locale changes, such as this one, look unquestionably
> correct, I don't see how I can be sure that all of the others are...

Heh. To preface this, for sdwilsh's review, I intended to point out that the change I made to the locale string ("Stylesheet" to "Style Sheet") was issue specific to that localization and therefore the entity name need not be changed to get the attention of localizers. But I just changed the names anyway. Then for your review, I decided to fix up the other locale's entity names by that rationale, because having changed the names, I found the new ones to be more logical anyway. I didn't change any of the strings, except for Greek, which fell back on the English "Stylesheet". German used English as a fallback as well, but already used "Style Sheet" (per CSS spec).

So that brings us here. I could just remove the other locales from the makefile, but I really don't think it's necessary.
Attachment #390753 - Flags: superreview?(neil)
Attachment #390753 - Attachment is obsolete: true
Comment on attachment 391017 [details] [diff] [review]
back to original for loop for toggleOpenState (use attachment 391018 [details] [diff] [review] for checkin to be friendly to case insensitive file systems)

OK, I'm convinced.
Attachment #391017 - Flags: superreview?(neil) → superreview+
Status: UNCONFIRMED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #391017 - Attachment description: back to original for loop for toggleOpenState → back to original for loop for toggleOpenState (use attachment 391018 for checkin to be friendly to case insensitive file systems)
Attachment #391017 - Attachment is obsolete: true
Are you sure the checkin should be done with the delete+create patch?
On my Windows, it looks like hg merges them into a simple diff ... and do not "rename" the files :-/

NB: You should not mark a bug as fixed until patches have been checked in.
Assignee: nobody → Sevenspade
Version: unspecified → Trunk
(In reply to comment #15)
> On my Windows, it looks like hg merges them into a simple diff ... and do not
> "rename" the files :-/

What do you mean? I'm on IRC as Sevenspade.

> NB: You should not mark a bug as fixed until patches have been checked in.

Thanks. Couldn't really tell from the Bugzilla help page.
Status: RESOLVED → UNCONFIRMED
Resolution: FIXED → ---
If you are renaming files you should enable git mode in hg see the configuration section here:

https://developer.mozilla.org/en/Mercurial_FAQ#How_do_I_install_Mercurial.3f
The first patch is done using git diffs. In comment 9 neil indicated since the filenames only differ by case, those with case insensitive file systems won't be able to apply the patch, which is why I came up with this approach in comment 11. Is there a proper way to do this?
No way I can cleanly apply these patches on my Windows:
*rename: "destination already exists", even with keeping 'stylesheets' as directory name.
*delete+create: applies but does not "rename" the 'stylesheets' directory.

My best suggestion would be to use 2 renaming patches:
1- change the directory name,
2- just "restore" the wanted name.
What do you think?

PS: Should I report this as a bug in Mercurial?
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
from bug 192841:
> from ./.irclogs/mozilla/#developers/#developers-2009-07-28.log:15:57
> > < NeilAway> Enn: I happened to notice on DOMi's stylesheets display that the
> > twisty updates when it is clicked but not when you use left/right arrow even
> > though the tree expands/collapses, is that likely to be a tree issue or a
> > DOMi issue?
> 
> I copied this behavior into the CSS Rules tree view.

So there. sr?neil for this small fix.

> My best suggestion would be to use 2 renaming patches:
> 1- change the directory name,
> 2- just "restore" the wanted name.
> What do you think?

It won't make a difference. There is no way to make Mercurial remove the old directory with only a patch. However, I don't think this is a problem, since we put everything into jars anyway.

During the build, JarMaker.py opens styleSheets; this is fine since it just maps to the old stylesheets on case insensitive filesystems, so that's what's handed over; JarMaker.py then throws the contents of that file into a jar, mapped to the styleSheets path inside the archive; all is well.

However, I haven't tested this.
Attachment #391018 - Attachment is obsolete: true
Attachment #401547 - Flags: superreview?(neil)
Attachment #401547 - Flags: review+
Attachment #401547 - Flags: superreview?(neil) → superreview+
(In reply to comment #21)
> However, I don't think this is a problem, since we
> put everything into jars anyway.
> 
> During the build, JarMaker.py opens styleSheets; this is fine since it just
> maps to the old stylesheets on case insensitive filesystems, so that's what's
> handed over; JarMaker.py then throws the contents of that file into a jar,
> mapped to the styleSheets path inside the archive; all is well.
> 
> However, I haven't tested this.

In fact, it seems like this is exactly what happens. Testing on Windows throws up complaints, but it builds fine.
Attachment #401547 - Attachment is obsolete: true
Attachment #416314 - Flags: superreview+
Attachment #416314 - Flags: review+
(In reply to comment #22)
> Testing on Windows throws up complaints, but it builds fine.

Can you copy/attach some example complaints, ftr?
Couldn't we find a solution which would lead to no complaints?
> $ hg import ../../../501178-mkVIII-win.patch
> applying ../../../501178-mkVIII-win.patch
> resources/locale/da/viewers/styleSheets.dtd not tracked!
> resources/locale/ru/viewers/styleSheets.dtd not tracked!
> resources/content/viewers/styleSheets/styleSheets.js not tracked!
> resources/locale/zh-CN/viewers/styleSheets.dtd not tracked!
> resources/locale/pl/viewers/styleSheets.dtd not tracked!
> resources/locale/hu/viewers/styleSheets.dtd not tracked!
> resources/content/viewers/styleSheets/styleSheets.xul not tracked!
> resources/locale/zh-TW/viewers/styleSheets.dtd not tracked!
> resources/locale/fr/viewers/styleSheets.dtd not tracked!
> resources/locale/ga-IE/viewers/styleSheets.dtd not tracked!
> resources/locale/ca/viewers/styleSheets.dtd not tracked!
> resources/locale/de/viewers/styleSheets.dtd not tracked!
> resources/locale/nb-NO/viewers/styleSheets.dtd not tracked!
> resources/locale/cs/viewers/styleSheets.dtd not tracked!
> resources/locale/sv-SE/viewers/styleSheets.dtd not tracked!
> resources/locale/el/viewers/styleSheets.dtd not tracked!
> resources/locale/en-US/viewers/styleSheets.dtd not tracked!
> resources/locale/sk/viewers/styleSheets.dtd not tracked!
> resources/locale/pt-BR/viewers/styleSheets.dtd not tracked!

It seemed fine, because Mercurial does track subsequent modifications, but further poking shows hg log gives no revision history for either form, and hg status lists clean files as "added". Ohrrm.
Keywords: checkin-needed
I give in. Go back to lowercase uid (and therefore lowercase filenames).
Attachment #416314 - Attachment is obsolete: true
Attachment #426946 - Flags: superreview+
Attachment #426946 - Flags: review+
Comment on attachment 426946 [details] [diff] [review]
no directory moves
[Checkin: Comment 26]


http://hg.mozilla.org/dom-inspector/rev/e71139b4dd49
Attachment #426946 - Attachment description: no directory moves → no directory moves [Checkin: Comment 26]
Status: ASSIGNED → RESOLVED
Closed: 15 years ago14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Blocks: DOMi2.0.5
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: