52381 - CSSSelectors needs to implement ToString()

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Edward Kandrot]

CSSSelector has a ToString() method, which will be needed for code to get the
current selector text in nsCSSStyleRule.cpp.  It currently only returns NS_OK.

Is there a test case for this?

Comment 1

[James Lariviere]

*** Bug 52370 has been marked as a duplicate of this bug. ***

Comment 2

[Joe Hewitt (gone)]

Ok, I've implemented nsCSSSelector::ToString() and it appears to work nicely.
I'll attach a patch in a segundo.  I apologize for the fact that the patch also
includes changes for bug 59560, which makes line number info available.  I
figure you can test both of these at once.  If for some reason you want them in
separate patches, I'd be happy to do that.  The following files are affected:

layout\html\style\src\nsCSSParser.cpp
layout\html\style\src\nsCSSScanner.cpp
layout\html\style\src\nsCSSScanner.h
layout\html\style\src\nsCSSStyleRule.cpp
layout\html\style\src\nsICSSStyleRule.h

I should probably note the one ugly thing about my implementation is that I had
to add an extra parameter (nsICSSStyleSheet*) to nsCSSSelector::ToString,
because in order to retrieve the namespace prefix of a selector, we need some
info from it's stylesheet.  This stylesheet is not referenced in the
nsCSSSelector struct, and I must get it from somewhere.

Comment 3

[Joe Hewitt (gone)]

Attachment: implementation patch

Comment 4

[Marc Attinasi]

Joe, do you have a testcase for this? I'm reviewing it now and any testcase you
have would help a bit- thanks.

Comment 5

[Joe Hewitt (gone)]

I'm about to attach a test case which is an HTML document that grabs all the 
selectorText values from it's DOM and displays them in a table.  It covers all 
the different selector types with different arrangements.

Comment 6

[Joe Hewitt (gone)]

Attachment: test case (html document)

Comment 7

[Marc Attinasi]

r=attinasi. Thanks for the patch and the testcase, and I'm sorry it took so long
for me to attend to.

Comment 8

[Hixie (not reading bugmail)]

Upon managerial request, adding the "testcase" keyword to 84 open layout bugs that
do not have the "testcase" keyword and yet have an attachement with the word
"test" in the description field. Apologies for any mistakes.

Comment 9

[Joe Hewitt (gone)]

Taking since I have the fix.

Comment 10

[Chris Waterson]

Is toString() accessable from JS? If so, are there issues with passing in an
nsICSSStyleSheet (which is not scriptable) to it? cc'ing jst...

Comment 11

[Johnny Stenback (:jst)]

ToString is what's used by the JS engine for automatic Object --> String
conversion but AFAICT from this diff the ToString method that is modified is not
part of something that is directly scriptable, and thus it's ok. If the ToString
method would've been in a scriptable interface then adding the stylesheet method
would not have been acceptable.

I didn't really do a review of the patch but I did notice some things that
should be optimized, code like this:

  if (prefixAtom) {
    nsAutoString prefix;
    prefixAtom->ToString(prefix);
    aString.Append(prefix);
    ...
  }

does one string copy that is easily avoided, make the code look like the code
below and you'll save that one copy (and possibly one allocation, although
probably not in this case):

  if (prefixAtom) {
    const PRUnichar prefix;
    prefixAtom->GetUnicode(&prefix);
    aString.Append(prefix);
    ...
  }

I think all occurances of code like this should be fixed before the patch is
checked in, it's trivial low hanging optimization-friute :-). Joe, could you
make this chage?

Comment 12

[Joe Hewitt (gone)]

Ok, I fixed the extra string copying that jst referenced... new patch forthcoming.

A note about the ToString method taking an nsICSSStyleSheet parameter... I hated
having to do this, but it was necessary in order to lookup the namespace prefix.
 nsCSSSelector holds no references that would enable me to look this up any
other way.  It seemed ok since nsCSSSelector is really just a limited-use data
holder and that ToString method isn't intended for the public...

waterson/jst, can you give this another look and perhaps an sr=?

Comment 13

[Joe Hewitt (gone)]

Attachment: updated nsCSSStyleRule.cpp patch

Comment 14

[Chris Waterson]

