Bug 62265 - Implement CSS border radius properies in CSSStyleApplyProperty
Summary: Implement CSS border radius properies in CSSStyleApplyProperty
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Luke Macpherson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-07 22:25 PDT by Luke Macpherson
Modified: 2011-06-14 23:31 PDT (History)
5 users (show)

See Also:


Attachments
Patch (16.00 KB, patch)
2011-06-07 22:31 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (15.47 KB, patch)
2011-06-13 23:15 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2011-06-07 22:25:37 PDT
Implement CSS border radius properies in CSSStyleApplyProperty
Comment 1 Luke Macpherson 2011-06-07 22:31:41 PDT
Created attachment 96381 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-06-07 22:41:59 PDT
Comment on attachment 96381 [details]
Patch

Why are we moving away from const &?  That's going to cause LengthSize to copy.  Is LengthSize expensive to copy?
Comment 3 Luke Macpherson 2011-06-07 22:47:44 PDT
I should prehaps have pr-empted this question. LengthSize contains exactly two lengths, and we pass Lengths by value already. Length is pretty small (< 64 bits). Seems pretty safe to me on most any modern architecture.
Comment 4 Luke Macpherson 2011-06-09 17:16:39 PDT
Ping.
Comment 5 Eric Seidel (no email) 2011-06-09 22:37:51 PDT
Comment on attachment 96381 [details]
Patch

Looks good.  I'm not sure the removal of const & will be a win on all platforms... but I don't really know.
Comment 6 Eric Seidel (no email) 2011-06-09 22:38:22 PDT
What's the disadvantage of using the const&?
Comment 7 Luke Macpherson 2011-06-09 22:53:29 PDT
The disadvantage is that at the moment the types are a bit mix-and-match, so it made the templates easier to write / understand if the same parameter type can be used everywhere. This could also go the other way (all to const &) I suppose.
Comment 8 Eric Seidel (no email) 2011-06-10 10:37:29 PDT
Comment on attachment 96381 [details]
Patch

OK.
Comment 9 WebKit Review Bot 2011-06-10 11:33:51 PDT
Comment on attachment 96381 [details]
Patch

Rejecting attachment 96381 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
395a87712d692b0fc87844413a797b237c0448a0
r88553 = 7ae9a37d521f373bda7f5faa56104624249d647b
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/8826458
Comment 10 Luke Macpherson 2011-06-13 23:15:33 PDT
Created attachment 97072 [details]
Patch
Comment 11 Luke Macpherson 2011-06-13 23:44:43 PDT
Manual merge
Comment 12 Eric Seidel (no email) 2011-06-14 08:06:25 PDT
Comment on attachment 97072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97072&action=review

> Source/WebCore/ChangeLog:1
> +2011-06-13  Luke Macpherson   <macpherson@chromium.org>

For whatever reason there seems to be an extra space between your name and your email here.
Comment 13 WebKit Review Bot 2011-06-14 08:46:37 PDT
Comment on attachment 97072 [details]
Patch

Clearing flags on attachment: 97072

Committed r88804: <http://trac.webkit.org/changeset/88804>
Comment 14 WebKit Review Bot 2011-06-14 08:46:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Pavel Feldman 2011-06-14 08:49:04 PDT
Committed r88805: <http://trac.webkit.org/changeset/88805>
Comment 16 Luke Macpherson 2011-06-14 23:17:49 PDT
Ok, I'm confused.
http://trac.webkit.org/changeset/88804 look like this patch
http://trac.webkit.org/changeset/88805 has the description of this patch, but clearly isn't this patch if you look at the diffs.

What happened?
Comment 17 Pavel Feldman 2011-06-14 23:28:02 PDT
(In reply to comment #16)
> Ok, I'm confused.
> http://trac.webkit.org/changeset/88804 look like this patch
> http://trac.webkit.org/changeset/88805 has the description of this patch, but clearly isn't this patch if you look at the diffs.
> 
> What happened?

@eric: A webkit-patch glitch? I think I was using "webkit-patch land" with the patch from https://bugs.webkit.org/show_bug.cgi?id=62640 in my working tree. It did not go through, so I pulled from git and did "webkit-patch land" again.
Comment 18 Pavel Feldman 2011-06-14 23:31:58 PDT
> @eric: A webkit-patch glitch? I think I was using "webkit-patch land" with the patch from https://bugs.webkit.org/show_bug.cgi?id=62640 in my working tree. It did not go through, so I pulled from git and did "webkit-patch land" again.

Oh, this is me to blame: my patch was based on the tree with my another patch being on top. ChangeLog resolves poorly in these cases and I just did not check it after pulling your change. We could fix ChangeLog resolving for this case though.