Last Comment Bug 887502 - Improve serialization of 'border-radius'
: Improve serialization of 'border-radius'
Status: RESOLVED FIXED
[mentor=dbaron]
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla25
Assigned To: Birunthan Mohanathas [:poiru]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-26 14:47 PDT by Sebastian Zartner [:sebo]
Modified: 2013-07-02 16:51 PDT (History)
0 users
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case for 'border-radius' serialization (3.57 KB, text/html)
2013-06-26 14:47 PDT, Sebastian Zartner [:sebo]
no flags Details
Part 1: Improve Declaration::GetValue for 'border-radius' (6.00 KB, patch)
2013-06-29 11:29 PDT, Birunthan Mohanathas [:poiru]
dbaron: review-
Details | Diff | Splinter Review
Part 2: Refactor Declaration::GetValue for 'margin' and friends (2.68 KB, patch)
2013-06-29 11:33 PDT, Birunthan Mohanathas [:poiru]
dbaron: review+
Details | Diff | Splinter Review
Part 1: Coalesce corners rather than repeating when serializing specified values of 'border-radius' (5.79 KB, patch)
2013-06-29 12:54 PDT, Birunthan Mohanathas [:poiru]
dbaron: review+
Details | Diff | Splinter Review
Part 2: Refactor Declaration::GetValue for 'margin' and friends (2.68 KB, patch)
2013-06-29 12:55 PDT, Birunthan Mohanathas [:poiru]
birunthan: review+
Details | Diff | Splinter Review
Part 1: Coalesce corners rather than repeating when serializing specified values of 'border-radius' (5.80 KB, patch)
2013-06-30 00:05 PDT, Birunthan Mohanathas [:poiru]
birunthan: review+
Details | Diff | Splinter Review
Part 1: Coalesce corners rather than repeating when serializing specified values of 'border-radius' (7.15 KB, patch)
2013-07-01 10:26 PDT, Birunthan Mohanathas [:poiru]
birunthan: review+
dbaron: review+
Details | Diff | Splinter Review

Description Sebastian Zartner [:sebo] 2013-06-26 14:47:14 PDT
Created attachment 768010 [details]
Test case for 'border-radius' serialization

When accessing the borderRadius property inside the cssRules of a style sheet the value is not serialized as you would expect.

E.g. when you define "border-radius: 5px;" in your CSS, the value will be expanded to "5px 5px 5px 5px" when you read it.

See the attached test case for some more clarification.

Sebastian
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-26 15:00:08 PDT
The code for this is in Declaration::GetValue in layout/style/Declaration.cpp; should be a relatively straightforward patch.
Comment 2 Sebastian Zartner [:sebo] 2013-06-27 00:19:03 PDT
Because I just saw the code for this right now, the same counts for the value of the -moz-outline-radius, of course.

Sebastian
Comment 3 Birunthan Mohanathas [:poiru] 2013-06-29 11:29:23 PDT
Created attachment 769409 [details] [diff] [review]
Part 1: Improve Declaration::GetValue for 'border-radius'
Comment 4 Birunthan Mohanathas [:poiru] 2013-06-29 11:33:57 PDT
Created attachment 769410 [details] [diff] [review]
Part 2: Refactor Declaration::GetValue for 'margin' and friends

Hi. I've separated the patch into two parts. Part 1 fixes the bug and Part 2 reduces code duplication.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-29 12:00:01 PDT
Comment on attachment 769409 [details] [diff] [review]
Part 1: Improve Declaration::GetValue for 'border-radius'

Thanks for the patch -- this largely looks good, though I have a bunch of comments, many of which are substantially about local style.

># HG changeset patch
># Parent cbb24a4a96afda6d2e765536b284b281c122a60f
># User Birunthan Mohanathas <birunthan@mohanathas.com>
>Bug 887502 - Part 1: Improve Declaration::GetValue for 'border-radius'
>Shorthand notation is now used for 'border-radius' when possible.

I think the first line isn't actually helpful; it's the second line that's important.  I'd suggest a commit message like:

Bug 887502 Part 1: Coalesce corners rather than repeating when serializing specified values of 'border-radius'.

