Skip to content

Commit

Permalink
Code review notes
Browse files Browse the repository at this point in the history
  • Loading branch information
@zadjii-msft
zadjii-msft committed Aug 19, 2022
1 parent 5849279 commit 38de95e
Show file tree
Hide file tree
Showing 9 changed files with 20 additions and 17 deletions.
3 changes: 2 additions & 1 deletion src/cascadia/CascadiaPackage/Package-Dev.appxmanifest
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@
<com:ComInterface>
<com:ProxyStub Id="DEC4804D-56D1-4F73-9FBE-6828E7C85C56" DisplayName="OpenConsoleHandoffProxy" Path="OpenConsoleProxy.dll"/>
<com:Interface Id="E686C757-9A35-4A1C-B3CE-0BCC8B5C69F4" ProxyStubClsid="DEC4804D-56D1-4F73-9FBE-6828E7C85C56"/>
<com:Interface Id="AA6B364F-4A50-4176-9002-0AE755E7B5EF" ProxyStubClsid="DEC4804D-56D1-4F73-9FBE-6828E7C85C56"/>
<com:Interface Id="59D55CCE-FC8A-48B4-ACE8-0A9286C6557F" ProxyStubClsid="DEC4804D-56D1-4F73-9FBE-6828E7C85C56"/> <!-- ITerminalHandoff -->
<com:Interface Id="AA6B364F-4A50-4176-9002-0AE755E7B5EF" ProxyStubClsid="DEC4804D-56D1-4F73-9FBE-6828E7C85C56"/> <!-- ITerminalHandoff2 -->
<com:Interface Id="746E6BC0-AB05-4E38-AB14-71E86763141F" ProxyStubClsid="DEC4804D-56D1-4F73-9FBE-6828E7C85C56"/>
</com:ComInterface>
</com:Extension>
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/CascadiaPackage/Package-Pre.appxmanifest
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,8 @@
<com:ComInterface>
<com:ProxyStub Id="1833E661-CC81-4DD0-87C6-C2F74BD39EFA" DisplayName="OpenConsoleHandoffProxy" Path="OpenConsoleProxy.dll"/>
<com:Interface Id="E686C757-9A35-4A1C-B3CE-0BCC8B5C69F4" ProxyStubClsid="1833E661-CC81-4DD0-87C6-C2F74BD39EFA"/>
<com:Interface Id="AA6B364F-4A50-4176-9002-0AE755E7B5EF" ProxyStubClsid="1833E661-CC81-4DD0-87C6-C2F74BD39EFA"/>
<com:Interface Id="59D55CCE-FC8A-48B4-ACE8-0A9286C6557F" ProxyStubClsid="1833E661-CC81-4DD0-87C6-C2F74BD39EFA"/> <!-- ITerminalHandoff -->
<com:Interface Id="AA6B364F-4A50-4176-9002-0AE755E7B5EF" ProxyStubClsid="1833E661-CC81-4DD0-87C6-C2F74BD39EFA"/> <!-- ITerminalHandoff2 -->
<com:Interface Id="746E6BC0-AB05-4E38-AB14-71E86763141F" ProxyStubClsid="1833E661-CC81-4DD0-87C6-C2F74BD39EFA"/>
</com:ComInterface>
</com:Extension>
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/CascadiaPackage/Package.appxmanifest
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,8 @@
<com:ComInterface>
<com:ProxyStub Id="3171DE52-6EFA-4AEF-8A9F-D02BD67E7A4F" DisplayName="OpenConsoleHandoffProxy" Path="OpenConsoleProxy.dll"/>
<com:Interface Id="E686C757-9A35-4A1C-B3CE-0BCC8B5C69F4" ProxyStubClsid="3171DE52-6EFA-4AEF-8A9F-D02BD67E7A4F"/>
<com:Interface Id="AA6B364F-4A50-4176-9002-0AE755E7B5EF" ProxyStubClsid="3171DE52-6EFA-4AEF-8A9F-D02BD67E7A4F"/>
<com:Interface Id="59D55CCE-FC8A-48B4-ACE8-0A9286C6557F" ProxyStubClsid="1833E661-CC81-4DD0-87C6-C2F74BD39EFA"/> <!-- ITerminalHandoff -->
<com:Interface Id="AA6B364F-4A50-4176-9002-0AE755E7B5EF" ProxyStubClsid="1833E661-CC81-4DD0-87C6-C2F74BD39EFA"/> <!-- ITerminalHandoff2 -->
<com:Interface Id="746E6BC0-AB05-4E38-AB14-71E86763141F" ProxyStubClsid="3171DE52-6EFA-4AEF-8A9F-D02BD67E7A4F"/>
</com:ComInterface>
</com:Extension>
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/TerminalPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3270,7 +3270,7 @@ namespace winrt::TerminalApp::implementation
{
NewTerminalArgs newTerminalArgs;
newTerminalArgs.Commandline(connection.Commandline());
newTerminalArgs.TabTitle(connection.Title());
newTerminalArgs.TabTitle(connection.StartingTitle());
// GH #12370: We absolutely cannot allow a defterm connection to
// auto-elevate. Defterm doesn't work for elevated scenarios in the
// first place. If we try accepting the connection, the spawning an
Expand Down
8 changes: 5 additions & 3 deletions src/cascadia/TerminalConnection/ConptyConnection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,10 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
THROW_IF_FAILED(ConptyPackPseudoConsole(hServerProcess, hRef, hSig, &_hPC));
_piClient.hProcess = hClientProcess;

_startupInfo.title = winrt::hstring{ startupInfo.pszTitle };
_startupInfo.iconPath = winrt::hstring{ startupInfo.pszIconPath };
_startupInfo.title = winrt::hstring{ startupInfo.pszTitle, SysStringLen(startupInfo.pszTitle) };
SysFreeString(startupInfo.pszTitle);

This comment has been minimized.

Sign in to view

Copy link
@DHowett

DHowett Aug 19, 2022

Member

Is this memory officially owned by us? Or is it owned by RPC?

This comment has been minimized.

Sign in to view

Copy link
@zadjii-msft

zadjii-msft Aug 19, 2022

Author Member

https://docs.microsoft.com/en-us/cpp/atl-mfc-shared/allocating-and-releasing-memory-for-a-bstr?view=msvc-170

When you call into a function that returns a BSTR, you must free the string yourself. For example:

so that's what I thought I should do

This comment has been minimized.

Sign in to view

Copy link
@zadjii-msft

zadjii-msft Aug 19, 2022

Author Member

I mean, freeing it twice (here and in srvinit) didn't explode?

This comment has been minimized.

Sign in to view

Copy link
@DHowett

DHowett Aug 19, 2022

Member

Hmm. Try running it with App Verifier stuff turned on for COM and RPC and Memory. I just want to be sure that this isn't gonna become a source of Watsons.
_startupInfo.iconPath = winrt::hstring{ startupInfo.pszIconPath, SysStringLen(startupInfo.pszIconPath) };
SysFreeString(startupInfo.pszIconPath);
_startupInfo.iconIndex = startupInfo.iconIndex;

try
Expand Down Expand Up @@ -293,7 +295,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
return _commandline;
}

