Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18770009:
Added a new policy setting as well as corrisponding registry key to allow GCF users the option to d… (Closed)
|
Created:
7 years, 5 months ago by JoeK.G Modified:
7 years, 4 months ago CC:
chromium-reviews, grt+watch_chromium.org, amit, joaodasilva+watch_chromium.org, robertshield Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdded a new policy setting as well as corrisponding registry key to allow GCF users the option to disable the meta tag check. That is, the tag that websites can use to enable GCF. This way the white/black lists are fully respected by GCF.
BUG=225971
TEST=Set HKEY_CURRENT_USER\Software\Google\ChromeFrame\SkipGCFMetaDataCheck to true to disable GCF on sites enabled through the meta data tag (fully respects white/black lists).
Contributed by joe.knoll@workday.com
Patch Set 1 #Patch Set 2 : #
Total comments: 14
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 3
Patch Set 6 : #Patch Set 7 : Updated policy file #Patch Set 8 : Re-basing policy file #Patch Set 9 : Update Policy and Authors file. #
Total comments: 1
Patch Set 10 : Rebased #Patch Set 11 : Rebased #Patch Set 12 : Uploading from *@workday.com #
Messages
Total messages: 37 (0 generated)
A few style comments https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settin... File chrome_frame/policy_settings.cc (right): https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settin... chrome_frame/policy_settings.cc:109: PerformMetadataCheck* metadata_check) { please use two space-char indents everywhere here. https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settin... chrome_frame/policy_settings.cc:129: value == METADATA_CHECK_YES) << the "value"s should be aligned. https://codereview.chromium.org/18770009/diff/7001/chrome_frame/utils.cc File chrome_frame/utils.cc (right): https://codereview.chromium.org/18770009/diff/7001/chrome_frame/utils.cc#newc... chrome_frame/utils.cc:742: /** Please use // style comments.
drive-by https://codereview.chromium.org/18770009/diff/7001/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/18770009/diff/7001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:2532: 'name': 'ChromeFrameMetadataCheckSettings', ChromeFrameMetadataCheckSettings -> SkipMetadataCheck ("Metadata" is much more prevalent than "MetaData" in the codebase) https://codereview.chromium.org/18770009/diff/7001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:2535: 'supported_on': ['chrome_frame:27-'], 27 -> 30 since that's what trunk is currently building. https://codereview.chromium.org/18770009/diff/7001/chrome/test/data/policy/po... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/18770009/diff/7001/chrome/test/data/policy/po... chrome/test/data/policy/policy_test_cases.json:2002: "ChromeFrameMetadataCheckSettings": { extra newline before this https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settin... File chrome_frame/policy_settings.cc (right): https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settin... chrome_frame/policy_settings.cc:118: for (int i = 0; i < arraysize(kRootKeys); ++i) { please extract this hunk of code into a private function in the unnamed namespace and make the other two places in this file that do the exact same thing call it. https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settin... chrome_frame/policy_settings.cc:129: value == METADATA_CHECK_YES) << and "<<" should be moved down to the next line https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settin... File chrome_frame/policy_settings.h (right): https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settin... chrome_frame/policy_settings.h:32: typedef enum PerformMetadataCheck { remove "typedef " here and on line 20, and move this up to line 26 so that it immediately follows the definition of RendererForUrl. https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settin... chrome_frame/policy_settings.h:32: typedef enum PerformMetadataCheck { based on how this is used, "SkipMetadataCheck" is more appropriate. https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settin... chrome_frame/policy_settings.h:33: METADATA_CHECK_NOT_SPECIFIED = -1, METADATA_CHECK_... - >SKIP_METADATA_CHECK_... https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settin... chrome_frame/policy_settings.h:38: PerformMetadataCheck metadata_check() const { metadata_check -> skip_metadata_check https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settin... chrome_frame/policy_settings.h:39: return metadata_check_; two-space indent https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settin... chrome_frame/policy_settings.h:88: PerformMetadataCheck metadata_check_; metadata_check_ -> skip_metadata_check_
Uploaded new patch with Roberts changes. I didn't make all of grts changes as a couple of the style changes would bring the new code out of sync with the old code. As for the typedef removal, why? On Mon, Jul 29, 2013 at 8:12 PM, <robertshield@chromium.org> wrote: > A few style comments > > > https://codereview.chromium.**org/18770009/diff/7001/chrome_** > frame/policy_settings.cc<https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settings.cc> > File chrome_frame/policy_settings.**cc (right): > > https://codereview.chromium.**org/18770009/diff/7001/chrome_** > frame/policy_settings.cc#**newcode109<https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settings.cc#newcode109> > chrome_frame/policy_settings.**cc:109: PerformMetadataCheck* > metadata_check) { > please use two space-char indents everywhere here. > > https://codereview.chromium.**org/18770009/diff/7001/chrome_** > frame/policy_settings.cc#**newcode129<https://codereview.chromium.org/18770009/diff/7001/chrome_frame/policy_settings.cc#newcode129> > chrome_frame/policy_settings.**cc:129: value == METADATA_CHECK_YES) << > the "value"s should be aligned. > > https://codereview.chromium.**org/18770009/diff/7001/chrome_** > frame/utils.cc<https://codereview.chromium.org/18770009/diff/7001/chrome_frame/utils.cc> > File chrome_frame/utils.cc (right): > > https://codereview.chromium.**org/18770009/diff/7001/chrome_** > frame/utils.cc#newcode742<https://codereview.chromium.org/18770009/diff/7001/chrome_frame/utils.cc#newcode742> > chrome_frame/utils.cc:742: /** > Please use // style comments. > > https://codereview.chromium.**org/18770009/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/18770009/diff/18001/chrome/app/policy/policy_... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/18770009/diff/18001/chrome/app/policy/policy_... chrome/app/policy/policy_templates.json:2640: 'name': 'ChromeFrameMetadataCheckSettings', why no name change? "ChromeFrameMetadataCheckSettings" doesn't tell the reader anything about this GPO, whereas "SkipMetadataCheck" is fairly clear: true means skip, false or unset means don't skip. https://codereview.chromium.org/18770009/diff/18001/chrome/test/data/policy/p... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/18770009/diff/18001/chrome/test/data/policy/p... chrome/test/data/policy/policy_test_cases.json:2060: "ChromeFrameMetadataCheckSettings": { please add a newline before this so that it follows the style of the surrounding items. https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_setti... File chrome_frame/policy_settings.cc (right): https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_setti... chrome_frame/policy_settings.cc:129: value == METADATA_CHECK_YES) << move << to next line https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_setti... File chrome_frame/policy_settings.h (right): https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_setti... chrome_frame/policy_settings.h:32: typedef enum PerformMetadataCheck { remove typedef here and above because they are meaningless. in c++, you can refer to enum Foo {}; as Foo within the scope that Foo is defined. the c way of doing things is typedef enum Foo {} Foo; so that you can refer to "enum Foo" as just "Foo". whoever put the typedef up above in place was mistaken: there's no need for it, and it's not correct. please don't propagate the bad style here. thanks. https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_setti... chrome_frame/policy_settings.h:32: typedef enum PerformMetadataCheck { Unless I'm mistaken, the polarity of this is backwards: METADATA_CHECK_YES is interpreted in utils.cc as skipping rather than performing the check. Please rename this to SkipMetadataCheck as I previously proposed. https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_setti... chrome_frame/policy_settings.h:32: typedef enum PerformMetadataCheck { as mentioned before, this enum must be moved up so that it immediately follows enum RendererForUrl above. https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_setti... chrome_frame/policy_settings.h:33: METADATA_CHECK_NOT_SPECIFIED = -1, two-space indent here rather than four. https://codereview.chromium.org/18770009/diff/18001/chrome_frame/utils.cc File chrome_frame/utils.cc (right): https://codereview.chromium.org/18770009/diff/18001/chrome_frame/utils.cc#new... chrome_frame/utils.cc:750: skip = (metadataCheck == PolicySettings::METADATA_CHECK_YES); |skip| is a dword, not a boolean. i suggest simplifying this to: // Check policy settings PolicySettings::PerformMetadataCheck metadataCheck = PolicySettings::GetInstance()->metadata_check(); if (metadataCheck != PolicySettings::METADATA_CHECK_NOT_SPECIFIED) return (metadataCheck == PolicySettings::METADATA_CHECK_YES); DWORD skip = 0; RegKey config_key; if (config_key.Open(HKEY_CURRENT_USER, kChromeFrameConfigKey, KEY_READ) == ERROR_SUCCESS) { config_key.ReadValueDW(kSkipGCFMetaDataCheck, &skip); } return skip != 0;
Okay, I uploaded a patch that I think (I hope) addresses everything. Thanks Greg for looking at this so quickly! On Thu, Aug 1, 2013 at 12:22 PM, <grt@chromium.org> wrote: > > https://codereview.chromium.**org/18770009/diff/18001/** > chrome/app/policy/policy_**templates.json<https://codereview.chromium.org/18770009/diff/18001/chrome/app/policy/policy_templates.json> > File chrome/app/policy/policy_**templates.json (right): > > https://codereview.chromium.**org/18770009/diff/18001/** > chrome/app/policy/policy_**templates.json#newcode2640<https://codereview.chromium.org/18770009/diff/18001/chrome/app/policy/policy_templates.json#newcode2640> > chrome/app/policy/policy_**templates.json:2640: 'name': > '**ChromeFrameMetadataCheckSettin**gs', > why no name change? "**ChromeFrameMetadataCheckSettin**gs" doesn't tell > the > reader anything about this GPO, whereas "SkipMetadataCheck" is fairly > clear: true means skip, false or unset means don't skip. > > https://codereview.chromium.**org/18770009/diff/18001/** > chrome/test/data/policy/**policy_test_cases.json<https://codereview.chromium.org/18770009/diff/18001/chrome/test/data/policy/policy_test_cases.json> > File chrome/test/data/policy/**policy_test_cases.json (right): > > https://codereview.chromium.**org/18770009/diff/18001/** > chrome/test/data/policy/**policy_test_cases.json#**newcode2060<https://codereview.chromium.org/18770009/diff/18001/chrome/test/data/policy/policy_test_cases.json#newcode2060> > chrome/test/data/policy/**policy_test_cases.json:2060: > "**ChromeFrameMetadataCheckSettin**gs": { > please add a newline before this so that it follows the style of the > surrounding items. > > https://codereview.chromium.**org/18770009/diff/18001/** > chrome_frame/policy_settings.**cc<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_settings.cc> > File chrome_frame/policy_settings.**cc (right): > > https://codereview.chromium.**org/18770009/diff/18001/** > chrome_frame/policy_settings.**cc#newcode129<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_settings.cc#newcode129> > > chrome_frame/policy_settings.**cc:129: value == METADATA_CHECK_YES) << > move << to next line > > https://codereview.chromium.**org/18770009/diff/18001/** > chrome_frame/policy_settings.h<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_settings.h> > File chrome_frame/policy_settings.h (right): > > https://codereview.chromium.**org/18770009/diff/18001/** > chrome_frame/policy_settings.**h#newcode32<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_settings.h#newcode32> > > chrome_frame/policy_settings.**h:32: typedef enum PerformMetadataCheck { > remove typedef here and above because they are meaningless. in c++, you > can refer to enum Foo {}; as Foo within the scope that Foo is defined. > the c way of doing things is typedef enum Foo {} Foo; so that you can > refer to "enum Foo" as just "Foo". whoever put the typedef up above in > place was mistaken: there's no need for it, and it's not correct. please > don't propagate the bad style here. thanks. > > https://codereview.chromium.**org/18770009/diff/18001/** > chrome_frame/policy_settings.**h#newcode32<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_settings.h#newcode32> > > chrome_frame/policy_settings.**h:32: typedef enum PerformMetadataCheck { > Unless I'm mistaken, the polarity of this is backwards: > METADATA_CHECK_YES is interpreted in utils.cc as skipping rather than > performing the check. Please rename this to SkipMetadataCheck as I > previously proposed. > > https://codereview.chromium.**org/18770009/diff/18001/** > chrome_frame/policy_settings.**h#newcode32<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_settings.h#newcode32> > > chrome_frame/policy_settings.**h:32: typedef enum PerformMetadataCheck { > as mentioned before, this enum must be moved up so that it immediately > follows enum RendererForUrl above. > > https://codereview.chromium.**org/18770009/diff/18001/** > chrome_frame/policy_settings.**h#newcode33<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_settings.h#newcode33> > > chrome_frame/policy_settings.**h:33: METADATA_CHECK_NOT_SPECIFIED = -1, > two-space indent here rather than four. > > https://codereview.chromium.**org/18770009/diff/18001/** > chrome_frame/utils.cc<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/utils.cc> > File chrome_frame/utils.cc (right): > > https://codereview.chromium.**org/18770009/diff/18001/** > chrome_frame/utils.cc#**newcode750<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/utils.cc#newcode750> > chrome_frame/utils.cc:750: skip = (metadataCheck == > PolicySettings::METADATA_**CHECK_YES); > |skip| is a dword, not a boolean. i suggest simplifying this to: > // Check policy settings > PolicySettings::**PerformMetadataCheck metadataCheck = > PolicySettings::GetInstance()-**>metadata_check(); > if (metadataCheck != PolicySettings::METADATA_**CHECK_NOT_SPECIFIED) > return (metadataCheck == PolicySettings::METADATA_**CHECK_YES); > > DWORD skip = 0; > RegKey config_key; > if (config_key.Open(HKEY_CURRENT_**USER, kChromeFrameConfigKey, > KEY_READ) == ERROR_SUCCESS) { > config_key.ReadValueDW(**kSkipGCFMetaDataCheck, &skip); > } > return skip != 0; > > https://codereview.chromium.**org/18770009/<https://codereview.chromium.org/1... >
And Robert too for going through all these steps with me. On Thu, Aug 1, 2013 at 2:41 PM, Joe Knoll <josephknoll@gmail.com> wrote: > Okay, I uploaded a patch that I think (I hope) addresses everything. > Thanks Greg for looking at this so quickly! > > > On Thu, Aug 1, 2013 at 12:22 PM, <grt@chromium.org> wrote: > >> >> https://codereview.chromium.**org/18770009/diff/18001/** >> chrome/app/policy/policy_**templates.json<https://codereview.chromium.org/18770009/diff/18001/chrome/app/policy/policy_templates.json> >> File chrome/app/policy/policy_**templates.json (right): >> >> https://codereview.chromium.**org/18770009/diff/18001/** >> chrome/app/policy/policy_**templates.json#newcode2640<https://codereview.chromium.org/18770009/diff/18001/chrome/app/policy/policy_templates.json#newcode2640> >> chrome/app/policy/policy_**templates.json:2640: 'name': >> '**ChromeFrameMetadataCheckSettin**gs', >> why no name change? "**ChromeFrameMetadataCheckSettin**gs" doesn't tell >> the >> reader anything about this GPO, whereas "SkipMetadataCheck" is fairly >> clear: true means skip, false or unset means don't skip. >> >> https://codereview.chromium.**org/18770009/diff/18001/** >> chrome/test/data/policy/**policy_test_cases.json<https://codereview.chromium.org/18770009/diff/18001/chrome/test/data/policy/policy_test_cases.json> >> File chrome/test/data/policy/**policy_test_cases.json (right): >> >> https://codereview.chromium.**org/18770009/diff/18001/** >> chrome/test/data/policy/**policy_test_cases.json#**newcode2060<https://codereview.chromium.org/18770009/diff/18001/chrome/test/data/policy/policy_test_cases.json#newcode2060> >> chrome/test/data/policy/**policy_test_cases.json:2060: >> "**ChromeFrameMetadataCheckSettin**gs": { >> please add a newline before this so that it follows the style of the >> surrounding items. >> >> https://codereview.chromium.**org/18770009/diff/18001/** >> chrome_frame/policy_settings.**cc<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_settings.cc> >> File chrome_frame/policy_settings.**cc (right): >> >> https://codereview.chromium.**org/18770009/diff/18001/** >> chrome_frame/policy_settings.**cc#newcode129<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_settings.cc#newcode129> >> >> chrome_frame/policy_settings.**cc:129: value == METADATA_CHECK_YES) << >> move << to next line >> >> https://codereview.chromium.**org/18770009/diff/18001/** >> chrome_frame/policy_settings.h<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_settings.h> >> File chrome_frame/policy_settings.h (right): >> >> https://codereview.chromium.**org/18770009/diff/18001/** >> chrome_frame/policy_settings.**h#newcode32<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_settings.h#newcode32> >> >> chrome_frame/policy_settings.**h:32: typedef enum PerformMetadataCheck { >> remove typedef here and above because they are meaningless. in c++, you >> can refer to enum Foo {}; as Foo within the scope that Foo is defined. >> the c way of doing things is typedef enum Foo {} Foo; so that you can >> refer to "enum Foo" as just "Foo". whoever put the typedef up above in >> place was mistaken: there's no need for it, and it's not correct. please >> don't propagate the bad style here. thanks. >> >> https://codereview.chromium.**org/18770009/diff/18001/** >> chrome_frame/policy_settings.**h#newcode32<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_settings.h#newcode32> >> >> chrome_frame/policy_settings.**h:32: typedef enum PerformMetadataCheck { >> Unless I'm mistaken, the polarity of this is backwards: >> METADATA_CHECK_YES is interpreted in utils.cc as skipping rather than >> performing the check. Please rename this to SkipMetadataCheck as I >> previously proposed. >> >> https://codereview.chromium.**org/18770009/diff/18001/** >> chrome_frame/policy_settings.**h#newcode32<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_settings.h#newcode32> >> >> chrome_frame/policy_settings.**h:32: typedef enum PerformMetadataCheck { >> as mentioned before, this enum must be moved up so that it immediately >> follows enum RendererForUrl above. >> >> https://codereview.chromium.**org/18770009/diff/18001/** >> chrome_frame/policy_settings.**h#newcode33<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/policy_settings.h#newcode33> >> >> chrome_frame/policy_settings.**h:33: METADATA_CHECK_NOT_SPECIFIED = -1, >> two-space indent here rather than four. >> >> https://codereview.chromium.**org/18770009/diff/18001/** >> chrome_frame/utils.cc<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/utils.cc> >> File chrome_frame/utils.cc (right): >> >> https://codereview.chromium.**org/18770009/diff/18001/** >> chrome_frame/utils.cc#**newcode750<https://codereview.chromium.org/18770009/diff/18001/chrome_frame/utils.cc#newcode750> >> chrome_frame/utils.cc:750: skip = (metadataCheck == >> PolicySettings::METADATA_**CHECK_YES); >> |skip| is a dword, not a boolean. i suggest simplifying this to: >> // Check policy settings >> PolicySettings::**PerformMetadataCheck metadataCheck = >> PolicySettings::GetInstance()-**>metadata_check(); >> if (metadataCheck != PolicySettings::METADATA_**CHECK_NOT_SPECIFIED) >> return (metadataCheck == PolicySettings::METADATA_**CHECK_YES); >> >> DWORD skip = 0; >> RegKey config_key; >> if (config_key.Open(HKEY_CURRENT_**USER, kChromeFrameConfigKey, >> KEY_READ) == ERROR_SUCCESS) { >> config_key.ReadValueDW(**kSkipGCFMetaDataCheck, &skip); >> } >> return skip != 0; >> >> https://codereview.chromium.**org/18770009/<https://codereview.chromium.org/1... >> > >
I'll take another look tonight.
looking good. see below for my final comments. please also replace MetaData with Metadata throughout for consistency. thanks. https://codereview.chromium.org/18770009/diff/30001/chrome/app/policy/policy_... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/18770009/diff/30001/chrome/app/policy/policy_... chrome/app/policy/policy_templates.json:2642: 'schema': { 'type': 'boolean'}, add a space before the closing brace https://codereview.chromium.org/18770009/diff/30001/chrome_frame/policy_setti... File chrome_frame/policy_settings.cc (right): https://codereview.chromium.org/18770009/diff/30001/chrome_frame/policy_setti... chrome_frame/policy_settings.cc:88: << "invalid default renderer setting: " << value; thanks for fixing this! https://codereview.chromium.org/18770009/diff/30001/chrome_frame/policy_setti... File chrome_frame/policy_settings.h (right): https://codereview.chromium.org/18770009/diff/30001/chrome_frame/policy_setti... chrome_frame/policy_settings.h:27: SKIP_METADATA_CHECK_NOT_SPECIFIED = -1, two-space indent here rather than four. https://codereview.chromium.org/18770009/diff/30001/chrome_frame/protocol_sin... File chrome_frame/protocol_sink_wrap.cc (right): https://codereview.chromium.org/18770009/diff/30001/chrome_frame/protocol_sin... chrome_frame/protocol_sink_wrap.cc:471: bool ignore_Use_GCF_MetaData = SkipMetaDataCheck(); since SkipMetaDataCheck may do work (hit the registry), move this down so that it's only called when it is needed. for example, on line 485: renderer_type_ = SkipMetadataCheck() ? RENDERER_TYPE_OTHER : DetermineRendererType(buffer_, buffer_size_, last_chance);
Thanks Greg. Uploaded new patch. On Thu, Aug 1, 2013 at 6:42 PM, <grt@chromium.org> wrote: > looking good. see below for my final comments. please also replace > MetaData with > Metadata throughout for consistency. thanks. > > > https://codereview.chromium.**org/18770009/diff/30001/** > chrome/app/policy/policy_**templates.json<https://codereview.chromium.org/18770009/diff/30001/chrome/app/policy/policy_templates.json> > File chrome/app/policy/policy_**templates.json (right): > > https://codereview.chromium.**org/18770009/diff/30001/** > chrome/app/policy/policy_**templates.json#newcode2642<https://codereview.chromium.org/18770009/diff/30001/chrome/app/policy/policy_templates.json#newcode2642> > chrome/app/policy/policy_**templates.json:2642: 'schema': { 'type': > 'boolean'}, > add a space before the closing brace > > https://codereview.chromium.**org/18770009/diff/30001/** > chrome_frame/policy_settings.**cc<https://codereview.chromium.org/18770009/diff/30001/chrome_frame/policy_settings.cc> > File chrome_frame/policy_settings.**cc (right): > > https://codereview.chromium.**org/18770009/diff/30001/** > chrome_frame/policy_settings.**cc#newcode88<https://codereview.chromium.org/18770009/diff/30001/chrome_frame/policy_settings.cc#newcode88> > chrome_frame/policy_settings.**cc:88: << "invalid default renderer > setting: " << value; > thanks for fixing this! > > https://codereview.chromium.**org/18770009/diff/30001/** > chrome_frame/policy_settings.h<https://codereview.chromium.org/18770009/diff/30001/chrome_frame/policy_settings.h> > File chrome_frame/policy_settings.h (right): > > https://codereview.chromium.**org/18770009/diff/30001/** > chrome_frame/policy_settings.**h#newcode27<https://codereview.chromium.org/18770009/diff/30001/chrome_frame/policy_settings.h#newcode27> > chrome_frame/policy_settings.**h:27: SKIP_METADATA_CHECK_NOT_**SPECIFIED = > > -1, > two-space indent here rather than four. > > https://codereview.chromium.**org/18770009/diff/30001/** > chrome_frame/protocol_sink_**wrap.cc<https://codereview.chromium.org/18770009/diff/30001/chrome_frame/protocol_sink_wrap.cc> > File chrome_frame/protocol_sink_**wrap.cc (right): > > https://codereview.chromium.**org/18770009/diff/30001/** > chrome_frame/protocol_sink_**wrap.cc#newcode471<https://codereview.chromium.org/18770009/diff/30001/chrome_frame/protocol_sink_wrap.cc#newcode471> > chrome_frame/protocol_sink_**wrap.cc:471: bool ignore_Use_GCF_MetaData = > SkipMetaDataCheck(); > since SkipMetaDataCheck may do work (hit the registry), move this down > so that it's only called when it is needed. for example, on line 485: > renderer_type_ = SkipMetadataCheck() ? RENDERER_TYPE_OTHER : > DetermineRendererType(buffer_, buffer_size_, last_chance); > > https://codereview.chromium.**org/18770009/<https://codereview.chromium.org/1... >
Hi Joe, Your change failed on the trybots due to the XML in the policy description. Please fix it and re-upload and I'll check the commit box for you. Thanks. https://codereview.chromium.org/18770009/diff/42001/chrome/app/policy/policy_... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/18770009/diff/42001/chrome/app/policy/policy_... chrome/app/policy/policy_templates.json:2650: 'desc': '''Normally pages with a certain meta tag, for example <meta http-equiv="X-UA-Compatible" content="chrome=1" />, will be rendered in <ph name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome Frame</ex></ph> regardless of the 'ChromeFrameRendererSettings' policy. it looks like the example meta tag you included here is causing a problem, as XML-looking stuff is not permitted in the description text. i suggest you remove the example meta tag. https://codereview.chromium.org/18770009/diff/42001/chrome_frame/policy_setti... File chrome_frame/policy_settings.h (right): https://codereview.chromium.org/18770009/diff/42001/chrome_frame/policy_setti... chrome_frame/policy_settings.h:38: SkipMetadataCheck skip_metadata_check() const { please add a unit test for this in chrome_frame/test/policy_settings_unittest.cc. this can be in a separate change if you'd like. https://codereview.chromium.org/18770009/diff/42001/chrome_frame/utils.h File chrome_frame/utils.h (right): https://codereview.chromium.org/18770009/diff/42001/chrome_frame/utils.h#newc... chrome_frame/utils.h:282: bool SkipMetadataCheck(); please add a unit test for this function in chrome_frame/test/util_unittests.cc. you can do this in a separate change if you'd like.
Thanks Greg. Changed the description and uploaded again. On 8/2/2013 8:15 AM, grt@chromium.org wrote: > Hi Joe, > > Your change failed on the trybots due to the XML in the policy > description. > Please fix it and re-upload and I'll check the commit box for you. > Thanks. > > > https://codereview.chromium.org/18770009/diff/42001/chrome/app/policy/policy_... > > File chrome/app/policy/policy_templates.json (right): > > https://codereview.chromium.org/18770009/diff/42001/chrome/app/policy/policy_... > > chrome/app/policy/policy_templates.json:2650: 'desc': '''Normally pages > with a certain meta tag, for example <meta http-equiv="X-UA-Compatible" > content="chrome=1" />, will be rendered in <ph > name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome Frame</ex></ph> regardless > of the 'ChromeFrameRendererSettings' policy. > it looks like the example meta tag you included here is causing a > problem, as XML-looking stuff is not permitted in the description text. > i suggest you remove the example meta tag. > > https://codereview.chromium.org/18770009/diff/42001/chrome_frame/policy_setti... > > File chrome_frame/policy_settings.h (right): > > https://codereview.chromium.org/18770009/diff/42001/chrome_frame/policy_setti... > > chrome_frame/policy_settings.h:38: SkipMetadataCheck > skip_metadata_check() const { > please add a unit test for this in > chrome_frame/test/policy_settings_unittest.cc. this can be in a separate > change if you'd like. > > https://codereview.chromium.org/18770009/diff/42001/chrome_frame/utils.h > File chrome_frame/utils.h (right): > > https://codereview.chromium.org/18770009/diff/42001/chrome_frame/utils.h#newc... > > chrome_frame/utils.h:282: bool SkipMetadataCheck(); > please add a unit test for this function in > chrome_frame/test/util_unittests.cc. you can do this in a separate > change if you'd like. > > https://codereview.chromium.org/18770009/
chrome_frame lgtm. +pastarmovj for OWNERS approval of policy change.
policy/ lgtm (Julian is currently OOO)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/JosephKnoll@gmail.com/18770009/57001
Failed to apply patch for chrome/app/policy/policy_templates.json:
While running patch -p0 --forward --force --no-backup-if-mismatch;
patching file chrome/app/policy/policy_templates.json
Hunk #1 FAILED at 112.
Hunk #2 succeeded at 2652 (offset 16 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/app/policy/policy_templates.json.rej
Patch: chrome/app/policy/policy_templates.json
Index: chrome/app/policy/policy_templates.json
===================================================================
--- chrome/app/policy/policy_templates.json (revision 214941)
+++ chrome/app/policy/policy_templates.json (working copy)
@@ -112,7 +112,7 @@
# persistent IDs for all fields (but not for groups!) are needed. These are
# specified by the 'id' keys of each policy. NEVER CHANGE EXISTING IDs,
# because doing so would break the deployed wire format!
-# For your editing convenience: highest ID currently used: 233
+# For your editing convenience: highest ID currently used: 234
#
# Placeholders:
# The following placeholder strings are automatically substituted:
@@ -2636,6 +2636,25 @@
If this policy is not set the default command line will be used.''',
},
+ {
+ 'name': 'SkipMetadataCheck',
+ 'type': 'main',
+ 'schema': { 'type': 'boolean' },
+ 'supported_on': ['chrome_frame:30-'],
+ 'features': {
+ 'dynamic_refresh': False,
+ },
+ 'example_value': False,
+ 'id': 234,
+ 'caption': '''Skip the meta tag check in <ph
name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome Frame</ex></ph>''',
+ 'desc': '''Normally pages with X-UA-Compatible set to chrome=1 will
be rendered in <ph name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome Frame</ex></ph>
regardless of the 'ChromeFrameRendererSettings' policy.
+
+ If you enable this setting, pages will not be scanned for meta tags.
+
+ If you disable this setting, pages will be scanned for meta tags.
+
+ If this policy is not set, pages will be scanned for meta tags.'''
+ },
],
},
{
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/JosephKnoll@gmail.com/18770009/57001
Failed to apply patch for chrome/app/policy/policy_templates.json:
While running patch -p0 --forward --force --no-backup-if-mismatch;
patching file chrome/app/policy/policy_templates.json
Hunk #1 FAILED at 112.
Hunk #2 succeeded at 2652 (offset 16 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/app/policy/policy_templates.json.rej
Patch: chrome/app/policy/policy_templates.json
Index: chrome/app/policy/policy_templates.json
===================================================================
--- chrome/app/policy/policy_templates.json (revision 214941)
+++ chrome/app/policy/policy_templates.json (working copy)
@@ -112,7 +112,7 @@
# persistent IDs for all fields (but not for groups!) are needed. These are
# specified by the 'id' keys of each policy. NEVER CHANGE EXISTING IDs,
# because doing so would break the deployed wire format!
-# For your editing convenience: highest ID currently used: 233
+# For your editing convenience: highest ID currently used: 234
#
# Placeholders:
# The following placeholder strings are automatically substituted:
@@ -2636,6 +2636,25 @@
If this policy is not set the default command line will be used.''',
},
+ {
+ 'name': 'SkipMetadataCheck',
+ 'type': 'main',
+ 'schema': { 'type': 'boolean' },
+ 'supported_on': ['chrome_frame:30-'],
+ 'features': {
+ 'dynamic_refresh': False,
+ },
+ 'example_value': False,
+ 'id': 234,
+ 'caption': '''Skip the meta tag check in <ph
name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome Frame</ex></ph>''',
+ 'desc': '''Normally pages with X-UA-Compatible set to chrome=1 will
be rendered in <ph name="PRODUCT_FRAME_NAME">$3<ex>Google Chrome Frame</ex></ph>
regardless of the 'ChromeFrameRendererSettings' policy.
+
+ If you enable this setting, pages will not be scanned for meta tags.
+
+ If you disable this setting, pages will be scanned for meta tags.
+
+ If this policy is not set, pages will be scanned for meta tags.'''
+ },
],
},
{
Updated policy file
Re-basing policy file
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/JosephKnoll@gmail.com/18770009/80001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Update Policy and Authors file.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/JosephKnoll@gmail.com/18770009/91001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
I'm not familiar with the acceptance process here... did the code make it in? Is there something else I need to do? Thanks. --Joe On Sat, Aug 10, 2013 at 11:32 AM, <commit-bot@chromium.org> wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/**tryserver.chromium/** > buildstatus?builder=chromium_**presubmit&number=20011<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=20011> > > https://chromiumcodereview.**appspot.com/18770009/<https://chromiumcodereview... >
Hi Joe. You're hitting two PRESUBMIT errors. One you can correct in the CL. For the other (relating to the AUTHORS file), have you already signed the CLA (see http://www.chromium.org/developers/contributing-code/external-contributor-che... https://codereview.chromium.org/18770009/diff/91001/chrome_frame/policy_setti... File chrome_frame/policy_settings.cc (right): https://codereview.chromium.org/18770009/diff/91001/chrome_frame/policy_setti... chrome_frame/policy_settings.cc:116: std::wstring settings_value( you're getting PRESUBMIT grief here for using std::wstring. change this to string16 to resolve.
Another comment: the CL description says that this chance affects window opening. Does it? If not, please remove that bit from the description. Thanks.
Rebased
Rebased
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/JosephKnoll@gmail.com/18770009/108001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
On 2013/08/16 02:33:50, I haz the power (commit-bot) wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... Looks like you'll need to re-upload using the same email address found in the AUTHORS file (joe.knoll@workday.com).
Uploading from *@workday.com
On 2013/08/16 23:41:31, JoeK wrote: > Uploading from mailto:*@workday.com Okay... got an error (and cached cookies caused problems). Created a new issue: https://codereview.chromium.org/23295005
I'm closing this issue since it's no longer relevant (see http://crrev.com/218626). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