Waaah! `cvs diff -u'

sr=waterson

Comment 15

[Johnny Stenback (:jst)]

Gimme an "cvs diff -u" and I'll review. Reading that diff is too painful!

Comment 16

[Joe Hewitt (gone)]

Attachment: nsCSSStyleRule.cpp (cvs diff -u format)

Comment 17

[Johnny Stenback (:jst)]

Looks good, r=jst

Comment 18

[Joe Hewitt (gone)]

Fixed. 

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

cc:ing Daniel, because he'll have to fix this ToString for his ID list code. 
(It's just a few lines that need to be changed.)

It would also be nice to condense this with the code in ListSelector /
ListNamespace (after making sure they're functionally equivalent).

Comment 20

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Bug reopened : pseudo-elements are not correctly handled. A '>' child combinator
is incorrectly inserted.
Adding a test case below.

Comment 21

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: HTML+js test calling StyleRule.selectorText with a rule using a pseudo-element

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think we actually store pseudo-elements that way.  Something like that
has bitten us before...

Comment 23

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

I also have to say that I can't really test the element type selectors,
universal selector and attribute selectors with a namespace prefix because the
implementation of @namespace seems buggy.

Comment 24

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Proposing a fix for this bug, to be applied after Joe Hewitt's patch.
Also attaching a new HTML test case.

Comment 25

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: fix to be applied after Joe's patch

Comment 26

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: New HTML test case

Comment 27

[Johnny Stenback (:jst)]

Daniel, two comments about the patch:

1. The IsPseudoElement() function currently does one extra string copy change:

  nsAutoString buffer;
  aAtom->ToString(buffer);
  return (':' == buffer.First());

to:

  const PRUnichar *str;
  aAtom->GetUnicode(&str);
  return str && (*str == ':');


2. The new ToString argument is named "isPElem", please follow the naming
convention used in this file (and most of the rest of mozilla) and name the
argument "aIsPElem" in stead, all arguments should be named 'a' something for
readability and consistency.

Make those changes and you'll have my r=. great work!

Comment 28

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: new patch integrating jst's comments; then requesting r=

Comment 29

[Johnny Stenback (:jst)]

Looks good to me now, I'm assuming aAtom in IsPseudoElement() is never null,
Mark, Pierre, do we need a null ptr check? r=jst with an answer from one of the
style guys...

Comment 30

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Arrrgggl. Enfer et damnation, pourquoi tant de haine ?

Very good catch, thank you jst. It seems that universal selectors are stored
with a null mTag. New patch on its way, with a very big test of all possible
kinds of selectors.

Comment 31

[Joe Hewitt (gone)]

Reassigning to glazman since he's working on this now.

Comment 32

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Accepting assignment

Comment 33

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: New patch including last jst's comments. jst: r=?

Comment 34

[Johnny Stenback (:jst)]

Ok, looking much better, but there's still one thing I'd like to change in the diff:

static PRBool IsPseudoElement(nsIAtom* aAtom)
{
  const PRUnichar *str;
  if (aAtom) {
    aAtom->GetUnicode(&str);
    return str && (*str == ':');
  }
  else {
    return PR_FALSE;
  }
}

Should be changed to:

static PRBool IsPseudoElement(nsIAtom* aAtom)
{
  if (aAtom) {
    const PRUnichar *str;
    aAtom->GetUnicode(&str);
    return str && (*str == ':');
  }

  return PR_FALSE;
}

i.e. move 'str' into the one and only block where it's used and skip the 'else'
since I imagine some compilers might complain about the lack of a return as the
last thing in a method returning PRBool (yeah, your method will always return
correctly but some compilers might not agree).

r=jst, make the above change when you check in.

Comment 35

[rubydoo123]

setting to moz0.9

Comment 36

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: new patch in answer to jst's last comment ; this gives r=jst

Comment 37

[vidur (gone)]

After Johnny's nitpicking, I don't feel like there's a need for much more. ;-)
sr=vidur.

Comment 38

[Johnny Stenback (:jst)]

:-)

Comment 39

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

checked-in

Comment 40

[Chris Petersen]

Marking verified per last comments

Comment 41

[James Paige]

I imagine this is probably a separate bug, but testing attachment 2 [details] [diff] [review], the test
case for this bug,
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=19691 with todays build
(2001030505) on Windows 95 crashes mozilla with a red-X-of-death:

MOZILLA caused an invalid page fault in
module GKLAYOUT.DLL at 014f:6034214f.

Comment 42

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Confirming !!!

1) this used to work some days ago : I checked the attached test case
   from the bug page when I added it to bugzilla
2) if I save the attachment locally and open it locally, mozilla does *NOT*
   crash !!!

I guess that reporter is then right assuming that the problem comes from
somewhere else. Should we reopen ?

Comment 43

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Confirming that the bug is *not* present in 0.8.
me grumbles...

Comment 44

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Correction : recent builds of mozilla crash on test case even if the file is
local.  0.8 is ok.

Reporter : since that was a RFE, and because it used to work fine,
can you please file a new bug with your test case ?

Comment 45

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

bug 52381 remains verified and fixed since attachment to bug 70971 shows
that the crash is not at all related to DOM and Selector::ToString.

Moving to bug 70971