Last Comment Bug 240080 - Implement CSS Editing Object Model
: Implement CSS Editing Object Model
Status: NEW
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: Trunk
: All All
-- enhancement with 2 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
: Jet Villegas (:jet)
Depends on:
Blocks: 16255 255016
  Show dependency treegraph
Reported: 2004-04-09 07:04 PDT by Daniel Glazman (:glazou)
Modified: 2012-07-14 07:40 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

First candidate patch (24.28 KB, patch)
2004-04-09 12:18 PDT, Daniel Glazman (:glazou)
no flags Details | Diff | Splinter Review
better fix (25.86 KB, patch)
2004-04-16 01:42 PDT, Daniel Glazman (:glazou)
bzbarsky: review-
Details | Diff | Splinter Review

Description User image Daniel Glazman (:glazou) 2004-04-09 07:04:01 PDT
Implement the CSS Editing Object Model (see URL above). This is needed for direct
class assignment to the selection (bug 16255) in the editor. Patch pending.
Comment 1 User image Daniel Glazman (:glazou) 2004-04-09 12:18:14 PDT
Created attachment 145757 [details] [diff] [review]
First candidate patch

This candidate patch is fully functional but not optimized/verified.
I already use it in Nvu and it's very very cool.
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2004-04-09 14:42:54 PDT
If the string list allocation fails, this code crashes, no?
Comment 3 User image Daniel Glazman (:glazou) 2004-04-10 00:23:02 PDT
(In reply to comment #2)

> If the string list allocation fails, this code crashes, no?

Boris: right. I said not optimized, not verified. But the basis is here.
Comment 4 User image Daniel Glazman (:glazou) 2004-04-16 01:42:05 PDT
Created attachment 146257 [details] [diff] [review]
better fix

This is a better patch. I am ok to put #ifdef MOZ_STANDALONE_COMPOSER
everywhere but I feel this could be useful (a) for Midas and other
self-modifyable web pages (b) for tests.
Comment 5 User image Daniel Glazman (:glazou) 2004-04-16 01:43:03 PDT
Comment on attachment 146257 [details] [diff] [review]
better fix

bz, could you review please ? Thanks a lot.
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2004-04-16 07:41:30 PDT
I won't be able to get to a review for at least a week...  At first glance,
though, this interface needs some documentation (like clearly explaining what it
actually does).

The question I raised in your blog still stands.  Why should this be on a
selector-specific interface as opposed to a more general CSSOM interface (like
nsIDOMDocumentCSSStyle or something)?
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2004-08-30 16:10:57 PDT
Comment on attachment 146257 [details] [diff] [review]
better fix

Marking r- in the hope of getting a response to comment 6 and because of the
problems listed below:

>Index: dom/public/nsIDOMClassInfo.h

>+  eDOMClassInfo_CSSSelectorQuery_id,

Please add this at the end of the list like the comments say to do.

>Index: content/html/style/src/nsCSSStyleRule.cpp

>+    mAtom->ToString(atomString);
>+    classString.Append(atomString);

I think it would be better to get the UTF8 string from the atom (no copy) and
then AppendUTF8toUTF16.  Saves a string copy...

>+nsCSSSelector::GetSelectorList(PRUint32 aSelectorFilter, nsIDOMDOMStringList 

>+    mTag->ToString(tagString);

This clobbers the namespace prefix, no?  I don't think you want that.

>+    nsCOMPtr<nsDOMStringList> list = do_QueryInterface(aStringList);

I'm not sure how this is supposed to work, exactly...  You're not QIing to an
interface here.

>+    mClassList->ToDOMStringList(NS_LITERAL_STRING("."), aStringList);

It makes sense to call ToDOMStringList AppendToDOMStringList instead. Perhaps
the first arg should be a PRUnichar, not an nsAString?

Why is this code passing around the sheet?  It just wants the nsINameSpace, so
it should pass that around, I think.

This code doesn't seem to differentiate between negated and non-negated simple
selectors.  This should probably be clearly documented in the interface.

>+DOMCSSStyleRuleImpl::GetSelectorList(PRUint32 aSelectorFilter, nsIDOMDOMStringList ** aStringList)
>+  if (!Rule()) {
>+    return NS_OK;

You need to null out *aStringList before returning here.

>+  *aStringList = list;
>+  NS_IF_ADDREF(*aStringList);

No need for the IF part.  "list" cannot be null here.  Just do:

  NS_ADDREF(*aStringList = list);

Given this function, why doesn't the rule's GetSelectorList just take an
nsDOMStringList* instead of taking nsIDOMDOMStringList*?  Since the rule has to
add things to the list, that would make a lot of sense to me...

>+CSSStyleRuleImpl::GetSelectorList(PRUint32 aSelectorFilter, nsIDOMDOMStringList * aStringList)

>+    nsCSSSelectorList * selectorList = mSelector;
>+    while (selectorList)

I think this wants to be a for loop.

>Index: content/html/style/src/nsICSSStyleRuleDOMWrapper.h

>+class nsICSSStyleRuleDOMWrapper : public nsISupports

Why this change?  This makes all DOM style rules 4 bytes bigger; is there
really a good reason for it?

>Index: content/html/style/src/nsCSSStyleSheet.cpp
>@@ -1002,6 +1014,7 @@


Why this change?

>+CSSStyleSheetImpl::GetSelectorListInternal(PRUint32 aSelectorFilter, nsIDOMDOMStringList * aStringList, nsIDOMCSSRuleList * aRuleList)

Wouldn't a lot of this logic be better off in the relevant rules themselves? 
Then new rule impls would not require changes to this code (eg see
@-moz-document rules).

>+CSSStyleSheetImpl::GetSelectorList(PRUint32 aSelectorFilter, nsIDOMDOMStringList ** aStringList)

>+  if (nsnull == mRuleCollection) {

!mRuleCollection, please.

Any reason this code isn't just using GetRules()?

>+  *aStringList = list;
>+  NS_IF_ADDREF(*aStringList);

Again, "list" can't be null here.

>Index: content/html/style/public/nsICSSStyleSheet.h
>+  NS_IMETHOD GetSelectorListInternal(PRUint32 aSelectorFilter, nsIDOMDOMStringList * aStringList,

Weird indent.

>Index: content/base/src/nsDocument.cpp

>+  PRInt32 sheetCount = GetNumberOfStyleSheets(false);

The GetNumberOfStyleSheets() call no longer needs a boolean (and needed a
PRBool before anyway; the code as written would break some platforms).

>+    nsIStyleSheet *sheet = GetStyleSheetAt(sheetIndex, PR_FALSE);

Again, no need for the PR_FALSE

>+  *aStringList = list;
>+  NS_IF_ADDREF(*aStringList);

Again, no need for the "IF" part.

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