Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
550173002:
Implement ManifestParser in content/. (Closed)
|
Created:
6 years, 3 months ago by mlamouri (slow - plz ping) Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@manifest_fetcher Project:
chromium Visibility:
Public. |
DescriptionImplement ManifestParser in content/.
It also adds a Manifest struct in content/public/common.
BUG=409696
Committed: https://crrev.com/161ea5d82dafeeb644ee04b1df52fa7dd6313105
Cr-Commit-Position: refs/heads/master@{#294839}
Patch Set 1 #
Total comments: 15
Patch Set 2 : #
Total comments: 2
Patch Set 3 : add ctor/dtor #
Total comments: 12
Patch Set 4 : rebase #
Total comments: 2
Patch Set 5 : #
Messages
Total messages: 33 (4 generated)
mlamouri@chromium.org changed reviewers: + jochen@chromium.org
mlamouri@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
Michael, could you take a look at this?
https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... File content/public/common/manifest.h (right): https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... content/public/common/manifest.h:16: struct CONTENT_EXPORT Manifest { This struct just represents a pair of strings, then? I think jam would say "just pass the two strings, no need to define a new type in the public content API if it can be so easily avoided". https://codereview.chromium.org/550173002/diff/1/content/renderer/manifest/ma... File content/renderer/manifest/manifest_parser.h (right): https://codereview.chromium.org/550173002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser.h:23: // It does implement: "It implements"
PTAL https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... File content/public/common/manifest.h (right): https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... content/public/common/manifest.h:16: struct CONTENT_EXPORT Manifest { On 2014/09/08 17:07:42, Michael van Ouwerkerk wrote: > This struct just represents a pair of strings, then? I think jam would say "just > pass the two strings, no need to define a new type in the public content API if > it can be so easily avoided". It's only a pair of strings for the moment. It is meant to be way more than that but I don't want to overweight the initial CL so I've only implemented the two very trivial Manifest properties here. https://codereview.chromium.org/550173002/diff/1/content/renderer/manifest/ma... File content/renderer/manifest/manifest_parser.h (right): https://codereview.chromium.org/550173002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser.h:23: // It does implement: On 2014/09/08 17:07:42, Michael van Ouwerkerk wrote: > "It implements" Acknowledged.
https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... File content/public/common/manifest.h (right): https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... content/public/common/manifest.h:16: struct CONTENT_EXPORT Manifest { On 2014/09/08 17:13:16, Mounir Lamouri wrote: > On 2014/09/08 17:07:42, Michael van Ouwerkerk wrote: > > This struct just represents a pair of strings, then? I think jam would say > "just > > pass the two strings, no need to define a new type in the public content API > if > > it can be so easily avoided". > > It's only a pair of strings for the moment. It is meant to be way more than that > but I don't want to overweight the initial CL so I've only implemented the two > very trivial Manifest properties here. If it's going to be more complex, that might be a problem as well. Please share a temporary patch or a detailed design indicating what this will do when complete.
https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... File content/public/common/manifest.h (right): https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... content/public/common/manifest.h:16: struct CONTENT_EXPORT Manifest { On 2014/09/08 17:36:27, Michael van Ouwerkerk wrote: > On 2014/09/08 17:13:16, Mounir Lamouri wrote: > > On 2014/09/08 17:07:42, Michael van Ouwerkerk wrote: > > > This struct just represents a pair of strings, then? I think jam would say > > "just > > > pass the two strings, no need to define a new type in the public content API > > if > > > it can be so easily avoided". > > > > It's only a pair of strings for the moment. It is meant to be way more than > that > > but I don't want to overweight the initial CL so I've only implemented the two > > very trivial Manifest properties here. > > If it's going to be more complex, that might be a problem as well. Please share > a temporary patch or a detailed design indicating what this will do when > complete. It will never be complete. The manifest spec (see link on top of |struct CONTENT_EXPORT Manifest {| is listing some properties that we might or might not support in the future. I'm planning to implement a few features in a near future, including start_url and icons. This structure will be bigger but not really more complex. It's going to stay a property bag.
https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... File content/public/common/manifest.cc (right): https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... content/public/common/manifest.cc:9: bool Manifest::IsEmpty() const { Random rename + edit detection. If you upload gain, maybe boost the similarity requirements. https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... File content/public/common/manifest.h (right): https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... content/public/common/manifest.h:16: struct CONTENT_EXPORT Manifest { On 2014/09/08 17:39:48, Mounir Lamouri wrote: > On 2014/09/08 17:36:27, Michael van Ouwerkerk wrote: > > On 2014/09/08 17:13:16, Mounir Lamouri wrote: > > > On 2014/09/08 17:07:42, Michael van Ouwerkerk wrote: > > > > This struct just represents a pair of strings, then? I think jam would say > > > "just > > > > pass the two strings, no need to define a new type in the public content > API > > > if > > > > it can be so easily avoided". > > > > > > It's only a pair of strings for the moment. It is meant to be way more than > > that > > > but I don't want to overweight the initial CL so I've only implemented the > two > > > very trivial Manifest properties here. > > > > If it's going to be more complex, that might be a problem as well. Please > share > > a temporary patch or a detailed design indicating what this will do when > > complete. > > It will never be complete. The manifest spec (see link on top of |struct > CONTENT_EXPORT Manifest {| is listing some properties that we might or might not > support in the future. I'm planning to implement a few features in a near > future, including start_url and icons. > > This structure will be bigger but not really more complex. It's going to stay a > property bag. For the record, I'm not opposed to adding types to the public content API the way jam is. Your current approach looks reasonable to me. But, I'm not an owner here. Mind you, with the addition of icons structures, Manifest will no longer be a simple (flat) property bag. https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... content/public/common/manifest.h:20: base::NullableString16 name; From nullable_string16.h: "This should be used only where the difference between null and empty is meaningful." How is it meaningful? Having empty strings as values for name and short_name seems undesirable as well, and could be considered 'empty'.
https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... File content/public/common/manifest.cc (right): https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... content/public/common/manifest.cc:9: bool Manifest::IsEmpty() const { On 2014/09/09 12:41:53, Michael van Ouwerkerk wrote: > Random rename + edit detection. If you upload gain, maybe boost the similarity > requirements. Will do. https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... File content/public/common/manifest.h (right): https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... content/public/common/manifest.h:16: struct CONTENT_EXPORT Manifest { On 2014/09/09 12:41:53, Michael van Ouwerkerk wrote: > On 2014/09/08 17:39:48, Mounir Lamouri wrote: > > On 2014/09/08 17:36:27, Michael van Ouwerkerk wrote: > > > On 2014/09/08 17:13:16, Mounir Lamouri wrote: > > > > On 2014/09/08 17:07:42, Michael van Ouwerkerk wrote: > > > > > This struct just represents a pair of strings, then? I think jam would > say > > > > "just > > > > > pass the two strings, no need to define a new type in the public content > > API > > > > if > > > > > it can be so easily avoided". > > > > > > > > It's only a pair of strings for the moment. It is meant to be way more > than > > > that > > > > but I don't want to overweight the initial CL so I've only implemented the > > two > > > > very trivial Manifest properties here. > > > > > > If it's going to be more complex, that might be a problem as well. Please > > share > > > a temporary patch or a detailed design indicating what this will do when > > > complete. > > > > It will never be complete. The manifest spec (see link on top of |struct > > CONTENT_EXPORT Manifest {| is listing some properties that we might or might > not > > support in the future. I'm planning to implement a few features in a near > > future, including start_url and icons. > > > > This structure will be bigger but not really more complex. It's going to stay > a > > property bag. > > For the record, I'm not opposed to adding types to the public content API the > way jam is. Your current approach looks reasonable to me. But, I'm not an owner > here. Mind you, with the addition of icons structures, Manifest will no longer > be a simple (flat) property bag. What would be a reasonable alternative? https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... content/public/common/manifest.h:20: base::NullableString16 name; On 2014/09/09 12:41:53, Michael van Ouwerkerk wrote: > From nullable_string16.h: "This should be used only where the difference between > null and empty is meaningful." > > How is it meaningful? Having empty strings as values for name and short_name > seems undesirable as well, and could be considered 'empty'. There actually is a difference. I want to be able to distinguish those situations: { } { "name": { } } { "name": "" } In the first two cases, the string will be null. In the last, it will be empty. I think it is better to have a clear distinction (for all the properties) between parsed or not parsed.
https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... File content/public/common/manifest.h (right): https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... content/public/common/manifest.h:16: struct CONTENT_EXPORT Manifest { On 2014/09/09 12:45:47, Mounir Lamouri wrote: > On 2014/09/09 12:41:53, Michael van Ouwerkerk wrote: > > On 2014/09/08 17:39:48, Mounir Lamouri wrote: > > > On 2014/09/08 17:36:27, Michael van Ouwerkerk wrote: > > > > On 2014/09/08 17:13:16, Mounir Lamouri wrote: > > > > > On 2014/09/08 17:07:42, Michael van Ouwerkerk wrote: > > > > > > This struct just represents a pair of strings, then? I think jam would > > say > > > > > "just > > > > > > pass the two strings, no need to define a new type in the public > content > > > API > > > > > if > > > > > > it can be so easily avoided". > > > > > > > > > > It's only a pair of strings for the moment. It is meant to be way more > > than > > > > that > > > > > but I don't want to overweight the initial CL so I've only implemented > the > > > two > > > > > very trivial Manifest properties here. > > > > > > > > If it's going to be more complex, that might be a problem as well. Please > > > share > > > > a temporary patch or a detailed design indicating what this will do when > > > > complete. > > > > > > It will never be complete. The manifest spec (see link on top of |struct > > > CONTENT_EXPORT Manifest {| is listing some properties that we might or might > > not > > > support in the future. I'm planning to implement a few features in a near > > > future, including start_url and icons. > > > > > > This structure will be bigger but not really more complex. It's going to > stay > > a > > > property bag. > > > > For the record, I'm not opposed to adding types to the public content API the > > way jam is. Your current approach looks reasonable to me. But, I'm not an > owner > > here. Mind you, with the addition of icons structures, Manifest will no longer > > be a simple (flat) property bag. > > What would be a reasonable alternative? Reasonable is subjective. Like I said, your current approach looks reasonable to me. https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... content/public/common/manifest.h:20: base::NullableString16 name; On 2014/09/09 12:45:47, Mounir Lamouri wrote: > On 2014/09/09 12:41:53, Michael van Ouwerkerk wrote: > > From nullable_string16.h: "This should be used only where the difference > between > > null and empty is meaningful." > > > > How is it meaningful? Having empty strings as values for name and short_name > > seems undesirable as well, and could be considered 'empty'. > > There actually is a difference. I want to be able to distinguish those > situations: > { > } > { > "name": { } > } > { > "name": "" > } > > In the first two cases, the string will be null. In the last, it will be empty. > I think it is better to have a clear distinction (for all the properties) > between parsed or not parsed. Perhaps a note for IsEmpty then, indicating that it returns true when none of the fields have been set, as opposed to (not set || falsy).
https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... File content/public/common/manifest.h (right): https://codereview.chromium.org/550173002/diff/1/content/public/common/manife... content/public/common/manifest.h:20: base::NullableString16 name; On 2014/09/09 13:11:58, Michael van Ouwerkerk wrote: > On 2014/09/09 12:45:47, Mounir Lamouri wrote: > > On 2014/09/09 12:41:53, Michael van Ouwerkerk wrote: > > > From nullable_string16.h: "This should be used only where the difference > > between > > > null and empty is meaningful." > > > > > > How is it meaningful? Having empty strings as values for name and short_name > > > seems undesirable as well, and could be considered 'empty'. > > > > There actually is a difference. I want to be able to distinguish those > > situations: > > { > > } > > { > > "name": { } > > } > > { > > "name": "" > > } > > > > In the first two cases, the string will be null. In the last, it will be > empty. > > I think it is better to have a clear distinction (for all the properties) > > between parsed or not parsed. > > Perhaps a note for IsEmpty then, indicating that it returns true when none of > the fields have been set, as opposed to (not set || falsy). Done.
lgtm
https://codereview.chromium.org/550173002/diff/20001/content/public/common/ma... File content/public/common/manifest.h (right): https://codereview.chromium.org/550173002/diff/20001/content/public/common/ma... content/public/common/manifest.h:19: bool IsEmpty() const; no methods on structs. if you want this to be a struct, it should have a ctor anyway
https://codereview.chromium.org/550173002/diff/20001/content/public/common/ma... File content/public/common/manifest.h (right): https://codereview.chromium.org/550173002/diff/20001/content/public/common/ma... content/public/common/manifest.h:19: bool IsEmpty() const; On 2014/09/10 08:05:21, jochen wrote: > no methods on structs. if you want this to be a struct, it should have a ctor > anyway What are the alternatives here? I keep struct and make IsEmpty a static method that takes a Manifest as parameter? or would that be preferable to move to a class, in which case I will need to have getters and setters, right?
On 2014/09/10 at 14:23:43, mlamouri wrote: > https://codereview.chromium.org/550173002/diff/20001/content/public/common/ma... > File content/public/common/manifest.h (right): > > https://codereview.chromium.org/550173002/diff/20001/content/public/common/ma... > content/public/common/manifest.h:19: bool IsEmpty() const; > On 2014/09/10 08:05:21, jochen wrote: > > no methods on structs. if you want this to be a struct, it should have a ctor > > anyway > > What are the alternatives here? I keep struct and make IsEmpty a static method that takes a Manifest as parameter? or would that be preferable to move to a class, in which case I will need to have getters and setters, right? The difference between struct and class is mostly the default visibility which can be changed with private: public:. You could just expose the internal variables by adding them to the public: section. The other use for struct is to save space, but then it needs to be POD which is probably what jochen is referring to. Then there are rules like no virtuals, etc. http://en.wikipedia.org/wiki/Plain_old_data_structure
On 2014/09/11 10:21:35, kenneth.christiansen wrote: > On 2014/09/10 at 14:23:43, mlamouri wrote: > > > https://codereview.chromium.org/550173002/diff/20001/content/public/common/ma... > > File content/public/common/manifest.h (right): > > > > > https://codereview.chromium.org/550173002/diff/20001/content/public/common/ma... > > content/public/common/manifest.h:19: bool IsEmpty() const; > > On 2014/09/10 08:05:21, jochen wrote: > > > no methods on structs. if you want this to be a struct, it should have a > ctor > > > anyway > > > > What are the alternatives here? I keep struct and make IsEmpty a static method > that takes a Manifest as parameter? or would that be preferable to move to a > class, in which case I will need to have getters and setters, right? > > The difference between struct and class is mostly the default visibility which > can be changed with private: public:. You could just expose the internal > variables by adding them to the public: section. > > The other use for struct is to save space, but then it needs to be POD which is > probably what jochen is referring to. Then there are rules like no virtuals, > etc. http://en.wikipedia.org/wiki/Plain_old_data_structure I know the difference between a struct and a class in C++ :) but my understanding is that the chromium coding style requires a class to have its members private - sometimes protected, but never public.
jochen@chromium.org changed reviewers: + palmer@chromium.org
lgtm +palmer who wanted to see the manifest struct def https://codereview.chromium.org/550173002/diff/40001/content/public/common/ma... File content/public/common/manifest.h (right): https://codereview.chromium.org/550173002/diff/40001/content/public/common/ma... content/public/common/manifest.h:17: Manifest(); ctor should take paramters for name and short_name https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... content/renderer/manifest/manifest_parser.cc:57: return base::NullableString16(name, false); enforce a max length here? https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... content/renderer/manifest/manifest_parser.cc:72: return base::NullableString16(short_name, false); and here?
https://codereview.chromium.org/550173002/diff/40001/content/public/common/ma... File content/public/common/manifest.h (right): https://codereview.chromium.org/550173002/diff/40001/content/public/common/ma... content/public/common/manifest.h:17: Manifest(); On 2014/09/12 11:55:18, jochen wrote: > ctor should take paramters for name and short_name Why? Those members might or might not be present in the manifest. Requesting them to be passed to the ctor would be quite a burden for the callers. Especially when the Manifest structure will contain more fields like start_url, icons, display, etc. https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... content/renderer/manifest/manifest_parser.cc:57: return base::NullableString16(name, false); On 2014/09/12 11:55:18, jochen wrote: > enforce a max length here? I would prefer to avoid that. Maybe we could truncate the strings before sending them to the browser process over IPC but I would prefer to not have that logic in the parser. https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... content/renderer/manifest/manifest_parser.cc:72: return base::NullableString16(short_name, false); On 2014/09/12 11:55:18, jochen wrote: > and here? ditto.
https://codereview.chromium.org/550173002/diff/40001/content/public/common/ma... File content/public/common/manifest.h (right): https://codereview.chromium.org/550173002/diff/40001/content/public/common/ma... content/public/common/manifest.h:17: Manifest(); On 2014/09/12 12:05:19, Mounir Lamouri wrote: > On 2014/09/12 11:55:18, jochen wrote: > > ctor should take paramters for name and short_name > > Why? Those members might or might not be present in the manifest. Requesting > them to be passed to the ctor would be quite a burden for the callers. > Especially when the Manifest structure will contain more fields like start_url, > icons, display, etc. currently, the parser always sets both fields, and that's how we do structs anyways https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... content/renderer/manifest/manifest_parser.cc:57: return base::NullableString16(name, false); On 2014/09/12 12:05:19, Mounir Lamouri wrote: > On 2014/09/12 11:55:18, jochen wrote: > > enforce a max length here? > > I would prefer to avoid that. Maybe we could truncate the strings before sending > them to the browser process over IPC but I would prefer to not have that logic > in the parser. well, it's not really a parser, but also a builder for the manifest class
https://codereview.chromium.org/550173002/diff/40001/content/public/common/ma... File content/public/common/manifest.h (right): https://codereview.chromium.org/550173002/diff/40001/content/public/common/ma... content/public/common/manifest.h:17: Manifest(); On 2014/09/12 12:09:40, jochen wrote: > On 2014/09/12 12:05:19, Mounir Lamouri wrote: > > On 2014/09/12 11:55:18, jochen wrote: > > > ctor should take paramters for name and short_name > > > > Why? Those members might or might not be present in the manifest. Requesting > > them to be passed to the ctor would be quite a burden for the callers. > > Especially when the Manifest structure will contain more fields like > start_url, > > icons, display, etc. > > currently, the parser always sets both fields, and that's how we do structs > anyways What do you mean by "that's how we do structs"? Most of the struct in content/public/common/ I found did not have ctors to initialize all members. https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... content/renderer/manifest/manifest_parser.cc:57: return base::NullableString16(name, false); On 2014/09/12 12:09:40, jochen wrote: > On 2014/09/12 12:05:19, Mounir Lamouri wrote: > > On 2014/09/12 11:55:18, jochen wrote: > > > enforce a max length here? > > > > I would prefer to avoid that. Maybe we could truncate the strings before > sending > > them to the browser process over IPC but I would prefer to not have that logic > > in the parser. > > well, it's not really a parser, but also a builder for the manifest class True. However, the string truncation would be a security measure for IPC, right? If there is a consumer of the Manifest in the renderer process, would it make sense to truncate those strings?
https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... content/renderer/manifest/manifest_parser.cc:57: return base::NullableString16(name, false); On 2014/09/12 at 12:16:47, Mounir Lamouri wrote: > On 2014/09/12 12:09:40, jochen wrote: > > On 2014/09/12 12:05:19, Mounir Lamouri wrote: > > > On 2014/09/12 11:55:18, jochen wrote: > > > > enforce a max length here? > > > > > > I would prefer to avoid that. Maybe we could truncate the strings before > > sending > > > them to the browser process over IPC but I would prefer to not have that logic > > > in the parser. > > > > well, it's not really a parser, but also a builder for the manifest class > > True. However, the string truncation would be a security measure for IPC, right? If there is a consumer of the Manifest in the renderer process, would it make sense to truncate those strings? is there a consumer?
https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... content/renderer/manifest/manifest_parser.cc:57: return base::NullableString16(name, false); On 2014/09/12 12:23:09, jochen wrote: > On 2014/09/12 at 12:16:47, Mounir Lamouri wrote: > > On 2014/09/12 12:09:40, jochen wrote: > > > On 2014/09/12 12:05:19, Mounir Lamouri wrote: > > > > On 2014/09/12 11:55:18, jochen wrote: > > > > > enforce a max length here? > > > > > > > > I would prefer to avoid that. Maybe we could truncate the strings before > > > sending > > > > them to the browser process over IPC but I would prefer to not have that > logic > > > > in the parser. > > > > > > well, it's not really a parser, but also a builder for the manifest class > > > > True. However, the string truncation would be a security measure for IPC, > right? If there is a consumer of the Manifest in the renderer process, would it > make sense to truncate those strings? > > is there a consumer? I have implemented a GetManifest() method in the renderer process manager. The reason is that Push API wants to use the Manifest so very likely the PushDispatcher will be a consumer.
On 2014/09/12 at 12:32:32, mlamouri wrote: > https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... > File content/renderer/manifest/manifest_parser.cc (right): > > https://codereview.chromium.org/550173002/diff/40001/content/renderer/manifes... > content/renderer/manifest/manifest_parser.cc:57: return base::NullableString16(name, false); > On 2014/09/12 12:23:09, jochen wrote: > > On 2014/09/12 at 12:16:47, Mounir Lamouri wrote: > > > On 2014/09/12 12:09:40, jochen wrote: > > > > On 2014/09/12 12:05:19, Mounir Lamouri wrote: > > > > > On 2014/09/12 11:55:18, jochen wrote: > > > > > > enforce a max length here? > > > > > > > > > > I would prefer to avoid that. Maybe we could truncate the strings before > > > > sending > > > > > them to the browser process over IPC but I would prefer to not have that > > logic > > > > > in the parser. > > > > > > > > well, it's not really a parser, but also a builder for the manifest class > > > > > > True. However, the string truncation would be a security measure for IPC, > > right? If there is a consumer of the Manifest in the renderer process, would it > > make sense to truncate those strings? > > > > is there a consumer? > > I have implemented a GetManifest() method in the renderer process manager. The reason is that Push API wants to use the Manifest so very likely the PushDispatcher will be a consumer. i dunno, it's odd to use different manifests in the renderer and the browser. Chris, wdyt?
On 2014/09/12 12:36:04, jochen wrote: > i dunno, it's odd to use different manifests in the renderer and the browser. > Chris, wdyt? I agree with that concern but I would prefer to avoid leaking current IPC security measures to the ManifestParser. Ideally, when we truncate those strings to be sent to the browser process, we will be careful about having high enough truncating values for the truncation to not be noticeable. No consumer would be able to notice anything for |name| and |short_name| but let's imagine that we add a |description| field, the field would be open ended. UI's might decide to truncate but we shouldn't make the decision at the parser level. If the consumers of the Manifest on the browser process end up with a truncated |description|, fair enough, but we shouldn't require all consumers to live with that requirements. To some extent, I think it would be similar than saying that |document.title| or WebDocument.title() should be truncated to 4k because this is the size of the string we send to the browser process. Consumers of the document title in the renderer process will unlikely be using the truncated version of the title.
jochen@, based on Chris' comment on the other CL [1], truncating the string on the renderer side is only for efficiency and we should actually make that happen on the browser process side. Based on that, I think it would be fair to land this CL as is and handle the IPC security implementation details in [1]. WDYT? [1] https://codereview.chromium.org/537053002/
lgtm https://codereview.chromium.org/550173002/diff/60001/content/renderer/manifes... File content/renderer/manifest/manifest_parser.h (right): https://codereview.chromium.org/550173002/diff/60001/content/renderer/manifes... content/renderer/manifest/manifest_parser.h:35: static base::NullableString16 ParseName( nit. can you move those helpers into an anonymous namespace in the cc file?
https://codereview.chromium.org/550173002/diff/60001/content/renderer/manifes... File content/renderer/manifest/manifest_parser.h (right): https://codereview.chromium.org/550173002/diff/60001/content/renderer/manifes... content/renderer/manifest/manifest_parser.h:35: static base::NullableString16 ParseName( On 2014/09/15 14:13:28, jochen wrote: > nit. can you move those helpers into an anonymous namespace in the cc file? Done.
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/550173002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as ef18022da89088218d70f9916988bfdb36489637
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/161ea5d82dafeeb644ee04b1df52fa7dd6313105 Cr-Commit-Position: refs/heads/master@{#294839} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
