|
|
Created:
7 years, 8 months ago by mihai.o.tica Modified:
7 years, 8 months ago CC:
blink-reviews Base URL:
http://src.chromium.org/blink/trunk/Source/ Visibility:
Public. |
DescriptionAdd support for runtime flags for experimental blending to chromium
BUG=154769, 229220
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148489
Patch Set 1 #Patch Set 2 : #
Messages
Total messages: 18 (0 generated)
Hi James, could you please review adding css blending under the experimental features flag please? The second part of the review (the actual chromium part) is https://codereview.chromium.org/12681024 Thanks!
I don't know enough about ENABLE_CSS_COMPOSITING to know if this makes sense. Is there a bug somewhere tracking this work I could follow?
On 2013/04/08 21:57:41, jamesr wrote: > I don't know enough about ENABLE_CSS_COMPOSITING to know if this makes sense. > Is there a bug somewhere tracking this work I could follow? This current implementation status can be followed at https://bugs.webkit.org/show_bug.cgi?id=91908 Currently, only parsing is enabled and we are currently working on adding the background-blend-mode property. You can find more details about CSS Blending and Compositing at https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html
Could you file a blink bug to track this? Some parts will have to be different from how it appears the WebKit patch is going - for instance we don't use vendor prefixes in Blink.
On 2013/04/09 06:43:54, jamesr wrote: > Could you file a blink bug to track this? Some parts will have to be different > from how it appears the WebKit patch is going - for instance we don't use vendor > prefixes in Blink. I've added the blink issue at https://code.google.com/p/chromium/issues/detail?id=229312, we will start porting the other webkit issues to blink in short time.
After a more detailed search, I've found the actual feature issue at: https://code.google.com/p/chromium/issues/detail?id=154769, please ignore the previous URL.
lgtm, although it'd be much better to simply eliminate the #ifs completely. Please do that as a follow-up. Also, update the description to point to the proper BUG=s
Sorry I'm still a little behind on the process. Can you schedule this feature for an API review so we can make sure we understand the implementation costs involved? This will have to involve the compositor at some point, for example, and I don't understand how that works.
I've updated the patch to point to the proper bugs, could you please give it a try when you have some time? Also, I've filed issue 229926 for removing the ifdefs which will be done as a follow-up. On 2013/04/09 19:33:10, jamesr wrote: > Sorry I'm still a little behind on the process. Can you schedule this feature > for an API review so we can make sure we understand the implementation costs > involved? This will have to involve the compositor at some point, for example, > and I don't understand how that works. Will do so, I'll send a request on the mailing list.
On 2013/04/09 19:33:10, jamesr wrote: > Sorry I'm still a little behind on the process. Can you schedule this feature > for an API review so we can make sure we understand the implementation costs > involved? This will have to involve the compositor at some point, for example, > and I don't understand how that works. Mihai is going to request an API review. I added a master bug for the various features of blending: https://codereview.chromium.org/13538003/ You are correct that some of them will most likely involve the compositor and we will need to make careful so it doesn't introduce instabilities or slowdowns in that code.
We've started the "intent to implement" thread and the conclusion was that this feature should be added, but for the moment, it should stay under a runtime flag. Do you this overall change, which also includes the chromium part from https://codereview.chromium.org/12681024/ would be ready to land?
On 2013/04/12 16:40:15, mihai.o.tica wrote: > We've started the "intent to implement" thread and the conclusion was that this > feature should be added, but for the moment, it should stay under a runtime > flag. > Do you this overall change, which also includes the chromium part from > https://codereview.chromium.org/12681024/ would be ready to land? I think my comment in #7 still applies - this is OK, but it would be much better to simply remove the ENABLE() completely assuming that everything is properly guarded by the runtime flag.
> > Do you this overall change, which also includes the chromium part from > > https://codereview.chromium.org/12681024/ would be ready to land? > > I think my comment in #7 still applies - this is OK, but it would be much better > to simply remove the ENABLE() completely assuming that everything is properly > guarded by the runtime flag. Mihai logged issue 229926 to remove all the #ifdef's. Is it fine to do that in a followup?
On 2013/04/12 17:31:55, RikC wrote: > > > Do you this overall change, which also includes the chromium part from > > > https://codereview.chromium.org/12681024/ would be ready to land? > > > > I think my comment in #7 still applies - this is OK, but it would be much > better > > to simply remove the ENABLE() completely assuming that everything is properly > > guarded by the runtime flag. > > Mihai logged issue 229926 to remove all the #ifdef's. Is it fine to do that in a > followup? Yes. (I feel like I've said the same thing three times on this review :/ )
Message was sent while issue was closed.
Committed patchset #2 manually as r148489 (presubmit successful).
Message was sent while issue was closed.
Reverted: https://src.chromium.org/viewvc/blink?revision=148519&view=revision since the new CSS property names are exposed to the web. You need to guard the whole feature, including property names, behind the runtime flag before this can be turned on at compile-time. See https://code.google.com/p/chromium/issues/detail?id=232181
Message was sent while issue was closed.
On 2013/04/17 00:58:31, jamesr wrote: > Reverted: https://src.chromium.org/viewvc/blink?revision=148519&view=revision > > since the new CSS property names are exposed to the web. You need to guard the > whole feature, including property names, behind the runtime flag before this can > be turned on at compile-time. See > https://code.google.com/p/chromium/issues/detail?id=232181 It seems like that's gated by writing support for autogenerated CSS properties. Do we have to wait until that is implemented?
Message was sent while issue was closed.
No. :) I've posted a proposal for guarding CSS properties at runtime in that bug. I'll be working on making that proposal a reality next Monday if no one else beats me to it. Unfortunately I am away from my desk much of the rest of the week. |