Split CSS error reporter to its own file

RESOLVED FIXED in mozilla19

Status

()

enhancement
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: zwol, Assigned: zwol)

Tracking

Trunk
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 11 obsolete attachments)

132.77 KB, text/plain
Details
127.67 KB, text/plain
Details
1.66 MB, text/plain
Details
1.66 MB, text/plain
Details
69.60 KB, patch
zwol
: review+
Details | Diff | Splinter Review
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #400194 - Flags: review?(dbaron)
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.
Assignee: nobody → zweinberg
Attachment #400194 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #403039 - Flags: review?(dbaron)
Attachment #400194 - Flags: review?(dbaron)
And here's the revised version of the original patch.
Attachment #403040 - Flags: review?(dbaron)
Posted patch re-revised patch 2/2 (obsolete) — Splinter Review
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.
Attachment #403040 - Attachment is obsolete: true
Attachment #403042 - Flags: review?(dbaron)
Attachment #403040 - Flags: review?(dbaron)
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.
Attachment #403039 - Flags: review?(dbaron) → review-
Attachment #403039 - Attachment is obsolete: true
Attachment #403042 - Attachment is obsolete: true
Attachment #423106 - Flags: review?(dbaron)
Attachment #403042 - Flags: review?(dbaron)
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.
Depends on: 541496
Oh, and I made there be less duplication in the header file.
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.
Attachment #423155 - Flags: review?(dbaron)
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.
Posted patch revised patch 1a: rediffed (obsolete) — Splinter Review
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.
Attachment #423463 - Flags: review?(dbaron)
Attachment #423106 - Attachment is obsolete: true
Attachment #423106 - Flags: review?(dbaron)
Posted patch revised patch 2a: rediffed (obsolete) — Splinter Review
Some cosmetic changes that were in earlier iterations of this patch got pushed to earlier patches in the series.
Attachment #423155 - Attachment is obsolete: true
Attachment #423464 - Flags: review?(dbaron)
Attachment #423155 - Flags: review?(dbaron)
Blocks: 455839
Attachment #423463 - Flags: review?(dbaron)
Comment on attachment 423464 [details] [diff] [review]
revised patch 2a: rediffed

This is not ready yet and may not happen in this form.
Attachment #423464 - Flags: review?(dbaron)
Depends on: 543151
No longer depends on: 541496
Depends on: 659963
No longer depends on: 543151
Posted patch patch v3 (obsolete) — Splinter Review
Here's a new edition of this patch on top of the current patchsets in bug 543151 and bug 659963.
Attachment #423463 - Attachment is obsolete: true
Attachment #423464 - Attachment is obsolete: true
Attachment #535994 - Flags: review?(dbaron)
Posted patch patch v4 (obsolete) — Splinter Review
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.
Attachment #535994 - Attachment is obsolete: true
Attachment #535994 - Flags: review?(dbaron)
Attachment #661640 - Flags: review?(dbaron)
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
Attachment #661640 - Flags: review?(dbaron) → review+
(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.
Posted patch patch v5 (obsolete) — Splinter Review
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.)
Attachment #661640 - Attachment is obsolete: true
Attachment #673656 - Flags: review?(dbaron)
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?
Attachment #673656 - Flags: review?(dbaron) → review+
Posted patch patch v5aSplinter Review
(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.
Attachment #681993 - Flags: review+
Attachment #673656 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/03b607d3bca6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.