516091 - Split CSS error reporter to its own file

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Zack Weinberg (:zwol)]

Attachment: patch

This patch moves the error reporting code from nsCSSScanner.cpp to its own file (nsCSSErrorReporter.cpp/h).  The new nsCSSErrorReporter class provides a bunch of inline stub methods when CSS_REPORT_PARSE_ERRORS is not defined, which means we don't need any #ifdef CSS_REPORT_PARSE_ERRORS anywhere in the scanner or parser proper.  However, the main purpose of this change is to make it easier to swap out the core scanner code for something else, since there's no unrelated code in nsCSSScanner.cpp.

I tested this by running the layout/style mochitests with the error console open; diagnostics still show up.

Comment 1

[Zack Weinberg (:zwol)]

Attachment: revised patch 1/2: macros for low level error reporting

I revised the patch to fix a bug in the line number handling, and to move the "low level error" tracking to the error reporter object as well.

Part 1 just wraps all calls to SetLowLevelError in the parser in macros, since if I'm going to change all sixty-odd such calls again, maybe it should be a bit more future proof this time.

Comment 2

[Zack Weinberg (:zwol)]

Attachment: revised patch 2/2: split error reporter to its own class

And here's the revised version of the original patch.

Comment 3

[Zack Weinberg (:zwol)]

Attachment: re-revised patch 2/2

I noticed, upon re-reading patch 2 after submission, that the !CSS_REPORT_PARSE_ERRORS mode was dropping low-level errors, which we don't want.  This fixes that.  Also, the combination (patch 1+2) has been verified to compile in both modes.

Comment 4

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

I'm not a big fan of the additional macro-ization in patch 1.

How about?
 1) skip patch 1
 2) make the error reporter a member of the scanner rather than the parser (and save the extra pointer)
 3) make give the parser a method nsCSSErrorReporter& ErrorReporter() { return mScanner.mErrorReporter; }

Also, it seems better to put the #ifdefs in nsCSSErrorReporter.h inside the class definition, and not duplicate the low-level-error code that's the same in both halves.

Comment 5

[Zack Weinberg (:zwol)]

Attachment: new revised patch 1: split error reporter

Comment 6

[Zack Weinberg (:zwol)]

The changes in bug 541496 and bug 523496 let me do this rather more cleanly.  The former "patch 1" with the extra macros for out-of-memory is no longer necessary.  The scanner still has a pointer to the error reporter rather than an embedded error reporter, though. I'm not insisting on that; it makes the headers a little less intertwined but it's not like these are separately usable classes anyway.

