1363169 - Add support for native Windows "Share" feature

Status: RESOLVED FIXED

(Firefox :: Menus, enhancement, P3)

Description

[Justin Dolske [:Dolske]]

Bug 1352697 is implementing a "page actions" menu (e.g. attachment 8853671 [details]). One item in the menu is the ability to share the current page. We'd like to make use of OS-supplied functionality to do this.

Bug 1360054 has the UX spec for this (https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/232626421_Explainer_-_Share).

On Windows, this is basically just bringing up the modal Windows 10 UI as shown. (For other Windows versions, the Share menu will just be hidden.)

I'm not exactly sure what API this actually is. One of these?

https://docs.microsoft.com/en-us/uwp/api/Windows.ApplicationModel.DataTransfer.DataTransferManager#Windows_ApplicationModel_DataTransfer_DataTransferManager_ShowShareUI
https://msdn.microsoft.com/en-us/library/windows/apps/hh465251.aspx

Comment 1

[Justin Dolske [:Dolske]]

Jim, can you help find an owner to implement this?

Comment 2

[Justin Dolske [:Dolske]]

Moving to reserve backlog for now.

Comment 4

[:Gijs (he/him)]

Downgrading the priority here while we focus on 57 (and have to wait for help with the native stuff).

Comment 5

[Dale Harvey (:daleharvey)]

So got the gecko side of this done, was wondering if there was anyone that could point me in the right direction for the windows side of it

I would like to add shareUrl() to https://github.com/mozilla/gecko-dev/blob/master/widget/windows/WindowsUIUtils.cpp, 

NS_IMETHODIMP
WindowsUIUtils::ShareUrl() 
{
// Ignore if mingw32
  ABI::Windows::ApplicationModel::DataTransfer::DataTransferManager::ShowShareUI();
  return NS_OK;
}

