From 2dd7f27175101b2700cafae435bb70285119e718 Mon Sep 17 00:00:00 2001 From: Edgar Chen Date: Mon, 26 Jun 2023 21:02:40 +0000 Subject: [PATCH] Bug 1810850 - Part 2: Move clipboard cache code for GetData to nsBaseClipboard; r=cmartin,mstange This patch introduce a preference for getting the data from cache directly which is enabled on Mac only. And now we really support getting data from cache for each type simultaneously, instead of only using the one that has the latest cached transferable. Depends on D178777 Differential Revision: https://phabricator.services.mozilla.com/D179993 --- modules/libpref/init/StaticPrefList.yaml | 10 +++ widget/cocoa/nsClipboard.mm | 35 ++-------- widget/nsBaseClipboard.cpp | 84 ++++++++++++++++++++---- widget/nsBaseClipboard.h | 9 ++- widget/tests/clipboard_helper.js | 37 ++++++++++- widget/tests/test_clipboard_cache.xhtml | 75 +++++++++++++++++++-- widget/windows/nsClipboard.cpp | 10 ++- 7 files changed, 202 insertions(+), 58 deletions(-) diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index 8da973daaae8..69f7287dbfd3 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/modules/libpref/init/StaticPrefList.yaml @@ -15670,6 +15670,16 @@ value: true mirror: once +# Whether the clipboard cached are used while getting system clipboard data. +- name: widget.clipboard.use-cached-data.enabled + type: bool +#if defined(XP_MACOSX) + value: true +#else + value: false +#endif + mirror: always + #--------------------------------------------------------------------------- # Prefs starting with "zoom." #--------------------------------------------------------------------------- diff --git a/widget/cocoa/nsClipboard.mm b/widget/cocoa/nsClipboard.mm index f3183b533086..766401e109fd 100644 --- a/widget/cocoa/nsClipboard.mm +++ b/widget/cocoa/nsClipboard.mm @@ -323,8 +323,10 @@ NS_IMETHODIMP nsClipboard::GetNativeClipboardData(nsITransferable* aTransferable, int32_t aWhichClipboard) { NS_OBJC_BEGIN_TRY_BLOCK_RETURN; - if ((aWhichClipboard != kGlobalClipboard && aWhichClipboard != kFindClipboard) || !aTransferable) - return NS_ERROR_FAILURE; + MOZ_DIAGNOSTIC_ASSERT(aTransferable); + // XXX we only support the set operation on kSelectionCache type, see bug 1835059. + MOZ_DIAGNOSTIC_ASSERT(kSelectionCache != aWhichClipboard); + MOZ_DIAGNOSTIC_ASSERT(nsIClipboard::IsClipboardTypeSupported(aWhichClipboard)); NSPasteboard* cocoaPasteboard = GetPasteboard(aWhichClipboard); if (!cocoaPasteboard) { @@ -335,35 +337,10 @@ nsClipboard::GetNativeClipboardData(nsITransferable* aTransferable, int32_t aWhi // conversion) nsTArray flavors; nsresult rv = aTransferable->FlavorsTransferableCanImport(flavors); - if (NS_FAILED(rv)) return NS_ERROR_FAILURE; - - // If we were the last ones to put something on the pasteboard, then just use the cached - // transferable. Otherwise clear it because it isn't relevant any more. - if (mCachedClipboard == aWhichClipboard) { - const auto& clipboardCache = mCaches[aWhichClipboard]; - MOZ_ASSERT(clipboardCache); - if (mChangeCount == [cocoaPasteboard changeCount]) { - if (nsITransferable* cachedTrans = clipboardCache->GetTransferable()) { - for (uint32_t i = 0; i < flavors.Length(); i++) { - nsCString& flavorStr = flavors[i]; - - nsCOMPtr dataSupports; - rv = cachedTrans->GetTransferData(flavorStr.get(), getter_AddRefs(dataSupports)); - if (NS_SUCCEEDED(rv)) { - aTransferable->SetTransferData(flavorStr.get(), dataSupports); - return NS_OK; // maybe try to fill in more types? Is there a point? - } - } - } - } else { - // Remove transferable cache only. Don't clear system clipboard. - clipboardCache->Clear(); - } + if (NS_FAILED(rv)) { + return NS_ERROR_FAILURE; } - // at this point we can't satisfy the request from cache data so let's look - // for things other people put on the system clipboard - return nsClipboard::TransferableFromPasteboard(aTransferable, cocoaPasteboard); NS_OBJC_END_TRY_BLOCK_RETURN(NS_ERROR_FAILURE); diff --git a/widget/nsBaseClipboard.cpp b/widget/nsBaseClipboard.cpp index b36b584371c8..dc9fd8f4cc52 100644 --- a/widget/nsBaseClipboard.cpp +++ b/widget/nsBaseClipboard.cpp @@ -5,6 +5,7 @@ #include "nsBaseClipboard.h" +#include "mozilla/StaticPrefs_widget.h" #include "nsIClipboardOwner.h" #include "nsError.h" #include "nsXPCOM.h" @@ -204,25 +205,60 @@ NS_IMETHODIMP nsBaseClipboard::SetData(nsITransferable* aTransferable, } /** - * Gets the transferable object - * + * Gets the transferable object from system clipboard. */ NS_IMETHODIMP nsBaseClipboard::GetData(nsITransferable* aTransferable, int32_t aWhichClipboard) { - NS_ASSERTION(aTransferable, "clipboard given a null transferable"); + CLIPBOARD_LOG("%s: clipboard=%d", __FUNCTION__, aWhichClipboard); - CLIPBOARD_LOG("%s", __FUNCTION__); - - if (!nsIClipboard::IsClipboardTypeSupported(kSelectionClipboard) && - !nsIClipboard::IsClipboardTypeSupported(kFindClipboard) && - aWhichClipboard != kGlobalClipboard) { + if (!aTransferable) { + NS_ASSERTION(false, "clipboard given a null transferable"); return NS_ERROR_FAILURE; } - if (aTransferable) - return GetNativeClipboardData(aTransferable, aWhichClipboard); + // XXX as of now, we only support the set operation on kSelectionCache type, + // should we also support the get operation? See also bug 1835059. + if (kSelectionCache == aWhichClipboard || + !nsIClipboard::IsClipboardTypeSupported(aWhichClipboard)) { + CLIPBOARD_LOG("%s: clipboard %d is not supported.", __FUNCTION__, + aWhichClipboard); + return NS_ERROR_FAILURE; + } - return NS_ERROR_FAILURE; + if (mozilla::StaticPrefs::widget_clipboard_use_cached_data_enabled()) { + // If we were the last ones to put something on the navtive clipboard, then + // just use the cached transferable. Otherwise clear it because it isn't + // relevant any more. + if (auto* clipboardCache = GetClipboardCacheIfValid(aWhichClipboard)) { + MOZ_ASSERT(clipboardCache->GetTransferable()); + + // get flavor list that includes all acceptable flavors (including ones + // obtained through conversion) + nsTArray flavors; + nsresult rv = aTransferable->FlavorsTransferableCanImport(flavors); + if (NS_FAILED(rv)) { + return NS_ERROR_FAILURE; + } + + for (const auto& flavor : flavors) { + nsCOMPtr dataSupports; + rv = clipboardCache->GetTransferable()->GetTransferData( + flavor.get(), getter_AddRefs(dataSupports)); + if (NS_SUCCEEDED(rv)) { + CLIPBOARD_LOG("%s: getting %s from cache.", __FUNCTION__, + flavor.get()); + aTransferable->SetTransferData(flavor.get(), dataSupports); + // maybe try to fill in more types? Is there a point? + return NS_OK; + } + } + } + + // at this point we can't satisfy the request from cache data so let's look + // for things other people put on the system clipboard + } + + return GetNativeClipboardData(aTransferable, aWhichClipboard); } RefPtr nsBaseClipboard::AsyncGetData( @@ -301,6 +337,32 @@ nsBaseClipboard::IsClipboardTypeSupported(int32_t aWhichClipboard, } } +nsBaseClipboard::ClipboardCache* nsBaseClipboard::GetClipboardCacheIfValid( + int32_t aClipboardType) { + MOZ_ASSERT(nsIClipboard::IsClipboardTypeSupported(aClipboardType)); + + const mozilla::UniquePtr& cache = mCaches[aClipboardType]; + MOZ_ASSERT(cache); + + if (!cache->GetTransferable()) { + MOZ_ASSERT(cache->GetSequenceNumber() == -1); + return nullptr; + } + + auto changeCountOrError = GetNativeClipboardSequenceNumber(aClipboardType); + if (changeCountOrError.isErr()) { + return nullptr; + } + + if (changeCountOrError.unwrap() != cache->GetSequenceNumber()) { + // Clipboard cache is invalid, clear it. + cache->Clear(); + return nullptr; + } + + return cache.get(); +} + void nsBaseClipboard::ClipboardCache::Clear() { if (mClipboardOwner) { mClipboardOwner->LosingOwnership(mTransferable); diff --git a/widget/nsBaseClipboard.h b/widget/nsBaseClipboard.h index a7e2501ca48e..43b7ecc9d797 100644 --- a/widget/nsBaseClipboard.h +++ b/widget/nsBaseClipboard.h @@ -106,7 +106,7 @@ class nsBaseClipboard : public ClipboardSetDataHelper { NS_IMETHOD SetData(nsITransferable* aTransferable, nsIClipboardOwner* anOwner, int32_t aWhichClipboard) override final; NS_IMETHOD GetData(nsITransferable* aTransferable, - int32_t aWhichClipboard) override; + int32_t aWhichClipboard) override final; NS_IMETHOD EmptyClipboard(int32_t aWhichClipboard) override; NS_IMETHOD HasDataMatchingFlavors(const nsTArray& aFlavorList, int32_t aWhichClipboard, @@ -114,7 +114,7 @@ class nsBaseClipboard : public ClipboardSetDataHelper { NS_IMETHOD IsClipboardTypeSupported(int32_t aWhichClipboard, bool* aRetval) override final; RefPtr AsyncGetData( - nsITransferable* aTransferable, int32_t aWhichClipboard) override; + nsITransferable* aTransferable, int32_t aWhichClipboard) override final; RefPtr AsyncHasDataMatchingFlavors( const nsTArray& aFlavorList, int32_t aWhichClipboard) override; @@ -149,6 +149,7 @@ class nsBaseClipboard : public ClipboardSetDataHelper { } nsITransferable* GetTransferable() const { return mTransferable; } nsIClipboardOwner* GetClipboardOwner() const { return mClipboardOwner; } + int32_t GetSequenceNumber() const { return mSequenceNumber; } private: nsCOMPtr mTransferable; @@ -160,6 +161,10 @@ class nsBaseClipboard : public ClipboardSetDataHelper { bool mEmptyingForSetData = false; private: + // Return clipboard cache if the cached data is valid, otherwise clear the + // cached data and returns null. + ClipboardCache* GetClipboardCacheIfValid(int32_t aClipboardType); + const mozilla::dom::ClipboardCapabilities mClipboardCaps; bool mIgnoreEmptyNotification = false; }; diff --git a/widget/tests/clipboard_helper.js b/widget/tests/clipboard_helper.js index 7315bd19ac26..aa229a69a772 100644 --- a/widget/tests/clipboard_helper.js +++ b/widget/tests/clipboard_helper.js @@ -24,6 +24,32 @@ function generateRandomString() { return "random number: " + Math.random(); } +function generateNewTransferable(aFlavor, aStr) { + let trans = Cc["@mozilla.org/widget/transferable;1"].createInstance( + Ci.nsITransferable + ); + trans.init(null); + trans.addDataFlavor(aFlavor); + + let supportsStr = Cc["@mozilla.org/supports-string;1"].createInstance( + Ci.nsISupportsString + ); + supportsStr.data = aStr; + trans.setTransferData(aFlavor, supportsStr); + + return trans; +} + +function addStringToTransferable(aFlavor, aStr, aTrans) { + aTrans.addDataFlavor(aFlavor); + + let supportsStr = Cc["@mozilla.org/supports-string;1"].createInstance( + Ci.nsISupportsString + ); + supportsStr.data = aStr; + aTrans.setTransferData(aFlavor, supportsStr); +} + function writeStringToClipboard( aStr, aFlavor, @@ -77,7 +103,12 @@ function getClipboardData(aFlavor, aClipboardType) { trans.addDataFlavor(aFlavor); clipboard.getData(trans, aClipboardType); - var data = {}; - trans.getTransferData(aFlavor, data); - return data.value.QueryInterface(SpecialPowers.Ci.nsISupportsString).data; + try { + var data = {}; + trans.getTransferData(aFlavor, data); + return data.value.QueryInterface(SpecialPowers.Ci.nsISupportsString).data; + } catch (ex) { + // If the clipboard is empty getTransferData will throw. + return null; + } } diff --git a/widget/tests/test_clipboard_cache.xhtml b/widget/tests/test_clipboard_cache.xhtml index 39cf8113dd65..5cf701ed51a6 100644 --- a/widget/tests/test_clipboard_cache.xhtml +++ b/widget/tests/test_clipboard_cache.xhtml @@ -20,10 +20,69 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1812543 diff --git a/widget/windows/nsClipboard.cpp b/widget/windows/nsClipboard.cpp index 637cf73891ed..08273348139f 100644 --- a/widget/windows/nsClipboard.cpp +++ b/widget/windows/nsClipboard.cpp @@ -1318,16 +1318,14 @@ bool nsClipboard ::IsInternetShortcut(const nsAString& inFileName) { NS_IMETHODIMP nsClipboard::GetNativeClipboardData(nsITransferable* aTransferable, int32_t aWhichClipboard) { + MOZ_DIAGNOSTIC_ASSERT(aTransferable); + MOZ_DIAGNOSTIC_ASSERT( + nsIClipboard::IsClipboardTypeSupported(aWhichClipboard)); + MOZ_LOG(gWin32ClipboardLog, LogLevel::Debug, ("%s aWhichClipboard=%i", __FUNCTION__, aWhichClipboard)); - // make sure we have a good transferable - if (!aTransferable || aWhichClipboard != kGlobalClipboard) { - return NS_ERROR_FAILURE; - } - nsresult res; - // This makes sure we can use the OLE functionality for the clipboard IDataObject* dataObj; if (S_OK == RepeatedlyTryOleGetClipboard(&dataObj)) {