(The first line of a commit message is semantically different from the remaining lines; only the first line is shown in a short view; in other views the full message is shown.  So the first line on its own should be a complete summary, with more details below; see, for example, https://hg.mozilla.org/mozilla-central/rev/830111e10951 .)

>diff --git a/layout/style/Declaration.cpp b/layout/style/Declaration.cpp
>--- a/layout/style/Declaration.cpp
>+++ b/layout/style/Declaration.cpp
>@@ -102,16 +102,53 @@ Declaration::AppendValueToString(nsCSSPr
>   if (!val) {
>     return false;
>   }
> 
>   val->AppendToString(aProperty, aResult);
>   return true;
> }
> 
>+namespace {

I think we actually tend to stay away from anonymous namespace because they don't get along well with debuggers. Could you just declare the function static instead?

>+
>+// Helper to append |aString| with the shorthand notation of CSS properties
>+// with the syntax "[ <length> | <percentage> ]{1,4}" (e.g. padding).
>+// |aProperties| and |aValues| are expected to have 4 elements.

I don't think this is at all specific to <length> or <percentage>; it simply works for any shorthand that uses the sides/corners coalescing pattern.

>+void
>+AppendOneToFourShorthandValuesToString(const nsCSSProperty aProperties[],
>+                                       const nsCSSValue* aValues[],
>+                                       nsAString& aString)

Rather than "OneToFourShorthandValues" I'd use "SidesShorthand" since this is a pattern for shorthands that are for sides or corners.

>@@ -235,38 +272,40 @@ Declaration::GetValue(nsCSSProperty aPro
>         data->ValueFor(subprops[1]),
>         data->ValueFor(subprops[2]),
>         data->ValueFor(subprops[3])
>       };
> 
>       // For compatibility, only write a slash and the y-values
>       // if they're not identical to the x-values.
>       bool needY = false;
>+      const nsCSSValue* xVals[4] = {};

drop the " = {}"

>       for (int i = 0; i < 4; i++) {
>         if (vals[i]->GetUnit() == eCSSUnit_Pair) {
>           needY = true;
>-          vals[i]->GetPairValue().mXValue.AppendToString(subprops[i], aValue);
>+          xVals[i] = &vals[i]->GetPairValue().mXValue;
>         } else {
>-          vals[i]->AppendToString(subprops[i], aValue);
>+          xVals[i] = vals[i];
>         }
>-        if (i < 3)
>-          aValue.Append(PRUnichar(' '));
>       }
> 
>+      AppendOneToFourShorthandValuesToString(subprops, xVals, aValue);
>+
>       if (needY) {
>         aValue.AppendLiteral(" / ");
>+        const nsCSSValue* yVals[4] = {};
>         for (int i = 0; i < 4; i++) {
>           if (vals[i]->GetUnit() == eCSSUnit_Pair) {
>-            vals[i]->GetPairValue().mYValue.AppendToString(subprops[i], aValue);
>+            yVals[i] = &vals[i]->GetPairValue().mYValue;
>           } else {
>-            vals[i]->AppendToString(subprops[i], aValue);
>+            yVals[i] = vals[i];
>           }
>-          if (i < 3)
>-            aValue.Append(PRUnichar(' '));
>         }
>+
>+        AppendOneToFourShorthandValuesToString(subprops, yVals, aValue);

While you're doing this, I think it makes sense to merge the accumulation of yVals into the same loop as xVals rather than have two separate loops.  (You still need |needY|.)


r=dbaron with those changes, but I'd like to have a look at the revised patch, so marking as review-
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-29 12:02:27 PDT
Comment on attachment 769410 [details] [diff] [review]
Part 2: Refactor Declaration::GetValue for 'margin' and friends

r=dbaron when adjusted to account for the rename in the previous patch

You'll want to upload a revised patch with "r=dbaron" included (and the revision) so that you can use checkin-needed per
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
(that's the only piece missing)
Comment 7 Birunthan Mohanathas [:poiru] 2013-06-29 12:54:10 PDT
Created attachment 769419 [details] [diff] [review]
Part 1: Coalesce corners rather than repeating when serializing specified values of 'border-radius'

Thanks for the comments. I've updated the patch to address them.
Comment 8 Birunthan Mohanathas [:poiru] 2013-06-29 12:55:53 PDT
Created attachment 769420 [details] [diff] [review]
Part 2: Refactor Declaration::GetValue for 'margin' and friends
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-29 13:08:06 PDT
Comment on attachment 769419 [details] [diff] [review]
Part 1: Coalesce corners rather than repeating when serializing specified values of 'border-radius'

r=dbaron
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-29 15:42:06 PDT
(If you attach a version of part 1 with the r=dbaron, then you can add the checkin-needed keyword, and somebody should then land it within a day or so.  Though maybe more due to holidays this week...)
Comment 11 Birunthan Mohanathas [:poiru] 2013-06-30 00:05:03 PDT
Created attachment 769464 [details] [diff] [review]
Part 1: Coalesce corners rather than repeating when serializing specified values of 'border-radius'

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #10)
> (If you attach a version of part 1 with the r=dbaron, then you can add the
> checkin-needed keyword, and somebody should then land it within a day or so.
> Though maybe more due to holidays this week...)

Can you assign me so that I can add checkin-needed?
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-30 00:43:16 PDT
(In reply to Birunthan Mohanathas [:poiru] from comment #11)
> Can you assign me so that I can add checkin-needed?

I also gave you canconfirm and editbugs so you can do this yourself in general.  You should read https://developer.mozilla.org/en-US/docs/What_to_do_and_what_not_to_do_in_Bugzilla
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-06-30 00:47:50 PDT
One other note (for the future; no need to correct anything here):  the way this was split into two patches was a little bit odd.  I think it would have been somewhat clearer to swap the order of the two patches, except that the new function would still be introduced in the new part 1.  This would make part 1 a refactoring patch that moves code from inside of GetValue into its own function, and then part 2 would change the behavior for border-radius by using that code.
Comment 14 Birunthan Mohanathas [:poiru] 2013-06-30 03:48:09 PDT
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #12)
> I also gave you canconfirm and editbugs so you can do this yourself in
> general.  You should read
> https://developer.mozilla.org/en-US/docs/
> What_to_do_and_what_not_to_do_in_Bugzilla

Great, thank you.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #13)
> One other note (for the future; no need to correct anything here):  the way
> this was split into two patches was a little bit odd.  I think it would have
> been somewhat clearer to swap the order of the two patches, except that the
> new function would still be introduced in the new part 1.  This would make
> part 1 a refactoring patch that moves code from inside of GetValue into its
> own function, and then part 2 would change the behavior for border-radius by
> using that code.

Agreed. I'll keep it in mind for the future.
Comment 17 Birunthan Mohanathas [:poiru] 2013-07-01 10:26:15 PDT
Created attachment 769746 [details] [diff] [review]
Part 1: Coalesce corners rather than repeating when serializing specified values of 'border-radius'

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> Backed out for test failures.

Fixed now. Sorry for the hassle!
Comment 20 Sebastian Zartner [:sebo] 2013-07-02 16:51:09 PDT
I think I never saw an issue I reported being fixed that fast. Thanks for that!

Sebastian

Note You need to log in before you can comment on or make changes to this bug.