Closed Bug 125246 Opened 23 years ago Closed 21 years ago

Further reduce dynamic footprint of nsCSSDeclaration and things it owns (nsCSSMargin, etc.)

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.5alpha

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: arch, memory-footprint, Whiteboard: [whitebox][patch])

Attachments

(2 files, 12 obsolete files)

testcase for color names and shorthand stuff
467 bytes, text/html
Details
patch 1.5
98.49 KB, application/x-gzip
bzbarsky
: review+
bzbarsky
: superreview+
Details 
nsCSSDeclaration is still pretty bloated (see bug 107270 comment 10).  It would
be good to further reduce the size of it, perhaps according to the plan I
described in bug 107270 comment 2, bug 107270 comment 3, and bug 107270 comment
5.  It would also be nice to make the nsCSSDeclaration.cpp code more table
driven and less based on huge case statements.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → Future
cc'ing myself
Whiteboard: [dev notes]
Whiteboard: [dev notes] → [whitebox]
nsCSSDeclaration shows up as the second biggest class in terms of code/static
data, in content/layout, after nsCSSFrameConstructor.
And the 4th biggest is CSSProperties2Tearoff.
That's basically independent of this bug, which is about data size, not code
size.  I think I have another bug somewhere on rewriting much of this stuff to
be table-driven...
Clarify summary.
Summary: further reduce size of nsCSSDeclaration and things it owns (nsCSSMargin, etc.) → Further reduce dynamic footprint of nsCSSDeclaration and things it owns (nsCSSMargin, etc.)
*** Bug 191232 has been marked as a duplicate of this bug. ***
This is the beginning (actually, perhaps even most, barring dealing with the
XXX comments I added in nsCSSPropList.h and dealing with the things that are
"CSS_PROP_(") of a patch to make the CheckData tables in nsRuleNode.cpp use the
same nsCSSPropList.h.  This would mean that things at both ends would be
getting the data from the same place, which would mean that likely most of what
would be needed for the stuff in the middle would be there.  I'm not
guaranteeing anything, though.

Any thoughts on whether this approach is too messy?  I'm still not quite sure
how I want to deal with shorthands and the other things in the XXX comments and
the CSS_PROP_ macro.

(Yes, I know I haven't expanded all the users of CSS_PROP to the new 8-argument
form.)
I think this is a great patch from a code simplification point of view. It's
good to reduce the number of places we have to change when adding style properties.

