277135 - [svg sr] style system

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Files:

  layout/style/nsCSSDataBlock.h
  layout/style/nsCSSDeclaration.cpp
  layout/style/nsCSSDeclaration.h
  layout/style/nsCSSKeywordList.h
  layout/style/nsCSSParser.cpp
  layout/style/nsCSSPropList.h
  layout/style/nsCSSProps.cpp
  layout/style/nsCSSProps.h
  layout/style/nsCSSStruct.cpp
  layout/style/nsCSSStruct.h
  layout/style/nsICSSParser.h
  layout/style/nsRuleData.h
  layout/style/nsRuleNode.cpp
  layout/style/nsRuleNode.h
  layout/style/nsStyleContext.cpp
  layout/style/nsStyleStruct.cpp
  layout/style/nsStyleStruct.h
  layout/style/nsStyleStructList.h

Comment 1

[tor]

Attachment: dummy attachment for bug tracking purposes

Comment 2

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

These files are fine:
  layout/style/nsCSSDataBlock.h
  layout/style/nsCSSDeclaration.cpp
  layout/style/nsCSSDeclaration.h
  layout/style/nsICSSParser.h
  layout/style/nsRuleData.h
  layout/style/nsRuleNode.h

These files I still have to review:
  layout/style/nsCSSKeywordList.h
  layout/style/nsCSSParser.cpp
  layout/style/nsCSSPropList.h
  layout/style/nsCSSProps.cpp
  layout/style/nsCSSProps.h
  layout/style/nsCSSStruct.cpp
  layout/style/nsCSSStruct.h
  layout/style/nsRuleNode.cpp
  layout/style/nsStyleContext.cpp
  layout/style/nsStyleStruct.cpp
  layout/style/nsStyleStruct.h
  layout/style/nsStyleStructList.h

Comment 3

[tor]

Missed file in original list:

  layout/base/nsStyleConsts.h

Comment 4

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

Review of style system SVG code at
checkout start: Wed Mar 9 10:41:26 EST 2005

  layout/style/nsCSSKeywordList.h
    The following new keywords are not needed:
      stop-color
      stop-opacity
  layout/style/nsCSSParser.cpp
    Do we implement in the SVG layout code all the properties that we
    parse and compute?  If not, we shouldn't parse them.

    In ParseSingleValueProperty, 'stroke-dashoffset' and 'stroke-width'
    should probably not take VARIANT_NUMBER (N).  Likewise for
    ParseDasharray.  This would allow simplification of SetSVGLength in
    nsRuleNode.cpp.  But perhaps we should leave it, but at least
    comment that it's noncompliant with CSS.

    In ParseSingleValueProperty, 'pointer-events' should use
    VARIANT_NONE (and the keyword should be removed in nsCSSProps.cpp
    and the code fixed in nsRuleNode.cpp).

    The XXX value > 1 comment for stroke-miterlimit really belongs
    elsewhere, since it's not a parser error according to the SVG spec
    (although it probably should be) -- it's the document needing to be
    "in error".  That said, if you want to implement it as a parser
    error, please do so.  It's much better there than in nsRuleNode.

    ParseDasharray does not enforce that 'none' must be the only element
    in the list (like it does for 'inherit' and 'initial').

    ParseDasharray could also probably be written with less duplicated
    code, as ParseQuotes is.

    In ParseDasharray, the case that handles 'inherit' and 'initial'
    being the only thing in the list should break instead of returning
    false; otherwise it leaks.

    In ParseDasharray, the if(A) {} else { break; } should just become
    if (!A) break; ...
    (where A is the inner ParseVariant call).

    The loop in ParseDasharray should probably be a for(;;) and the
    out-of-memory case in the middle should just have an explicit break.
    If not, then it should at least be a do-while loop.

  layout/style/nsCSSPropList.h
    The first two and last two ifdefs in the file don't need ifdefs, and
    that would probably make it a little clearer.

    'stop-color' and 'stop-opacity' are not inherited.  This means they
    need to be declared CSS_PROP_SVGRESET and need to move structs
    (changes to pretty much all files involved in implementing them
    except the CSS parser).

    'stroke-dasharray' needs to be eCSSType_ValueList, not Value.

  layout/style/nsCSSProps.cpp
    'pointer-events' should use VARIANT_NONE in the CSS parser.  This
    means removing the none line here as well and fixing nsRuleNode to
    compute things correctly.

    The kSIDTable stuff doesn't need ifdefs, the macros simply won't be
    used when MOZ_SVG isn't defined.

  layout/style/nsCSSProps.h
    All OK

  layout/style/nsCSSStruct.cpp
    At least copy the XXX comment from the mQuotes code in its ::List
    method.

  layout/style/nsCSSStruct.h
    All OK

  layout/style/nsRuleNode.cpp

    In SetSVGPaint, is the XXX comment now no longer needed?  (I'm
    guessing so, since it looks like SetColor handles it.)  If this is
    the case, please remove it.  If not, please fix it.

    In SetSVGOpacity, the number handling code needs to clamp the value
    between 0.0 and 1.0.  The SVG spec is vague, but I think nsRuleNode
    is the right place based on other precedents in the CSS spec.

    SetSVGLength needs to pass aInherited through to SetCoord, not use
    |dummy|.

    SetSVGLength needs to handle percentages; it tells SetCoord that
    they're OK but doesn't handle them correctly.  You probably need to
    make the values in the structs use nsStyleCoord to do this.

    SetSVGLength should not test for pixels-to-twips being zero; this
    should never be the case.  Assert if you want.  And if you want to
    go through the device context code and really make sure, you're
    welcome to, but we shouldn't have runtime checks for that at any
    time other than when we set it.

    In the dasharray code, use preincrement rather than postincrement
    for           svg->mStrokeDasharrayLength++;
    (Always use preincrement unless you need postincrement.)

    For the stroke-miterlimit check, the check (XXX comment) shouldn't
    be in nsRuleNode per the spec.  I wouldn't mind if it were
    implemented in the parser; that's more consistent with CSS, and
    doing it right is both hard and disagrees with the CSS spec..

    In the beginning of ComputeSVGResetData, the initial assignment of
    svgReset to null is unnecessary.

  layout/style/nsStyleContext.cpp
    The DO_STRUCT_DIFFERENCE line is too far up -- it should (at least
    currently) be in the VISUAL section next to color.

    nsStyleContext::DumpRegressionData is missing a %s for the markers
    line.

  layout/style/nsStyleStruct.cpp
    In CalcDifference, comment before the array comparison that you
    already know the lengths are equal since they were compared above.

    Is NS_STYLE_HINT_VISUAL really sufficient for all of these?

    nsStyleSVGPaint::operator=3D=3D is incorrect for the None case -- it
    compares the colors, which may be different, since operator=3D doesn't
    copy the color in the none case so you can't count on it being
    black.

  layout/style/nsStyleStruct.h
    The pixels comment should probably be associated with
    stroke-dashoffset and stroke-dasharray as well.

  layout/style/nsStyleStructList.h
    Looks OK.

  layout/base/nsStyleConsts.h
    NS_STYLE_DOMINANT_BASELINE_TEXT_TOP and
    NS_STYLE_DOMINANT_BASELINE_TEXT_BOTTOM are unused, at least from the
    style system end.  Please either remove them or comment as to why
    they are there.