winrt::hstring ConptyConnection::Title() const
winrt::hstring ConptyConnection::StartingTitle() const
{
return _startupInfo.title;
}
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalConnection/ConptyConnection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation

winrt::guid Guid() const noexcept;
winrt::hstring Commandline() const;
winrt::hstring Title() const;
winrt::hstring StartingTitle() const;

static void StartInboundListener();
static void StopInboundListener();
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalConnection/ConptyConnection.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace Microsoft.Terminal.TerminalConnection
ConptyConnection();
Guid Guid { get; };
String Commandline { get; };
String Title { get; };
String StartingTitle { get; };

void ClearBuffer();

Expand Down
8 changes: 2 additions & 6 deletions src/host/proxy/ITerminalHandoff.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import "ocidl.idl";
typedef struct _TERMINAL_STARTUP_INFO
{
// In STARTUPINFO
LPCWSTR pszTitle;
BSTR pszTitle;

// Also wanted
LPCWSTR pszIconPath;
BSTR pszIconPath;
LONG iconIndex;

// The rest of STARTUPINFO
Expand All @@ -24,10 +24,6 @@ typedef struct _TERMINAL_STARTUP_INFO
DWORD dwFillAttribute;
DWORD dwFlags;
WORD wShowWindow;

// Something else we may be interested in?
LPCWSTR fontFace;
LONG fontSize;
} TERMINAL_STARTUP_INFO;

// LOAD BEARING!
Expand Down
6 changes: 4 additions & 2 deletions src/host/srvinit.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -576,9 +576,11 @@ try
// END code from SetUpConsole

// Take what we've collected, and bundle it up for handoff.
auto title = wil::make_bstr(Cac.Title);
auto iconPath = wil::make_bstr(icon.path.data());
TERMINAL_STARTUP_INFO myStartupInfo{
Cac.Title,
icon.path.c_str(),
title.get(),
iconPath.get(),
icon.index
};

Expand Down

0 comments on commit 38de95e

Please sign in to comment.