Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1246953002:
content: make theme_color more resilient to casting issues (Closed)
|
Created:
5 years, 5 months ago by Lalit Maganti Modified:
5 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-manifest_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncontent: perform theme color conversion to signed int earlier
* it is easier and clearer to do the conversion explicitly in C++ rather than a hidden implicit conversion in Java.
BUG=510402
Committed: https://crrev.com/4349518e48f1ceda5f77be4fdf1ad2ad98ff0b73
Cr-Commit-Position: refs/heads/master@{#342559}
Patch Set 1 #Patch Set 2 : Fix compile #Patch Set 3 : Fix compile #Patch Set 4 : Fix tests #Patch Set 5 : Fix unit tests #
Total comments: 4
Patch Set 6 : Implement comments #
Total comments: 6
Patch Set 7 : Address avi@'s comment #
Messages
Total messages: 28 (9 generated)
lalitm@google.com changed reviewers: + mlamouri@chromium.org
lalitm@google.com changed reviewers: + bernrb@chromium.org
mlamouri@chromium.org changed reviewers: + bauerb@chromium.org - bernrb@chromium.org
bauerb: this should hopefully be it. Also I do know about expected and actual being the wrong way round in the unit tests but this is true of the entire file so I'd like to fix all of them in a separate CL.
https://codereview.chromium.org/1246953002/diff/80001/content/renderer/manife... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/1246953002/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:430: int32_t signedColor = reinterpret_cast<int32_t&>(color); Local variables are unix_hacker_style. Also, can you add a comment that explains why we do this? https://codereview.chromium.org/1246953002/diff/80001/content/renderer/manife... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/1246953002/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:14: uint32_t extractColor(int64_t color) { Method names start with capital letters. Also, contents of namespaces are not indented. (But blank lines would be nice.)
https://codereview.chromium.org/1246953002/diff/80001/content/renderer/manife... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/1246953002/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:430: int32_t signedColor = reinterpret_cast<int32_t&>(color); On 2015/07/31 at 09:58:56, Bernhard Bauer wrote: > Local variables are unix_hacker_style. Also, can you add a comment that explains why we do this? Done. https://codereview.chromium.org/1246953002/diff/80001/content/renderer/manife... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/1246953002/diff/80001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:14: uint32_t extractColor(int64_t color) { On 2015/07/31 at 09:58:56, Bernhard Bauer wrote: > Method names start with capital letters. > > Also, contents of namespaces are not indented. > > (But blank lines would be nice.) Done and added comment for anonymous as well.
LGTM, thanks! https://codereview.chromium.org/1246953002/diff/100001/content/renderer/manif... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/1246953002/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:1145: } Out of curiosity, do we allow transparent colors here?
lalitm@google.com changed reviewers: + nasko@chromium.org
nasko: could you please review the manifest change? https://codereview.chromium.org/1246953002/diff/100001/content/renderer/manif... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/1246953002/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:1145: } On 2015/07/31 at 10:07:45, Bernhard Bauer wrote: > Out of curiosity, do we allow transparent colors here? As far as I know there is no check against it here because we simply want to store whatever is in the manifest. Higher up, we may well disallow transparent colors as it doesn't make any sense to theme the UI transparent.
https://codereview.chromium.org/1246953002/diff/100001/content/renderer/manif... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/1246953002/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:1145: } On 2015/07/31 10:13:17, Lalit Maganti wrote: > On 2015/07/31 at 10:07:45, Bernhard Bauer wrote: > > Out of curiosity, do we allow transparent colors here? > > As far as I know there is no check against it here because we simply want to > store whatever is in the manifest. Higher up, we may well disallow transparent > colors as it doesn't make any sense to theme the UI transparent. OK, in that case would it make sense to add tests for transparent values (can be in a followup CL)? That would exercise the code paths for a) parsing argb color values, and b) converting values with the MSB _not_ set (which probably is the less problematic case, but it's still good to have coverage).
lgtm https://codereview.chromium.org/1246953002/diff/100001/content/renderer/manif... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/1246953002/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:1145: } On 2015/07/31 at 10:26:55, Bernhard Bauer wrote: > On 2015/07/31 10:13:17, Lalit Maganti wrote: > > On 2015/07/31 at 10:07:45, Bernhard Bauer wrote: > > > Out of curiosity, do we allow transparent colors here? > > > > As far as I know there is no check against it here because we simply want to > > store whatever is in the manifest. Higher up, we may well disallow transparent > > colors as it doesn't make any sense to theme the UI transparent. > > OK, in that case would it make sense to add tests for transparent values (can be in a followup CL)? That would exercise the code paths for a) parsing argb color values, and b) converting values with the MSB _not_ set (which probably is the less problematic case, but it's still good to have coverage). We could add a test with AA=00 in there, right?
lalitm@google.com changed reviewers: + avi@chromium.org
Avi: could you please take a look at the manifest change? I'd like this to land pretty soon because there is another patch depending on this.
https://codereview.chromium.org/1246953002/diff/100001/content/public/common/... File content/public/common/manifest.cc (right): https://codereview.chromium.org/1246953002/diff/100001/content/public/common/... content/public/common/manifest.cc:11: static_cast<int64_t>(std::numeric_limits<int32_t>::max()) + 1; I'm having trouble wrapping my brain around this. I get that we don't have unsigned 32 bit integers in Java so we embed it in a signed 64 bit one. But what is going on here? Can you clarify with a comment?
https://codereview.chromium.org/1246953002/diff/100001/content/public/common/... File content/public/common/manifest.cc (right): https://codereview.chromium.org/1246953002/diff/100001/content/public/common/... content/public/common/manifest.cc:11: static_cast<int64_t>(std::numeric_limits<int32_t>::max()) + 1; On 2015/08/07 at 14:48:08, Avi wrote: > I'm having trouble wrapping my brain around this. > > I get that we don't have unsigned 32 bit integers in Java so we embed it in a signed 64 bit one. But what is going on here? Can you clarify with a comment? Have added a comment in the new patch.
lgtm Thanks!
The CQ bit was checked by lalitm@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1246953002/#ps110001 (title: "Address avi@'s comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1246953002/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1246953002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by lalitm@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1246953002/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1246953002/110001
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4349518e48f1ceda5f77be4fdf1ad2ad98ff0b73 Cr-Commit-Position: refs/heads/master@{#342559} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
