Fixing get-help <cmdletName> -online for .NET Core on Desktop. - #2174

I assume this exists on Linux

Actually, the Registry is not available on Linux, so I am updating this part of the code.

in the case that this does not throw, you should add the following:

throw "Execution should not have succeeded"
``` #Closed

since you're now sure that the try will throw, this can become:

catch
{
    $_.FullyQualifiedErrorId | Should be "InvalidOperation,Microsoft.PowerShell.Commands.GetHelpCommand"
}

#Closed

James Truher (@JamesWTruher) thanks for your comments. I like this approach better. I will update the test code as you suggested.
. #Closed

Fixed.

foreach [](start = 17, length = 7)

please use foreach-object and avoid aliases #Closed

Fixed.

$actualURI = $actualURI.Replace("Help URI:","").Trim() [](start = 20, length = 54)

if you think it's needed, the testhook change can simply return the uri and you can eliminate this bit of stuff #Closed

Thanks James Truher (@JamesWTruher). I like the current implementation of the testhook, because it tells you that the fwlink is pointing to a Help URI. #Closed

This error should have more information to enable trouble-shooting

The default exception will be wrapped, and the user will get the following error:
"Starting a browser to display online Help failed. No program or browser is associated to open the URI {0}."

these browserProcess.Start(); line is duplicated

There were three different calls: Unix, coreclr, and fullclr. This has been fixed in the latest iteration.

You can replace this by using (RegistryKey regKey = ...) { ... } syntax.
It's more modern way of doing the same.

I agree. Fixed.

4 is a magic number here.
It's better to re-write it as

const string exeExtention = ".exe";
browserPath = browserPath.Substring(0, browserPath.LastIndexOf(exeExtension) + exeExtension.Length);

Same goes about the "34".
It' good that there is a comment above, but if it's a comment, it should be very close to the number, i.e.

.Repalce(/* " */ (char)34 + ...)

Good suggestions sergei (@vors) . Fixed.

That all looks hacky. Isn't there a nicer API?

why not Replace("\"", "")?

Fixed.

If you are able to get the default browser executable path, then maybe it's better to run that executable directly with the url, so that we can avoid this obscure cmd syntax.

If you decide to go with using the browser executable path directly, maybe you can do this for both CoreCLR win platform and FullCLR win platform. But it's up to you to decide if you want it this way :)

Actually, this is a very good idea. We should keep parity between FullCLR and CoreCRL. So, I've updated the code to launch the default browser with the HelpURI as an argument.

Can you please fix the indentation here?

Fixed.

there are a few problems with this method:

1. WebBrowserAvailable should be renamed to IsWebBrowserAvailable. If you want to use the default browser executable path, then it should be renamed to GetDefaultWebBrowser
2. regKey could be null, on Nano or IoT. So your current code might raise NullReferenceException
3. The registry path HKCR\\HTTP\\shell\\open\\command is not pointing to the default browser. It's actually always pointing to IE. The default web browser executable path can be found with 2 steps:
   (1) HKEY_CURRENT_USER\SOFTWARE\Microsoft\Windows\Shell\Associations\UrlAssociations\http\UserChoice -- get the value for ProgId of this key, say the value is ChromeHTML
   (2) HKEY_CLASSES_ROOT\ChromeHTML\shell\open\command -- get the value of the default name. The value would be the path to chrome.exe
   If we want to run the web browser executable directly, then we need to make sure we really get the default browser.

Thank you for your comments Dongbo Wang (@daxian-dbw) . I've updated the code based on these guidance.

why not Replace("\"", "")?

The current implementation doesn't get the path to default browser.

Yes, this is fixed now.

Can you update the .csv to put 'https' there? It's probably more efficient.

Will do, even though the actual values in the Microsoft fwlink database is http.

-not ([System.Management.Automation.Platform]::IsIoT -or [System.Management.Automation.Platform]::IsNanoServer

This will skip NanoServer

Actually, this will only skip on when the current system is Nano Server or IoT. The concept of -skip can be a bit confusing. For coreclr on desktop, the evaluation is as follows
$shouldSkip = -not ([System.Management.Automation.Platform]::IsIoT -or [System.Management.Automation.Platform]::IsNanoServer)
$shouldSkip = $true # this means it should be skip on desktop.

This is the output from running the tests on CoreCLR on Desktop.
Describing Get-Help -Online is not supported on Nano Server and IoT
[!] Get-help -online throws InvalidOperation 27ms

is -ErrorAction stop necessary here? The code path should already throw and InvalidOperationException.

And, when the test hook property is set to true, I don't think the code path will ever reach the point to throw exception.

Good point. The code has been updated. Thanks.

why are you skipping IoT, NanoServer and Linux? when BypassOnlineHelpRetrieval is set, the code won't reach the Process.Start part anyway ...

Good point. I am enabling the test across all platforms.

The tests don't test the process launch at all. Shall we at least have 1 test exercise the new code?

Dongbo Wang (@daxian-dbw) I've added a tests, but it will leave the default browser opened. The issue here is that once the process is started, I no longer have access to its reference.

we don't use ProcessStartInfo.UseShellExecute directly near this code. Not very clear from the comment what's the need of UseShellExecute.

sergei (@vors) I have updated the comment. On Full CLR, ProcessStartInfo.UseShellExecute is true by default.

How about:

object progId = Registry.GetValue(httpRegkey, "ProgId", null);
if (progId != null)

Thank you for your suggestion. Fixed.

there are 2 issues in line 687 and 788:

1. Possible NullReferenceException at browserRegKey.GetValue(null).ToString() when GetValue(null) returns null.
2. If line 687 runs fine, then browserPath must not be null, because Replace(...) never returns null.

This is a chance to use the new C# 6.0 feature null-conditional operator '?.' to make the code concise, like:

using (RegistryKey browserRegKey = Registry.ClassesRoot.OpenSubKey(progId + @"\shell\open\command", false))
{
    const string exeExtension = ".exe";
    string browserPath = browserRegKey?.GetValue(null)?.ToString().Replace(/* remove the quotes */ "\"", "");
    if (!string.IsNullOrEmpty(browserPath))
    { ...

This could reduce the brackets needed for this method.

Fixed.

I know it would be super rare, but what if LastIndexOf(exeExtension) returns -1? Maybe change it to something like:

int extIndex = browserPath.LastIndexOf(exeExtension);
browserPath = extIndex > 0 ? browserPath.Substring(0, extIndex + exeExtension.Length) : string.Emtpy;

Nitpick: for LastIndexOf, it's better to always use the overload that takes a culture parameter int LastIndexOf(string value, System.StringComparison comparisonType)

Thanks for the suggestion. Fixed.

Maybe we should have a message for this InvalidOperationException

No need to have an error message. We can use the existing HelpErrors.CannotLaunchURI.

This skipTest setting will make this test run on OSX.

Why are we skipping linux?
And with $skipTest, the test will run on OSX. Are you intend to run this test on OSX platform?