There will shortly be a new "patch 2" that streamlines initialization of all three of these classes (we don't need the two-phase init anymore).  I'm piggybacking that on this bug just because this is where it fits in the complete set.

Comment 7

[Zack Weinberg (:zwol)]

Oh, and I made there be less duplication in the header file.

Comment 8

[Zack Weinberg (:zwol)]

Attachment: new patch 2: get rid of two-phase parser init

This follow-on patch removes the two-phase initialization setup that we had for nsCSSParser and nsCSSScanner, where the constructor didn't do anything meaningful and then there was an Init() function that did real work.  It also removes further vestiges of the parser recycling scheme -- we don't need to clear out state in these objects anymore, they live on the stack and are destroyed after one use.

Comment 9

[Zack Weinberg (:zwol)]

I should maybe mention that patch 2 also includes a fix for a nasty infinite-loop bug in the scanner introduced by patch 1.  I'll reorganize things properly on Monday.

Comment 10

[Zack Weinberg (:zwol)]

Attachment: revised patch 1a: rediffed

Rediffed after changes to earlier patches in the series.  Also, no longer introduces a bug that makes nsCSSScanner::ParseString go into an infinite loop on any input.

Comment 11

[Zack Weinberg (:zwol)]

Attachment: revised patch 2a: rediffed

Some cosmetic changes that were in earlier iterations of this patch got pushed to earlier patches in the series.

Comment 12

[Zack Weinberg (:zwol)]

Comment on attachment 423464 [details] [diff] [review]
revised patch 2a: rediffed

This is not ready yet and may not happen in this form.

Comment 13

[Zack Weinberg (:zwol)]

Attachment: patch v3

Here's a new edition of this patch on top of the current patchsets in bug 543151 and bug 659963.

Comment 14

[Zack Weinberg (:zwol)]

Attachment: patch v4

Dusting this off now we're finally in a position to land bug 663291 and therefore to test CSS error reporting in detail.

No substantive changes from the previous version.

Comment 15

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

Comment on attachment 661640 [details] [diff] [review]
patch v4

Regarding the name:  I think we've decided at this point that
mozilla::css::ErrorReporter is more namespacing than we want, and
the preferred name would be mozilla::CSSErrorReporter.  I'm ok with
this landing as-is, but please keep that in mind for the future.

Have you checked that:
 (1) things still build when CSS_REPORT_PARSE_ERRORS is not defined, and
 (2) things get optimized away as well as before in an optimized build
     when CSS_REPORT_PARSE_ERRORS is not defined?  (I ask since you've changed
     things from macros to inline functions.)

r=dbaron

Comment 16

[Zack Weinberg (:zwol)]

(In reply to David Baron [:dbaron] from comment #15)
> Have you checked that:
>  (1) things still build when CSS_REPORT_PARSE_ERRORS is not defined, and
>  (2) things get optimized away as well as before in an optimized build
>      when CSS_REPORT_PARSE_ERRORS is not defined?  (I ask since you've
>      changed things from macros to inline functions.)

I hadn't, in fact.  Re (1), it turns out that the *unmodified* layout/style doesn't build with CSS_REPORT_PARSE_ERRORS off (not wholly surprising since CSS_REPORT_PARSE_ERRORS is hardwired on in nsCSSScanner.h...):

nsCSSScanner.cpp: In member function ‘virtual nsresult DeferredCleanupRunnable::Run()’:
nsCSSScanner.cpp:261:17: error: ‘class nsCSSScanner’ has no member named ‘PerformDeferredCleanup’
nsCSSScanner.cpp: In destructor ‘nsCSSScanner::~nsCSSScanner()’:
nsCSSScanner.cpp:297:9: error: ‘Reset’ was not declared in this scope
nsCSSScanner.cpp: In member function ‘void nsCSSScanner::Close()’:
nsCSSScanner.cpp:599:9: error: ‘Reset’ was not declared in this scope
nsCSSScanner.cpp: At global scope:
nsCSSScanner.cpp:616:21: error: no ‘void nsCSSScanner::Reset()’ member function declared in class ‘nsCSSScanner’

It looks like this is a relatively recent problem, introduced with bug 786108.  I have incorporated the necessary fixes into the next rev of the patch (it has to be rev'ed for bug 786108 anyway).

Re (2), having fixed the compile problems, I did trial compiles before-and-after with gcc 4.7 on Linux64, and it appears to me that all of the code changes in functions that just potentially report errors are down to things having moved around in nsCSSScanner and/or normal register allocation variation.  I've made sure that the most commonly accessed nsCSSScanner fields are at the beginning, and I think that should be good enough as far as assessing non-regression of a mode that never actually gets used. :)  I'll attach the assembly dumps in case anyone is curious.

Comment 17

[Zack Weinberg (:zwol)]

Attachment: nsCSSScanner assembly dump before patch (error reporting disabled)

Comment 18

[Zack Weinberg (:zwol)]

Attachment: nsCSSScanner assembly dump after patch (error reporting disabled)

Comment 19

[Zack Weinberg (:zwol)]

Attachment: nsCSSParser assembly dump before patch (error reporting disabled)

Comment 20

[Zack Weinberg (:zwol)]

Attachment: nsCSSParser assembly dump after patch (error reporting disabled)

Comment 21

[Zack Weinberg (:zwol)]

Attachment: patch v5

Here's the new revision of the patch.  Nontrivial surgery was required to preserve the optimization done in bug 786108, so I'm asking for a new review.  Besides that, the other significant change is that nsCSSScanner objects are now allocated on the stack rather than embedded in an nsCSSParser, which means they are no longer involved in the parser-recycling mess.  (I did this mainly to simplify how the aforementioned optimization works -- only ErrorReporter.cpp has to know anything about it now.)

Comment 22

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

Comment on attachment 673656 [details] [diff] [review]
patch v5

r=dbaron

One stylistic nit:
  >+  void SetErrorReporter(mozilla::css::ErrorReporter* aReporter)
  >+  { mReporter = aReporter; }
Could you either use the 3-line form or indent the { 2 more spaces?

Comment 23

[Zack Weinberg (:zwol)]

Attachment: patch v5a

(In reply to David Baron [:dbaron] from comment #22)
> One stylistic nit:
>   >+  void SetErrorReporter(mozilla::css::ErrorReporter* aReporter)
>   >+  { mReporter = aReporter; }
> Could you either use the 3-line form or indent the { 2 more spaces?

I have changed it to the 3-line form, as that is consistent with the next two inlines.

This will land as soon as bug 663291 gets reviewed.

Comment 24

[Zack Weinberg (:zwol)]

On reexamining my whole patch queue, there is no need for this or bug 455839 to wait for bug 663291, so I've gone ahead and landed both of those now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/03b607d3bca6

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/03b607d3bca6