452275 - Eliminate aErrorCode argument to most nsCSSScanner/CSSParserImpl functions

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Zack Weinberg (:zwol)]

Attachment: (rev 1) initial patch

It occurred to me this morning that the 'aErrorCode' argument that's threaded through most of the CSS parser and scanner, set rarely, and inspected only by top level parser entry points could be sensibly replaced with a nsCSSScanner member variable.  The attached patch does this.

Since this touched just about every function definition in the two files, I took the opportunity to standardize them on the "return value on preceding line" style convention (both were inconsistent).  I also did a trailing-whitespace sweep in nsCSSParser.cpp only.

I have one concern - it's a preexisting condition but it might as well be fixed with this patch, if it needs fixing: CSSParserImpl::Parse(), CSSParserImpl::ParseStyleAttribute(), and CSSParserImpl::ParseRule() all ignore this error code, always returning NS_OK even if it's been set to something else.  Should that be corrected?

Comment 1

[Boris Zbarsky [:bzbarsky]]

See bug 399223?

Comment 2

[Zack Weinberg (:zwol)]

This patch is, functionally, a subset of that patch - I am not convinced it makes sense to turn aToken into a member variable too.  It should be easier to review this since it only changes aErrorCode.

Comment 3

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

Comment on attachment 335558 [details] [diff] [review]
(rev 1) initial patch

In the future, please do whitespace reformatting as a separate patch.

In ParseMediaList, I wonder whether we want to continue propagating the
error code whether or not GatherMedia fails, or do what you chose and
only propagate it if GatherMedia fails.  It might be better to keep the
behavior as-is.

In ParseBorderImage, the 4 ParsePositiveVariant calls that parse the
border widths after the / look like they can be on one line now, rather
than split over 2 lines.

Likewise for the arguments in the definition of CSSParserImpl::ParseRect.


nsCSSScanner.cpp:

nsCSSScanner::SetLowLevelError should probably also assert that the
error is currently NS_OK, since we shouldn't be continuing to do things
that could cause an error after causing one.

In nsCSSScanner::EnsureData, rename |err| to |rv| so we don't have yet
another convention, and declare it on the same line it's first used,
i.e.,
  nsresult rv = mInputStream->Read(...);

r+sr=dbaron

Comment 4

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

And if you posted an updated and merged-to-tip patch (or hg bundle), I should be able to land it for you (although send email, since I may not read bugmail as quickly).

Comment 5

[Zack Weinberg (:zwol)]

Attachment: (rev 2) with dbaron's requested changes

Here's the revised patch.  I made all the changes you requested.  I'll be mailing you a bundle with this + 452278 + 452518 shortly.

Comment 6

[Zack Weinberg (:zwol)]

landing seems to have stuck.