Bug 62265 – Implement CSS border radius properies in CSSStyleApplyProperty

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Luke Macpherson]

Implement CSS border radius properies in CSSStyleApplyProperty

[Luke Macpherson]

Created attachment 96381 [details]
Patch

[Eric Seidel (no email)]

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?

[Luke Macpherson]

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.

[Luke Macpherson]

Ping.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

What's the disadvantage of using the const&?

[Luke Macpherson]

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.

[Eric Seidel (no email)]

Comment on attachment 96381 [details]
Patch

OK.

[WebKit Review Bot]

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

[Luke Macpherson]

Created attachment 97072 [details]
Patch

[Luke Macpherson]

Manual merge

[Eric Seidel (no email)]

Comment on attachment 97072 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> [phone]  Luke Macpherson   <[e-mail]>

For whatever reason there seems to be an extra space between your name and your email here.

[WebKit Review Bot]

Comment on attachment 97072 [details]
Patch

Clearing flags on attachment: 97072

Committed r88804: <http://trac.webkit.org/changeset/88804>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Pavel Feldman]

Committed r88805: <http://trac.webkit.org/changeset/88805>

[Luke Macpherson]

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?

[Pavel Feldman]

(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.

[Pavel Feldman]

> @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.