Comment 5

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

>     SetSVGLength needs to pass aInherited through to SetCoord, not use
>     |dummy|.

Oops, this one's wrong.  Never mind.

Comment 6

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

(In reply to comment #4)
>     ParseDasharray could also probably be written with less duplicated
>     code, as ParseQuotes is.

Ignore this one too.

Comment 7

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

Comment on attachment 170360 [details]
dummy attachment for bug tracking purposes

marking sr- for now; please re-request once comments are addressed

Comment 8

[tor]

Attachment: snapshot patch

Work in progress, addresses most of the comments.  Need to think
more about the SVGLength percentage issue.

Comment 9

[tor]

Attachment: above plus percentage handling in svg css

Comment 10

[tor]

Attachment: fix problem with dasharray

Comment 11

[Scooter Morris]

Comment on attachment 178599 [details] [diff] [review]
fix problem with dasharray

Looks good to me.

Comment 12

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

Comment on attachment 178599 [details] [diff] [review]
fix problem with dasharray

I haven't yet checked that this addresses all the comments.  In fact, I think I
noticed a few that it didn't.

nsSVGUtils.h should say that the method handles percentages as a percentage of
the current viewport, since that's appropriate only for some percentage values.
 (Does it actually?  I really don't understand what the percentage handling
code there does, and it seems like it ought to be easier and ought not to
require the creation of new objects.)

Comment 13

[tor]

Checked in patch with a comment added.

Were you thinking of error states for out-of-range values as the missing bits?

Comment 14

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

Attachment: fix debug code

Comment 15

[tor]

Comment on attachment 179819 [details] [diff] [review]
fix debug code

>+  for (PRUint32 i = 0; i < svg->mStrokeDasharrayLength; i++) {
>+    svg->mStrokeDasharray[i].ToString(str);
>+    fprintf(out, "%s%c",
>+            NS_ConvertUCS2toUTF8(str).get(),
>             (i == svg->mStrokeDasharrayLength) ? ' ' : ',');
>+  }

That should be "i == svg->mStrokeDashArrayLength - 1", shouldn't it?

With that, r+sr=tor.

Comment 16

[Jonathan Watt [:jwatt]]

dbaron, should we keep this open? SVG landed long ago.

Comment 17

[Jonathan Watt [:jwatt]]

David, Boris, reopen this if you think anything further could and will happen here.