Closed
Bug 1363169
Opened 8 years ago
Closed 6 years ago
Add support for native Windows "Share" feature
Categories
(Firefox :: Menus, enhancement, P3)
Tracking
()
People
(Reporter: Dolske, Assigned: daleharvey)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: feature, Whiteboard: [reserve-photon-structure][tpi:+])
Attachments
(2 files, 15 obsolete files)
1.92 KB,
patch
|
Details | Diff | Splinter Review | |
12.67 KB,
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•8 years ago
|
||
Jim, can you help find an owner to implement this?
Flags: needinfo?(jmathies)
Updated•8 years ago
|
Flags: needinfo?(jmathies)
Whiteboard: [photon-structure][tpi:+]
Updated•8 years ago
|
Flags: qe-verify?
Priority: -- → P2
Updated•8 years ago
|
Flags: qe-verify? → qe-verify-
Updated•8 years ago
|
Whiteboard: [photon-structure][tpi:+] → [reserve-photon-structure][tpi:+]
Updated•7 years ago
|
Priority: P3 → P4
Updated•7 years ago
|
Priority: P4 → P5
Updated•7 years ago
|
Priority: P5 → P3
Comment 4•7 years ago
|
||
Downgrading the priority here while we focus on 57 (and have to wait for help with the native stuff).
Priority: P3 → P4
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dharvey
Assignee | ||
Comment 5•7 years ago
|
||
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
Flags: needinfo?(gijskruitbosch+bugs)
Comment 6•7 years ago
|
||
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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dharvey)
Comment 7•7 years ago
|
||
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•7 years ago
|
||
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 .
Assignee | ||
Comment 9•7 years ago
|
||
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
Flags: needinfo?(dharvey)
Assignee | ||
Comment 10•7 years ago
|
||
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
Assignee | ||
Comment 11•7 years ago
|
||
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
Flags: needinfo?(jimm)
Assignee | ||
Comment 12•7 years ago
|
||
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
Flags: needinfo?(jimm) → needinfo?(cpeterson)
Assignee | ||
Comment 13•7 years ago
|
||
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
Flags: needinfo?(gijskruitbosch+bugs)
Comment 14•7 years ago
|
||
(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.
status-firefox60:
--- → wontfix
status-firefox61:
--- → wontfix
status-firefox62:
--- → affected
status-firefox-esr52:
--- → wontfix
status-firefox-esr60:
--- → wontfix
Flags: needinfo?(cpeterson)
OS: Unspecified → Windows
Priority: P4 → P3
Comment 15•7 years ago
|
||
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?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 16•7 years ago
|
||
> 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
Assignee | ||
Comment 17•7 years ago
|
||
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?
Flags: needinfo?(gijskruitbosch+bugs)
Comment 18•7 years ago
|
||
(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...
Flags: needinfo?(jmathies)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dharvey)
Assignee | ||
Comment 19•7 years ago
|
||
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
Flags: needinfo?(dharvey)
Assignee | ||
Comment 20•6 years ago
|
||
Just upping the full patch for reference
Assignee | ||
Comment 21•6 years ago
|
||
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
Flags: needinfo?(mhowell)
Comment 22•6 years ago
|
||
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.
Flags: needinfo?(mhowell)
Flags: needinfo?(jmathies)
Comment 23•6 years ago
|
||
(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.
Assignee | ||
Comment 24•6 years ago
|
||
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
Flags: needinfo?(mhowell)
Comment 25•6 years ago
|
||
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.
Flags: needinfo?(mhowell)
Assignee | ||
Comment 26•6 years ago
|
||
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.
Assignee | ||
Comment 27•6 years ago
|
||
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 ¯\_(ツ)_/¯
Assignee | ||
Comment 28•6 years ago
|
||
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•6 years ago
|
||
This should be part of the release notes when the feature is ready.
relnote-firefox:
--- → ?
Comment 30•6 years ago
|
||
I guess we are not targeting 62 anymore...
status-firefox63:
--- → affected
Assignee | ||
Comment 31•6 years ago
|
||
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•6 years ago
|
||
(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.
Assignee | ||
Comment 33•6 years ago
|
||
> 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
Flags: needinfo?(jmathies)
Comment 34•6 years ago
|
||
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
Flags: needinfo?(dharvey)
Assignee | ||
Comment 35•6 years ago
|
||
Not aimed for 63, still working on it but not a priority and happy for it to ride the trains.
Flags: needinfo?(dharvey)
Assignee | ||
Comment 36•6 years ago
|
||
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?
Flags: needinfo?(mhowell)
Flags: needinfo?(jmathies)
Flags: needinfo?(aklotz)
Comment 37•6 years ago
|
||
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.
Flags: needinfo?(mhowell)
Flags: needinfo?(aklotz)
Assignee | ||
Comment 38•6 years ago
|
||
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?
Flags: needinfo?(aklotz)
Comment 39•6 years ago
|
||
Sure, you can use that, but please change it for the parent process only. I want to keep the existing setting for content processes.
Flags: needinfo?(aklotz)
Assignee | ||
Comment 40•6 years ago
|
||
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•6 years ago
|
||
(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.
Assignee | ||
Comment 42•6 years ago
|
||
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
Assignee | ||
Comment 43•6 years ago
|
||
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
Assignee | ||
Comment 44•6 years ago
|
||
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
Assignee | ||
Comment 45•6 years ago
|
||
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
Flags: needinfo?(mhowell)
Comment 46•6 years ago
|
||
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.
Flags: needinfo?(mhowell)
Assignee | ||
Comment 47•6 years ago
|
||
Updated working patch
Attachment #8980217 -
Attachment is obsolete: true
Assignee | ||
Comment 48•6 years ago
|
||
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?
Flags: needinfo?(aklotz)
Comment 49•6 years ago
|
||
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.
Flags: needinfo?(aklotz)
Comment 50•6 years ago
|
||
(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•6 years ago
|
||
I made a mistake in the first cut of the patch. Here is a corrected revision.
Attachment #9001354 -
Attachment is obsolete: true
Assignee | ||
Comment 52•6 years ago
|
||
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
Flags: needinfo?(aklotz)
Comment 53•6 years ago
|
||
Try
mozilla::mscom::SetNegotiableSecurityOnProxy(dtmInterop.Get());
Flags: needinfo?(aklotz)
Assignee | ||
Comment 54•6 years ago
|
||
Attachment #9000108 -
Attachment is obsolete: true
Assignee | ||
Comment 55•6 years ago
|
||
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.
Assignee | ||
Comment 56•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
Dale, can you try the patch that I attached to bug 1484758?
Flags: needinfo?(dharvey)
Assignee | ||
Comment 60•6 years ago
|
||
Hey Aaron, that works great for me, thanks so much
Flags: needinfo?(dharvey)
Comment 61•6 years ago
|
||
Great! I'll get that reviewed and landed ASAP.
Assignee | ||
Comment 62•6 years ago
|
||
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
Attachment #9002191 -
Attachment is obsolete: true
Assignee | ||
Comment 63•6 years ago
|
||
Thanks to Aarons patch this is all working now, many thanks :)
Attachment #9004250 -
Attachment is obsolete: true
Attachment #9010079 -
Flags: review?(gijskruitbosch+bugs)
Attachment #9010079 -
Flags: review?(aklotz)
Comment 64•6 years ago
|
||
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.
Attachment #9010079 -
Flags: review?(gijskruitbosch+bugs)
Comment 65•6 years ago
|
||
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?
Assignee | ||
Comment 66•6 years ago
|
||
> + 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)
Assignee | ||
Comment 67•6 years ago
|
||
*breaking
Assignee | ||
Comment 68•6 years ago
|
||
Attachment #9010079 -
Attachment is obsolete: true
Attachment #9010079 -
Flags: review?(aklotz)
Assignee | ||
Comment 69•6 years ago
|
||
Attachment #9011192 -
Attachment is obsolete: true
Assignee | ||
Comment 70•6 years ago
|
||
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
Attachment #9011194 -
Attachment is obsolete: true
Attachment #9011218 -
Flags: review?(gijskruitbosch+bugs)
Attachment #9011218 -
Flags: review?(aklotz)
Assignee | ||
Comment 71•6 years ago
|
||
Ugh apologies, switching this patch between windows lost the new files for some reason, fixed now
Attachment #9011218 -
Attachment is obsolete: true
Attachment #9011218 -
Flags: review?(gijskruitbosch+bugs)
Attachment #9011218 -
Flags: review?(aklotz)
Attachment #9011373 -
Flags: review?(gijskruitbosch+bugs)
Attachment #9011373 -
Flags: review?(aklotz)
Comment 72•6 years ago
|
||
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).
Attachment #9011373 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 73•6 years ago
|
||
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.
Attachment #9011373 -
Flags: review?(aklotz) → review-
Comment 74•6 years ago
|
||
(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
Assignee | ||
Comment 75•6 years ago
|
||
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
Flags: needinfo?(aklotz)
Comment 76•6 years ago
|
||
(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
Flags: needinfo?(aklotz)
Assignee | ||
Comment 77•6 years ago
|
||
That worked great thanks, update with those review changes
Attachment #9011373 -
Attachment is obsolete: true
Attachment #9012528 -
Flags: review?(aklotz)
Comment 78•6 years ago
|
||
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;
}
Attachment #9012528 -
Flags: review?(aklotz) → review-
Assignee | ||
Comment 79•6 years ago
|
||
> 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
Attachment #9012528 -
Attachment is obsolete: true
Attachment #9013079 -
Flags: review?(aklotz)
Comment 80•6 years ago
|
||
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
Attachment #9013079 -
Flags: review?(aklotz) → review+
Assignee | ||
Comment 81•6 years ago
|
||
Great thanks, apologies for the whitespace errors, was using a new editor and missed that they werent being stripped
Attachment #9013079 -
Attachment is obsolete: true
Attachment #9017137 -
Flags: review+
Assignee | ||
Comment 82•6 years ago
|
||
Green try run including TV - https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfa8ce1a55bd856f5d3584b60716a479a5a7ebd8
Comment 83•6 years ago
|
||
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c3a9571f63
Add support for native windows share. r=gijs, r=aklotz
Comment 84•6 years ago
|
||
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
Flags: needinfo?(dharvey)
Assignee | ||
Comment 85•6 years ago
|
||
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?
Flags: needinfo?(dharvey) → needinfo?(aklotz)
Comment 86•6 years ago
|
||
Call QueryInterface on your IDataPackage to resolve the IDataPackage2.
Flags: needinfo?(aklotz)
Assignee | ||
Comment 87•6 years ago
|
||
Great thanks, will run on try then rerequest review just in case
Attachment #9017137 -
Attachment is obsolete: true
Assignee | ||
Comment 88•6 years ago
|
||
And without trailing whitespace
Attachment #9017243 -
Attachment is obsolete: true
Assignee | ||
Comment 89•6 years ago
|
||
Attachment #9017244 -
Attachment is obsolete: true
Assignee | ||
Comment 90•6 years ago
|
||
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
Attachment #9017245 -
Flags: review?(aklotz)
Updated•6 years ago
|
Attachment #9017245 -
Flags: review?(aklotz) → review+
Assignee | ||
Comment 91•6 years ago
|
||
Thanks, second time lucky https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c4b4126c1ffc3dbbb64afda1e53270f38c89d02
Comment 92•6 years ago
|
||
Pushed by dharvey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f197a31d28f
Add support for native windows share. r=gijs, r=aklotz
Comment 93•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Updated•6 years ago
|
Updated•6 years ago
|
See Also: → 1573029
You need to log in
before you can comment on or make changes to this bug.
Description
•