Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1969243002:
Fix regex to parse Variations UI override strings (Closed)
DescriptionFix regex to parse Variations UI override strings
Regex macro used to parse the UI override strings does not work for the
generation enabled. This CL adds support to handle the pragma directive
present in #define macro format.
BUG=611273
Committed: https://crrev.com/e60a3b28edd120d48777c7bd16b5b8e9793e4d57
Cr-Commit-Position: refs/heads/master@{#393295}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Addressed rkaplow and alexei comments #Messages
Total messages: 16 (6 generated)
rajendrant@chromium.org changed reviewers: + asvitkine@chromium.org, bengr@chromium.org, rkaplow@chromium.org
asvitkine@ rkaplow@: Could one of you ptal. I just included both of you to fix this faster, and merge to M51. https://codereview.chromium.org/1969243002/diff/1/components/variations/servi... File components/variations/service/generate_ui_string_overrider.py (right): https://codereview.chromium.org/1969243002/diff/1/components/variations/servi... components/variations/service/generate_ui_string_overrider.py:15: The pragma format is defined in https://code.google.com/p/chromium/codesearch#chromium/src/tools/grit/grit_ru... https://codereview.chromium.org/1969243002/diff/1/components/variations/servi... components/variations/service/generate_ui_string_overrider.py:25: RESOURCE_EXTRACT_REGEX_PRAGMA_WINDOWS = re.compile( Not tested with Windows yet. Will test asap.
Description was changed from ========== Fix regex to parse Variations UI override strings Regex macro used to parse the UI override strings does not work for the generation enabled. This CL adds support to handle the pragma directive present in #define macro format. BUG= ========== to ========== Fix regex to parse Variations UI override strings Regex macro used to parse the UI override strings does not work for the generation enabled. This CL adds support to handle the pragma directive present in #define macro format. BUG=611273 ==========
https://codereview.chromium.org/1969243002/diff/1/components/variations/servi... File components/variations/service/generate_ui_string_overrider.py (right): https://codereview.chromium.org/1969243002/diff/1/components/variations/servi... components/variations/service/generate_ui_string_overrider.py:25: RESOURCE_EXTRACT_REGEX_PRAGMA_WINDOWS = re.compile( On 2016/05/12 01:40:27, Raj wrote: > Not tested with Windows yet. Will test asap. I tested in linux for android build, that builds with or without enable_resource_whitelist_generation generate the same gen/chrome/browser/metrics/variations/ui_string_overrider_factory.cc
can you also update the unit tests https://codereview.chromium.org/1969243002/diff/1/components/variations/servi... File components/variations/service/generate_ui_string_overrider.py (right): https://codereview.chromium.org/1969243002/diff/1/components/variations/servi... components/variations/service/generate_ui_string_overrider.py:20: # lines without pragma, could you give samples for the cases here https://codereview.chromium.org/1969243002/diff/1/components/variations/servi... components/variations/service/generate_ui_string_overrider.py:26: '^#define (\S*) __pragma\(message\("whitelisted_resource_\d*"\)\) (\d*)$', just to verify this isn't a typo - this is lowercase pragma compared to the other regex?
rkaplow@chromium.org changed reviewers: + jwd@chromium.org
Glad we got to the bottom of things - thanks for your investigation! https://codereview.chromium.org/1969243002/diff/1/components/variations/servi... File components/variations/service/generate_ui_string_overrider.py (right): https://codereview.chromium.org/1969243002/diff/1/components/variations/servi... components/variations/service/generate_ui_string_overrider.py:23: '^#define (\S*) _Pragma\("whitelisted_resource_\d*"\) (\d*)$', Looks like these are defined here: https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&... Can you add a comment both to this file referencing those locations and to the GYPI and GN files referencing this script and mentioning that they have to be kept in sync. Also, I wonder if it makes sense to make this Python script fail (causing a build error) if it found no entries matching the regexes.
ptal. I just want to keep this CL small for easy merge to M51. https://codereview.chromium.org/1969243002/diff/1/components/variations/servi... File components/variations/service/generate_ui_string_overrider.py (right): https://codereview.chromium.org/1969243002/diff/1/components/variations/servi... components/variations/service/generate_ui_string_overrider.py:20: # lines without pragma, On 2016/05/12 01:52:53, rkaplow wrote: > could you give samples for the cases here Done. https://codereview.chromium.org/1969243002/diff/1/components/variations/servi... components/variations/service/generate_ui_string_overrider.py:23: '^#define (\S*) _Pragma\("whitelisted_resource_\d*"\) (\d*)$', On 2016/05/12 14:40:41, Alexei Svitkine wrote: > Looks like these are defined here: > > https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&... > > Can you add a comment both to this file referencing those locations and to the > GYPI and GN files referencing this script and mentioning that they have to be > kept in sync. > > Also, I wonder if it makes sense to make this Python script fail (causing a > build error) if it found no entries matching the regexes. Sure. I will add the comment in a separate CL. I do not want to wait for another owner approval, which may delay this CL merge to M51. I just want to keep this CL small for easy merge. WDYT. Yes. It is better to fail this script when no entries are found. In fact we should parse line-by-line and if one line does not match any of the regex, the script could fail. I will add this in next CL, which may need more testing with many platforms. WDYT. https://codereview.chromium.org/1969243002/diff/1/components/variations/servi... components/variations/service/generate_ui_string_overrider.py:26: '^#define (\S*) __pragma\(message\("whitelisted_resource_\d*"\)\) (\d*)$', On 2016/05/12 01:52:53, rkaplow wrote: > just to verify this isn't a typo - this is lowercase pragma compared to the > other regex? Yes. The pragma format is defined in: https://code.google.com/p/chromium/codesearch#chromium/src/tools/grit/grit_ru...
lgtm I'm fine with you doing the other requested changes in a follow-up CL. Thanks!
The CQ bit was checked by rajendrant@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1969243002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1969243002/20001
Message was sent while issue was closed.
Description was changed from ========== Fix regex to parse Variations UI override strings Regex macro used to parse the UI override strings does not work for the generation enabled. This CL adds support to handle the pragma directive present in #define macro format. BUG=611273 ========== to ========== Fix regex to parse Variations UI override strings Regex macro used to parse the UI override strings does not work for the generation enabled. This CL adds support to handle the pragma directive present in #define macro format. BUG=611273 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix regex to parse Variations UI override strings Regex macro used to parse the UI override strings does not work for the generation enabled. This CL adds support to handle the pragma directive present in #define macro format. BUG=611273 ========== to ========== Fix regex to parse Variations UI override strings Regex macro used to parse the UI override strings does not work for the generation enabled. This CL adds support to handle the pragma directive present in #define macro format. BUG=611273 Committed: https://crrev.com/e60a3b28edd120d48777c7bd16b5b8e9793e4d57 Cr-Commit-Position: refs/heads/master@{#393295} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e60a3b28edd120d48777c7bd16b5b8e9793e4d57 Cr-Commit-Position: refs/heads/master@{#393295} |
