|
|
Created:
4 years, 1 month ago by Xianzhu Modified:
4 years, 1 month ago CC:
chromium-reviews, Dirk Pranke, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake blink setting/feature names case-sensitive
Previously we match blink setting/feature names ignoring case. This
causes trouble when the name is used in a case-sensitive environment,
e.g. Linux file system. Make them case-sensitive to avoid the trouble.
third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=slimmingPaintInvalidation
which is a violation of the new case-sensitive rule is removed in this
CL. It will be re-added in https://codereview.chromium.org/2448633002/
to avoid problems on platforms using case-insensitive file names.
Committed: https://crrev.com/1e4fbec3b5701d9f605e88f14dafa442be98b648
Cr-Commit-Position: refs/heads/master@{#427219}
Patch Set 1 #Patch Set 2 : Remove the files first and will readthe files and will re-add them in a later patch because the ren… #Patch Set 3 : case-sensitive #
Dependent Patchsets: Messages
Total messages: 24 (12 generated)
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org, skobes@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Convert --additional-driver-flags to lowercase Some flag values are case-insensitive (e.g. --enable-blink-features), so user may use different cases for the same flag mapped to different paths of flag-specific expectation files and baseline directories. For example, using --additional-driver-flags=--enable-blink-features=SlimmingPaintInvalidation will not use FlagExpectationg/enable-blink-features=slimmingPaintInvalidation. Convert --additional-driver-flags to lowercase to avoid the problem. Also renamed the existing flag-specific files and directories to lowercase. ========== to ========== Convert --additional-driver-flags to lowercase Some flag values are case-insensitive (e.g. --enable-blink-features), so user may use different cases for the same flag mapped to different paths of flag-specific expectation files and baseline directories. For example, using --additional-driver-flags=--enable-blink-features=SlimmingPaintInvalidation will not use FlagExpectationg/enable-blink-features=slimmingPaintInvalidation. Convert --additional-driver-flags to lowercase to avoid the problem. The existing flag-expectations containing uppercase characters are removed in this CL and will be re-added in https://codereview.chromium.org/2448633002/ to avoid problems on platforms using case-insensitive file names. ==========
Nice find! What if we made --enable-blink-features case sensitive (in WebKit/Source/build/scripts/templates/RuntimeEnabledFeatures.cpp.tmpl)? I worry that having this case sensitivity difference will lead to more confusion later, such as this one you found.
On 2016/10/24 21:44:16, pdr. wrote: > Nice find! > > What if we made --enable-blink-features case sensitive (in > WebKit/Source/build/scripts/templates/RuntimeEnabledFeatures.cpp.tmpl)? I worry > that having this case sensitivity difference will lead to more confusion later, > such as this one you found. I'm afraid that would break developers, tools and field trials that use blink features and settings names different the original names.
On 2016/10/24 at 22:12:15, wangxianzhu wrote: > On 2016/10/24 21:44:16, pdr. wrote: > > Nice find! > > > > What if we made --enable-blink-features case sensitive (in > > WebKit/Source/build/scripts/templates/RuntimeEnabledFeatures.cpp.tmpl)? I worry > > that having this case sensitivity difference will lead to more confusion later, > > such as this one you found. > > I'm afraid that would break developers, tools and field trials that use blink features and settings names different the original names. I poked around and couldn't find any examples of existing code or documentation that would break from making --enable-blink-features case sensitive. Do you know of any cases other than the flag expectations issue? (if so, this change LGTM.)
On 2016/10/24 22:19:50, pdr. wrote: > On 2016/10/24 at 22:12:15, wangxianzhu wrote: > > On 2016/10/24 21:44:16, pdr. wrote: > > > Nice find! > > > > > > What if we made --enable-blink-features case sensitive (in > > > WebKit/Source/build/scripts/templates/RuntimeEnabledFeatures.cpp.tmpl)? I > worry > > > that having this case sensitivity difference will lead to more confusion > later, > > > such as this one you found. > > > > I'm afraid that would break developers, tools and field trials that use blink > features and settings names different the original names. > > I poked around and couldn't find any examples of existing code or documentation > that would break from making --enable-blink-features case sensitive. Do you know > of any cases other than the flag expectations issue? (if so, this change LGTM.) To fix the case issue, we also need to make blink settings case-sensitive. I checked the following: - Callers of WebSettings::setFromStrings() - Callers of WebEnabledFeatures::enableFeatureFromString() - Users of switches::kBlinkSettings - Users of switches::kEnableBlinkFeatures I feel a bit lost when checking the usages of names of settings/features in https://cs.chromium.org/chromium/src/components/variations/. I wonder if there are something outside of chrome that can pass a setting/feature name into chrome, e.g. field-trial settings. It would be bad to break any field trial. Still checking them.
On 2016/10/24 22:56:08, Xianzhu wrote: > On 2016/10/24 22:19:50, pdr. wrote: > > On 2016/10/24 at 22:12:15, wangxianzhu wrote: > > > On 2016/10/24 21:44:16, pdr. wrote: > > > > Nice find! > > > > > > > > What if we made --enable-blink-features case sensitive (in > > > > WebKit/Source/build/scripts/templates/RuntimeEnabledFeatures.cpp.tmpl)? I > > worry > > > > that having this case sensitivity difference will lead to more confusion > > later, > > > > such as this one you found. > > > > > > I'm afraid that would break developers, tools and field trials that use > blink > > features and settings names different the original names. > > > > I poked around and couldn't find any examples of existing code or > documentation > > that would break from making --enable-blink-features case sensitive. Do you > know > > of any cases other than the flag expectations issue? (if so, this change > LGTM.) > > To fix the case issue, we also need to make blink settings case-sensitive. > > I checked the following: > - Callers of WebSettings::setFromStrings() > - Callers of WebEnabledFeatures::enableFeatureFromString() > - Users of switches::kBlinkSettings > - Users of switches::kEnableBlinkFeatures > > I feel a bit lost when checking the usages of names of settings/features in > https://cs.chromium.org/chromium/src/components/variations/. I wonder if there > are something outside of chrome that can pass a setting/feature name into > chrome, e.g. field-trial settings. It would be bad to break any field trial. > Still checking them. Examined finch field trial configuration files and they all look good.
Description was changed from ========== Convert --additional-driver-flags to lowercase Some flag values are case-insensitive (e.g. --enable-blink-features), so user may use different cases for the same flag mapped to different paths of flag-specific expectation files and baseline directories. For example, using --additional-driver-flags=--enable-blink-features=SlimmingPaintInvalidation will not use FlagExpectationg/enable-blink-features=slimmingPaintInvalidation. Convert --additional-driver-flags to lowercase to avoid the problem. The existing flag-expectations containing uppercase characters are removed in this CL and will be re-added in https://codereview.chromium.org/2448633002/ to avoid problems on platforms using case-insensitive file names. ========== to ========== Make blink setting/feature names case-sensitive Previously we match blink setting/feature names ignoring case. This causes trouble when the name is used in a case-sensitive environment, e.g. Linux file system. Make them case-sensitive to avoid the trouble. third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=slimmingPaintInvalidation which is a violation of the new case-sensitive rule is removed in this CL. It will be re-added in https://codereview.chromium.org/2448633002/ to avoid problems on platforms using case-insensitive file names. ==========
On 2016/10/24 23:27:48, Xianzhu wrote: > On 2016/10/24 22:56:08, Xianzhu wrote: > > On 2016/10/24 22:19:50, pdr. wrote: > > > On 2016/10/24 at 22:12:15, wangxianzhu wrote: > > > > On 2016/10/24 21:44:16, pdr. wrote: > > > > > Nice find! > > > > > > > > > > What if we made --enable-blink-features case sensitive (in > > > > > WebKit/Source/build/scripts/templates/RuntimeEnabledFeatures.cpp.tmpl)? > I > > > worry > > > > > that having this case sensitivity difference will lead to more confusion > > > later, > > > > > such as this one you found. > > > > > > > > I'm afraid that would break developers, tools and field trials that use > > blink > > > features and settings names different the original names. > > > > > > I poked around and couldn't find any examples of existing code or > > documentation > > > that would break from making --enable-blink-features case sensitive. Do you > > know > > > of any cases other than the flag expectations issue? (if so, this change > > LGTM.) > > > > To fix the case issue, we also need to make blink settings case-sensitive. > > > > I checked the following: > > - Callers of WebSettings::setFromStrings() > > - Callers of WebEnabledFeatures::enableFeatureFromString() > > - Users of switches::kBlinkSettings > > - Users of switches::kEnableBlinkFeatures > > > > I feel a bit lost when checking the usages of names of settings/features in > > https://cs.chromium.org/chromium/src/components/variations/. I wonder if there > > are something outside of chrome that can pass a setting/feature name into > > chrome, e.g. field-trial settings. It would be bad to break any field trial. > > Still checking them. > > Examined finch field trial configuration files and they all look good. The latest patch make setting/feature names case-sensitive. Ptal.
Looks great! LGTM
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
Message was sent while issue was closed.
Description was changed from ========== Make blink setting/feature names case-sensitive Previously we match blink setting/feature names ignoring case. This causes trouble when the name is used in a case-sensitive environment, e.g. Linux file system. Make them case-sensitive to avoid the trouble. third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=slimmingPaintInvalidation which is a violation of the new case-sensitive rule is removed in this CL. It will be re-added in https://codereview.chromium.org/2448633002/ to avoid problems on platforms using case-insensitive file names. ========== to ========== Make blink setting/feature names case-sensitive Previously we match blink setting/feature names ignoring case. This causes trouble when the name is used in a case-sensitive environment, e.g. Linux file system. Make them case-sensitive to avoid the trouble. third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=slimmingPaintInvalidation which is a violation of the new case-sensitive rule is removed in this CL. It will be re-added in https://codereview.chromium.org/2448633002/ to avoid problems on platforms using case-insensitive file names. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Make blink setting/feature names case-sensitive Previously we match blink setting/feature names ignoring case. This causes trouble when the name is used in a case-sensitive environment, e.g. Linux file system. Make them case-sensitive to avoid the trouble. third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=slimmingPaintInvalidation which is a violation of the new case-sensitive rule is removed in this CL. It will be re-added in https://codereview.chromium.org/2448633002/ to avoid problems on platforms using case-insensitive file names. ========== to ========== Make blink setting/feature names case-sensitive Previously we match blink setting/feature names ignoring case. This causes trouble when the name is used in a case-sensitive environment, e.g. Linux file system. Make them case-sensitive to avoid the trouble. third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=slimmingPaintInvalidation which is a violation of the new case-sensitive rule is removed in this CL. It will be re-added in https://codereview.chromium.org/2448633002/ to avoid problems on platforms using case-insensitive file names. Committed: https://crrev.com/1e4fbec3b5701d9f605e88f14dafa442be98b648 Cr-Commit-Position: refs/heads/master@{#427219} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1e4fbec3b5701d9f605e88f14dafa442be98b648 Cr-Commit-Position: refs/heads/master@{#427219} |