Now when I do that, I get : 

 0:46.20 force-cargo-library-build
 0:58.97 [m[m[32m[1m    Finished[m release [optimized] target(s) in 0.54 secs
 1:38.51 WindowsUIUtils.cpp
 1:38.52 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(192): error C2027: use of undefined type 'ABI::Windows::ApplicationModel::DataTransfer::DataTransferManager'
 1:38.52 c:\program files (x86)\windows kits\10\include\10.0.16299.0\winrt\Windows.ApplicationModel.DataTransfer.h(1226): note: see declaration of 'ABI::Windows::ApplicationModel::DataTransfer::DataTransferManager'
 1:38.53 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(192): error C3861: 'ShowShareUI': identifier not found
 1:38.54 mozmake.EXE[4]: *** [c:/Users/daleharvey/src/gecko-dev/config/rules.mk:1026: WindowsUIUtils.obj] Error 2
 1:38.54 mozmake.EXE[3]: *** [c:/Users/daleharvey/src/gecko-dev/config/recurse.mk:73: widget/windows/target] Error 2
 1:38.54 mozmake.EXE[3]: *** Waiting for unfinished jobs....
 1:38.65 mozmake.EXE[2]: *** [c:/Users/daleharvey/src/gecko-dev/config/recurse.mk:33: compile] Error 2
 1:38.65 mozmake.EXE[1]: *** [c:/Users/daleharvey/src/gecko-dev/config/rules.mk:418: default] Error 2
 1:38.65 mozmake.EXE: *** [client.mk:168: build] Error 2
1:38.68 123 compiler warnings present.

Inside c:\program files (x86)\windows kits\10\include\10.0.16299.0\winrt\Windows.ApplicationModel.DataTransfer.h there are 10k lines of various definitions and I can see like 3000 references to datatransfer and 8 references to ShowShareUI, it seem like from the other code in WindowsUIUtils I want to be copying some of the definitions across, but not sure which ones (or quite why)

Gijs you pointed me in the right direction about this before, is this something you know about or know someone who will do? many thanks

Comment 6

[:Gijs (he/him)]

You're not a UWP app so this is a bit more tricky than just calling that method. For one, all the UWP stuff is mirrored into win32 land a bit differently. But even then, apparently you normally need a CoreWindow instance for this particular method, which we don't have because (again) we're not a UWP app.

Fortunately, it seems some MSFT person has written a tutorial for this exact problem and this exact API. See https://blogs.msdn.microsoft.com/oldnewthing/20170315-00/?p=95735 . Hopefully that helps?

Comment 7

[:Gijs (he/him)]

Note that we'll need to check both on the Gecko side and on the frontend side that we're actually on Win10+ (not win8 or win7 or something). For frontend, you can use AppConstant.isPlatformAndVersionAtLeast("win", "6.4"). For the C++ side, you can use IsWin10OrLater() .

Comment 8

[:Gijs (he/him)]

Also (ugh, sorry for the triple post) - note that folks filed a few deps on the macOS bug that seem to imply we may want to pass more than just a URL. I wouldn't be surprised if you would need to ensure you provide that (e.g. window title info) here, too. It's kind of sad that the route we took in the mac bug means we have to have separate interfaces, however... it seems on Windows you would need to pass a window reference to get an hwnd from... so looks like the API needs more/different data on Windows anyway. :-(

Note that to provide actual sharing data on Windows, you can look at this blogpost: https://blogs.msdn.microsoft.com/oldnewthing/20170316-00/?p=95745 .

Comment 9

[Dale Harvey (:daleharvey)]

Hugely helpful thanks, trying to construct the code as per the demo is giving me a compile error, specifically: 

 0:47.12 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(207): error C2039: 'IDataTransferManagerInterop': is not a member of 'ABI::Windows::ApplicationModel::DataTransfer'
 0:47.19 C:\Program Files (x86)\Windows Kits\10\include\10.0.16299.0\winrt\windows.applicationmodel.datatransfer.h(4755): note: see declaration of 'ABI::Windows::ApplicationModel::DataTransfer'
 0:47.30 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(209): error C2787: 'IDataTransferManagerInterop': no GUID has been associated with this object
 0:47.34 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(208): error C2660: 'RoGetActivationFactory': function does not take 2 arguments
 0:47.41 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(210): error C2039: 'ShowShareUIForWindow': is not a member of 'Microsoft::WRL::ComPtr<IDataTransferManagerInterop>'
 0:47.52 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(207): note: see declaration of 'Microsoft::WRL::ComPtr<IDataTransferManagerInterop>'
0:47.64 mozmake.EXE[4]: *** [c:/Users/daleharvey/src/gecko-dev/config/rules.mk:1026: WindowsUIUtils.obj] Error 2

trying to figure out why now, apologies for being fairly lost here finding this win32 stuff pretty tricky to figure out, (the cocoa stuff was also new but was a lot easier), will try figuring out and ping you / someone if I cant make any progress, thanks again

Comment 10

[Dale Harvey (:daleharvey)]

Full gist @ https://gist.github.com/daleharvey/c1e76edaa7abe548fd766391cfbad2f1 (I realise you didnt recommend getting the hwnd that way, was going to fix after). Also probably worth mentioning

$ grep -r "IDataTransferManagerInterop" "/c/Program Files (x86)/Windows Kits/10/include/10.0.16299.0/winrt"

is giving me nothing

Comment 11

[Dale Harvey (:daleharvey)]

Hi Jim, Gijs mentioned I should talk to you about windows related issues, I have all the firefox frontent side done and basically am just trying to call a single windows api function, ShowShareUI(). Gis pointed me to an article explaining how to do that in a win32 environment (https://blogs.msdn.microsoft.com/oldnewthing/20170315-00/?p=95735) however using that code pretty much as is @ https://gist.github.com/daleharvey/c1e76edaa7abe548fd766391cfbad2f1 gives me a compile error @ https://bugzilla.mozilla.org/show_bug.cgi?id=1363169#c9

Any chance you could point me in the right direction here, not super familiar with any of the win32 intricacies

Cheers
Dale

Comment 12

[Dale Harvey (:daleharvey)]

Seems like Jim is busy, Chris found your name touching a few of the WindowsUtils files so wondering if you by any chance could help me here? Thanks

Comment 13

[Dale Harvey (:daleharvey)]

Ok taking a shot at trying to do what UIViewSettings looks like its doing (copying and pasting over definitioned from windows kits)

New patch and new / less errors @ https://gist.github.com/daleharvey/147f1ff56f2a5114708c3c859a4ba470

Currently having problems finding the equivalent of https://github.com/mozilla/gecko-dev/blob/master/widget/windows/WindowsUIUtils.cpp#L62 for IDataTransferManager

daleharvey@DESKTOP-UELI0OF /c/Program Files (x86)/Windows Kits/10/Include
$ grep -r "interface IDataTransferManager;" .
./10.0.14393.0/winrt/Windows.ApplicationModel.datatransfer.h:                interface IDataTransferManager;
./10.0.14393.0/winrt/Windows.ApplicationModel.datatransfer.h:interface IDataTransferManager;
./10.0.14393.0/winrt/Windows.ApplicationModel.datatransfer.idl:            interface IDataTransferManager;

Where those definitions are stubs and dont contain the MIDL_INTERFACE

Gijs since I was basing this off https://github.com/mozilla/gecko-dev/commit/6c7bdf65167060865d9f9a09c0aa3dae0d91680c I was wondering if you could help out here, am I on the right track and if so do you know where I could find the definitions? even googling the guid of the uiwindowssettings one doesnt give me anywhere

Comment 14

[Chris Peterson [:cpeterson]]

(In reply to Dale Harvey (:daleharvey) from comment #12)
> Seems like Jim is busy, Chris found your name touching a few of the
> WindowsUtils files so wondering if you by any chance could help me here?

Sorry, Dale. I don't know anything about the Windows Share feature. <:)

Looks like Gijs is the best contact for this feature.

(In reply to :Gijs (he/him) from comment #4)
> Downgrading the priority here while we focus on 57 (and have to wait for
> help with the native stuff).

I'm bumping this bug's priority from P4 to P3 because we've shipped 57 and the current Bugzilla prioritization regime recommends using P3 or P5 instead of P4.

Comment 15

[:Gijs (he/him)]

The type of definition as used in https://github.com/mozilla/gecko-dev/blob/master/widget/windows/WindowsUIUtils.cpp#L62 shouldn't be necessary anymore (in fact, maybe I should file a good first bug to remove it) because we now require the windows 10 sdk to build, whereas when that code was written we "only" required the win8 sdk.

As far as I can tell the confusion is as follows:

based on:

https://blogs.msdn.microsoft.com/oldnewthing/20170316-00/?p=95745

#include <shlobj.h> // IDataTransferManagerInterop

namespace dt = ABI::Windows::ApplicationModel::DataTransfer;

using Microsoft::WRL::Wrappers::HStringReference;

WRL::ComPtr<IDataTransferManagerInterop> g_dtmInterop;
WRL::ComPtr<DT::IDataTransferManager> g_dtm;

I can only assume that IDataTransferManagerInterop isn't actually in the 'dt' namespace. I also don't see it being in a namespace in either your fallback definition or in the definition in shobjidl.h

IDataTransferManagerStatics and IDataTransferManager look like they *are* in the `dt` namespace. Or something. Does that help make things work?

Comment 16

[Dale Harvey (:daleharvey)]

> because we now require the windows 10 sdk to build,

Knowing that that was what that code was for and that its no longer needed is a huge help 

> I can only assume that IDataTransferManagerInterop isn't actually in the 'dt' namespace.

Without the <dt::IDataTransferManagerInterop> I still get "'IDataTransferManagerInterop': no GUID has been associated with this object" which seems to suggest it still doesnt find it correctly, but knowing that the .h files can be accessed I will keep on digging and see if I can figure out whats up. Seems strange that those examples are different

Cheers

Comment 17

[Dale Harvey (:daleharvey)]

So yeh this is still confusing me, I have reduced the code down to a single definition

    Microsoft::WRL::ComPtr<IDataTransferManagerInterop> dtmInterop;

And with either using #include <ShObjIdl.h> or #include <shlobj.h> I see 

 0:46.56 C:\Program Files (x86)\Windows Kits\10\include\10.0.16299.0\winrt\wrl\client.h(226): error C2027: use of undefined type 'IDataTransferManagerInterop'
 0:46.59 c:\program files (x86)\windows kits\10\include\10.0.16299.0\um\shobjidl_core.h(1654): note: see declaration of 'IDataTransferManagerInterop'
 0:46.64 C:\Program Files (x86)\Windows Kits\10\include\10.0.16299.0\winrt\wrl\client.h(219): note: while compiling class template member function 'unsigned long Microsoft::WRL::ComPtr<IDataTransferManagerInterop>::InternalRelease(void) throw()'
 0:46.67 C:\Program Files (x86)\Windows Kits\10\include\10.0.16299.0\winrt\wrl\client.h(281): note: see reference to function template instantiation 'unsigned long Microsoft::WRL::ComPtr<IDataTransferManagerInterop>::InternalRelease(void) throw()' being compiled
 0:46.70 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(207): note: see reference to class template instantiation 'Microsoft::WRL::ComPtr<IDataTransferManagerInterop>' being compiled
 0:46.72 C:\Program Files (x86)\Windows Kits\10\include\10.0.16299.0\winrt\wrl\client.h(226): error C2227: left of '->Release' must point to class/struct/union/generic type

I have found some other examples that use IDataTransferManagerInterop that use either of the above includes fine, I dont understand how it can find a definition that it cant use? Gijs hopefully this is my last need info here, know whats up with this?

Comment 18

[:Gijs (he/him)]

(In reply to Dale Harvey (:daleharvey) from comment #17)
> So yeh this is still confusing me, I have reduced the code down to a single
> definition
> 
>     Microsoft::WRL::ComPtr<IDataTransferManagerInterop> dtmInterop;
> 
> And with either using #include <ShObjIdl.h> or #include <shlobj.h> I see 
> 
>  0:46.56 C:\Program Files (x86)\Windows
> Kits\10\include\10.0.16299.0\winrt\wrl\client.h(226): error C2027: use of
> undefined type 'IDataTransferManagerInterop'
>  0:46.59 c:\program files (x86)\windows
> kits\10\include\10.0.16299.0\um\shobjidl_core.h(1654): note: see declaration
> of 'IDataTransferManagerInterop'
>  0:46.64 C:\Program Files (x86)\Windows
> Kits\10\include\10.0.16299.0\winrt\wrl\client.h(219): note: while compiling
> class template member function 'unsigned long
> Microsoft::WRL::ComPtr<IDataTransferManagerInterop>::InternalRelease(void)
> throw()'
>  0:46.67 C:\Program Files (x86)\Windows
> Kits\10\include\10.0.16299.0\winrt\wrl\client.h(281): note: see reference to
> function template instantiation 'unsigned long
> Microsoft::WRL::ComPtr<IDataTransferManagerInterop>::InternalRelease(void)
> throw()' being compiled
>  0:46.70
> c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(207):
> note: see reference to class template instantiation
> 'Microsoft::WRL::ComPtr<IDataTransferManagerInterop>' being compiled
>  0:46.72 C:\Program Files (x86)\Windows
> Kits\10\include\10.0.16299.0\winrt\wrl\client.h(226): error C2227: left of
> '->Release' must point to class/struct/union/generic type
> 
> I have found some other examples that use IDataTransferManagerInterop that
> use either of the above includes fine, I dont understand how it can find a
> definition that it cant use? Gijs hopefully this is my last need info here,
> know whats up with this?

I don't know, unfortunately... are you sure you have the right headers for the ComPtr stuff? Does including the headers in https://blogs.msdn.microsoft.com/oldnewthing/20030723-00/?p=43073 help? If neither of those help, :jimm is my goto person for the mysteries of windows API header/linkage woes...

Comment 19

[Dale Harvey (:daleharvey)]

Yeh I tried using all includes from those 3 examples, also included anything I can see from examples @ https://github.com/search?l=C%2B%2B&q=IDataTransferManagerInterop&type=Code and still getting the same error, Cheers for looking

Comment 20

[Dale Harvey (:daleharvey)]

Attachment: 1363169.patch

Just upping the full patch for reference

Comment 21

[Dale Harvey (:daleharvey)]

It seems like Jim is super busy, Mat I also got pointed in your direction and wondered if you had any idea what to do here? https://bugzilla.mozilla.org/show_bug.cgi?id=1363169#c17 is the sum of the question, what do I need to do to be able to use |Microsoft::WRL::ComPtr<IDataTransferManagerInterop> dtmInterop;| in code without compile failures, thanks

Comment 22

[Molly Howell (she/her) (no longer active)]

The problem is that the definition of IDataTransferManagerInterop in shobjidl.h is wrapped in a _WIN32_WINNT version check, and we set that version to Windows 7 since that's the platform we usually need to be supporting; because of that you're only seeing a forward declaration, and trying to create pointers to an incomplete type is causing these cryptic errors. For cases like this where we're adding support for a feature that isn't in Windows 7 we sometimes copy the interface definition into our source files, and that's what I would recommend doing here rather than overriding the version define.

Comment 23

[Molly Howell (she/her) (no longer active)]

(In reply to Matt Howell [:mhowell] from comment #22)
> The problem is that the definition of IDataTransferManagerInterop in
> shobjidl.h is wrapped in a _WIN32_WINNT version check

That should say ShObjIdl_core.h; in the 10.0.15063 SDK the version check is on line 27157, and IDataTransferManagerInterop is defined starting from line 27255.

Comment 24

[Dale Harvey (:daleharvey)]

Thanks for that Matt, so that fixed it for me, I can now get the share dialog to show however I cant seem to provide data to it

Uploaded new patch @ https://gist.github.com/daleharvey/f859f8785a1d752f82f8bd3c414c726b#file-gistfile1-txt-L184, I have the same code pretty much copy and pasted into a plain win32 app @ https://gist.github.com/daleharvey/2d9a21bdf2ed7329981ab12d08a3e5c9#file-test-cpp-L130 and I can compile and run this and it shows the relevant text in the share dialog, when I run the same code inside firefox, the callback gets called and completes (I can see the printf("Asked for data")), however the share dialog shows a loading screen then dissapears

I realise this is quite API specific but wondering if you had any ideas as I really dont know whats up from this point, Cheers

Comment 25

[Molly Howell (she/her) (no longer active)]

Yeah, I don't have any experience with this particular API, so my advice won't be exactly revelatory, sorry. I don't immediately see anything suspicious about your code (and it looks identical to a Raymond Chen example, which is normally a good sign). My first thought would be to log the return values from some of these calls, see if anything is giving you error codes. Since the dialog is opening first and _then_ something is going wrong, I'd probably focus on the callback to start with.

Comment 26

[Dale Harvey (:daleharvey)]

Yup I found 2 examples, both worked in the native win32 test app and neither when inside Firefox. There isnt anything different in the return values and the callback completes to return S_OK; so I dont know what could be wrong here. 

Possibly a problem being called from the wrong process, or the interaction with GetForegroundWindow() is not what I am supposed to be using but it seems like it should be working and not sure where to look now.

Comment 27

[Dale Harvey (:daleharvey)]

On recommendation from Mike I tried adding the ShareUrl method to nsDomWindowsUtils.cpp and accessing the HWND via 

    nsComPtr<nsIWidget> widget = GetWidget();
    HWND hwnd = (HWND)widget->GetNativeData(NS_NATIVE_WINDOW);

The callback code clearly works in the test code so it seems like something must be up with the context, however accessing the hwnd this way doesnt make any difference, the share dialog shows in a loading state momentarily then dissapears. So pretty stumped here, I have attached a console and see no errors being logged, callback complete fine ¯\_(ツ)_/¯

Comment 28

[Dale Harvey (:daleharvey)]

And the most recent patch (mostly cleaned up previous versions) https://gist.github.com/daleharvey/ef84258cf33fc84605617084b4fdd618, If anyone wants the domwindowutils one I can post that too

Comment 29

[Sylvestre Ledru [:Sylvestre]]

This should be part of the release notes when the feature is ready.

Comment 30

[Sylvestre Ledru [:Sylvestre]]

I guess we are not targeting 62 anymore...

Comment 31

[Dale Harvey (:daleharvey)]

Ok so some microsoft devs have been helping me out with this, posting some of their comments below

> it looks like SharePickerUI is throwing an Access Denied 
> exception when trying to retrieve IDataContextChangedEventArgs::NewValue.

> I suspect Firefox is calling CoInitializeSecurity or has registry 
> configuration, and its AccessPermissions are not AppContainer-aware 
> (or maybe they are and intentionally excluding it, but not aware 
> that this can break some app-to-app scenarios).

I am going try to see what effect changing / removing 
https://dxr.mozilla.org/mozilla-central/source/ipc/mscom/MainThreadRuntime.cpp#233
does

and if that works maybe we can modify it to something that allows share to work

Comment 32

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Dale Harvey (:daleharvey) from comment #31)
> I am going try to see what effect changing / removing 
> https://dxr.mozilla.org/mozilla-central/source/ipc/mscom/MainThreadRuntime.
> cpp#233
> does
> 
> and if that works maybe we can modify it to something that allows share to
> work

Please ensure that I receive the review request for any changes to that code. Thanks.

Comment 33

[Dale Harvey (:daleharvey)]

> Please ensure that I receive the review request for any changes to that code. Thanks

Will do

So I replaced CoInitializeSecurity with |return S_OK;| and everything started working so confirmed that is the problem. However the security protections are there for a reason and the only fix seems to be to remove them. 

Some suggestions have been made that we could start a new process that has open permissions and host the sharing data only, killing that process when the share is complete. That sounds safe enough but fairly complex (in my opinion as a frontend dev) for a relatively minor feature (a new process is also going to have issues interfacing with the HWND) 

Jim was wondering if 1. you knew of any feasible workaround for the permissions issue, or 2. whether the spinning of a new process was a simple thing or a bunch of risky work. Cheers

Comment 34

[Pascal Chevrel:pascalc (PTO until April 26)]

Dale, is this feature targetting Firefox 63? If it lands during the 63 nightly cycle, do you intend to let it ride the trains or will it be behind a nightly only pref and ship in a later release? Thanks

Comment 35

[Dale Harvey (:daleharvey)]

Not aimed for 63, still working on it but not a priority and happy for it to ride the trains.

Comment 36

[Dale Harvey (:daleharvey)]

Also I meant to needinfo Matt @ https://bugzilla.mozilla.org/show_bug.cgi?id=1363169#c33, Matt or Aaron (since you introduced InitializeSecurity) do you have any suggestions here, can we avoid starting a whole new process just to pass this object / is it worth avoiding?

Comment 37

[(No longer employed by Mozilla) Aaron Klotz]

I haven't had much interaction with UWP APIs, but I'd be curious whether we can adjust what we pass into CoInitializeSecurity to make this work. Any changes would have to be specific to the parent process, however.

One suggestion that I have is to try s/RPC_C_IMP_LEVEL_IDENTIFY/RPC_C_IMP_LEVEL_IMPERSONATE/ in that call and see if you get anywhere.

Comment 38

[Dale Harvey (:daleharvey)]

Sorry busy week, I tried all the values from https://docs.microsoft.com/en-us/windows/desktop/com/com-impersonation-level-constants and it worked with RPC_C_IMP_LEVEL_DEFAULT but none of the others. Would that be an acceptable value to switch to?

Comment 39

[(No longer employed by Mozilla) Aaron Klotz]

Sure, you can use that, but please change it for the parent process only. I want to keep the existing setting for content processes.

Comment 40

[Dale Harvey (:daleharvey)]

Feel pretty silly not being able to figure this out, but cant seem to get a nsAString to turn into a HSTRING, tried several variations @ 

      //WindowsUIUtils::ShareUrl(const nsAString& aUrlToShare,
      //                         const nsAString& aShareTitle)

      // Shows title fine
      printf("Title is: %s\n", NS_ConvertUTF16toUTF8(aShareTitle).get());

      // Works
      //spDataPacakgeProperties->put_Title(HStringReference(L"A Title!").Get());

      // Compiles but shows garbled string
      //spDataPacakgeProperties->put_Title(HStringReference(PromiseFlatString(aShareTitle).get()).Get());

      // error C2440: 'default argument': cannot convert from 'Microsoft::WRL::Details::Dummy' to 'int'
      //spDataPacakgeProperties->put_Title(HStringReference(aShareTitle.BeginReading()).Get());
      //spDataPacakgeProperties->put_Title(HStringReference(NS_ConvertUTF16toUTF8(aShareTitle).get()).Get());
      //spDataPacakgeProperties->put_Title(HStringReference(NS_LossyConvertUTF16toASCII(aShareTitle).get()).Get());
      //spDataPacakgeProperties->put_Title(HStringReference(aShareTitle).Get());

Comment 41

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Aaron Klotz [:aklotz] from comment #39)
> Sure, you can use that, but please change it for the parent process only. I
> want to keep the existing setting for content processes.

Actually, I was mistaken on that. The docs state that RPC_C_IMP_LEVEL_DEFAULT is not a valid option for CoInitializeSecurity. While that may allow you to finish working on your patch, I'd rather that we didn't land a change that uses a flag that we're explicitly not supposed to be using.

Further exploration might be in order here.

Comment 42

[Dale Harvey (:daleharvey)]

Ok Aaron, yeh the patch is working for now, although seemed to cause problems in debug mode but either way can finish it up as is then look at fixing the coinitialise

So my string problem wasnt actually a conversion problem, it looks like the strings are getting GC'd or something before the callback runs - https://gist.github.com/daleharvey/9a9f29969a58c191edc0ceda0df841ff

Comment 43

[Dale Harvey (:daleharvey)]

Even capturing values corrupts once inside the callback context - https://gist.github.com/daleharvey/36cc005ea5aa91e9d13c5cbf0992a78f, I figure there must be a way to define callback paramters, but having problems figuring it out

Comment 44

[Dale Harvey (:daleharvey)]

ok! got some help and figured out the above problem, was simply a change from a & to a = to have the callback reference by value. Now all that works just need to get the share to accept the Uri, getting a surprising compile error for 

  ABI::Windows::Foundation::Uri link = new ABI::Windows::Foundation::Uri(url);

 0:36.31 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(214): error C2079: 'link' uses undefined class 'ABI::Windows::Foundation::Uri'
 0:36.31 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(214): error C2027: use of undefined type 'ABI::Windows::Foundation::Uri'
 0:36.31 c:\program files (x86)\windows kits\10\include\10.0.16299.0\winrt\Windows.ApplicationModel.h(2439): note: see declaration of 'ABI::Windows::Foundation::Uri'
 0:36.31 mozmake.EXE[4]: *** [c:/Users/daleharvey/src/gecko-dev/config/rules.mk:1054: WindowsUIUtils.obj] Error 2
 0:36.31 mozmake.EXE[3]: *** [c:/Users/daleharvey/src/gecko-dev/config/recurse.mk:74: widget/windows/target] Error 2
 0:36.31 mozmake.EXE[2]: *** [c:/Users/daleharvey/src/gecko-dev/config/recurse.mk:34: compile] Error 2
 0:36.33 mozmake.EXE[1]: *** [c:/Users/daleharvey/src/gecko-dev/config/rules.mk:423: default] Error 2

Which is weird since I think I am including all the headers, but going on what this bug has been like so far probably something silly is wrong

Comment 45

[Dale Harvey (:daleharvey)]

So fairly sure I need a forward declaration again here, I tried copying https://gist.github.com/daleharvey/ee771dceae8256a32ef0af89f93fdb5d over from windows.applicationmodel.h but looks like it needs more (pasted the error in that gist)

Matt hopefully the last question about this, do you know what I need to be able to use Uri here? Thanks

Comment 46

[Molly Howell (she/her) (no longer active)]

There's definitely a definition for Windows.Foundation.Uri in windows.foundation.idl, but I can't find it in the windows.foundation.h header which was supposedly generated from that. I would definitely try including windows.foundation.h if you haven't already though.

I did find https://msdn.microsoft.com/en-us/library/hh973459.aspx which seems to indicate it might not be possible to construct a Uri object directly like that, you might have to go through this IUriRuntimeClassFactory thing.

I also found a page (https://docs.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/interop-winrt-abi) which has some info about how this WinRT stuff works in C++, so some of that might help to understand what's going on if you haven't seen it yet.

Comment 47

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1363169-Add-support-for-native-windows-share.patch

Updated working patch

Comment 48

[Dale Harvey (:daleharvey)]

Thanks Matt that was a huge help.

This is a working patch, however as Aaron pointed out RPC_C_IMP_LEVEL_DEFAULT isnt a valid param for that and I need to disable the result assertions or it crashes in debug mode. None of the other values allow us to access the params when doing a share so we need a solution for that, either to relax the permissions / not call CoInitializeSecurity on the parent process, or to spin off a new process with relaxed permissions. 

Either options would be touching sensitive platform code I dont have much background on so would definitely need guidance to carry on at this point.

Aaron what do you think are the options here?

Comment 49

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: SetNegotiableSecurityOnProxy

Removing CoInitializeSecurity is not an option, unfortunately.

My first suggestion is based on the fact that CoInitializeSecurity establishes the baseline COM security configuration for the entire process. However, the security configuration can also be set on a per-proxy basis.

I am attaching a patch that adds a function called SetNegotiableSecurityOnProxy to mozilla/mscom/Utils.h. Try calling this for each of your dtm and dtmInterop interfaces, and see if it helps. If it does, I'll file a dependent bug and land it in MSCOM.

If that doesn't work, I suggest we follow up on the Microsoft developers' comments about needing CoInitializeSecurity to be "AppContainer-aware" to find out what they actually mean by that. Maybe there's a canonical security configuration there that we should be using.

Comment 50

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Aaron Klotz [:aklotz] from comment #49)
> Created attachment 9001354 [details] [diff] [review]
> SetNegotiableSecurityOnProxy
> I am attaching a patch that adds a function called
> SetNegotiableSecurityOnProxy to mozilla/mscom/Utils.h. Try calling this for
> each of your dtm and dtmInterop interfaces, and see if it helps.

To clarify: You should call SetNegotiableSecurityOnProxy as soon as you have the interface, before you do anything else with it.

Comment 51

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: SetNegotiableSecurityOnProxy (r2)

I made a mistake in the first cut of the patch. Here is a corrected revision.

Comment 52

[Dale Harvey (:daleharvey)]

Thanks for that, sorry for the bother but having a little trouble using it

  mozilla::mscom::SetNegotiableSecurityOnProxy(&dtmInterop);

gives me 

 0:41.44 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(228): error C2664: 'bool mozilla::mscom::SetNegotiableSecurityOnProxy(IUnknown *)': cannot convert argument 1 from 'Microsoft::WRL::Details::ComPtrRef<Microsoft::WRL::ComPtr<IDataTransferManagerInterop>>' to 'IUnknown *'
 0:41.44 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(228): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called

I tried 

  mozilla::mscom::SetNegotiableSecurityOnProxy(reinterpret_cast<IUnknown**>(&dtmInterop));

from https://docs.microsoft.com/en-us/windows/desktop/api/dwrite/nf-dwrite-dwritecreatefactory but getting the same error, any pointers on how to cast it correctly? cheers

Comment 53

[(No longer employed by Mozilla) Aaron Klotz]

Try

mozilla::mscom::SetNegotiableSecurityOnProxy(dtmInterop.Get());

Comment 54

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1363169-Add-support-for-native-windows-share.patch

Comment 55

[Dale Harvey (:daleharvey)]

Doh thanks, should have realised as I have had to .Get() to fix things a bunch of times here. The patch however didnt work, updated with my current version, will ask if there is anything about that App-Container aware comment on the mailing list.

Comment 56

[Dale Harvey (:daleharvey)]

Got a hint from the mailing list, it seems like explorer.exe has the same problems and they use https://gist.github.com/daleharvey/4e3d04ff7705301abdae6b6d98788c8c to fix it, that code doesnt compile for us (we dont have access to wil/resources.h?), trying to convert it to something that works for us but again not familiar with this code at all so if anyone was able to help out that would be great

Comment 57

[Molly Howell (she/her) (no longer active)]

The wil::unique_any thing looks like it's just a smart pointer? If so then we should be able to substitute a mozilla::UniquePtr, passing it a deleter that calls LocalFree. I don't see anything else in that gist that looks nonstandard.

Comment 58

[(No longer employed by Mozilla) Aaron Klotz]

I'm not a fan of SDDL strings in our code, as they're too cryptic for my liking.

But it is easy enough to translate that string into code that works with our existing security descriptor setup.

Comment 59

[(No longer employed by Mozilla) Aaron Klotz]

Dale, can you try the patch that I attached to bug 1484758?

Comment 60

[Dale Harvey (:daleharvey)]

Hey Aaron, that works great for me, thanks so much

Comment 61

[(No longer employed by Mozilla) Aaron Klotz]

Great! I'll get that reviewed and landed ASAP.

Comment 62

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1363169-Add-support-for-native-windows-share.patch

Ok, thanks so much Aaron

However with my last update after our builds switched to clang and I had to update my SDK this no longer runs for me, code compiles fine however the share UI doesnt display at all, no errors on both debug and obj builds, I tried checking if the IDataTransferManagerInterop definition changed in the 134 SDK but its the same guid so I have no idea whats up here

Comment 63

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1363169-Add-support-for-native-windows-share.patch

Thanks to Aarons patch this is all working now, many thanks :)

Comment 64

[:Gijs (he/him)]

Comment on attachment 9010079 [details] [diff] [review]
0001-Bug-1363169-Add-support-for-native-windows-share.patch

Review of attachment 9010079 [details] [diff] [review]:
-----------------------------------------------------------------

The implementation looks OK, I have some comments on the test.

::: browser/base/content/test/urlbar/browser_page_action_menu_share_win.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Nit: test code should normally be licensed public domain.

@@ +6,5 @@
> +
> +/* global sinon */
> +Services.scriptloader.loadSubScript("resource://testing-common/sinon-2.3.2.js");
> +
> +const URL = "http://example.org/";

Nit: please don't shadow the window global `URL` constructor, and pick a different variable name, e.g. TEST_URL or kTestURL.

@@ +23,5 @@
> +
> +registerCleanupFunction(async function() {
> +  stub.restore();
> +  delete window.sinon;
> +  await EventUtils.synthesizeNativeMouseMove(document.documentElement, 0, 0);

What is this trying to do?

@@ +24,5 @@
> +registerCleanupFunction(async function() {
> +  stub.restore();
> +  delete window.sinon;
> +  await EventUtils.synthesizeNativeMouseMove(document.documentElement, 0, 0);
> +  await PlacesUtils.history.clear();

I don't think we need to do this?

@@ +41,5 @@
> +    await hiddenPromise;
> +
> +    Assert.equal(sharedUrl, "http://example.org/",
> +                 "Shared correct URL");
> +    Assert.equal(sharedTitle, "mochitest index /",

Please add a test page for this test with a more deterministic title.

Comment 65

[:Gijs (he/him)]

Comment on attachment 9010079 [details] [diff] [review]
0001-Bug-1363169-Add-support-for-native-windows-share.patch

Review of attachment 9010079 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/PageActions.jsm
@@ +1179,4 @@
>    });
>  }
>  
> +if (AppConstants.isPlatformAndVersionAtLeast("win", "6.4")) {

Oh, I almost forgot - the dep bug added support for Windows 8. Does the WindowsUIUtils code only work on win10?

Comment 66

[Dale Harvey (:daleharvey)]

> +  await EventUtils.synthesizeNativeMouseMove(document.documentElement, 0, 0);
> +  await PlacesUtils.history.clear();

I based this test off the osx version of the same test which had problems braking unrelated tests if there was any extra history and if I left the mouse inside the url bar (https://bugzilla.mozilla.org/show_bug.cgi?id=1363168#c44)

Comment 67

[Dale Harvey (:daleharvey)]

*breaking

Comment 68

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

Comment 69

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

Comment 70

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

Updated with suggested changes to tests, cheers

I gave this a shot on Windows 8.1 and it failed silently, can debug further but dont think it needs to block landing for 10

Comment 71

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

Ugh apologies, switching this patch between windows lost the new files for some reason, fixed now

Comment 72

[:Gijs (he/him)]

Comment on attachment 9011373 [details] [diff] [review]
0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

Review of attachment 9011373 [details] [diff] [review]:
-----------------------------------------------------------------

ok, r=me . I'm not sure if we need a follow-up for win8, AIUI its usage share is decreasing rapidly in favour of win10 (while users on win7 sit it out, but can't benefit from any of this stuff anyway).

Comment 73

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 9011373 [details] [diff] [review]
0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

Review of attachment 9011373 [details] [diff] [review]:
-----------------------------------------------------------------

I am, obviously, only reviewing the C++ bits.

Please add return code checks to all the things!

::: widget/windows/WindowsUIUtils.cpp
@@ +91,4 @@
>  };
>  #endif
>  
> +#ifndef IDataTransferManagerInterop

|IDataTransferManagerInterop| is never defined as a macro, so this check will never succeed. From what I see in the headers, you need to use |__IDataTransferManagerInterop_INTERFACE_DEFINED__| instead.

@@ +203,5 @@
> +    return NS_OK;
> +  }
> +
> +  HSTRING title;
> +  WindowsCreateString(PromiseFlatString(aShareTitle).get(), aShareTitle.Length(), &title);

Please check return codes. HRESULTs use the SUCCEEDED and FAILED macros to test.

@@ +205,5 @@
> +
> +  HSTRING title;
> +  WindowsCreateString(PromiseFlatString(aShareTitle).get(), aShareTitle.Length(), &title);
> +  HSTRING url;
> +  WindowsCreateString(PromiseFlatString(aUrlToShare).get(), aUrlToShare.Length(), &url);

It looks to me like you're leaking these HSTRINGs. To fix this, I would suggest using UniquePtr with a custom deleter that calls WindowsDeleteString, that way the compiler will be able to clean these up when you return early from failed return codes.

Comment 74

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Aaron Klotz [:aklotz] from comment #73)
> It looks to me like you're leaking these HSTRINGs. To fix this, I would
> suggest using UniquePtr with a custom deleter that calls
> WindowsDeleteString, that way the compiler will be able to clean these up
> when you return early from failed return codes.

Use CertStoreUniquePtr [1] and CertStoreDeleter [2] as an example of how to do this:

https://searchfox.org/mozilla-central/rev/ffe6eaf2f032e58ec3b0650a87df2c62ae4ca441/mozglue/build/Authenticode.cpp#47
https://searchfox.org/mozilla-central/rev/ffe6eaf2f032e58ec3b0650a87df2c62ae4ca441/mozglue/build/Authenticode.cpp#92

Comment 75

[Dale Harvey (:daleharvey)]

Cheers for the review, done the first 2 but having an issue implementing the HStringDeleter, I am using the code from Authenticode.cpp (but with HSTRING) and it seems to work, however if I access the UniquePtr [1] inside the callback [2] I get the following error. I tried making the capture [3] an explicit [title] but it gave the same error, do you have any suggestions? Many thanks

 0:38.00 c:/Users/daleharvey/src/gecko-dev/widget/windows/WindowsUIUtils.cpp(271,42):  error: call to deleted constructor of 'HStringUniquePtr' (aka 'UniquePtr<HSTRING__ *, HStringDeleter>')
 0:38.00       spDataPackageProperties->put_Title(title.get());
 0:38.00                                          ^~~~~
 0:38.00 c:/Users/daleharvey/src/gecko-dev/obj-i686-pc-mingw32/dist/include\mozilla/UniquePtr.h(352,3):  note: 'UniquePtr' has been explicitly marked deleted here
 0:38.00   UniquePtr(const UniquePtr& aOther) = delete; // construct using std::move()!
 0:38.00   ^
 0:38.15 1 error generated.


 [1] https://gist.github.com/daleharvey/0e37d38b0d6fa05f148746ff4d6344fb#file-gistfile1-txt-L318
 [2] https://gist.github.com/daleharvey/0e37d38b0d6fa05f148746ff4d6344fb#file-gistfile1-txt-L272
 [3] https://gist.github.com/daleharvey/0e37d38b0d6fa05f148746ff4d6344fb#file-gistfile1-txt-L309

Comment 76

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Dale Harvey (:daleharvey) from comment #75)
> Cheers for the review, done the first 2 but having an issue implementing the
> HStringDeleter, I am using the code from Authenticode.cpp (but with HSTRING)
> and it seems to work, however if I access the UniquePtr [1] inside the
> callback [2] I get the following error. I tried making the capture [3] an
> explicit [title] but it gave the same error, do you have any suggestions?
> Many thanks

The closure is trying to capture |title| by copying it. Based on what I see in your code, you want to std::move it into the closure. For example, https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/widget/windows/JumpListBuilder.cpp#259

Comment 77

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

That worked great thanks, update with those review changes

Comment 78

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 9012528 [details] [diff] [review]
0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

Review of attachment 9012528 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see this once more, please.

::: widget/windows/WindowsUIUtils.cpp
@@ +215,5 @@
> +    return NS_OK;
> +  }
> +
> +  HSTRING rawTitle;
> +  HStringUniquePtr aTitle;

You should only use the 'a' prefix on a variable name for function arguments. This should just be named title.

@@ +217,5 @@
> +
> +  HSTRING rawTitle;
> +  HStringUniquePtr aTitle;
> +  HSTRING rawUrl;
> +  ComPtr<IUriRuntimeClass> aUri;

Same for this one. s/aUri/uri/

@@ +223,5 @@
> +  HRESULT hr = WindowsCreateString(PromiseFlatString(aShareTitle).get(), aShareTitle.Length(), &rawTitle);
> +  if (FAILED(hr)) {
> +    return NS_OK;
> +  }
> +  aTitle.reset(rawTitle);

Instead of creating the HStringUniquePtr up above, and then calling reset here, just create the pointer here:
HStringUniquePtr title(rawTitle);

@@ +228,5 @@
> +
> +  ComPtr<IUriRuntimeClassFactory> uriFactory;
> +  hr = GetActivationFactory(HStringReference(RuntimeClass_Windows_Foundation_Uri).Get(), &uriFactory);
> +  if (FAILED(hr)) {
> +    return NS_OK;

Given that you're returning NS_OK down all of these failure paths, am I correct to assume that you intend any failures in this function to just happen silently?

@@ +237,5 @@
> +    return NS_OK;
> +  }
> +
> +  hr = uriFactory->CreateUri(rawUrl, &aUri);
> +  WindowsDeleteString(rawUrl);

Please use HStringUniquePtr for rawUrl as well for the sake of future-proofing.

@@ +242,5 @@
> +  if (FAILED(hr)) {
> +    return NS_OK;
> +  }
> +
> +  HWND hwnd = GetForegroundWindow();

GetForegroundWindow may return nullptr. Return if !hwnd please.

@@ +263,5 @@
> +  auto callback = Callback < ITypedEventHandler<DataTransferManager*, DataRequestedEventArgs* >> (
> +          [aUri, title = std::move(aTitle)](IDataTransferManager*, IDataRequestedEventArgs* pArgs) -> HRESULT
> +    {
> +      ComPtr<IDataRequest> spDataRequest;
> +      pArgs->get_Request(&spDataRequest);

Add result checking in here too, please. Probably the best pattern to use here, since you're returning an HRESULT, is:

if (FAILED(hr)) {
  return hr;
}

Comment 79

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

> Given that you're returning NS_OK down all of these 
> failure paths, am I correct to assume that you intend any 
> failures in this function to just happen silently?

Yup, I copied the just return NS_OK from the function above, there is no UX for the failure path and doesnt look to be any reason it should fail. Seems like something if we get reports about we could improve if needed.

Cheers for the review, made all the changes you mentioned

Comment 80

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 9013079 [details] [diff] [review]
0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

Review of attachment 9013079 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for C++ parts, provided that comments are addressed.

::: widget/windows/WindowsUIUtils.cpp
@@ +214,5 @@
> +  if (!IsWin10OrLater()) {
> +    return NS_OK;
> +  }
> +
> +  HSTRING rawTitle;

Nit: Can you please move these declarations to the line immediately above where they're first used?

@@ +224,5 @@
> +  if (FAILED(hr)) {
> +    return NS_OK;
> +  }
> +  HStringUniquePtr title(rawTitle);
> +  

Nit: you've left some spaces in this blank line.

@@ +242,5 @@
> +    return NS_OK;
> +  }
> +
> +  HWND hwnd = GetForegroundWindow();
> +  if (!hwnd) { 

Nit: trailing space

@@ +245,5 @@
> +  HWND hwnd = GetForegroundWindow();
> +  if (!hwnd) { 
> +    return NS_OK;
> +  }
> +  

Nit: Spaces in blank line

@@ +247,5 @@
> +    return NS_OK;
> +  }
> +  
> +  ComPtr<IDataTransferManagerInterop> dtmInterop;
> +  ComPtr<IDataTransferManager> dtm;

Nit: Move this to the line above first use.

@@ +248,5 @@
> +  }
> +  
> +  ComPtr<IDataTransferManagerInterop> dtmInterop;
> +  ComPtr<IDataTransferManager> dtm;
> +  EventRegistrationToken dataRequestedToken;

Nit: Move this to the line above first use.

@@ +263,5 @@
> +    return NS_OK;
> +  }
> +
> +  auto callback = Callback < ITypedEventHandler<DataTransferManager*, DataRequestedEventArgs* >> (
> +          [uri, title = std::move(title)](IDataTransferManager*, IDataRequestedEventArgs* pArgs) -> HRESULT

I think you can std::move uri into this lambda as well, right? I don't see it referenced outside of callback from here forward.

@@ +265,5 @@
> +
> +  auto callback = Callback < ITypedEventHandler<DataTransferManager*, DataRequestedEventArgs* >> (
> +          [uri, title = std::move(title)](IDataTransferManager*, IDataRequestedEventArgs* pArgs) -> HRESULT
> +    {
> +      ComPtr<IDataRequest> spDataRequest;

Nit: Move these declarations to the line above first use.

@@ +271,5 @@
> +      ComPtr<IDataPackagePropertySet> spDataPackageProperties;
> +
> +      HRESULT hr = pArgs->get_Request(&spDataRequest);
> +      if (FAILED(hr)) {
> +        return hr; 

Nit: Trailing space

@@ +273,5 @@
> +      HRESULT hr = pArgs->get_Request(&spDataRequest);
> +      if (FAILED(hr)) {
> +        return hr; 
> +      }
> +      

Nit: Several spaces present on this blank line. Please remove them.

@@ +276,5 @@
> +      }
> +      
> +      hr = spDataRequest->get_Data(&spDataPackage);
> +      if (FAILED(hr)) {
> +        return hr; 

Nit: Trailing space

@@ +281,5 @@
> +      }
> +
> +      hr = spDataPackage->get_Properties(&spDataPackageProperties);
> +      if (FAILED(hr)) {
> +        return hr; 

Nit: Trailing space

@@ +286,5 @@
> +      }
> +
> +      hr = spDataPackageProperties->put_Title(title.get());
> +      if (FAILED(hr)) {
> +        return hr; 

Nit: Trailing space

@@ +291,5 @@
> +      }
> +
> +      hr = spDataPackage->SetUri(uri.Get());
> +      if (FAILED(hr)) {
> +        return hr; 

Nit: Trailing space

Comment 81

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

Great thanks, apologies for the whitespace errors, was using a new editor and missed that they werent being stripped

Comment 82

[Dale Harvey (:daleharvey)]

Green try run including TV - https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfa8ce1a55bd856f5d3584b60716a479a5a7ebd8

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c3a9571f63
Add support for native windows share. r=gijs, r=aklotz

Comment 84

[Cristina Coroiu [:ccoroiu]]

Backed out changeset c3c3a9571f63 (bug 1363169) for Bmsvc build bustage at build/src/widget/windows/WindowsUIUtils.cpp 

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/59345f79f963cb35478a1cab1f1aecfe76eaccb7

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=205546962&revision=c3c3a9571f6306dd0df49e779977f0ee45a37c8e

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=205546976&repo=mozilla-inbound&lineNumber=23843

15:50:35     INFO -  mkdir -p '.deps/'
15:50:35     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/layout/base/gtest'
15:50:35     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/layout/base'
15:50:35     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/layout/base'
15:50:35     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/layout/base/gtest'
15:50:35     INFO -  layout/base/gtest
15:50:35     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/layout/base/gtest'
15:50:35     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/layout/base/gtest'
15:50:35     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/layout/base/gtest'
15:50:39     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/widget/windows'
15:50:39     INFO -  z:/build/build/src/sccache2/sccache.exe z:/build/build/src/vs2017_15.8.4/VC/bin/Hostx64/x86/cl.exe -FoWindowsUIUtils.obj -c  -Iz:/build/build/src/obj-firefox/dist/stl_wrappers -DNDEBUG=1 -DTRIMMED=1 -DWIN32_LEAN_AND_MEAN -D_WIN32 -DWIN32 -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DOS_WIN=1 -D_UNICODE -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DUNICODE -D_WINDOWS -D_SECURE_ATL -DCOMPILER_MSVC -DMOZ_UNICODE -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -Iz:/build/build/src/widget/windows -Iz:/build/build/src/obj-firefox/widget/windows -Iz:/build/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -Iz:/build/build/src/ipc/chromium/src -Iz:/build/build/src/ipc/glue -Iz:/build/build/src/layout/forms -Iz:/build/build/src/layout/generic -Iz:/build/build/src/layout/xul -Iz:/build/build/src/toolkit/xre -Iz:/build/build/src/widget -Iz:/build/build/src/widget/headless -Iz:/build/build/src/xpcom/base -Iz:/build/build/src/obj-firefox/dist/include -Iz:/build/build/src/obj-firefox/dist/include/nspr -Iz:/build/build/src/obj-firefox/dist/include/nss -MD -FI z:/build/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -utf-8 -TP -nologo -w15038 -wd5026 -wd5027 -Zc:sizedDealloc- -wd4091 -wd4577 -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -arch:SSE2 -Gw -wd4251 -wd4244 -wd4267 -wd4800 -wd4595 -wd4065 -we4553 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -GR- -Z7 -O1 -Oi -Oy- -WX -Iz:/build/build/src/obj-firefox/dist/include/cairo  -deps.deps/WindowsUIUtils.obj.pp   -wd5038 z:/build/build/src/widget/windows/WindowsUIUtils.cpp
15:50:39     INFO -  WindowsUIUtils.cpp
15:50:39     INFO -  z:/build/build/src/widget/windows/WindowsUIUtils.cpp(289): error C2220: warning treated as error - no 'object' file generated
15:50:39     INFO -  z:/build/build/src/widget/windows/WindowsUIUtils.cpp(289): warning C4996: 'ABI::Windows::ApplicationModel::DataTransfer::IDataPackage::SetUri': SetUri may be altered or unavailable for releases after Windows Phone 'OSVersion' (TBD). Instead, use SetWebLink or SetApplicationLink.
15:50:39     INFO -  z:\build\build\src\vs2017_15.8.4\sdk\include\10.0.17134.0\winrt\Windows.ApplicationModel.DataTransfer.h(3092): note: see declaration of 'ABI::Windows::ApplicationModel::DataTransfer::IDataPackage::SetUri'
15:50:39     INFO -  z:/build/build/src/config/rules.mk:1118: recipe for target 'WindowsUIUtils.obj' failed
15:50:39     INFO -  mozmake.EXE[4]: *** [WindowsUIUtils.obj] Error 2
15:50:39     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/widget/windows'
15:50:39     INFO -  mozmake.EXE[4]: *** Waiting for unfinished jobs....
15:50:39     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/layout/style'
15:50:39     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/layout/style'
15:50:39     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/layout/style'
15:50:39     INFO -  mozmake.EXE[4]: Leaving directory 'z:/build/build/src/obj-firefox/layout/style'
15:50:41     INFO -  mozmake.EXE[4]: Entering directory 'z:/build/build/src/obj-firefox/layout/style'
15:50:41     INFO -  z:/build/build/src/sccache2/sccache.exe z:/build/build/src/vs2017_15.8.4/VC/bin/Hostx64/x86/cl.exe -FonsLayoutStylesheetCache.obj -c  -Iz:/build/build/src/obj-firefox/dist/stl_wrappers -DNDEBUG=1 -DTRIMMED=1 -DWIN32_LEAN_AND_MEAN -D_WIN32 -DWIN32 -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -DOS_WIN=1 -D_UNICODE -DCHROMIUM_BUILD -DU_STATIC_IMPLEMENTATION -DUNICODE -D_WINDOWS -D_SECURE_ATL -DCOMPILER_MSVC -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -Iz:/build/build/src/layout/style -Iz:/build/build/src/obj-firefox/layout/style -Iz:/build/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -Iz:/build/build/src/ipc/chromium/src -Iz:/build/build/src/ipc/glue -Iz:/build/build/src/layout/base -Iz:/build/build/src/layout/generic -Iz:/build/build/src/layout/svg -Iz:/build/build/src/layout/xul -Iz:/build/build/src/dom/base -Iz:/build/build/src/dom/html -Iz:/build/build/src/dom/xbl -Iz:/build/build/src/dom/xul -Iz:/build/build/src/image -Iz:/build/build/src/obj-firefox/dist/include -Iz:/build/build/src/obj-firefox/dist/include/nspr -Iz:/build/build/src/obj-firefox/dist/include/nss -MD -FI z:/build/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -utf-8 -TP -nologo -w15038 -wd5026 -wd5027 -Zc:sizedDealloc- -wd4091 -wd4577 -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -arch:SSE2 -Gw -wd4251 -wd4244 -wd4267 -wd4800 -wd4595 -wd4065 -we4553 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -GR- -Z7 -O1 -Oi -Oy- -WX  -deps.deps/nsLayoutStylesheetCache.obj.pp    z:/build/build/src/layout/style/nsLayoutStylesheetCache.cpp
15:50:41     INFO -  nsLayoutStylesheetCache.cpp

Comment 85

[Dale Harvey (:daleharvey)]

argh sorry, neither SetWebLink nor SetApplicationLink are members of IDataPackage

*error: no member names 'SetApplicationLink in 'ABI::Windows::ApplicationModel::DataTransfer::IDataPackage'*

Looks like they are in IDataPackage2, but thats not what |spDataRequest->get_Data(| returns and I cant see how to get it, Aaron do you have a pointer here?

Comment 86

[(No longer employed by Mozilla) Aaron Klotz]

Call QueryInterface on your IDataPackage to resolve the IDataPackage2.

Comment 87

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

Great thanks, will run on try then rerequest review just in case

Comment 88

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

And without trailing whitespace

Comment 89

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

Comment 90

[Dale Harvey (:daleharvey)]

Comment on attachment 9017245 [details] [diff] [review]
0001-Bug-1363169-Add-support-for-native-windows-share.-r-.patch

Ok is looking green now, Cheers for the tip Aaron this has only got the IDataPackage2 change but figured be safe and get a rereview, Thanks

Comment 91

[Dale Harvey (:daleharvey)]

Thanks, second time lucky https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c4b4126c1ffc3dbbb64afda1e53270f38c89d02

Comment 92

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f197a31d28f
Add support for native windows share. r=gijs, r=aklotz

Comment 93

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5f197a31d28f