Remove NS_CSS_PARSER_DROP_DECLARATION

RESOLVED FIXED in Future

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
17 years ago
15 years ago

People

(Reporter: bz, Assigned: dbaron)

Tracking

Trunk
Future
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

BUILD:  2001-06-28 source

If one calls CSSParserImpl::ParseDeclaration with the declaration "font-style:
bold", then the return value is PR_FALSE and aErrorCode is NS_OK.  It would make
more sense to have the error code be NS_CSS_PARSER_DROP_DECLARATION.

The problem seems to be that CSSParserImpl::ParseVariant will return PR_FALSE
and leave aErrorCode as NS_OK in this case.

Perhaps CSSParserImpl::ParseDeclaration should make sure that aErrorCode is set
to NS_CSS_PARSER_DROP_DECLARATION any time it drops a declaration?  Something
like |if (aErrorCode == NS_OK) aErrorCode = NS_CSS_PARSER_DROP_DECLARATION| in
all the cases when it returns PR_FALSE?
Blocks: 88421
To clarify.  This is the function with the prototype:

CSSParserImpl::ParseDeclaration(PRInt32& aErrorCode,
                                nsICSSDeclaration* aDeclaration,
                                PRBool aCheckForBraces,
                                PRInt32& aChangeHint)
Created attachment 40690 [details] [diff] [review]
Proposed patch
Keywords: patch, review
(Assignee)

Comment 3

17 years ago
IIRC, we should *never* return NS_CSS_PARSER_DROP_DECLARATION since it means
DROP_DECLARATION_BLOCK.
(Assignee)

Comment 4

17 years ago
Why is a parsing error an exceptional circumstance anyway?  We're responding to
user input, and should handle it gracefully, as I think we do.
(Assignee)

Comment 5

17 years ago
FWIW (IIRC), the NS_CSS_PARSER_DROP_DECLARATION error code was created due to a
misunderstanding but never removed from the codebase as it probably should have
been.

Comment 6

17 years ago
David is correct.  Renaming the bug to "Remove NS_CSS_PARSER_DROP_DECLARATION" 
from "CSSParserImpl::ParseDeclaration can return false and leave aErrorCode as 
NS_OK"...

Status: NEW → ASSIGNED
Keywords: patch, review
Summary: CSSParserImpl::ParseDeclaration can return false and leave aErrorCode as NS_OK → Remove NS_CSS_PARSER_DROP_DECLARATION

Comment 7

17 years ago
Created attachment 42631 [details] [diff] [review]
patch to remove NS_CSS_PARSER_DROP_DECLARATION

Comment 8

17 years ago
david, marc: please r/sr...

Comment 9

17 years ago
I'm not sure I understand why you can just drop the code in GenericElement and
nsCSSStyleRule that calls 
  SetCSSDeclaration(declClone);

Is it because the return code NS_CSS_PARSER_DROP_DECLARATION is never set in the
first place, so that code can never be executed? If that is the case,
[s]r=attinasi , otherwise please clarify my murky understanding.

The code that sets the declaration to declClone is there for the following
reason, as far as I can tell:

The DOM CSS spec requires that setting .cssText throw a DOM_SYNTAX_ERR exception
when the string it's being set to is "unparsable".  What this means is not
clear, but throwing an exception means you don't change the actual cssText or
the declaration, so you have to throw away your changes (including any clearing
that happened).

The working group has been contacted for a clarification of this by dbaron, but
I don't think they've said anything useful yet.

Comment 11

17 years ago
Reassigned to dbaron. Depending on what the WG decides, we may have to either 
apply the patch above (+ remove the declaration of 'declClone' that I had 
forgotten), or restore the original declaration if ParseAndAppendDeclaration() 
returns an error:

     if (aClearOldDecl && NS_FAILED(result)) {
       SetCSSDeclaration(declClone);
     }
Assignee: pierre → dbaron
Status: ASSIGNED → NEW
Summary: Remove NS_CSS_PARSER_DROP_DECLARATION → [WG]Remove NS_CSS_PARSER_DROP_DECLARATION
Target Milestone: --- → Future
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla0.9.4
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5
(Assignee)

Updated

17 years ago
Priority: -- → P4
(Assignee)

Updated

16 years ago
Target Milestone: mozilla0.9.5 → mozilla0.9.6
(Assignee)

Updated

16 years ago
Target Milestone: mozilla0.9.6 → mozilla0.9.7
(Assignee)

Updated

16 years ago
Priority: P4 → P3
Target Milestone: mozilla0.9.7 → Future
(Assignee)

Updated

15 years ago
Depends on: 197212
Summary: [WG]Remove NS_CSS_PARSER_DROP_DECLARATION → Remove NS_CSS_PARSER_DROP_DECLARATION
Fixed by bug 197212 checkin -- NS_CSS_PARSER_DROP_DECLARATION is no more.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.