[PATCH 1/6] shell32: Implement process-wide explicit AppUserModelID

[GOUJON Alexandre]

I'll let others comment on implementation part.
Here are my remarks on the test part.

On 07/14/2015 05:07 PM, Jonas Kümmerlin wrote:
> +HRESULT (WINAPI *pSetCurrentProcessExplicitAppUserModelID)(const WCHAR *id);
> +HRESULT (WINAPI *pGetCurrentProcessExplicitAppUserModelID)(WCHAR **id);
you can make them static as in other test files

> +#define RESOLVE(hDll, proc) p##proc = (void*)GetProcAddress(hDll, #proc)
> +
> +static void test_process_aum_id(void)
> +{
> <snip>
> +
> +    hShell32 = GetModuleHandleA("shell32.dll");
> +
> +    RESOLVE(hShell32, SetCurrentProcessExplicitAppUserModelID);
> +    RESOLVE(hShell32, GetCurrentProcessExplicitAppUserModelID);
> +
> +    if (!pGetCurrentProcessExplicitAppUserModelID
> +        || !pSetCurrentProcessExplicitAppUserModelID)
> +    {
> +        win_skip("SetCurrentProcessExplicitAppUserModelID is not available");
> +        return;
> +    }
You can move that initialization part to an init method (see 
tests/shellole.c), defining RESOLVE there and undef'ining it at the end 
of the init method
or doind that in START_TEST block (like tests/appbar.c).

Do you plan on adding other test_* functions ?

> +    /* MSDN claims the maximum length to be 128 chars, but in reality,
> +       it is 127 chars + terminating NUL byte                          */
prove/test it

> +    WCHAR test_id[] = {
> +        'W','i','n','e','.','T','e','s','t','.','A','a','a','a','a','a',
> +        'a','a','a','a','a','a','a','a','a','a','a','a','a','a','a','a',
> +        'a','a','a','a','a','a','a','a','a','a','a','a','a','a','a','a',
> +        'a','a','a','a','a','a','a','a','a','a','a','a','a','a','a','a',
> +        'a','a','a','a','a','a','a','a','a','a','a','a','a','a','a','a',
> +        'a','a','a','a','a','a','a','a','a','a','a','a','a','a','a','a',
> +        'a','a','a','a','a','a','a','a','a','a','a','a','a','a','a','a',
> +        'a','a','a','a','a','a','a','a','a','a','a','a','a','a','a', 0 };
If format is not important, a for loop can make it (so that you can test 
different lengths)

> +    /* Receiving it without setting will fail with an unspecified error code */
> +    hr = pGetCurrentProcessExplicitAppUserModelID(&received);
> +    ok(FAILED(hr), "receiving the AppUserModelID succeeded where it shouldn't\n");
> +    ok(received == NULL, "AppUserModelID '%s' has been returned even though none was set\n",
> +                         wine_dbgstr_w(received));
What do you mean by unspecified?
Do you refer to MSDN?
Is error code constant but not in the SDK headers?

> +    if (received)
> +    {
> <snip>
> +        ok(length == test_id_length, "Expected id with length 127, got %d\n", length);
Text is not what you're testing

Does Get* crash with NULL pointer? Same for Set*.
What does happen when setting twice ? Error ? Get* retrives the latest ?