Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10832071:
[Chromoting] Let the Windows host policy watcher read string-valued policies. (Closed)
|
Created:
8 years, 4 months ago by simonmorris Modified:
8 years, 4 months ago Reviewers:
alexeypa (please no reviews) CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Chromoting] Let the Windows host policy watcher read string-valued policies.
BUG=132684
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=149184
Patch Set 1 #
Total comments: 6
Patch Set 2 : Improve use of wide strings. #
Total comments: 2
Patch Set 3 : Use wstring instead of string16. #Messages
Total messages: 11 (0 generated)
ptal
http://codereview.chromium.org/10832071/diff/1/remoting/host/policy_hack/poli... File remoting/host/policy_hack/policy_watcher_win.cc (right): http://codereview.chromium.org/10832071/diff/1/remoting/host/policy_hack/poli... remoting/host/policy_hack/policy_watcher_win.cc:115: bool GetRegistryPolicyString(const string16& value_name, Mixture of string16 and std::string looks strange. Especially because ASCIIToUTF16 is used below to get UTF-16 encoded name. http://codereview.chromium.org/10832071/diff/1/remoting/host/policy_hack/poli... remoting/host/policy_hack/policy_watcher_win.cc:119: if (policy_key.ReadValue(value_name.c_str(), &value) == ERROR_SUCCESS) { It should be UTF16ToWide(value_name).c_str(). I think it is better to stick to UTF8 in this case. http://codereview.chromium.org/10832071/diff/1/remoting/host/policy_hack/poli... remoting/host/policy_hack/policy_watcher_win.cc:177: if (GetRegistryPolicyString(name, &string_value)) { nit: Why don't pass ASCIIToUTF16(policy_name) directly to the call?
I've made parallel changes to existing code. http://codereview.chromium.org/10832071/diff/1/remoting/host/policy_hack/poli... File remoting/host/policy_hack/policy_watcher_win.cc (right): http://codereview.chromium.org/10832071/diff/1/remoting/host/policy_hack/poli... remoting/host/policy_hack/policy_watcher_win.cc:115: bool GetRegistryPolicyString(const string16& value_name, On 2012/07/30 21:27:06, alexeypa wrote: > Mixture of string16 and std::string looks strange. Especially because > ASCIIToUTF16 is used below to get UTF-16 encoded name. Done. http://codereview.chromium.org/10832071/diff/1/remoting/host/policy_hack/poli... remoting/host/policy_hack/policy_watcher_win.cc:119: if (policy_key.ReadValue(value_name.c_str(), &value) == ERROR_SUCCESS) { On 2012/07/30 21:27:06, alexeypa wrote: > It should be UTF16ToWide(value_name).c_str(). I think it is better to stick to > UTF8 in this case. Done. http://codereview.chromium.org/10832071/diff/1/remoting/host/policy_hack/poli... remoting/host/policy_hack/policy_watcher_win.cc:177: if (GetRegistryPolicyString(name, &string_value)) { On 2012/07/30 21:27:06, alexeypa wrote: > nit: Why don't pass ASCIIToUTF16(policy_name) directly to the call? Done.
LGTM when my comment is addressed. PS. Strings are mess aren't they? :) http://codereview.chromium.org/10832071/diff/3001/remoting/host/policy_hack/p... File remoting/host/policy_hack/policy_watcher_win.cc (right): http://codereview.chromium.org/10832071/diff/3001/remoting/host/policy_hack/p... remoting/host/policy_hack/policy_watcher_win.cc:117: string16 value_name_wide = UTF8ToWide(value_name); string16 -> std::wstring because you are converting it to a "wide" string.
fyi http://codereview.chromium.org/10832071/diff/3001/remoting/host/policy_hack/p... File remoting/host/policy_hack/policy_watcher_win.cc (right): http://codereview.chromium.org/10832071/diff/3001/remoting/host/policy_hack/p... remoting/host/policy_hack/policy_watcher_win.cc:117: string16 value_name_wide = UTF8ToWide(value_name); On 2012/07/30 22:15:45, alexeypa wrote: > string16 -> std::wstring because you are converting it to a "wide" string. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10832071/3002
Presubmit check for 10832071-3002 failed and returned exit status 1.
Running presubmit commit checks ...
Traceback (most recent call last):
File "/b/commit-queue/verification/presubmit_shim.py", line 43, in <module>
sys.exit(presubmit_support.Main(argv))
File "/b/depot_tools/presubmit_support.py", line 1257, in Main
rietveld_obj)
File "/b/depot_tools/presubmit_support.py", line 1105, in DoPresubmitChecks
results += executer.ExecPresubmitScript(presubmit_script, filename)
File "/b/depot_tools/presubmit_support.py", line 1022, in ExecPresubmitScript
result = eval(function_name + '(*__args)', context)
File "<string>", line 1, in <module>
File "<string>", line 476, in CheckChangeOnCommit
File "<string>", line 381, in _CommonChecks
File "/b/depot_tools/presubmit_canned_checks.py", line 920, in
PanProjectChecks
input_api, output_api, source_file_filter=None))
File "/b/depot_tools/presubmit_canned_checks.py", line 754, in CheckOwners
approval_needed=input_api.is_committing)
File "/b/depot_tools/presubmit_canned_checks.py", line 792, in
_RietveldOwnerAndReviewers
int(input_api.change.issue), True)
File "/b/depot_tools/rietveld.py", line 88, in get_issue_properties
return json.loads(self.get(url))
File "/b/depot_tools/rietveld.py", line 305, in get
return self._send(request_path, **kwargs)
File "/b/depot_tools/rietveld.py", line 330, in _send
result = self.rpc_server.Send(request_path, **kwargs)
File "/b/depot_tools/third_party/upload.py", line 409, in Send
ErrorExit(e.read())
File "/b/depot_tools/rietveld.py", line 322, in trap_http_500
raise urllib2.HTTPError(request_path, m.group(1), msg, None, None)
urllib2.HTTPError: HTTP Error 500: <h1>500 Server Error</h1>
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10832071/3002
Presubmit check for 10832071-3002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Warnings **
New code should not use wstrings. If you are calling an API that accepts a
wstring, fix the API.
remoting/host/policy_hack/policy_watcher_win.cc:117
remoting/host/policy_hack/policy_watcher_win.cc:118
remoting/host/policy_hack/policy_watcher_win.cc:139
On 2012/07/30 23:34:53, I haz the power (commit-bot) wrote: > New code should not use wstrings. You'll have to dcommit this CL, unfortunately.
On 2012/07/30 23:44:29, alexeypa wrote: > On 2012/07/30 23:34:53, I haz the power (commit-bot) wrote: > > New code should not use wstrings. > > You'll have to dcommit this CL, unfortunately. Yes - I'll do it tomorrow morning, after the try bot has given it a green. |
