Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
919293002:
Add related_applications field to manifest parser. (Closed)
|
Created:
5 years, 10 months ago by benwells Modified:
5 years, 8 months ago CC:
chrome-apps-syd-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_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. |
DescriptionAdd related_applications field to manifest parser.
The related_applications field will be used for sites to specify which apps the banners should show for.
This change also adds the prefer_related_applications field.
BUG=457403
Committed: https://crrev.com/e2aa9da48d4eafe05d449fab01c8bbc43087a441
Cr-Commit-Position: refs/heads/master@{#325819}
Patch Set 1 #
Total comments: 25
Patch Set 2 : Feedback #Patch Set 3 : Do IPC stuff #
Total comments: 4
Patch Set 4 : Fix stuffup #
Total comments: 2
Patch Set 5 : Improve comment for new array #
Total comments: 2
Patch Set 6 : Update for additions to spec #
Total comments: 18
Patch Set 7 : Rebase #Patch Set 8 : Review feedback #
Total comments: 6
Patch Set 9 : Feedback #
Messages
Total messages: 57 (25 generated)
benwells@chromium.org changed reviewers: + mlamouri@chromium.org
Mounir - initial review pls.. https://codereview.chromium.org/919293002/diff/1/content/public/common/manife... File content/public/common/manifest.cc (right): https://codereview.chromium.org/919293002/diff/1/content/public/common/manife... content/public/common/manifest.cc:40: chrome_related_applications.empty(); Hmmm this was git cl format. I can put it back as it was if you like.
lgtm with comments applied https://codereview.chromium.org/919293002/diff/1/content/public/common/manife... File content/public/common/manifest.cc (right): https://codereview.chromium.org/919293002/diff/1/content/public/common/manife... content/public/common/manifest.cc:40: chrome_related_applications.empty(); On 2015/02/13 at 05:42:37, benwells wrote: > Hmmm this was git cl format. I can put it back as it was if you like. Please :) `git cl format` knows nothing about readability :) https://codereview.chromium.org/919293002/diff/1/content/public/common/manife... File content/public/common/manifest.h (right): https://codereview.chromium.org/919293002/diff/1/content/public/common/manife... content/public/common/manifest.h:30: enum RelatedApplicationPlatform { Could you prefix that with Chrome? https://codereview.chromium.org/919293002/diff/1/content/public/common/manife... content/public/common/manifest.h:64: struct CONTENT_EXPORT RelatedApplication { ditto. https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser.cc:352: base::NullableString16 display = ParseString(application, "platform", Trim); s/display/platform/ https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser.cc:354: return Manifest::RELATED_APPLICATION_PLATFORM_UNSPECIFIED; Could you add an error in that case? It seems that 'platform' is a requirement here. https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser.cc:398: continue; Could you add a comment pointing that no error is required because ParseRelatedApplicationPlatform() should have done that already? https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... File content/renderer/manifest/manifest_parser.h (right): https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser.h:140: Manifest::RelatedApplicationPlatform ParseRelatedApplicationPlatform( nit: maybe prefix with Chrome? https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser.h:145: base::NullableString16 ParseRelatedApplicationId( ditto https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser_unittest.cc:885: "]}"); nit: maybe keep it on one line? https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser_unittest.cc:895: "{}]}"); nit: ditto https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser_unittest.cc:917: "{\"id\": \"foo\"}]}"); Seems like an error in that case would be good. https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser_unittest.cc:917: "{\"id\": \"foo\"}]}"); Seems like an error in that case would be good. https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser_unittest.cc:922: Could you add a test where you have two invalid packages, one because the platform is invalid, another because the platform isn't specified and then a third package that is valid and make sure that the manifest is not empty, has the valid application platform and the errors are correctly thrown.
New patchsets have been uploaded after l-g-t-m from mlamouri@chromium.org
benwells@chromium.org changed reviewers: + avi@chromium.org
+avi for content/public owners review https://codereview.chromium.org/919293002/diff/1/content/public/common/manife... File content/public/common/manifest.cc (right): https://codereview.chromium.org/919293002/diff/1/content/public/common/manife... content/public/common/manifest.cc:40: chrome_related_applications.empty(); On 2015/02/16 20:20:39, Mounir Lamouri wrote: > On 2015/02/13 at 05:42:37, benwells wrote: > > Hmmm this was git cl format. I can put it back as it was if you like. > > Please :) `git cl format` knows nothing about readability :) Done. https://codereview.chromium.org/919293002/diff/1/content/public/common/manife... File content/public/common/manifest.h (right): https://codereview.chromium.org/919293002/diff/1/content/public/common/manife... content/public/common/manifest.h:30: enum RelatedApplicationPlatform { On 2015/02/16 20:20:39, Mounir Lamouri wrote: > Could you prefix that with Chrome? Done. https://codereview.chromium.org/919293002/diff/1/content/public/common/manife... content/public/common/manifest.h:64: struct CONTENT_EXPORT RelatedApplication { On 2015/02/16 20:20:39, Mounir Lamouri wrote: > ditto. Done. https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser.cc:352: base::NullableString16 display = ParseString(application, "platform", Trim); On 2015/02/16 20:20:40, Mounir Lamouri wrote: > s/display/platform/ Done. https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser.cc:354: return Manifest::RELATED_APPLICATION_PLATFORM_UNSPECIFIED; On 2015/02/16 20:20:39, Mounir Lamouri wrote: > Could you add an error in that case? It seems that 'platform' is a requirement > here. Done. https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... File content/renderer/manifest/manifest_parser.h (right): https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser.h:140: Manifest::RelatedApplicationPlatform ParseRelatedApplicationPlatform( On 2015/02/16 20:20:40, Mounir Lamouri wrote: > nit: maybe prefix with Chrome? Done. https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser.h:145: base::NullableString16 ParseRelatedApplicationId( On 2015/02/16 20:20:40, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser_unittest.cc:885: "]}"); On 2015/02/16 20:20:40, Mounir Lamouri wrote: > nit: maybe keep it on one line? Done. https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser_unittest.cc:895: "{}]}"); On 2015/02/16 20:20:40, Mounir Lamouri wrote: > nit: ditto Done. https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser_unittest.cc:917: "{\"id\": \"foo\"}]}"); On 2015/02/16 20:20:40, Mounir Lamouri wrote: > Seems like an error in that case would be good. Done. https://codereview.chromium.org/919293002/diff/1/content/renderer/manifest/ma... content/renderer/manifest/manifest_parser_unittest.cc:922: On 2015/02/16 20:20:40, Mounir Lamouri wrote: > Could you add a test where you have two invalid packages, one because the > platform is invalid, another because the platform isn't specified and then a > third package that is valid and make sure that the manifest is not empty, has > the valid application platform and the errors are correctly thrown. Done.
lgtm with the manifest_messages.h comment addressed; the nits are optional. https://codereview.chromium.org/919293002/diff/40001/content/browser/manifest... File content/browser/manifest/manifest_manager_host.cc (right): https://codereview.chromium.org/919293002/diff/40001/content/browser/manifest... content/browser/manifest/manifest_manager_host.cc:130: for (size_t i = 0; i < manifest.chrome_related_applications.size(); ++i) { nit: maybe that would be easier to read: for (ChromeRelatedApplication& application : manifest.chrome_related_applications) { ... } https://codereview.chromium.org/919293002/diff/40001/content/common/manifest_... File content/common/manifest_manager_messages.h (right): https://codereview.chromium.org/919293002/diff/40001/content/common/manifest_... content/common/manifest_manager_messages.h:22: content::Manifest::CHROME_RELATED_APPLICATION_PLATFORM_ANDROID) Shouldn't that be _WEB instead? https://codereview.chromium.org/919293002/diff/40001/content/renderer/manifes... File content/renderer/manifest/manifest_manager.cc (right): https://codereview.chromium.org/919293002/diff/40001/content/renderer/manifes... content/renderer/manifest/manifest_manager.cc:74: for (size_t i = 0; i < ipc_manifest.chrome_related_applications.size(); ++i) { nit: maybe that would be easier to read: for (ChromeRelatedApplication& application : manifest.chrome_related_applications) { ... }
New patchsets have been uploaded after l-g-t-m from mlamouri@chromium.org
https://codereview.chromium.org/919293002/diff/40001/content/common/manifest_... File content/common/manifest_manager_messages.h (right): https://codereview.chromium.org/919293002/diff/40001/content/common/manifest_... content/common/manifest_manager_messages.h:22: content::Manifest::CHROME_RELATED_APPLICATION_PLATFORM_ANDROID) On 2015/02/17 12:43:20, Mounir Lamouri wrote: > Shouldn't that be _WEB instead? Derrr .... Done.
btw will do the optional nits another day; it's late and I want to test properly after making that change which is a touch involved.
benwells@chromium.org changed reviewers: + jschuh@chromium.org
+jschuh for manifest_manager_messages.h
I'm starting to get a bit weirded out here. Manifest is a core web concept, but this particular field is explicitly even calling itself out as a Chrome feature. The line we draw is: "is this going to be useful to everyone who needs to support HTML, or just Chrome?" Right now this seems to be the latter. https://codereview.chromium.org/919293002/diff/60001/content/public/common/ma... File content/public/common/manifest.h (right): https://codereview.chromium.org/919293002/diff/60001/content/public/common/ma... content/public/common/manifest.h:116: std::vector<ChromeRelatedApplication> chrome_related_applications; set? does order matter?
Weirding out noted. I've contacted you offline about it... https://codereview.chromium.org/919293002/diff/60001/content/public/common/ma... File content/public/common/manifest.h (right): https://codereview.chromium.org/919293002/diff/60001/content/public/common/ma... content/public/common/manifest.h:116: std::vector<ChromeRelatedApplication> chrome_related_applications; On 2015/02/17 16:20:22, Avi wrote: > set? does order matter? Order does matter. Improved comment to make it clear.
On 2015/02/17 20:31:13, benwells wrote: > Weirding out noted. I've contacted you offline about it... > > https://codereview.chromium.org/919293002/diff/60001/content/public/common/ma... > File content/public/common/manifest.h (right): > > https://codereview.chromium.org/919293002/diff/60001/content/public/common/ma... > content/public/common/manifest.h:116: std::vector<ChromeRelatedApplication> > chrome_related_applications; > On 2015/02/17 16:20:22, Avi wrote: > > set? does order matter? > > Order does matter. Improved comment to make it clear. Thanks. Meanwhile, this is a Chrome feature. We definitely should have some kind of delegate callback (content client?) to allow embedders to handle non-content-level keys in the manifest, and I'm a bit surprised that we don't already have that. But let's not put Chrome features into content.
https://codereview.chromium.org/919293002/diff/80001/content/public/common/ma... File content/public/common/manifest.h (right): https://codereview.chromium.org/919293002/diff/80001/content/public/common/ma... content/public/common/manifest.h:73: base::NullableString16 id; Any way we can make this type more specific? I'm just very wary of complex data getting smuggled around in strings.
https://codereview.chromium.org/919293002/diff/80001/content/public/common/ma... File content/public/common/manifest.h (right): https://codereview.chromium.org/919293002/diff/80001/content/public/common/ma... content/public/common/manifest.h:73: base::NullableString16 id; On 2015/02/17 21:55:12, jschuh wrote: > Any way we can make this type more specific? I'm just very wary of complex data > getting smuggled around in strings. It's a string like "com.chrome.beta". In some cases it won't be present at all. Note sure if it can represented in anything else... Note: this CL will change fairly significantly once avi's changes are worked in.
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
Mounir - I've udpated this for the spec updates. Wanna have another look?
Thanks! :) I have a couple of comments. See below. https://codereview.chromium.org/919293002/diff/140001/content/browser/manifes... File content/browser/manifest/manifest_manager_host.cc (right): https://codereview.chromium.org/919293002/diff/140001/content/browser/manifes... content/browser/manifest/manifest_manager_host.cc:133: manifest.related_applications[i].id= base::NullableString16( nit: space before the "=". https://codereview.chromium.org/919293002/diff/140001/content/browser/manifes... content/browser/manifest/manifest_manager_host.cc:138: } Can you sanitize the url too? https://codereview.chromium.org/919293002/diff/140001/content/public/common/m... File content/public/common/manifest.h (right): https://codereview.chromium.org/919293002/diff/140001/content/public/common/m... content/public/common/manifest.h:33: }; Shouldn't we have the embedder decide which platforms are recognized? The spec do not allow invalid string but doesn't have a list of known values. If another embedder than Chrome wants to accept values like "itunes" or "windows8", we should probably allow them to do so, right? https://codereview.chromium.org/919293002/diff/140001/content/public/common/m... content/public/common/manifest.h:75: base::NullableString16 id; Could you add a comment saying that if the |id| is null |url| shouldn't and vice versa? https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:356: } I would do: return ParseString(application, "platform", Trim); then in the |ParseRelatedApplications| check if it's not null. https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... File content/renderer/manifest/manifest_parser.h (right): https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... content/renderer/manifest/manifest_parser.h:132: const base::DictionaryValue& application); The spec say it should return a string. https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:822: // If invalid platform, application is ignored. Keep that test but make it invalid per type like having an object set to the platform property. https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:857: // Android application, with url. I don't think we should test that on the content layer, as far as the content layer is concerned |platform| could be anything. https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:871: // Android application, with id. Could you add a test where there is no id nor url? It should fail.
Patchset #8 (id:180001) has been deleted
https://codereview.chromium.org/919293002/diff/140001/content/browser/manifes... File content/browser/manifest/manifest_manager_host.cc (right): https://codereview.chromium.org/919293002/diff/140001/content/browser/manifes... content/browser/manifest/manifest_manager_host.cc:133: manifest.related_applications[i].id= base::NullableString16( On 2015/04/10 09:49:44, Mounir Lamouri wrote: > nit: space before the "=". Done. https://codereview.chromium.org/919293002/diff/140001/content/browser/manifes... content/browser/manifest/manifest_manager_host.cc:138: } On 2015/04/10 09:49:44, Mounir Lamouri wrote: > Can you sanitize the url too? Done. https://codereview.chromium.org/919293002/diff/140001/content/public/common/m... File content/public/common/manifest.h (right): https://codereview.chromium.org/919293002/diff/140001/content/public/common/m... content/public/common/manifest.h:33: }; On 2015/04/10 09:49:44, Mounir Lamouri wrote: > Shouldn't we have the embedder decide which platforms are recognized? The spec > do not allow invalid string but doesn't have a list of known values. If another > embedder than Chrome wants to accept values like "itunes" or "windows8", we > should probably allow them to do so, right? OK, makes sense. Done. https://codereview.chromium.org/919293002/diff/140001/content/public/common/m... content/public/common/manifest.h:75: base::NullableString16 id; On 2015/04/10 09:49:44, Mounir Lamouri wrote: > Could you add a comment saying that if the |id| is null |url| shouldn't and vice > versa? Done. https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... File content/renderer/manifest/manifest_parser.cc (right): https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... content/renderer/manifest/manifest_parser.cc:356: } On 2015/04/10 09:49:45, Mounir Lamouri wrote: > I would do: > return ParseString(application, "platform", Trim); > > then in the |ParseRelatedApplications| check if it's not null. Done. https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... File content/renderer/manifest/manifest_parser.h (right): https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... content/renderer/manifest/manifest_parser.h:132: const base::DictionaryValue& application); On 2015/04/10 09:49:45, Mounir Lamouri wrote: > The spec say it should return a string. Done. https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:822: // If invalid platform, application is ignored. On 2015/04/10 09:49:45, Mounir Lamouri wrote: > Keep that test but make it invalid per type like having an object set to the > platform property. Done. https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:857: // Android application, with url. On 2015/04/10 09:49:45, Mounir Lamouri wrote: > I don't think we should test that on the content layer, as far as the content > layer is concerned |platform| could be anything. Sure, I've reworded the comment and used itunes for some of the platforms. https://codereview.chromium.org/919293002/diff/140001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:871: // Android application, with id. On 2015/04/10 09:49:45, Mounir Lamouri wrote: > Could you add a test where there is no id nor url? It should fail. It is two tests up already, "If missing id and url, application is ignored".
lgtm, thanks! :) https://codereview.chromium.org/919293002/diff/200001/content/renderer/manife... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/919293002/diff/200001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:915: "{\"platform\": \"play\", \"id\": \"foo\"},{}]}"); Could you put the empty application in a new line? The "{}" got lost in the noise and this test was a bit confusing for a few seconds ;)
not lgtm My objection has not been addressed. This is not core to the web, or part of a spec, or something that every embedder of content would need or even find useful. If you want this functionality, find a way for content to provide the embedder the opportunity to parse non-standard parts of the manifest. You started that in https://codereview.chromium.org/936413003/ and I would be happy to continue working with you there. But you may not land this change as-is.
On 2015/04/15 15:21:07, Avi wrote: > not lgtm > > My objection has not been addressed. This is not core to the web, or part of a > spec, or something that every embedder of content would need or even find > useful. > > If you want this functionality, find a way for content to provide the embedder > the opportunity to parse non-standard parts of the manifest. You started that in > https://codereview.chromium.org/936413003/ and I would be happy to continue > working with you there. > > But you may not land this change as-is. Avi, once I had Mounir's lg tm I was going to explain but you're too fast for me. This is now part of the spec (see http://www.w3.org/TR/appmanifest/#related_applications-member) and we have the required lgs on the intent to ship thread (https://groups.google.com/a/chromium.org/forum/m/#!topic/blink-dev/OcJ0b7JPZ5M). So I hope all concerns about proprietariness are ok now.
Oh. That really does change things. A few nits, but this lgtm. BTW, in the other CL you were trying to split off the "gcm_" stuff from the manifest, and it would be nice if you could do that. Unless of course you can provide a spec reference. :) https://codereview.chromium.org/919293002/diff/200001/content/browser/manifes... File content/browser/manifest/manifest_manager_host.cc (right): https://codereview.chromium.org/919293002/diff/200001/content/browser/manifes... content/browser/manifest/manifest_manager_host.cc:132: for (size_t i = 0; i < manifest.related_applications.size(); ++i) { for (auto& related_app : manifest.related_applications) ? https://codereview.chromium.org/919293002/diff/200001/content/renderer/manife... File content/renderer/manifest/manifest_manager.cc (right): https://codereview.chromium.org/919293002/diff/200001/content/renderer/manife... content/renderer/manifest/manifest_manager.cc:79: } ditto re for (auto x : y)
jschuh: this is now ready (again) for a security / IPC review. When you last looked you asked if all the strings in the IPCs had to be strings. I think they do. They are not formatted information that needs to be parsed, but strings that are used as is (e.g. "play" or "itunes" for platform, and "com.google.chrome" for id). https://codereview.chromium.org/919293002/diff/200001/content/browser/manifes... File content/browser/manifest/manifest_manager_host.cc (right): https://codereview.chromium.org/919293002/diff/200001/content/browser/manifes... content/browser/manifest/manifest_manager_host.cc:132: for (size_t i = 0; i < manifest.related_applications.size(); ++i) { On 2015/04/15 20:36:34, Avi wrote: > for (auto& related_app : manifest.related_applications) > > ? Done. https://codereview.chromium.org/919293002/diff/200001/content/renderer/manife... File content/renderer/manifest/manifest_manager.cc (right): https://codereview.chromium.org/919293002/diff/200001/content/renderer/manife... content/renderer/manifest/manifest_manager.cc:79: } On 2015/04/15 20:36:34, Avi wrote: > ditto re for (auto x : y) Done. https://codereview.chromium.org/919293002/diff/200001/content/renderer/manife... File content/renderer/manifest/manifest_parser_unittest.cc (right): https://codereview.chromium.org/919293002/diff/200001/content/renderer/manife... content/renderer/manifest/manifest_parser_unittest.cc:915: "{\"platform\": \"play\", \"id\": \"foo\"},{}]}"); On 2015/04/15 08:46:31, Mounir Lamouri wrote: > Could you put the empty application in a new line? The "{}" got lost in the > noise and this test was a bit confusing for a few seconds ;) Done.
Sorry, I have a family emergency that will be keeping me away from work for a bit, and I'm handing things off. Would you mind looping in tsepez@ to review?
benwells@chromium.org changed reviewers: + tsepez@chromium.org - jschuh@chromium.org
-jschuh +tsepez for ipc security review
On 2015/04/16 22:10:31, benwells wrote: > -jschuh > +tsepez for ipc security review Messages LGTM.
The CQ bit was checked by benwells@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/919293002/#ps220001 (title: "Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/919293002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by benwells@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/919293002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by benwells@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/919293002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by benwells@chromium.org
The CQ bit was unchecked by benwells@chromium.org
The CQ bit was checked by benwells@chromium.org
The CQ bit was unchecked by benwells@chromium.org
The CQ bit was checked by benwells@chromium.org
The CQ bit was unchecked by benwells@chromium.org
The CQ bit was checked by benwells@chromium.org
The CQ bit was unchecked by benwells@chromium.org
The CQ bit was checked by benwells@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/919293002/220001
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e2aa9da48d4eafe05d449fab01c8bbc43087a441 Cr-Commit-Position: refs/heads/master@{#325819} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