However, maybe I'm a bit slow tonight, but I don't see how this reduces our data
size at all.
It isn't "the patch".  It's "the patch before the patch" (or maybe the one
before that).  I want to be able to redesign nsCSSDeclaration using something
other than case statements (see comment 0).
Attached patch work-in-progress (obsolete) — — Splinter Review 
[patch #f] Here's some further work-in-progress that I think gets me to roughly
where I would be able to start the main part of this bug.

The main work involves:
 * setting up infrastructure so that the parser can parse into a temporary
structure so we can do exact allocation most of the time for the buffer
described below.
 * rewriting the storage in nsCSSDeclaration so that it stores all the values
in a single chunk of memory, identified by the property id (and using a table
that has the sizes for each type and the types for each property ID), and all
the methods of nsCSSDeclaration to use that storage.

I think I'm leaning towards the idea of not storing the chunks
per-style-struct, but of just keeping a set of bits saying for which style
structs a given declaration has data.

This patch won't come anywhere close to compiling since nsCSSDeclaration.cpp is
pretty much broken (i.e., unmodified).
Attachment #113070 - Attachment is obsolete: true
Blocks: 70022
Blocks: 178668
Attached patch work-in-progress (obsolete) — — Splinter Review 
This is further work-in-progress [patch #u].  It doesn't compile yet.

After a while of not working on this, I sat down this afternoon and wrote the
expansion/compression logic.  So now all (!) that's left is to make
nsCSSDeclaration and nsCSSParser use this.  Plus I need to make it compile,
since I haven't tried that yet.
Attachment #114032 - Attachment is obsolete: true
Attached patch work-in-progress (obsolete) — — Splinter Review 
[patch #z1] similar to previous patch, with the same major issues remaining. 
But I got it a lot closer to compiling, and discovered that I really need to
figure out how shorthands belong in the property list.
Attachment #120102 - Attachment is obsolete: true
Blocks: 62894
Blocks: 203448
Attached file work-in-progress (obsolete) — 
[#zq] Further work in progress.  There's still a bit of NS_NOTYETIMPLEMENTED
stuff and some XXX comments that need to be dealt with, as well as a tiny bit
of code that still doesn't compile (the shorthand stuff in
nsCSSDeclaration::GetValue).

I've added tables for the subproperties of shorthands and used them in a few
places already (I think).  I also put some more thought into how to handle
'!important', although some of the code isn't written yet.  I've been changing
things like mad in nsCSSDeclaration and nsCSSParser, but I haven't really
gotten a chance yet to make sure all the changes are coherent and that I
haven't missed major things there.

I'll note two somewhat hidden intentional semantic changes in this patch:
 * changed CSSStyleRuleImpl::GetValue so it checks '!important' too, since
   inspector should want that.
 * changed nsDOMCSSAttributeDeclaration::RemoveProperty so that it fills
   in |aReturn| as the spec says it should (with the old value).

At the rate I'm going I'm hoping the next patch I attach will compile and run.
Attachment #122360 - Attachment is patch: false
Attachment #122360 - Attachment mime type: text/plain → application/x-gzip
One other hidden change (although not so hidden soon, since in my next patch
I'll have an nsIDOMNSCSS2Properties IDL change) -- there's a property called
-moz-box-flex-group in the property list and in nsIDOMNSCSS2Properties, but with
no implementation to back it up.  I'm removing that, unless someone tells me
something else should happen to it.
-moz-box-flex-group is Hyatt's baby; I'd recommend passing that past him.
Attached file work-in-progress (obsolete) — 
[patch #zw] This is the current state.	The browser starts up, but has some
problems -- assertions about not having an absolute positioning containing
block, and problems typing in textareas (leading to crashes) once they hit the
overflow point.  I need to figure out what's causing them, but I really haven't
yet thought of a good way to figure it out.  There are also a bunch of problems
noted in the patch with XXX or NS_NOTYETIMPLEMENTED, as well as the review I'd
like to do of my own modifications to the CSS parser and nsDOMCSSDeclaration,
etc.

I asked hyatt whether he objected to my removal of -moz-box-flex-group, and he
replied:
| Nope.  Support never got added, and I see no reason why it will be
| needed in the near future.
Attachment #122360 - Attachment is obsolete: true
Attached file work-in-progress (obsolete) — 
[patch #zz0] This fixes the problems I mentioned in comment 16.  I still need
to review my own changes (especially to nsCSSParser / nsDOMCSSDeclaration) and
a a lot more comments before it's ready for review, but I think it is ready for
some performance / footprint / codesize testing.  I'll probably spend tomorrow
doing a mix of the two (reading / cleaning up the patch patch and the testing).


I'm attaching this patch in a build using the patch.
Attachment #122517 - Attachment is obsolete: true
memory (heap) use statistics ('debug' tree)
===========================================
./mozilla -P default http://mozilla.org/ --trace-malloc=malloc.log
./run-mozilla.sh ./leakstats ./malloc.log

alternating runs with and without patch
                                                                                
with patch:
                                                                                
    Leaks: 392740 bytes, 3706 allocations
    Maximum Heap Size: 8333089 bytes
    56152395 bytes were allocated in 533545 allocations.
                                                                                
    Leaks: 392740 bytes, 3706 allocations
    Maximum Heap Size: 8332977 bytes
    53161245 bytes were allocated in 408774 allocations.
                                                                                
    Leaks: 392740 bytes, 3706 allocations
    Maximum Heap Size: 8332661 bytes
    54587013 bytes were allocated in 468210 allocations.
                                                                                
without patch:
                                                                                
    Leaks: 394029 bytes, 3711 allocations
    Maximum Heap Size: 8398337 bytes
    54158204 bytes were allocated in 451876 allocations.
                                                                                
    Leaks: 394029 bytes, 3711 allocations
    Maximum Heap Size: 8398093 bytes
    55468982 bytes were allocated in 506768 allocations.
                                                                                
    Leaks: 394029 bytes, 3711 allocations
    Maximum Heap Size: 8398005 bytes
    52830530 bytes were allocated in 396792 allocations.

Summary:  About a 65K reduction (0.78%) in maximum heap size.  (We could gain a
bit more if we removed |mOrder|.)  No noticeable leaks in the patch.
code size statistics
====================
In non-debug build with RedHat 9's gcc 3.2.2, -Os, and MOZ_PHOENIX the
size of libgklayout.so changes:
                                                                                
with patch: 6476256
without patch: 6585846
                                                                                
The code size improvement is thus 109590 bytes, or 1.7% of the size of
the libgklayout library (layout+content+view, or whatever it is these
days), or 0.5% of the size of what is packaged into dist/Embed by the
embedding manifest.
Attached file patch 1.0 (obsolete) — 
This patch has an improved comment in nsCSSDataBlock.cpp.  Writing that comment
led me to change a good bit of the file -- the way I ensure alignment is now
better, and might lead to slightly smaller structures on some platforms as
well.

I haven't rerun the memory statistics, but they should be very similar.   I
need to run some performance statistics on machines where patches can change
the performance.  Running page load tests on my really fast Linux didn't
provide too much information, although it's possible that the patch slowed
things down a little bit.
Target Milestone: Future → mozilla1.5alpha
I ran some performance tests on jrgm's 400 MHz Pentium II with 128MB RAM running
RedHat 7.2.  I ran pageload tests that showed a slight performance improvement
and startup+shutdown tests that showed only noise.  My attempt to run window
open-close tests was foiled by my window close code not working, and I have up,
since it's late.  However, given the other two I think this is good enough,
performance-wise, to land and see what tinderbox has to say.

My page-load results were:

Baseline:

    Test id: 3EC064A903
    Avg. Median : 1467 msec     Minimum     : 323 msec
    Average     : 1462 msec     Maximum     : 4188 msec

    Test id: 3EC0736B45
    Avg. Median : 1466 msec     Minimum     : 327 msec
    Average     : 1467 msec     Maximum     : 4192 msec

    Test id: 3EC079B38D
    Avg. Median : 1474 msec     Minimum     : 295 msec
    Average     : 1475 msec     Maximum     : 4210 msec

With patch:

    Test id: 3EC06DAB0F
    Avg. Median : 1457 msec     Minimum     : 327 msec
    Average     : 1460 msec     Maximum     : 4150 msec

    Test id: 3EC076D501
    Avg. Median : 1459 msec     Minimum     : 324 msec
    Average     : 1466 msec     Maximum     : 4970 msec

    Test id: 3EC07CB28E
    Avg. Median : 1459 msec     Minimum     : 327 msec
    Average     : 1468 msec     Maximum     : 5009 msec


My startup results, using
  time tcsh -c "repeat 10 ./mozilla /mozilla/close.html"
were:

Baseline:
    real 0m44.763s     user 0m39.120s    sys 0m2.180s
    real 0m44.583s     user 0m38.870s    sys 0m2.290s
    real 0m44.656s     user 0m38.890s    sys 0m2.280s

With patch:
    real 0m45.481s     user 0m38.960s    sys 0m2.090s
    real 0m44.869s     user 0m38.690s    sys 0m2.220s
    real 0m44.708s     user 0m38.740s    sys 0m2.280s
I might add a more detailed description here, later, but the patch
consists of two basic changes:

 1. Convert the nsCSSDeclaration backend to store data as data rather
    than as code -- i.e., replace the huge switch statements in
    nsCSSDeclaration.cpp with information resulting from preprocessing
    nsCSSPropList.h.  (Some tables in nsRuleNode now also use
    preprocessing on nsCSSPropList.h.)

 2. Change the data storage format in nsCSSDeclaration to use less
    memory.  The new format is encapsulated in nsCSSCompressedDataBlock.
    However, parsing and most modification are accomplished by expanding
    into an nsCSSExpandedDataBlock, which uses the old structures,
    except all allocated within a single object rather than as separate
    objects (i.e., object members rather than pointer members).  The
    expanded block is either stack allocated or a member of nsCSSParser.
    (There's actually a second stack-allocated block, used during
    parsing, so that a partially parsed property that contains an error
    doesn't affect the data.  This change also fixes some parsing bugs
    with error handling around '!important' parsing -- although the spec
    isn't entirely clear how such errors should be handled.)

There is some further work to be done -- nsCSSProps.cpp has one
remaining switch statement that I'd like to convert, but I'm leaving it
to an additional patch.
dbaron: Any idea why the "Maximum" values for the page load tests with the patch
are so much higher? Is there a particular page that this patch affects badly?
The maximum values are noisy -- the "with patch" runs contain both the lowest
and the two highest.  I don't think it means anything.
It doesn't contain the minimum value. I wasn't concerned about the other values
because they were all in the margin of error. The maximums, though, are both
about 800ms higher (in the region of 20%), which is considerable, and not within
any sensible error margin.
4150 is smaller than 4188, 4192, and 4210, so it does contain the minimum.

You can't just make up a "sensible error margin".  It depends on the accuracy of
the data, which in this case is rather low.
My bad, I thought you meant the minimum minimum, as opposed to the minimum
maximum, which the with-patch data does indeed contain.

Ok, if you want to do the real maths on this:

   mean(maximum without patch) = 4200ms +/- 11.7ms (to 3sf)
   mean value is within the range 4180ms .. 4210ms (to 3sf)

   mean(maximum with patch)    = 4700ms +/- 485ms (to 3sf)
   mean value is within the range 4220ms .. 5200ms (to 3sf)

Thus, statistically, we should ignore the minimum maximum value (datum 1 in the
with patch data) as an outlier, and the data would support the theory that the
proposed patch increases the maximum time...

As you say though, it might well not mean anything. I was just curious as to
whether it really was an unfortunate noise effect, or if there was something to
it. The only real way to find out would be to get more results.
Whiteboard: [whitebox] → [whitebox][patch]
Some initial comments nsCSSPropList.h: we may want to hoist some comments up
toward the top -- the initial comment only mentions CSS_PROP, and the world is
now more complicated than that.  There needs to be some documentation for
CSS_PROP_INCLUDE_NOT_CSS.  DEFINED_CSS_PROP really meas DEFINED_CSS_PROP_STAR or
something... I was a little confused by it, since that define does _not_ mean we
in fact defined CSS_PROP, unlike the other DEFINED_* defines.  Not sure what a
better name would be there.
+  8. 'iscoord' says whether the property is a coordinate property for
+  which we use an explicit inherit value in the *style structs* (since
+  inheritance requires knowledge of layout).

In theory, the recent changes to CSS2.1's Computed Value line means that this is
no longer necessary, right? Do we want to file a new bug on optimising the code
based on this new world where inheritance is not layout dependent? (or are there
still some properties left that do depend on layout for inheritance?)
Blocks: 52531
Attached file patch 1.1 (obsolete) — 
This incorporates existing comments above and on IRC, and is also more current.
Attachment #122869 - Attachment is obsolete: true
I've skimmed the whole patch, and read carefully up to
nsCSSDeclaration::AppendCSSValueToString, so far.... Here are some comments on
the part I've read.  The only part that's not a nit or minor problem is OOM
handling in nsCSSDataBlock.

> Index: content/base/src/nsRuleNode.cpp

> +// for nsCSSPropList.h
> +#define CSS_PROP_INCLUDE_NOT_CSS

One-line comment on what this is actually supposed to do, please?

For mOffset (in the position struct), maybe use the FOR_CSS_SIDES construct?

> Index: content/html/style/src/nsCSSDataBlock.cpp

Weird comment indentation in the CDB*Storage_advance enum definition.

Document that the non-const getters return pointers that can be assigned into
for the various pointer data types, which is why they return foo*&

Add some debug-only default: cases that assert on unknown types?  This is
applicable to the vast majority of
nsCSSCompressedDataBlock/nsCSSExpandedDataBlock functions.

Would there be any advantage to moving the ValueAt(), RectAt(), etc. calls
inside the target->GetUnit() == eCSSUnit_Null checks (in various
nsCSSCompressedDataBlock functions)?  I doubt it would really
affect perf, so either way is fine, I guess.

Why is nsCSSExpandedDataBlock::RuleDataPropertyAt on nsCSSExpandedDataBlock?
The only caller is nsCSSCompressedDataBlock.  Is there really a good reason for
having kOffsetTable private instead of protected?  (I like the encapsulation we
get from having it private, mind you; if the only drawback is this one code
ugliness, we should probably keep it that way.)

It may make sense to have an inline function for the
|*NS_REINTERPRET_CAST(const nsCSSProperty*, cursor)| bit that we have to do all
over.  PropAtCursor(), perhaps?

The unit != null assertions could use better text than "oops".  Same for the
various null-check assertions in nsCSSCompressedDataBlock::Clone.

In nsCSSCompressedDataBlock::Clone, why is the value being assigned to called
"rval"?  Is that 'r' for "return"?  To me, "rval" recalled "rvalue" too much,
and it's most decidedly an lvalue here... ;)

nsCSSCompressedDataBlock::Clone() needs to hadle OOM in a somewhat more sane
fashion when allocating "result" and the various list types (since the code
assumes that such values stored in a compressed data block are necessarily
non-null).

It would be good to separate the nsCSSExpandedDataBlock impl from the
nsCSSCompressedDataBlock impl somehow (just a /////// line would do fine).

In nsCSSExpandedDataBlock::nsCSSExpandedDataBlock, would it be more efficient
to use memset to zero out those arrays?  Or does it not really matter here?

The way you copy around nsCSSRect and nsCSSValue data is not consistent -- in
nsCSSCompressedDataBlock::Clone you just call RectAt or ValueAt and assign to
the resulting object, while in nsCSSExpandedDataBlock::DoExpand you use memcpy
(granted, you have a void* pointer there.... and the two methods should have
the same end result... but it may be good to be consistent, eg by casting the
void* pointer to the right type and then assigning into it or something?).

In nsCSSExpandedDataBlock::ComputeSize, you have:

>+    NS_ASSERTION(sizeof(nsCSSProperty) + sizeof(void*) <= sizeof(nsCSSValue),
>+                 "need more padding to ensure alignment of list types");

Is that correct?  We're now padding by the difference between
sizeof(nsCSSValue) and sizeof(void*), no?  This assert happens in a few other
places too.  (I'm still slightly fuzzy on this alignment stuff; just getting
this down so I don't forget about it.)

nsCSSExpandedDataBlock::Compress needs to handle OOM when allocating the
compressed blocks.

Do we want an NS_WARNING in nsCSSExpandedDataBlock::Compress if a property bit
is set for a value property but the value is null?  If there is a perf hit from
such things, it would be good to know about them...

Still in Compress(), please comment that you are placement new-ing the val in
the nsCSSValue/nsCSSRect cases to clear the data in it so it won't look like a
real set value anymore?  And wouldn't it be clearer to make use of Reset()
rather than placement new?

We may want to add a function on nsCSSRect that checks whether any of the
sub-values is null.  We're ending up doing that check in a number of places...
Maybe nsCSSRect::HasValue() or something?  (Not necessarily as part of this
patch; I can file a separate bug on this.)

Still in Compress(), weird indentation in the two "size miscalculation"
asserts.

Do we want a helper function (inline?) for those "clear our property set bits"
loops?  In any case, the mPropertiesImportant loop at the end of Compress() is
using NS_ARRAY_LENGTH(mPropertiesSet) as the iteration upper bound; please
change that.

We may want a Reset() on nsCSSRect too (looking at ClearProperty here).

Still in ClearProperty(), for the pointer types you could make val a reference
to a pointer and then not have to do the casting again to set it to null, no?

> Index: content/html/style/src/nsCSSDataBlock.h

Comment nits for nsCSSCompressedDataBlock.  It's not at all clear what
StorageFor actually returns; please document this.  MapRuleInfoInto could maybe
use a line of comment (though this one is more obvious).  What Clone() does on
OOM should be documented.  There is an extraneous comma in the comment for
Destroy().  the operator new() comment calls mBlock_ 'mBlock'.

Could nsCSSExpandedDataBlock::Expand() take references to pointers instead of
just pointers and set them to null as well as destroying them?  That should at
least lead to reliable crashes if someone messes up, instead of bizarre
behavior as deleted memory is accessed.

The comment before ComputeSizeResult refers to mSize; there does not seem to be
an mSize around anymore.

It would be good to document what PropertyAt() returns (so that a casual-ish
reader does not have to sort through the pointer arithmetic).

Similar for RuleDataPropertyAt().

> Index: content/html/style/src/nsCSSDeclaration.cpp

> +  : mOrder(eCSSProperty_COUNT_no_shorthands, 8),

We may want to gather some data on the most appropriate number here... (though
8 seems fairly reasonable based on what CSS I've seen out there).

The only place we add things to mOrder is in ValueAppended(); the only caller
is in the CSS parser.  For shorthands, it looks like the shorthand prop will be
added to the mOrder array but the subprops will not be; I would think we would
want to do the exact opposite, especially given the "max value" number we give
mOrder in our constructor.  Then the code in RemoveProperty() only needs to
remove the subprops in the shorthand case.

As a separate issue, I see very little reason to keep mOrder around.  The only
thing it keeps track of is ordering of longhands, really, and I can't think of
cases when that would actually matter.  The only places we use mOrder, as far
as I can see, are for stringifying the decl and for implementing
nsIDOMCSSStyleDeclaration::Item() (which the spec explicitly says need not be
order-preserving).  This could be a separate patch (probably should be, since
it requires lots of changes to the stringification code).

nsCSSDeclaration::AppendComment() is never called and does nothing.  Any
reasons to not remove it?

Creating a new CSSDeclaration() and calling RemoveProperty() on it immediately
looks like it will assert twice and then crash.  This function should probably
just bail if mData is null, though an assert/warning may be a good
idea... Alternately, we could document in the header that no access should be
made to the declaration until an attempt has been made to parse into it...

Why remove eCSSProperty_text_shadow_color from the list of color properties in
IsColorProperty()?

The rect case in nsCSSDeclaration::AppendValueOrImportantValueToString needs to
insert spaces or better yet ", " between the top/right/bottom/left values.

In the same function, what happens if one of the pointer types is null (this
depends on how you decide to handle OOM situations in the data block code)?

Same function, any reason |nsCSSCounterData* counter| is not a
|const nsCSSCounterData*|? 

Still AppendValueOrImportantValueToString, the shadow code looks like it will
produce an extra trailing space...

Still same function, a debug-only default: case to catch unknown types may be a
good idea.

Is IsColorProperty() called?  May want to check with glazou on it (or write a
testcase?).  I suspect it is called, since the CSS parser makes color keywords
into eCSSUnit_String (see CSSParserImpl::ParseColor(), ident token case).

> Index: content/html/style/src/nsCSSDeclaration.h

Document what the boolean return of AppendCSSValueToString means, please?

I'll have a more detailed response in a bit, but there's one comment that I'm
still thinking about:

> Creating a new CSSDeclaration() and calling RemoveProperty() on it immediately
> looks like it will assert twice and then crash.  This function should probably
> just bail if mData is null, though an assert/warning may be a good
> idea... Alternately, we could document in the header that no access should be
> made to the declaration until an attempt has been made to parse into it...

My intent was that there should be no access until an attempt has been made to
parse into it.  In the current code it looks like that's the case, except for
some failure cases from things within
nsDOMCSSAttributeDeclaration::ParsePropertyValue and
nsDOMCSSAttributeDeclaration::ParseDeclaration (failure of
GetCSSParsingEnvironment, which could be solved by reordering the function, or
failure of the ParseProperty / ParseAndAppendDeclaration calls that they make). 

I'm not quite sure how to solve this, although I'd prefer to keep the model that
an nsCSSDeclaration is invalid unless its |mData| is non-null (which I did say
in the comment next to the definition of |mData|).
As far as I'm concerned, just documenting the access restrictions clearly on
nsCSSDeclaration, and making sure that there are no error cases that put us in
an invalid state, should be fine.  The case I was thinking about was someone
adding code somewhere that creates nsCSSDeclaration objects and messes with
them; if we clearly document when one can and cannot mess with them, that should
be sufficient (eg ValueAppended could/should be called on an "invalid"
nsCSSDeclaration in the current model, while other functions should not be...).
 Asserts of state in all the functions as needed would also make things clearer
to someone reading the code.
Blocks: 205850
Attached file patch 1.2 (obsolete) — 
> > Index: content/base/src/nsRuleNode.cpp
>
> > +// for nsCSSPropList.h
> > +#define CSS_PROP_INCLUDE_NOT_CSS
>
> One-line comment on what this is actually supposed to do, please?
 
Done.
 
> For mOffset (in the position struct), maybe use the FOR_CSS_SIDES construct?
 
Done.
 
> > Index: content/html/style/src/nsCSSDataBlock.cpp
>
> Weird comment indentation in the CDB*Storage_advance enum definition.
 
Fixed, although I sort of liked it...
 
> Document that the non-const getters return pointers that can be assigned into

> for the various pointer data types, which is why they return foo*&
 
Done (with a big comment at the beginning of the bunch of functions).
 
> Add some debug-only default: cases that assert on unknown types?  This is
> applicable to the vast majority of
> nsCSSCompressedDataBlock/nsCSSExpandedDataBlock functions.
 
I'd prefer not to (and I haven't yet).	I'm hoping that some compilers
might be able to generate better code given that it's an enum with a
limited set of values, when there's no default case.  Furthermore, it
would be quite obvious if we hit one of these cases, since we'd go into
an infinite loop.
 
> Would there be any advantage to moving the ValueAt(), RectAt(), etc. calls
> inside the target->GetUnit() == eCSSUnit_Null checks (in various
> nsCSSCompressedDataBlock functions)?	I doubt it would really
> affect perf, so either way is fine, I guess.
 
I'm not sure which ones you were talking about.
 
> Why is nsCSSExpandedDataBlock::RuleDataPropertyAt on nsCSSExpandedDataBlock?
> The only caller is nsCSSCompressedDataBlock.	Is there really a good reason
for
> having kOffsetTable private instead of protected?  (I like the encapsulation
we
> get from having it private, mind you; if the only drawback is this one code
> ugliness, we should probably keep it that way.)
 
I wasn't crazy about this, either (and I changed it a few times while I
was writing it), but I didn't see a better alternative.
 
> It may make sense to have an inline function for the
> |*NS_REINTERPRET_CAST(const nsCSSProperty*, cursor)| bit that we have to do
all
> over.  PropAtCursor(), perhaps?
 
Done.  I renamed all the other *At functions to be *AtCursor as well.
 
> The unit != null assertions could use better text than "oops".  Same for the
> various null-check assertions in nsCSSCompressedDataBlock::Clone.
 
I'm lazy, and the code in the assertion should be good enough.	(After
all, in many APIs, assertion macros have no separate text -- you just
see the code of the assertion.)  I'd rather encourage people to write
assertions with poor text than have no assertions at all.
 
> In nsCSSCompressedDataBlock::Clone, why is the value being assigned to called

> "rval"?  Is that 'r' for "return"?  To me, "rval" recalled "rvalue" too much,

> and it's most decidedly an lvalue here... ;)
 
Well, I change it to result_val.  And then I removed the variable
entirely, since the RectAtCursor, ValueAtCursor, etc., allow the whole
thing to fit on one line.  (Can you believe early version of the patch
didn't have those functions? :-)
 
> nsCSSCompressedDataBlock::Clone() needs to hadle OOM in a somewhat more sane
> fashion when allocating "result" and the various list types (since the code
> assumes that such values stored in a compressed data block are necessarily
> non-null).
 
Done, although:
 * the nsCSSValue assignment operator doesn't propagate OOM
 * I did an ugly switch-within-switch to avoid code duplication.  I
   almost used goto instead.  Perhaps I should.
 
> It would be good to separate the nsCSSExpandedDataBlock impl from the
> nsCSSCompressedDataBlock impl somehow (just a /////// line would do fine).
 
Done.
 
> In nsCSSExpandedDataBlock::nsCSSExpandedDataBlock, would it be more efficient

> to use memset to zero out those arrays?  Or does it not really matter here?
 
Done.  Except I put that it a new method called |ClearSets|.
 
> The way you copy around nsCSSRect and nsCSSValue data is not consistent -- in

> nsCSSCompressedDataBlock::Clone you just call RectAt or ValueAt and assign to

> the resulting object, while in nsCSSExpandedDataBlock::DoExpand you use
memcpy> (granted, you have a void* pointer there.... and the two methods should
have
> the same end result... but it may be good to be consistent, eg by casting the

> void* pointer to the right type and then assigning into it or something?).
 
This is intentional.  There are two separate things going on here --
Clone is actually duplicating existing data, while Expand and Compress
are transferring ownership by moving chunks of memory around.  I added
comments to DoExpand and Compress to explain this.
 
> In nsCSSExpandedDataBlock::ComputeSize, you have:
>
> >+	NS_ASSERTION(sizeof(nsCSSProperty) + sizeof(void*) <=
sizeof(nsCSSValue),
> >+		     "need more padding to ensure alignment of list types");
>
> Is that correct?  We're now padding by the difference between
> sizeof(nsCSSValue) and sizeof(void*), no?  This assert happens in a few other

> places too.  (I'm still slightly fuzzy on this alignment stuff; just getting
> this down so I don't forget about it.)
 
These assertions were obsolete -- related to my old strategy for
ensuring alignment -- and I've removed them.
 
> nsCSSExpandedDataBlock::Compress needs to handle OOM when allocating the
> compressed blocks.
 
Done, although the caller still isn't handling it properly...
 
> Do we want an NS_WARNING in nsCSSExpandedDataBlock::Compress if a property
bit> is set for a value property but the value is null?  If there is a perf hit
from
> such things, it would be good to know about them...
 
Not yet.  I might try to make it exact in a later patch, but I think I
still have some cases where it isn't.  I'm not sure, though.
 
> Still in Compress(), please comment that you are placement new-ing the val in

> the nsCSSValue/nsCSSRect cases to clear the data in it so it won't look like
a> real set value anymore?  And wouldn't it be clearer to make use of Reset()
> rather than placement new?
 
No, because I'm transferring ownership (see above, about Clone -- I
added a comment here too).
 
> We may want to add a function on nsCSSRect that checks whether any of the
> sub-values is null.  We're ending up doing that check in a number of
places...> Maybe nsCSSRect::HasValue() or something?  (Not necessarily as part
of this
> patch; I can file a separate bug on this.)
 
Maybe.	Nothing done for now.
 
> Still in Compress(), weird indentation in the two "size miscalculation"
> asserts.
 
I added a new |DataSize| method so that they could fit on one line.
 
> Do we want a helper function (inline?) for those "clear our property set
bits"> loops?  In any case, the mPropertiesImportant loop at the end of
Compress() is> using NS_ARRAY_LENGTH(mPropertiesSet) as the iteration upper
bound; please
> change that.
 
Separate function done (see above).
 
> We may want a Reset() on nsCSSRect too (looking at ClearProperty here).
 
Nothing done for now.
 
> Still in ClearProperty(), for the pointer types you could make val a
reference> to a pointer and then not have to do the casting again to set it to
null, no?
 
Done.
 
> > Index: content/html/style/src/nsCSSDataBlock.h
>
> Comment nits for nsCSSCompressedDataBlock.  It's not at all clear what
> StorageFor actually returns; please document this.  MapRuleInfoInto could
maybe
> use a line of comment (though this one is more obvious).  What Clone() does
on> OOM should be documented.  There is an extraneous comma in the comment for
> Destroy().  the operator new() comment calls mBlock_ 'mBlock'.
 
Done.
 
> Could nsCSSExpandedDataBlock::Expand() take references to pointers instead of

> just pointers and set them to null as well as destroying them?  That should
at> least lead to reliable crashes if someone messes up, instead of bizarre
> behavior as deleted memory is accessed.
 
Done.  I wanted to avoid this because I didn't want to be working with a
reference the whole time, but now that I have DoExpand I can just pass a
pointer to DoExpand.
 
> The comment before ComputeSizeResult refers to mSize; there does not seem to
be
> an mSize around anymore.
 
Fixed.
 
> It would be good to document what PropertyAt() returns (so that a casual-ish
> reader does not have to sort through the pointer arithmetic).
>
> Similar for RuleDataPropertyAt().
 
Done.
 
> > Index: content/html/style/src/nsCSSDeclaration.cpp
>
> > +  : mOrder(eCSSProperty_COUNT_no_shorthands, 8),
>
> We may want to gather some data on the most appropriate number here...
(though> 8 seems fairly reasonable based on what CSS I've seen out there).
>
> The only place we add things to mOrder is in ValueAppended(); the only caller

> is in the CSS parser.  For shorthands, it looks like the shorthand prop will
be
> added to the mOrder array but the subprops will not be; I would think we
would> want to do the exact opposite, especially given the "max value" number
we give> mOrder in our constructor.  Then the code in RemoveProperty() only
needs to
> remove the subprops in the shorthand case.
>
> As a separate issue, I see very little reason to keep mOrder around.	The
only> thing it keeps track of is ordering of longhands, really, and I can't
think of> cases when that would actually matter.  The only places we use
mOrder, as far
> as I can see, are for stringifying the decl and for implementing
> nsIDOMCSSStyleDeclaration::Item() (which the spec explicitly says need not be

> order-preserving).  This could be a separate patch (probably should be, since

> it requires lots of changes to the stringification code).
 
I didn't want to touch mOrder in this patch.  We can discuss what to do
about it in another bug.  (I think there may already be a bug about it.
Daniel wants it for an "editor mode".)
 
> nsCSSDeclaration::AppendComment() is never called and does nothing.  Any
> reasons to not remove it?
 
Another patch, perhaps?  (Daniel may want this in an "editor mode".)
 
> Creating a new CSSDeclaration() and calling RemoveProperty() on it
immediately> looks like it will assert twice and then crash.  This function
should probably> just bail if mData is null, though an assert/warning may be a
good
> idea... Alternately, we could document in the header that no access should be

> made to the declaration until an attempt has been made to parse into it...
 
I documented that the constructor leaves things in an invalid state, and
added nsCSSDeclaration::InitializeEmpty and
nsCSSCompressedDataBlock::CreateEmptyBlock (the former calls the latter,
and nsDOMCSSAttributeDeclaration::GetCSSDeclaration calls the former).
 
> Why remove eCSSProperty_text_shadow_color from the list of color properties
in> IsColorProperty()?
 
It doesn't exist anymore.  And the text-shadow output code actually has
a slight chance of working now.
 
> The rect case in nsCSSDeclaration::AppendValueOrImportantValueToString needs
to
> insert spaces or better yet ", " between the top/right/bottom/left values.
 
Done.
 
> In the same function, what happens if one of the pointer types is null (this
> depends on how you decide to handle OOM situations in the data block code)?
 
These things aren't supposed to be null, and Clone should be fixed now
to prevent them from ever being null.
 
> Same function, any reason |nsCSSCounterData* counter| is not a
> |const nsCSSCounterData*|?
 
Fixed.
 
> Still AppendValueOrImportantValueToString, the shadow code looks like it will

> produce an extra trailing space...
 
Fixed, and made it add a comma as well.
 
> Still same function, a debug-only default: case to catch unknown types may be

a
> good idea.
 
Same reason as before why I don't want this.
 
> Is IsColorProperty() called?	May want to check with glazou on it (or write a

> testcase?).  I suspect it is called, since the CSS parser makes color
keywords> into eCSSUnit_String (see CSSParserImpl::ParseColor(), ident token
case).
 
It is called, but I'm hoping that we never hit that code since we
*should* be using eCSSUnit_Color for that.  Or if we are hitting it,
then perhaps we should remove eCSSUnit_Color.
 
But I should really investigate this a little more before I check in...
 
> > Index: content/html/style/src/nsCSSDeclaration.h
>
> Document what the boolean return of AppendCSSValueToString means, please?
 
Done.  And the same for AppendValueOrImportantValueToString also done.
> I'd prefer not to (and I haven't yet).	I'm hoping that some compilers
> might be able to generate better code

I had meant something like:

#ifdef DEBUG
                 default:
                    NS_NOTREACHED("Unknown data type");
                    break;
#endif

so it would be a non-issue for performance.  I agree that we would probably
notice quickly if one of these places did not get updated, so I'm ok with it
either way, I guess.

The target->GetUnit() == eCSSUnit_Null checks I meant were, e.g. the one in
nsCSSCompressedDataBlock::MapRuleInfoInto in the |case eCSSType_Value| block of
the |nsCSSProps::kSIDTable[iProp] == aRuleData->mSID| branch.  Same for the
other cases of that switch.

> I didn't want to touch mOrder in this patch.

That's fine; just fix the problem of ValueAppended putting the shorthand prop
into the array instead of putting in the corresponding longhand props?  Without
that, your changes look like they will pretty much completely break shorthand
serialization from nsCSSDeclaration.  I'll attach a testcase for this and for
the color thing (eCSSUnit_Color means the CSSValue stores an rgba color; it
looks like for color names we actually store an integer referring to the color
name in the cssvalue and do the conversion to rgba in nsRuleNode, in the
SetColor() function).
Oh, and the other reason removing the text_shadow_color thing from
IsColorProperty is OK is that I changed the shadow output code to say that the
property was color, just for the color part.  But I just changed that bit back
to a switch statement in my tree.
Attached file patch 1.3 (obsolete) — 
This fixes the color stuff, fixes the "inside the eCSSUnit_Null" stuff, and
unmakes IsColorProperty (turns it back into a switch).
Attachment #124941 - Attachment is obsolete: true
Attachment #124972 - Attachment is patch: false
Attachment #124972 - Attachment mime type: text/plain → application/x-gzip
More comments on patch, starting where I left off last time, pretty much.  These
apply to "patch 1.3".  Not much left to go at this point; I'm hoping to finish
up sometime tomorrow or at worst of Friday.

> Index: content/html/style/src/nsCSSDeclaration.cpp

In nsCSSDeclaration::AppendCSSValueToString, in the color name code, the tmpStr
declaration could be moved to the |else| where it's actually used.

In nsCSSDeclaration::GetValue(nsCSSProperty, nsAString&):

  It looks like our existing code also screws up in the case when a shorthand
  has both !important and not !important parts.  File a bug to deal with your
  XXX comment, please?  It would be good to decide what exactly we should
  return in such cases....  Same applies to "border".

  In the 'background' section of this function, please put in some line breaks
  before the if bodies to bring the code closer to 80 chars in width?  I know
  you just renamed the function here, but....

  Does 'border-spacing' need an assert similar to the one 'background-position'
  has?

  Add some line breaks in the 'list-style' section (before the if bodies).

  Would it make sense to reverse the shorthand-or-'play-during' test, do an
  early return after calling AppendCSSValueToString in the simple case, then
  outdent all that shorthand code?

> Index: content/html/style/src/nsCSSDeclaration.h

Document when ValueAppended should be called and who is responsible for calling
it?

Document what GetValueOrImportantValue does when the data for that property ID
is not a nsCSSValue?

CompressFrom needs better text in the assertions, as does ExpandTo.  Both
should assert that aExpandedData is not null.

Make it clearer that the only access allowed to a nsCSSDeclaration after
ExpandTo has been called is calling CompressFrom or ValueAppended?  More to the
point, things like ExpandTo or ClearData should be clearly prohibited.

> Index: content/html/style/src/nsCSSParser.cpp

|CSSParserImpl::ParseProperty(const nsAString& aPropName, ...| never calls
ExpandTo() on the aDeclaration, but _does_ call CompressFrom.  Won't this wipe
out all the data in the declaration block?  I think we just want to ExpandTo
before calling the internal ParseProperty and to CompressFrom unconditionally,
even if there was an error according to |errorCode|.

CSSParserImpl::DoTransferTempData could use better assert text than "oops" and
could use another "using memcpy and placement new to avoid calling destructors"
comment.

Same function, could use a debug-only default: case that asserts.

Looking over this code, could we introduce a nsCSSValuePair type of data and
then eliminate the -x-* properties for 'border-spacing', 'size', 'play-during',
'background-position'?  That could simplify some more code, and arguably make
some more correct (see the XXX comment you added in nsCSSPropList.h).  Probably
do this as a separate patch after this one lands...

In ParseProperty (the internal one), we call ValueAppended any time
ParseProperty succeeds.  This can cause mOrder to get out of sync with the
actual stored data in nsCSSDeclaration. For example, consider the following
stylesheet:

  "body { color: green ! foo; background-color: white }"

This will succeed in ParseProperty for 'color', then fail the check for
"important" in ParseDeclaration, clear the temp data, and bail out... but the
nsCSSDeclaration will think it has a 'color' set.  Perhaps it would be better
to call ValueAppended from TransferTempData once the transfer succeeds?  Then
we could easily call ValueAppended for longhand properties only, which is sort
of what we want anyway....

In ParseBorderColors, could we get the hint from nsCSSProps::kHintTable instead
of hardcoding the VISUAL hint?  I think we want to try and limit the number of
places that have prop-to-hint mappings to no more than two (kHintTable and the
nsStyleContext code)....

> +static void SetAllRectValues(nsCSSRect& aRect, const nsCSSValue& aValue)

Wouldn't this be better off as a member method on nsCSSRect?  Granted, I can't
really see anyone else calling it...

In DoParseRect, why have the |values| temporary?  The only caller is ParseRect,
and that passes in a temporary as aRect anyway.  Why not just parse directly
into the members of aRect (using the nsCSSRect::sides stuff)?

In ParseContent, the inherit/initial case looks like it should trigger the type
error assert in AppendValue (since 'content' is a valuelist type).  Just
removing that whole |if| block should work correctly (compare to
ParseBorderColors, eg).

CSSParserImpl::SetSingleCounterValue could use an OOM check after that new
call... then again, a lot of other places in nsCSSParser could use such checks
too.  :(

Same function, shouldn't we delete *aResult?  We do it elsewhere in similar
circumstances...

CSSParserImpl::ParseCounterData second ExpectEndProperty check in the main
while loop (in the "number" case), need to delete *aResult?

CSSParserImpl::ParseCursor, do we need to delete
mTempData.mUserInterface.mCursor before assigning to it?

CSSParserImpl::ParseQuotes, do we need to delete mTempData.mContent.mQuotes
before assigning to it?

CSSParserImpl::ParseTextShadow, do we need to delete
mTempData.mText.mTextShadow before assigning to it?

> Index: content/html/style/src/nsCSSStruct.cpp

Don't you need a MOZ_DECL_CTOR_COUNTER(nsCSSValueListRect) somewhere?  I guess
refcount logging doesn't actually do anything useful with that at the moment...

I'm a little confused by the "// temp fix for bug 24000" comments and the
removal of the mPageBreakAfter/mPageBreakBefore members... According to lxr,
those are not really used anywhere, but neither is mPageBreakInside...  Is any
of the nsCSSBreaks stuff used anywhere?

> Index: content/shared/public/nsCSSPropList.h

Maybe the comments should explain that SHORTHAND and INTERNAL properties
only get the first 4 of the 8 args?

Some attempt at 80-char-wide lines would be nice...
Attached file patch 1.4 (obsolete) — 
> > Index: content/html/style/src/nsCSSDeclaration.cpp
>
> In nsCSSDeclaration::AppendCSSValueToString, in the color name code, the
tmpStr
> declaration could be moved to the |else| where it's actually used.
 
Done.
 
> In nsCSSDeclaration::GetValue(nsCSSProperty, nsAString&):
>
>   It looks like our existing code also screws up in the case when a shorthand

>   has both !important and not !important parts.  File a bug to deal with your

>   XXX comment, please?  It would be good to decide what exactly we should
>   return in such cases....  Same applies to "border".
 
*** Need to do this.
 
>   In the 'background' section of this function, please put in some line
breaks
>   before the if bodies to bring the code closer to 80 chars in width?  I know

>   you just renamed the function here, but....
 
Done.  I also did another rename (since a layer of function call has
been removed) so that what's being called here is now
AppendValueToString instead of AppendValueOrImportantValueToString.
 
>   Does 'border-spacing' need an assert similar to the one
'background-position'
>   has?
 
Done.
 
>   Add some line breaks in the 'list-style' section (before the if bodies).
 
Done.
 
>   Would it make sense to reverse the shorthand-or-'play-during' test, do an
>   early return after calling AppendCSSValueToString in the simple case, then
>   outdent all that shorthand code?
 
Done.
 
> > Index: content/html/style/src/nsCSSDeclaration.h
>
> Document when ValueAppended should be called and who is responsible for
calling
> it?
 
Done.
 
> Document what GetValueOrImportantValue does when the data for that property
ID
> is not a nsCSSValue?
 
Done (it's invalid to call it).
 
> CompressFrom needs better text in the assertions, as does ExpandTo.  Both
> should assert that aExpandedData is not null.
 
I disagree.  I think assertion expressions are often self-explanatory,
and people who pass null out parameters can happily crash without an
assertion.
 
> Make it clearer that the only access allowed to a nsCSSDeclaration after
> ExpandTo has been called is calling CompressFrom or ValueAppended?  More to
the
> point, things like ExpandTo or ClearData should be clearly prohibited.
 
Done.
 
> > Index: content/html/style/src/nsCSSParser.cpp
>
> |CSSParserImpl::ParseProperty(const nsAString& aPropName, ...| never calls
> ExpandTo() on the aDeclaration, but _does_ call CompressFrom.  Won't this
wipe
> out all the data in the declaration block?  I think we just want to ExpandTo
> before calling the internal ParseProperty and to CompressFrom
unconditionally,
> even if there was an error according to |errorCode|.
 
Done.
 
> CSSParserImpl::DoTransferTempData could use better assert text than "oops"
and
> could use another "using memcpy and placement new to avoid calling
destructors"
> comment.
 
The latter is done.
 
> Same function, could use a debug-only default: case that asserts.
 
As I've said before, I really don't like such things.
 
> Looking over this code, could we introduce a nsCSSValuePair type of data and
> then eliminate the -x-* properties for 'border-spacing', 'size',
'play-during',
> 'background-position'?  That could simplify some more code, and arguably make

> some more correct (see the XXX comment you added in nsCSSPropList.h). 
Probably
> do this as a separate patch after this one lands...
 
*** todo
 
> In ParseProperty (the internal one), we call ValueAppended any time
> ParseProperty succeeds.  This can cause mOrder to get out of sync with the
> actual stored data in nsCSSDeclaration. For example, consider the following
> stylesheet:
>
>   "body { color: green ! foo; background-color: white }"
>
> This will succeed in ParseProperty for 'color', then fail the check for
> "important" in ParseDeclaration, clear the temp data, and bail out... but the

> nsCSSDeclaration will think it has a 'color' set.  Perhaps it would be better

> to call ValueAppended from TransferTempData once the transfer succeeds?  Then

> we could easily call ValueAppended for longhand properties only, which is
sort
> of what we want anyway....
 
Fixed.	(The mTempData stuff was one of the last parts of the patch I
wrote -- to fix existing bugs).
 
> In ParseBorderColors, could we get the hint from nsCSSProps::kHintTable
instead
> of hardcoding the VISUAL hint?  I think we want to try and limit the number
of
> places that have prop-to-hint mappings to no more than two (kHintTable and
the
> nsStyleContext code)....
 
Done, although I want to get rid of those hints completely.
 
> > +static void SetAllRectValues(nsCSSRect& aRect, const nsCSSValue& aValue)
>
> Wouldn't this be better off as a member method on nsCSSRect?	Granted, I
can't
> really see anyone else calling it...
 
Done.
 
> In DoParseRect, why have the |values| temporary?  The only caller is
ParseRect,
> and that passes in a temporary as aRect anyway.  Why not just parse directly
> into the members of aRect (using the nsCSSRect::sides stuff)?
 
Done.
 
> In ParseContent, the inherit/initial case looks like it should trigger the
type
> error assert in AppendValue (since 'content' is a valuelist type).  Just
> removing that whole |if| block should work correctly (compare to
> ParseBorderColors, eg).
 
Fixed, although I needed a test within the loop to make sure we don't
accept 'initial "foo"'.
 
> CSSParserImpl::SetSingleCounterValue could use an OOM check after that new
> call... then again, a lot of other places in nsCSSParser could use such
checks
> too.	:(
 
Fixed.	(Changed return type to PRBool and added aErrorCode parameter.)
 
> Same function, shouldn't we delete *aResult?	We do it elsewhere in similar
> circumstances...
>
> CSSParserImpl::ParseCounterData second ExpectEndProperty check in the main
> while loop (in the "number" case), need to delete *aResult?
>
> CSSParserImpl::ParseCursor, do we need to delete
> mTempData.mUserInterface.mCursor before assigning to it?
>
> CSSParserImpl::ParseQuotes, do we need to delete mTempData.mContent.mQuotes
> before assigning to it?
>
> CSSParserImpl::ParseTextShadow, do we need to delete
> mTempData.mText.mTextShadow before assigning to it?
 
No (to all), since mTempData is cleared for every new declaration parsed.
 
> > Index: content/html/style/src/nsCSSStruct.cpp
>
> Don't you need a MOZ_DECL_CTOR_COUNTER(nsCSSValueListRect) somewhere?  I
guess
> refcount logging doesn't actually do anything useful with that at the
moment...
 
Done, although we should probably remove the macro entirely.
 
> I'm a little confused by the "// temp fix for bug 24000" comments and the
> removal of the mPageBreakAfter/mPageBreakBefore members... According to lxr,
> those are not really used anywhere, but neither is mPageBreakInside...  Is
any
> of the nsCSSBreaks stuff used anywhere?
 
Not really.  It was intended to be, but when someone wanted to use only
two members within it, they didn't want a new style struct.  (I think
aftr this patch we shouldn't be restricted to ensuring that the nsCSS*
-> nsStyle* mapping is a function.)
 
> > Index: content/shared/public/nsCSSPropList.h
>
> Maybe the comments should explain that SHORTHAND and INTERNAL properties
> only get the first 4 of the 8 args?
 
I think it's obvious.
 
> Some attempt at 80-char-wide lines would be nice...
 
I really don't want to attempt that.
Attachment #124972 - Attachment is obsolete: true
Just looking at the changes you say you made from 1.3 to 1.4:

The "return" at the top of nsCSSDeclaration::GetValue should be |return NS_OK;|,
since the function returns nsresult.

The nsCSSDeclaration::RemoveProperty() code has a GetValueIsImportant(aProperty)
check that looks wrong.

nsCSSParser::ParseProperty(nsAString &, ...) will still blow away all existing
inline style in the case when an attempt is set to set an invalid inline style
(eg |.style.top = 15| in standards mode).  We want to always CompressFrom in
this function before returning; we just don't always want to update the hint or
TransferTempData.

> No (to all), since mTempData is cleared for every new declaration parsed.

Then we also should not call |delete *aResult;| in ParseBorderColors() and
ParseCounterData() (_first_ ExpectEndProperty check).

It sounds like we need a separate bug to sort out the nsCSSBreaks business.

> I really don't want to attempt that.

I can understand that.  :)

Comments on what remains of the patch coming up in a few hours, with luck.
Comments on rest of patch 1.4:

> Index: content/shared/public/nsCSSPropList.h

Let me see if I understand the situation correctly.  Field 5 in nsCSSPropList.h
contains the name of the ruledata struct that holds the property value in the
ruledata.  The macro name in nsCSSPropList.h matches the style struct the
computed style for the property lives in.  The mSID variable of nsRuleData
contains the name of the style struct being resolved.  So when a nsRuleData
struct has mSID == eStyleStruct_Visibility, we want to store things in the
.mDisplayData member of the nsRuleData, since that's the ruledata struct which
lets us compute an nsStyleVisibility.

Is this correct?  If so, could we document the difference between field 5 and
the macro name clearly?  If not, could we document what's going on, because if
I can't figure it out, that's not a good sign.... ;)

> Index: content/shared/public/nsCSSProperty.h

The header guard should match the header name.

> +// The types of values that can be in these structs.  See

In which structs?  nsCSS*?  nsRuleData?  Or style structs?  I assume this means
nsCSS* structs?

I have to say, I love the huge chunks of code that got removed.... :)
Here are the outstanding issues left after the patch lands; I just culled them
from the discussion with no filtering for now, so we don't have to dig for them:

1) Maybe add a function on nsCSSRect that returns true if and only if at least
   one of the members has non-null unit.
2) Maybe add a Reset() function on nsCSSRect.
3) Decide whether we need mOrder in nsCSSDeclaration.
4) Decide whether we need AppendComment on nsCSSDeclaration.
5) Create a nsCSSValuePair for 'border-spacing', 'size', 'play-during',
   'background-position'.
6) nsCSSDeclaration::GetValue has issues when a shorthand prop is asked
   for and we have mixed !important and not-!important longhands for
   that shorthand.

Once this lands, we should file some bugs (or look for existing ones for #3 and #4).
Attached file patch 1.5 — 
I made sure my build finished this time. :-)

This should address all of the comments in comment 43 and comment 44.

To respond to the first comment in comment 44:	the macro name is which
nsStyle* the property goes in, but the fifth field is which nsRuleData*/nsCSS*
it is in.
Attachment #125031 - Attachment is obsolete: true
Attachment #125067 - Flags: superreview?(bz-bugspam)
Attachment #125067 - Flags: review?(bz-bugspam)
Comment on attachment 125067 [details]
patch 1.5

r+sr=me.  (One caveat is that when the patch got updated I only reread the
parts that I had commented on and that you said you changed (because there was
no way I was reading the whole thing over again).  That should be just fine,
but let me know if there is something else I should look at again.)

Land this baby!
Attachment #125067 - Flags: superreview?(bz-bugspam)
Attachment #125067 - Flags: superreview+
Attachment #125067 - Flags: review?(bz-bugspam)
Attachment #125067 - Flags: review+
Fix checked in 2003-06-07 15:13/14 -0700, with bustage fixes at 16:29 and 17:36.

Filed additional bugs:
 * bug 208726
 * bug 208727 (comment 45, issue 3)
 * bug 208728 (comment 45, issue 4)
 * bug 208729 (comment 45, issue 5)
 * bug 208730 (comment 45, issue 6)
 * bug 208731 (comment 45, issues 1 and 2)
 * bug 208732

Tinderbox performance statistics didn't show any significant changes for any of
the numbers, as far as I noticed.

Tinderbox codesize statistics:

Linux comet:
Z:20.00MB
Zdiff:-99708 (+67043/-166751)
mZ:12.87MB
mZdiff:-101020 (+65219/-166239)

Linux luna:
Z:19.01MB
Zdiff:-112501 (+47493/-159994)
mZ:12.23MB
mZdiff:-112059 (+45833/-157892)

WINNT 5.0 beast:
Z:14.44MB
Zdiff:-51777 (+27564/-79341)
mZ:8.139MB
mZdiff:-53033 (+25896/-78929)

Linux egg dep RH8 (minimo):
mZ:7.774MB
mZdiff:-92247 (+39660/-131907)
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Oh, and the MH (trace-malloc bloat stats max heap) number on brad fell from
7.81MB to 7.72MB, and total allocations (A) fell from 252K to 248K.
Blocks: 208868
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: