Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1235883007:
manifest: add theme_color value to the manifest (Closed)
|
Created:
5 years, 5 months ago by Lalit Maganti Modified:
5 years, 5 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. |
Descriptionmanifest: add theme_color value to the manifest
Add parsing capabilities of a theme_color value in a website's manifest
to the manifest parser.
Store this value in the manifest object.
BUG=510402
Committed: https://crrev.com/48cfd739f58bfdc72a98167991153cf0882c93a2
Cr-Commit-Position: refs/heads/master@{#339444}
Patch Set 1 #Patch Set 2 : Fix Mounir's comments on old patch #Patch Set 3 : Reflect change in Blink patch #
Total comments: 36
Patch Set 4 : Fix Mounir's comments #Patch Set 5 : Change type for invalid theme color #Patch Set 6 : Fix some small mistakes #
Total comments: 15
Patch Set 7 : Fix second round of Mounir's comments #Patch Set 8 : Use UTF16ToUTF8 instead of assuming ASCII #
Total comments: 2
Patch Set 9 : Fix Mounir's comment #
Total comments: 5
Patch Set 10 : Fix nasko's comments #
Total comments: 4
Patch Set 11 : Fix nasko's comments #
Total comments: 3
Patch Set 12 : Add braces to if statement #Patch Set 13 : Fix color range check #Patch Set 14 : Remove color from checks #
Total comments: 2
Patch Set 15 : Add comments #
Dependent Patchsets: Messages
Total messages: 34 (4 generated)
lalitm@google.com changed reviewers: + mlamouri@chromium.org
Mounir: this should now be good to go I think.
Looks pretty good. A lot of small mistakes here and there but the logic is fine! https://codereview.chromium.org/1235883007/diff/40001/content/public/common/m... File content/public/common/manifest.cc (right): https://codereview.chromium.org/1235883007/diff/40001/content/public/common/m... content/public/common/manifest.cc:28: theme_color(-1), Use the constant. https://codereview.chromium.org/1235883007/diff/40001/content/public/common/m... content/public/common/manifest.cc:41: theme_color == -1 && ditto. https://codereview.chromium.org/1235883007/diff/40001/content/public/common/m... File content/public/common/manifest.h (right): https://codereview.chromium.org/1235883007/diff/40001/content/public/common/m... content/public/common/manifest.h:97: // Set to -1 if parsing failed or field is not present. Replace -1 by kInvalidOrMissingThemeColor. https://codereview.chromium.org/1235883007/diff/40001/content/public/common/m... content/public/common/manifest.h:98: int64_t theme_color; nit: I'm not sure which order you are following here. It's not following the spec order, nor the alphabetical order. Could you add that before gcm_sender_id? https://codereview.chromium.org/1235883007/diff/40001/content/public/common/m... content/public/common/manifest.h:129: static const long kInvalidThemeColor; kInvalidOrMissingThemeColor maybe? Also, you might want to have the same type as theme_color, which is int64_t. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:228: if (display.is_null()) s/display/theme_color/g https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:231: blink::WebColor color = SK_ColorTRANSPARENT; I don't think you need to initialize that. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:234: if (!success) { You don't need the |success| variable. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:236: "unable to parse 'theme_color' as color."); nit: follow the same language as the other errors: """ "property 'theme_color' ignored, " + theme_color.string() + " is not a valid color." """ https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:241: return int_color; return static_cast<int64>(color); https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... File content/renderer/manifest/manifest_parser.h (right): https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser.h:88: // Returns the parsed theme color if any, -1 if the parsing failed. Replace -1 with kInvalidOrMissingThemeColor. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:361: Manifest manifest = ParseManifest("{ \"theme_color\": \"blue\" }"); Remove that test, it's not a smoke test. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:377: Manifest manifest = ParseManifest("{ \"theme_color\": \"blue\" }"); You don't actually have whitespaces here. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:382: // Don't parse if name isn't a string. nit: s/name/theme_color/ https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:392: // Don't parse if name isn't a string. ditto. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:413: Manifest manifest = ParseManifest("{ \"theme_color\": \"blue\" }"); Could you also test a less common value like 'chartreuse'. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:420: Manifest manifest = ParseManifest("{ \"theme_color\": \"#FFF\" }"); Try #ABC too. #FFF could be a side effect. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:430: } Could you add a test checking that we don't parse if there are two values?
https://codereview.chromium.org/1235883007/diff/40001/content/public/common/m... File content/public/common/manifest.cc (right): https://codereview.chromium.org/1235883007/diff/40001/content/public/common/m... content/public/common/manifest.cc:28: theme_color(-1), On 2015/07/16 at 12:35:40, Mounir Lamouri wrote: > Use the constant. Done. https://codereview.chromium.org/1235883007/diff/40001/content/public/common/m... content/public/common/manifest.cc:41: theme_color == -1 && On 2015/07/16 at 12:35:40, Mounir Lamouri wrote: > ditto. Done. https://codereview.chromium.org/1235883007/diff/40001/content/public/common/m... File content/public/common/manifest.h (right): https://codereview.chromium.org/1235883007/diff/40001/content/public/common/m... content/public/common/manifest.h:97: // Set to -1 if parsing failed or field is not present. On 2015/07/16 at 12:35:40, Mounir Lamouri wrote: > Replace -1 by kInvalidOrMissingThemeColor. Done https://codereview.chromium.org/1235883007/diff/40001/content/public/common/m... content/public/common/manifest.h:98: int64_t theme_color; On 2015/07/16 at 12:35:40, Mounir Lamouri wrote: > nit: I'm not sure which order you are following here. It's not following the spec order, nor the alphabetical order. Could you add that before gcm_sender_id? Done. https://codereview.chromium.org/1235883007/diff/40001/content/public/common/m... content/public/common/manifest.h:129: static const long kInvalidThemeColor; On 2015/07/16 at 12:35:40, Mounir Lamouri wrote: > kInvalidOrMissingThemeColor maybe? Also, you might want to have the same type as theme_color, which is int64_t. Done. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:228: if (display.is_null()) On 2015/07/16 at 12:35:41, Mounir Lamouri wrote: > s/display/theme_color/g Done. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:231: blink::WebColor color = SK_ColorTRANSPARENT; On 2015/07/16 at 12:35:41, Mounir Lamouri wrote: > I don't think you need to initialize that. Done. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:234: if (!success) { On 2015/07/16 at 12:35:40, Mounir Lamouri wrote: > You don't need the |success| variable. Done. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:236: "unable to parse 'theme_color' as color."); On 2015/07/16 at 12:35:40, Mounir Lamouri wrote: > nit: follow the same language as the other errors: > """ > "property 'theme_color' ignored, " + theme_color.string() + " is not a valid color." > """ Almost done - the concatenation of the strings did not work due to type issues so I changed the wording slightly to match other properties. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:241: return int_color; On 2015/07/16 at 12:35:41, Mounir Lamouri wrote: > return static_cast<int64>(color); Done. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... File content/renderer/manifest/manifest_parser.h (right): https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser.h:88: // Returns the parsed theme color if any, -1 if the parsing failed. On 2015/07/16 at 12:35:41, Mounir Lamouri wrote: > Replace -1 with kInvalidOrMissingThemeColor. Done. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:361: Manifest manifest = ParseManifest("{ \"theme_color\": \"blue\" }"); On 2015/07/16 at 12:35:41, Mounir Lamouri wrote: > Remove that test, it's not a smoke test. Done. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:377: Manifest manifest = ParseManifest("{ \"theme_color\": \"blue\" }"); On 2015/07/16 at 12:35:41, Mounir Lamouri wrote: > You don't actually have whitespaces here. Removed this test - doesn't really apply. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:382: // Don't parse if name isn't a string. On 2015/07/16 at 12:35:41, Mounir Lamouri wrote: > nit: s/name/theme_color/ Done. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:392: // Don't parse if name isn't a string. On 2015/07/16 at 12:35:41, Mounir Lamouri wrote: > ditto. Done. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:413: Manifest manifest = ParseManifest("{ \"theme_color\": \"blue\" }"); On 2015/07/16 at 12:35:41, Mounir Lamouri wrote: > Could you also test a less common value like 'chartreuse'. Done. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:420: Manifest manifest = ParseManifest("{ \"theme_color\": \"#FFF\" }"); On 2015/07/16 at 12:35:41, Mounir Lamouri wrote: > Try #ABC too. #FFF could be a side effect. Done. https://codereview.chromium.org/1235883007/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:430: } On 2015/07/16 at 12:35:41, Mounir Lamouri wrote: > Could you add a test checking that we don't parse if there are two values? Done.
Thanks, it looks way better! I mostly want you to add a couple of tests and it will be ready to go. https://codereview.chromium.org/1235883007/diff/100001/content/common/manifes... File content/common/manifest_manager_messages.h (right): https://codereview.chromium.org/1235883007/diff/100001/content/common/manifes... content/common/manifest_manager_messages.h:38: IPC_STRUCT_TRAITS_MEMBER(theme_color) Can you keep the same order as content::Manifest? https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser.cc:19: #include "third_party/skia/include/core/SkColor.h" I don't think you need SkColor. https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser.cc:426: "invalid color specified."); Could you include theme_color.string() it would be easier for developers to debug with that: """ "property 'theme_color' ignored, " + theme_color.string() + " is not a valid color." """ https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:358: TEST_F(ManifestParserTest, ThemeColorParserRules) { I wanted you to remove the first smoke test, not both, please add: // Smoke test. { Manifest manifest = ParseManifest("{ \"theme_color\": \"#FF0000\" }"); EXPECT_EQ(manifest.theme_color, 0xFFFF0000); EXPECT_FALSE(manifest.IsEmpty()); EXPECT_EQ(0u, GetErrorCount()); } https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:359: // Don't parse if theme_color isn't a string. You removed the whitespace test too. Could you add it back? https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:379: // Parse fails if string is not in a known format. Could you add more test for this case? Things closer to the format like: bleu FF00FF (the first is a typo for blue, the second is missing #) https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:391: Manifest manifest = ParseManifest("{ \"theme_color\": \"#ABC#DEF\" }"); This sounds more like a wrong format. You could keep this test but please add a test like: Manifest manifest = ParseManifest("{ \"theme_color\": \"#AABBCC #DDEEFF\" }");
https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser.cc:426: "invalid color specified."); On 2015/07/16 at 14:53:31, Mounir Lamouri wrote: > Could you include theme_color.string() it would be easier for developers to debug with that: > """ > "property 'theme_color' ignored, " + theme_color.string() + " is not a valid color." > """ You can use UTF16ToASCII(theme_color.string()).
https://codereview.chromium.org/1235883007/diff/100001/content/common/manifes... File content/common/manifest_manager_messages.h (right): https://codereview.chromium.org/1235883007/diff/100001/content/common/manifes... content/common/manifest_manager_messages.h:38: IPC_STRUCT_TRAITS_MEMBER(theme_color) On 2015/07/16 at 14:53:31, Mounir Lamouri wrote: > Can you keep the same order as content::Manifest? Done. https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser.cc:19: #include "third_party/skia/include/core/SkColor.h" On 2015/07/16 at 14:53:31, Mounir Lamouri wrote: > I don't think you need SkColor. Done. https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser.cc:426: "invalid color specified."); On 2015/07/16 at 14:53:31, Mounir Lamouri wrote: > Could you include theme_color.string() it would be easier for developers to debug with that: > """ > "property 'theme_color' ignored, " + theme_color.string() + " is not a valid color." > """ Done https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:358: TEST_F(ManifestParserTest, ThemeColorParserRules) { On 2015/07/16 at 14:53:31, Mounir Lamouri wrote: > I wanted you to remove the first smoke test, not both, please add: > // Smoke test. > { > Manifest manifest = ParseManifest("{ \"theme_color\": \"#FF0000\" }"); > EXPECT_EQ(manifest.theme_color, 0xFFFF0000); > EXPECT_FALSE(manifest.IsEmpty()); > EXPECT_EQ(0u, GetErrorCount()); > } Done. https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:359: // Don't parse if theme_color isn't a string. On 2015/07/16 at 14:53:31, Mounir Lamouri wrote: > You removed the whitespace test too. Could you add it back? Done. https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:379: // Parse fails if string is not in a known format. On 2015/07/16 at 14:53:31, Mounir Lamouri wrote: > Could you add more test for this case? Things closer to the format like: > bleu > FF00FF > > (the first is a typo for blue, the second is missing #) Done. https://codereview.chromium.org/1235883007/diff/100001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:391: Manifest manifest = ParseManifest("{ \"theme_color\": \"#ABC#DEF\" }"); On 2015/07/16 at 14:53:31, Mounir Lamouri wrote: > This sounds more like a wrong format. You could keep this test but please add a test like: > > Manifest manifest = ParseManifest("{ \"theme_color\": \"#AABBCC #DDEEFF\" }"); Done.
lalitm@google.com changed reviewers: + nasko@chromium.org
nasko@: could you please review the following: content/common/manifest_manager_messages.h content/public/common/manifest.cc content/public/common/manifest.h
lgtm! Thanks! One last comment below that I should have caught before, sorry. https://codereview.chromium.org/1235883007/diff/140001/content/renderer/manif... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/1235883007/diff/140001/content/renderer/manif... content/renderer/manifest/manifest_parser.cc:425: base::UTF16ToUTF8(theme_color.string()) + nit: could you add ' ' around the string? ie: "property 'theme_color' ignored, '" + base::UTF16ToUTF8(theme_color.string()) + "' is not a valid color.");
https://codereview.chromium.org/1235883007/diff/140001/content/renderer/manif... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/1235883007/diff/140001/content/renderer/manif... content/renderer/manifest/manifest_parser.cc:425: base::UTF16ToUTF8(theme_color.string()) + On 2015/07/16 at 17:19:00, Mounir Lamouri wrote: > nit: could you add ' ' around the string? > > ie: > "property 'theme_color' ignored, '" + > base::UTF16ToUTF8(theme_color.string()) + > "' is not a valid color."); Done.
This CL doesn't have the consumer of the theme color, just the parsing. We usually don't add code for the sake of adding it and it should go along with its usage. https://codereview.chromium.org/1235883007/diff/160001/content/renderer/manif... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/1235883007/diff/160001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:384: // Don't parse if theme_color isn't a string. If this is meant to be exhaustive test for different JSON data types that aren't expected, then it is missing some (boolean, array, null). https://codereview.chromium.org/1235883007/diff/160001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:434: // Parse fails if multiple values for theme_color are given. What happens if there are multiple theme_color instances? Do we pick first/last/random? It should be tested to ensure we don't regress the expectation we set.
On 2015/07/17 at 09:48:53, nasko wrote: > This CL doesn't have the consumer of the theme color, just the parsing. We usually don't add code for the sake of adding it and it should go along with its usage. > > https://codereview.chromium.org/1235883007/diff/160001/content/renderer/manif... > File content/renderer/manifest/manifest_parser_unittest.cc (right): > > https://codereview.chromium.org/1235883007/diff/160001/content/renderer/manif... > content/renderer/manifest/manifest_parser_unittest.cc:384: // Don't parse if theme_color isn't a string. > If this is meant to be exhaustive test for different JSON data types that aren't expected, then it is missing some (boolean, array, null). > > https://codereview.chromium.org/1235883007/diff/160001/content/renderer/manif... > content/renderer/manifest/manifest_parser_unittest.cc:434: // Parse fails if multiple values for theme_color are given. > What happens if there are multiple theme_color instances? Do we pick first/last/random? It should be tested to ensure we don't regress the expectation we set. I've added some sanity checking on the browser side to make sure invalid colors don't sneak through.
nasko@: comments you raised should be fixed. https://codereview.chromium.org/1235883007/diff/160001/content/renderer/manif... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/1235883007/diff/160001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:384: // Don't parse if theme_color isn't a string. On 2015/07/17 at 09:48:53, nasko (paris) wrote: > If this is meant to be exhaustive test for different JSON data types that aren't expected, then it is missing some (boolean, array, null). Done. https://codereview.chromium.org/1235883007/diff/160001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:434: // Parse fails if multiple values for theme_color are given. On 2015/07/17 at 09:48:53, nasko (paris) wrote: > What happens if there are multiple theme_color instances? Do we pick first/last/random? It should be tested to ensure we don't regress the expectation we set. This depends on the JSON parser used - the JSON spec does not specify the order and websites should not be assuming an order being set. Since this is a unit test for manifest parser it doesn't make much sense to test the JSON parser implementation.
BTW nasko@, theme_color is being used here: https://codereview.chromium.org/1234653004
https://codereview.chromium.org/1235883007/diff/160001/content/renderer/manif... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/1235883007/diff/160001/content/renderer/manif... content/renderer/manifest/manifest_parser_unittest.cc:434: // Parse fails if multiple values for theme_color are given. On 2015/07/17 10:30:00, Lalit Maganti wrote: > On 2015/07/17 at 09:48:53, nasko (paris) wrote: > > What happens if there are multiple theme_color instances? Do we pick > first/last/random? It should be tested to ensure we don't regress the > expectation we set. > > This depends on the JSON parser used - the JSON spec does not specify the order > and websites should not be assuming an order being set. Since this is a unit > test for manifest parser it doesn't make much sense to test the JSON parser > implementation. Well, there is one JSON parser that Chrome will use, so the argument about different parsers seems moot to me. I'm fine if you think it is not worth testing this particular case, though it helps to ensure that we don't break poorly coded manifests. https://codereview.chromium.org/1235883007/diff/180001/content/browser/manife... File content/browser/manifest/manifest_manager_host.cc (right): https://codereview.chromium.org/1235883007/diff/180001/content/browser/manife... content/browser/manifest/manifest_manager_host.cc:139: if (manifest.theme_color < 0) Can we be a bit more strict? Is it valid for a color value to have the highest 3-4 bytes being non-zero? Why not use a bitmask comparison to ensure we don't let unexpected values? https://codereview.chromium.org/1235883007/diff/180001/content/browser/manife... content/browser/manifest/manifest_manager_host.cc:140: manifest.theme_color = -1; Please don't use integers, use the symbolic constant you already have defined.
My point was more that this test would break unnecessarily if the JSON parser were to change which theme color attribute was chosen, considering that it would be perfectly legitimate for us to change it. Developers should not be relying on this behaviour. Moreover, it would also break every other JSON we parse so we would need to add tests across the board if we did think this was a major issue. https://codereview.chromium.org/1235883007/diff/180001/content/browser/manife... File content/browser/manifest/manifest_manager_host.cc (right): https://codereview.chromium.org/1235883007/diff/180001/content/browser/manife... content/browser/manifest/manifest_manager_host.cc:139: if (manifest.theme_color < 0) On 2015/07/17 at 16:36:18, nasko (paris) wrote: > Can we be a bit more strict? Is it valid for a color value to have the highest 3-4 bytes being non-zero? Why not use a bitmask comparison to ensure we don't let unexpected values? No it is not valid but we do a cast on the renderer side so it would techincally impossible for this to be the case. I could add the comparison in I guess because it wouldn't hurt.
On 2015/07/17 at 16:36:18, nasko wrote: > https://codereview.chromium.org/1235883007/diff/160001/content/renderer/manif... > File content/renderer/manifest/manifest_parser_unittest.cc (right): > > https://codereview.chromium.org/1235883007/diff/160001/content/renderer/manif... > content/renderer/manifest/manifest_parser_unittest.cc:434: // Parse fails if multiple values for theme_color are given. > On 2015/07/17 10:30:00, Lalit Maganti wrote: > > On 2015/07/17 at 09:48:53, nasko (paris) wrote: > > > What happens if there are multiple theme_color instances? Do we pick > > first/last/random? It should be tested to ensure we don't regress the > > expectation we set. > > > > This depends on the JSON parser used - the JSON spec does not specify the order > > and websites should not be assuming an order being set. Since this is a unit > > test for manifest parser it doesn't make much sense to test the JSON parser > > implementation. > > Well, there is one JSON parser that Chrome will use, so the argument about different parsers seems moot to me. I'm fine if you think it is not worth testing this particular case, though it helps to ensure that we don't break poorly coded manifests. I don't think it would be a good idea to test the JSON parser implementation details in the manifest parsing unittests. Unit tests should try to avoid testing something that isn't implemented by the tested files.
https://codereview.chromium.org/1235883007/diff/180001/content/browser/manife... File content/browser/manifest/manifest_manager_host.cc (right): https://codereview.chromium.org/1235883007/diff/180001/content/browser/manife... content/browser/manifest/manifest_manager_host.cc:139: if (manifest.theme_color < 0) On 2015/07/18 17:52:05, Lalit Maganti wrote: > On 2015/07/17 at 16:36:18, nasko (paris) wrote: > > Can we be a bit more strict? Is it valid for a color value to have the highest > 3-4 bytes being non-zero? Why not use a bitmask comparison to ensure we don't > let unexpected values? > > No it is not valid but we do a cast on the renderer side so it would techincally > impossible for this to be the case. I could add the comparison in I guess > because it wouldn't hurt. Please do. What we do in the renderer process is irrelevant, as an exploited process can do whatever it wants. All parameters received should be verified as strictly as possible.
On 2015/07/20 at 09:22:17, nasko wrote: > https://codereview.chromium.org/1235883007/diff/180001/content/browser/manife... > File content/browser/manifest/manifest_manager_host.cc (right): > > https://codereview.chromium.org/1235883007/diff/180001/content/browser/manife... > content/browser/manifest/manifest_manager_host.cc:139: if (manifest.theme_color < 0) > On 2015/07/18 17:52:05, Lalit Maganti wrote: > > On 2015/07/17 at 16:36:18, nasko (paris) wrote: > > > Can we be a bit more strict? Is it valid for a color value to have the highest > > 3-4 bytes being non-zero? Why not use a bitmask comparison to ensure we don't > > let unexpected values? > > > > No it is not valid but we do a cast on the renderer side so it would techincally > > impossible for this to be the case. I could add the comparison in I guess > > because it wouldn't hurt. > > Please do. What we do in the renderer process is irrelevant, as an exploited process can do whatever it wants. All parameters received should be verified as strictly as possible. OK have added this check in. Instead of using a bit compare which looks very cryptic, I've put in a color range check which much puts the intent of the check across much better.
On 2015/07/20 10:27:17, Lalit Maganti wrote: > On 2015/07/20 at 09:22:17, nasko wrote: > > > https://codereview.chromium.org/1235883007/diff/180001/content/browser/manife... > > File content/browser/manifest/manifest_manager_host.cc (right): > > > > > https://codereview.chromium.org/1235883007/diff/180001/content/browser/manife... > > content/browser/manifest/manifest_manager_host.cc:139: if > (manifest.theme_color < 0) > > On 2015/07/18 17:52:05, Lalit Maganti wrote: > > > On 2015/07/17 at 16:36:18, nasko (paris) wrote: > > > > Can we be a bit more strict? Is it valid for a color value to have the > highest > > > 3-4 bytes being non-zero? Why not use a bitmask comparison to ensure we > don't > > > let unexpected values? > > > > > > No it is not valid but we do a cast on the renderer side so it would > techincally > > > impossible for this to be the case. I could add the comparison in I guess > > > because it wouldn't hurt. > > > > Please do. What we do in the renderer process is irrelevant, as an exploited > process can do whatever it wants. All parameters received should be verified as > strictly as possible. > > OK have added this check in. Instead of using a bit compare which looks very > cryptic, I've put in a color range check which much puts the intent of the check > across much better. Even better! Thanks for doing it.
https://codereview.chromium.org/1235883007/diff/200001/content/browser/manife... File content/browser/manifest/manifest_manager_host.cc (right): https://codereview.chromium.org/1235883007/diff/200001/content/browser/manife... content/browser/manifest/manifest_manager_host.cc:141: || manifest.theme_color > SK_ColorWHITE) Need {} around the body of the if statement, as it spans more than one line. https://codereview.chromium.org/1235883007/diff/200001/content/public/common/... File content/public/common/manifest.h (right): https://codereview.chromium.org/1235883007/diff/200001/content/public/common/... content/public/common/manifest.h:117: int64_t theme_color; I'm sorry I missed this earlier. Is there a reason why we are using 64bit value here? It seems that the source of the data is 32bit and the value range is in the 32bit space.
Hopefully this should be good now. https://codereview.chromium.org/1235883007/diff/200001/content/public/common/... File content/public/common/manifest.h (right): https://codereview.chromium.org/1235883007/diff/200001/content/public/common/... content/public/common/manifest.h:117: int64_t theme_color; On 2015/07/20 at 11:07:03, nasko (paris) wrote: > I'm sorry I missed this earlier. Is there a reason why we are using 64bit value here? It seems that the source of the data is 32bit and the value range is in the 32bit space. Yes. The color is represented as an unsigned 32 bit integer but we need -1 to represent a parsing issue. Unfortunately it causes a wastage of 31 bits but it is the most elegant solution I can come up with.
I think it is good. I'd like to see some comments that will make the job easier for people trying to understand this code later in time. Once that's done, I think it is good to go. https://codereview.chromium.org/1235883007/diff/260001/content/browser/manife... File content/browser/manifest/manifest_manager_host.cc (right): https://codereview.chromium.org/1235883007/diff/260001/content/browser/manife... content/browser/manifest/manifest_manager_host.cc:139: if (manifest.theme_color < 0 || manifest.theme_color > 0xFFFFFFFF) Please add a comment giving a bit of background why this check is the right one - color is only 32bit value and we only use the highest bit to communicate parsing error. https://codereview.chromium.org/1235883007/diff/260001/content/public/common/... File content/public/common/manifest.h (right): https://codereview.chromium.org/1235883007/diff/260001/content/public/common/... content/public/common/manifest.h:116: // present. Please include a small comment based on what we discussed - the fact that this is 64bit integer, mainly to be able to encode a parsing failure state, which is not possible to fit in the 32bit int.
On 2015/07/20 at 12:02:00, nasko wrote: > I think it is good. I'd like to see some comments that will make the job easier for people trying to understand this code later in time. Once that's done, I think it is good to go. > > https://codereview.chromium.org/1235883007/diff/260001/content/browser/manife... > File content/browser/manifest/manifest_manager_host.cc (right): > > https://codereview.chromium.org/1235883007/diff/260001/content/browser/manife... > content/browser/manifest/manifest_manager_host.cc:139: if (manifest.theme_color < 0 || manifest.theme_color > 0xFFFFFFFF) > Please add a comment giving a bit of background why this check is the right one - color is only 32bit value and we only use the highest bit to communicate parsing error. > > https://codereview.chromium.org/1235883007/diff/260001/content/public/common/... > File content/public/common/manifest.h (right): > > https://codereview.chromium.org/1235883007/diff/260001/content/public/common/... > content/public/common/manifest.h:116: // present. > Please include a small comment based on what we discussed - the fact that this is 64bit integer, mainly to be able to encode a parsing failure state, which is not possible to fit in the 32bit int. Have added the comments.
LGTM
The CQ bit was checked by lalitm@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1235883007/#ps280001 (title: "Add comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1235883007/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/48cfd739f58bfdc72a98167991153cf0882c93a2 Cr-Commit-Position: refs/heads/master@{#339444} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
