0

win: Remove a few instances of the "goto error" pattern.

This makes the code simpler, and also fixes a bunch of warnings that look like:

    goto done;
    ^
..\..\remoting\host\win\rdp_client_window.cc(331,14) :
      note(clang): jump bypasses variable initialization
  const LONG kDesiredFlags = WTS_PERF_ENABLE_ENHANCED_GRAPHICS |
             ^
No intended behavior change.

BUG=505296

Review URL: https://codereview.chromium.org/1226243004

Cr-Commit-Position: refs/heads/master@{#338720}
This commit is contained in:
thakis
2015-07-14 11:44:10 -07:00
committed by Commit bot
parent c95972cc75
commit e03b42a708
3 changed files with 72 additions and 87 deletions

@@ -6,6 +6,8 @@
#include "base/bind.h" #include "base/bind.h"
#include "base/message_loop/message_loop.h" #include "base/message_loop/message_loop.h"
#include "base/win/scoped_handle.h"
#include "base/win/scoped_hdc.h"
#include "printing/backend/printing_info_win.h" #include "printing/backend/printing_info_win.h"
#include "printing/backend/win_helper.h" #include "printing/backend/win_helper.h"
#include "printing/print_settings.h" #include "printing/print_settings.h"
@@ -34,6 +36,18 @@ class PrintingContextTest : public PrintingTest<testing::Test>,
PrintingContext::Result result_; PrintingContext::Result result_;
}; };
namespace {
struct FreeHandleTraits {
typedef HANDLE Handle;
static void CloseHandle(HANDLE handle) { GlobalFree(handle); }
static bool IsHandleValid(HANDLE handle) { return handle != NULL; }
static HANDLE NullHandle() { return NULL; }
};
typedef base::win::GenericScopedHandle<FreeHandleTraits,
base::win::DummyVerifierTraits>
ScopedGlobalAlloc;
}
class MockPrintingContextWin : public PrintingContextSytemDialogWin { class MockPrintingContextWin : public PrintingContextSytemDialogWin {
public: public:
MockPrintingContextWin(Delegate* delegate) MockPrintingContextWin(Delegate* delegate)
@@ -58,41 +72,31 @@ class MockPrintingContextWin : public PrintingContextSytemDialogWin {
if (!printer.OpenPrinter(printer_name.c_str())) if (!printer.OpenPrinter(printer_name.c_str()))
return E_FAIL; return E_FAIL;
scoped_ptr<uint8[]> buffer;
const DEVMODE* dev_mode = NULL; const DEVMODE* dev_mode = NULL;
HRESULT result = S_OK;
lppd->hDC = NULL; lppd->hDC = NULL;
lppd->hDevMode = NULL; lppd->hDevMode = NULL;
lppd->hDevNames = NULL; lppd->hDevNames = NULL;
PrinterInfo2 info_2; PrinterInfo2 info_2;
if (info_2.Init(printer.Get())) { if (info_2.Init(printer.Get()))
dev_mode = info_2.get()->pDevMode; dev_mode = info_2.get()->pDevMode;
} if (!dev_mode)
if (!dev_mode) { return E_FAIL;
result = E_FAIL;
goto Cleanup;
}
lppd->hDC = CreateDC(L"WINSPOOL", printer_name.c_str(), NULL, dev_mode); base::win::ScopedCreateDC hdc(
if (!lppd->hDC) { CreateDC(L"WINSPOOL", printer_name.c_str(), NULL, dev_mode));
result = E_FAIL; if (!hdc.Get())
goto Cleanup; return E_FAIL;
}
size_t dev_mode_size = dev_mode->dmSize + dev_mode->dmDriverExtra; size_t dev_mode_size = dev_mode->dmSize + dev_mode->dmDriverExtra;
lppd->hDevMode = GlobalAlloc(GMEM_MOVEABLE, dev_mode_size); ScopedGlobalAlloc dev_mode_mem(GlobalAlloc(GMEM_MOVEABLE, dev_mode_size));
if (!lppd->hDevMode) { if (!dev_mode_mem.Get())
result = E_FAIL; return E_FAIL;
goto Cleanup; void* dev_mode_ptr = GlobalLock(dev_mode_mem.Get());
} if (!dev_mode_ptr)
void* dev_mode_ptr = GlobalLock(lppd->hDevMode); return E_FAIL;
if (!dev_mode_ptr) {
result = E_FAIL;
goto Cleanup;
}
memcpy(dev_mode_ptr, dev_mode, dev_mode_size); memcpy(dev_mode_ptr, dev_mode, dev_mode_size);
GlobalUnlock(lppd->hDevMode); GlobalUnlock(dev_mode_mem.Get());
dev_mode_ptr = NULL; dev_mode_ptr = NULL;
size_t driver_size = size_t driver_size =
@@ -102,16 +106,12 @@ class MockPrintingContextWin : public PrintingContextSytemDialogWin {
size_t port_size = 2 + sizeof(wchar_t) * lstrlen(info_2.get()->pPortName); size_t port_size = 2 + sizeof(wchar_t) * lstrlen(info_2.get()->pPortName);
size_t dev_names_size = size_t dev_names_size =
sizeof(DEVNAMES) + driver_size + printer_size + port_size; sizeof(DEVNAMES) + driver_size + printer_size + port_size;
lppd->hDevNames = GlobalAlloc(GHND, dev_names_size); ScopedGlobalAlloc dev_names_mem(GlobalAlloc(GHND, dev_names_size));
if (!lppd->hDevNames) { if (!dev_names_mem.Get())
result = E_FAIL; return E_FAIL;
goto Cleanup; void* dev_names_ptr = GlobalLock(dev_names_mem.Get());
} if (!dev_names_ptr)
void* dev_names_ptr = GlobalLock(lppd->hDevNames); return E_FAIL;
if (!dev_names_ptr) {
result = E_FAIL;
goto Cleanup;
}
DEVNAMES* dev_names = reinterpret_cast<DEVNAMES*>(dev_names_ptr); DEVNAMES* dev_names = reinterpret_cast<DEVNAMES*>(dev_names_ptr);
dev_names->wDefault = 1; dev_names->wDefault = 1;
dev_names->wDriverOffset = sizeof(DEVNAMES) / sizeof(wchar_t); dev_names->wDriverOffset = sizeof(DEVNAMES) / sizeof(wchar_t);
@@ -128,27 +128,13 @@ class MockPrintingContextWin : public PrintingContextSytemDialogWin {
memcpy(reinterpret_cast<uint8*>(dev_names_ptr) + dev_names->wOutputOffset, memcpy(reinterpret_cast<uint8*>(dev_names_ptr) + dev_names->wOutputOffset,
info_2.get()->pPortName, info_2.get()->pPortName,
port_size); port_size);
GlobalUnlock(lppd->hDevNames); GlobalUnlock(dev_names_mem.Get());
dev_names_ptr = NULL; dev_names_ptr = NULL;
Cleanup: lppd->hDC = hdc.Take();
// Note: This section does proper deallocation/free of DC/global handles. We lppd->hDevMode = dev_mode_mem.Take();
// did not use ScopedHGlobal or ScopedHandle because they did not lppd->hDevNames = dev_names_mem.Take();
// perform what we need. Goto's are used based on Windows programming return S_OK;
// idiom, to avoid deeply nested if's, and try-catch-finally is not
// allowed in Chromium.
if (FAILED(result)) {
if (lppd->hDC) {
DeleteDC(lppd->hDC);
}
if (lppd->hDevMode) {
GlobalFree(lppd->hDevMode);
}
if (lppd->hDevNames) {
GlobalFree(lppd->hDevNames);
}
}
return result;
} }
}; };

@@ -247,10 +247,8 @@ LRESULT RdpClientWindow::OnCreate(CREATESTRUCT* create_struct) {
RECT rect = { 0, 0, screen_size_.width(), screen_size_.height() }; RECT rect = { 0, 0, screen_size_.width(), screen_size_.height() };
activex_window.Create(m_hWnd, rect, nullptr, activex_window.Create(m_hWnd, rect, nullptr,
WS_CHILD | WS_VISIBLE | WS_BORDER); WS_CHILD | WS_VISIBLE | WS_BORDER);
if (activex_window.m_hWnd == nullptr) { if (activex_window.m_hWnd == nullptr)
result = HRESULT_FROM_WIN32(GetLastError()); return LogOnCreateError(HRESULT_FROM_WIN32(GetLastError()));
goto done;
}
// Instantiate the RDP ActiveX control. // Instantiate the RDP ActiveX control.
result = activex_window.CreateControlEx( result = activex_window.CreateControlEx(
@@ -261,71 +259,71 @@ LRESULT RdpClientWindow::OnCreate(CREATESTRUCT* create_struct) {
__uuidof(mstsc::IMsTscAxEvents), __uuidof(mstsc::IMsTscAxEvents),
reinterpret_cast<IUnknown*>(static_cast<RdpEventsSink*>(this))); reinterpret_cast<IUnknown*>(static_cast<RdpEventsSink*>(this)));
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
result = control.QueryInterface(client_.Receive()); result = control.QueryInterface(client_.Receive());
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Use 32-bit color. // Use 32-bit color.
result = client_->put_ColorDepth(32); result = client_->put_ColorDepth(32);
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Set dimensions of the remote desktop. // Set dimensions of the remote desktop.
result = client_->put_DesktopWidth(screen_size_.width()); result = client_->put_DesktopWidth(screen_size_.width());
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
result = client_->put_DesktopHeight(screen_size_.height()); result = client_->put_DesktopHeight(screen_size_.height());
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Set the server name to connect to. // Set the server name to connect to.
result = client_->put_Server(server_name); result = client_->put_Server(server_name);
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Fetch IMsRdpClientAdvancedSettings interface for the client. // Fetch IMsRdpClientAdvancedSettings interface for the client.
result = client_->get_AdvancedSettings2(client_settings_.Receive()); result = client_->get_AdvancedSettings2(client_settings_.Receive());
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Disable background input mode. // Disable background input mode.
result = client_settings_->put_allowBackgroundInput(0); result = client_settings_->put_allowBackgroundInput(0);
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Do not use bitmap cache. // Do not use bitmap cache.
result = client_settings_->put_BitmapPersistence(0); result = client_settings_->put_BitmapPersistence(0);
if (SUCCEEDED(result)) if (SUCCEEDED(result))
result = client_settings_->put_CachePersistenceActive(0); result = client_settings_->put_CachePersistenceActive(0);
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Do not use compression. // Do not use compression.
result = client_settings_->put_Compress(0); result = client_settings_->put_Compress(0);
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Enable the Ctrl+Alt+Del screen. // Enable the Ctrl+Alt+Del screen.
result = client_settings_->put_DisableCtrlAltDel(0); result = client_settings_->put_DisableCtrlAltDel(0);
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Disable printer and clipboard redirection. // Disable printer and clipboard redirection.
result = client_settings_->put_DisableRdpdr(FALSE); result = client_settings_->put_DisableRdpdr(FALSE);
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Do not display the connection bar. // Do not display the connection bar.
result = client_settings_->put_DisplayConnectionBar(VARIANT_FALSE); result = client_settings_->put_DisplayConnectionBar(VARIANT_FALSE);
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Do not grab focus on connect. // Do not grab focus on connect.
result = client_settings_->put_GrabFocusOnConnect(VARIANT_FALSE); result = client_settings_->put_GrabFocusOnConnect(VARIANT_FALSE);
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Enable enhanced graphics, font smoothing and desktop composition. // Enable enhanced graphics, font smoothing and desktop composition.
const LONG kDesiredFlags = WTS_PERF_ENABLE_ENHANCED_GRAPHICS | const LONG kDesiredFlags = WTS_PERF_ENABLE_ENHANCED_GRAPHICS |
@@ -333,12 +331,12 @@ LRESULT RdpClientWindow::OnCreate(CREATESTRUCT* create_struct) {
WTS_PERF_ENABLE_DESKTOP_COMPOSITION; WTS_PERF_ENABLE_DESKTOP_COMPOSITION;
result = client_settings_->put_PerformanceFlags(kDesiredFlags); result = client_settings_->put_PerformanceFlags(kDesiredFlags);
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Set the port to connect to. // Set the port to connect to.
result = client_settings_->put_RDPPort(server_endpoint_.port()); result = client_settings_->put_RDPPort(server_endpoint_.port());
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Disable audio in the session. // Disable audio in the session.
// TODO(alexeypa): re-enable audio redirection when http://crbug.com/242312 is // TODO(alexeypa): re-enable audio redirection when http://crbug.com/242312 is
@@ -347,12 +345,12 @@ LRESULT RdpClientWindow::OnCreate(CREATESTRUCT* create_struct) {
if (SUCCEEDED(result)) { if (SUCCEEDED(result)) {
result = secured_settings2->put_AudioRedirectionMode(kRdpAudioModeNone); result = secured_settings2->put_AudioRedirectionMode(kRdpAudioModeNone);
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
} }
result = client_->get_SecuredSettings(secured_settings.Receive()); result = client_->get_SecuredSettings(secured_settings.Receive());
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
// Set the terminal ID as the working directory for the initial program. It is // Set the terminal ID as the working directory for the initial program. It is
// observed that |WorkDir| is used only if an initial program is also // observed that |WorkDir| is used only if an initial program is also
@@ -363,21 +361,11 @@ LRESULT RdpClientWindow::OnCreate(CREATESTRUCT* create_struct) {
// This code should be in sync with WtsTerminalMonitor::LookupTerminalId(). // This code should be in sync with WtsTerminalMonitor::LookupTerminalId().
result = secured_settings->put_WorkDir(terminal_id); result = secured_settings->put_WorkDir(terminal_id);
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
result = client_->Connect(); result = client_->Connect();
if (FAILED(result)) if (FAILED(result))
goto done; return LogOnCreateError(result);
done:
if (FAILED(result)) {
LOG(ERROR) << "RDP: failed to initiate a connection to "
<< server_endpoint_.ToString() << ": error="
<< std::hex << result << std::dec;
client_.Release();
client_settings_.Release();
return -1;
}
return 0; return 0;
} }
@@ -462,6 +450,15 @@ HRESULT RdpClientWindow::OnConfirmClose(VARIANT_BOOL* allow_close) {
return S_OK; return S_OK;
} }
int RdpClientWindow::LogOnCreateError(HRESULT error) {
LOG(ERROR) << "RDP: failed to initiate a connection to "
<< server_endpoint_.ToString() << ": error="
<< std::hex << error << std::dec;
client_.Release();
client_settings_.Release();
return -1;
}
void RdpClientWindow::NotifyConnected() { void RdpClientWindow::NotifyConnected() {
if (event_handler_) if (event_handler_)
event_handler_->OnConnected(); event_handler_->OnConnected();

@@ -126,6 +126,8 @@ class RdpClientWindow
STDMETHOD(OnFatalError)(long error_code); STDMETHOD(OnFatalError)(long error_code);
STDMETHOD(OnConfirmClose)(VARIANT_BOOL* allow_close); STDMETHOD(OnConfirmClose)(VARIANT_BOOL* allow_close);
int LogOnCreateError(HRESULT error);
// Wrappers for the event handler's methods that make sure that // Wrappers for the event handler's methods that make sure that
// OnDisconnected() is the last notification delivered and is delevered // OnDisconnected() is the last notification delivered and is delevered
// only once. // only once.