Closed Bug 88423 Opened 23 years ago Closed 21 years ago

Remove NS_CSS_PARSER_DROP_DECLARATION

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Future

People

(Reporter: bzbarsky, Assigned: dbaron)

References

Details

Attachments

(2 files)

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)
Attached patch Proposed patchSplinter Review
Keywords: patch, review
IIRC, we should *never* return NS_CSS_PARSER_DROP_DECLARATION since it means
DROP_DECLARATION_BLOCK.
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.
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.
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
david, marc: please r/sr...
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.
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
Status: NEW → ASSIGNED
Target Milestone: Future → mozilla0.9.4
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Priority: P4 → P3
Target Milestone: mozilla0.9.7 → Future
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
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: