[GTK] WEBKIT_[DISABLE|FORCE]_COMPOSITING_MODE does not work properly
Created attachment 311022 [details] Patch
Comment on attachment 311022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311022&action=review You are supposed to do WEBKIT_[DISABLE|FORCE]_COMPOSITING_MODE=1 > Source/WebKit2/UIProcess/gtk/HardwareAccelerationManager.cpp:57 > - if (disableCompositing && strcmp(disableCompositing, "0")) { > + if (disableCompositing && !strcmp(disableCompositing, "0")) { This is correct. if the variable is present and 0 we disable AC. > Source/WebKit2/UIProcess/gtk/HardwareAccelerationManager.cpp:83 > - if (forceCompositing && strcmp(forceCompositing, "0")) > + if (forceCompositing && !strcmp(forceCompositing, "0")) Same here.
Comment on attachment 311022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311022&action=review >> Source/WebKit2/UIProcess/gtk/HardwareAccelerationManager.cpp:57 >> + if (disableCompositing && !strcmp(disableCompositing, "0")) { > > This is correct. if the variable is present and 0 we disable AC. No, current behavior is, if the variable is present and non-0, we disable AC. Because the return value from strcmp is 0, if given values are equal, which interpreted as a false in if statement. I'm not sure about what is our expected behavior about those. For example, we does not have consistent policies how to handle environmental variables: * WEBKIT2_PAUSE_WEB_PRECESS_ON_LAUNCH : activated if the env is set with any values * WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS : activated if the env is set with "1" * WEBKIT_FORCE_COMPOSITING_MODE : activated if the env is set with non-0 Do you have any idea about it?
(In reply to Gwang Yoon Hwang from comment #3) > Comment on attachment 311022 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=311022&action=review > > >> Source/WebKit2/UIProcess/gtk/HardwareAccelerationManager.cpp:57 > >> + if (disableCompositing && !strcmp(disableCompositing, "0")) { > > > > This is correct. if the variable is present and 0 we disable AC. > > No, current behavior is, if the variable is present and non-0, we disable AC. Well, right. But that's the idea you pass WEBKIT_[DISABLE|FORCE]_COMPOSITING_MODE=1 to disable. > Because the return value from strcmp is 0, if given values are equal, which > interpreted as a false in if statement. > > I'm not sure about what is our expected behavior about those. > For example, we does not have consistent policies how to handle > environmental variables: > * WEBKIT2_PAUSE_WEB_PRECESS_ON_LAUNCH : activated if the env is set with any > values > * WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS : activated if the env is set with > "1" > * WEBKIT_FORCE_COMPOSITING_MODE : activated if the env is set with non-0 > > Do you have any idea about it? I guess we should be consistent. This was changed by Tomas, I guess we can explain the reasoning. I personally don't have any preference.
Comment on attachment 311022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311022&action=review >>>> Source/WebKit2/UIProcess/gtk/HardwareAccelerationManager.cpp:57 >>>> + if (disableCompositing && !strcmp(disableCompositing, "0")) { >>> >>> This is correct. if the variable is present and 0 we disable AC. >> >> No, current behavior is, if the variable is present and non-0, we disable AC. >> Because the return value from strcmp is 0, if given values are equal, which interpreted as a false in if statement. >> >> I'm not sure about what is our expected behavior about those. >> For example, we does not have consistent policies how to handle environmental variables: >> * WEBKIT2_PAUSE_WEB_PRECESS_ON_LAUNCH : activated if the env is set with any values >> * WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS : activated if the env is set with "1" >> * WEBKIT_FORCE_COMPOSITING_MODE : activated if the env is set with non-0 >> >> Do you have any idea about it? > > Well, right. But that's the idea you pass WEBKIT_[DISABLE|FORCE]_COMPOSITING_MODE=1 to disable. Sorry, Yoon, what were you thinking here? Your patch makes these environment variables do nothing unless they're set to 0, in which case they would take effect, which is pretty confusing.
Comment on attachment 311022 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311022&action=review >>>>> Source/WebKit2/UIProcess/gtk/HardwareAccelerationManager.cpp:57 >>>>> + if (disableCompositing && !strcmp(disableCompositing, "0")) { >>>> >>>> This is correct. if the variable is present and 0 we disable AC. >>> >>> No, current behavior is, if the variable is present and non-0, we disable AC. >>> Because the return value from strcmp is 0, if given values are equal, which interpreted as a false in if statement. >>> >>> I'm not sure about what is our expected behavior about those. >>> For example, we does not have consistent policies how to handle environmental variables: >>> * WEBKIT2_PAUSE_WEB_PRECESS_ON_LAUNCH : activated if the env is set with any values >>> * WEBKIT_SHOW_COMPOSITING_DEBUG_VISUALS : activated if the env is set with "1" >>> * WEBKIT_FORCE_COMPOSITING_MODE : activated if the env is set with non-0 >>> >>> Do you have any idea about it? >> >> Well, right. But that's the idea you pass WEBKIT_[DISABLE|FORCE]_COMPOSITING_MODE=1 to disable. > > Sorry, Yoon, what were you thinking here? Your patch makes these environment variables do nothing unless they're set to 0, in which case they would take effect, which is pretty confusing. I'm not talking about to apply this patch. At first I take a look at this to use this variable, I thought it was intended to use 0 to activate, since it was a pattern how we write these things. What I'm asking about is, our policy how to handle environmental variables. between non-0 or 1 or any variables.
(In reply to Gwang Yoon Hwang from comment #6) > At first I take a look at this to use this variable, I thought it was > intended to use 0 to activate, since it was a pattern how we write these > things. No... 0 means off, non-zero means on. > What I'm asking about is, our policy how to handle environmental variables. > between non-0 or 1 or any variables. 0 should mean off, nonzero (including empty) should mean on. The reason to check the value of the environment variable is to allow overriding it if set elsewhere in the environment. I know we don't have a lot of consistency here right now with our other environment variables, and it would be good to bring them into line.