Closed Bug 1472338 Opened 5 years ago Closed 9 months ago

Improve performance of `clipboard.readText()` by reading data from clipboard asynchronously

Categories

(Core :: DOM: Copy & Paste and Drag & Drop, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
99 Branch
Tracking Status
firefox99 --- fixed

People

(Reporter: annyG, Assigned: mbrodesser)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(4 files)

No description provided.
Assignee: nobody → agakhokidze
Attachment #8989236 - Flags: review?(enndeakin)
Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

https://reviewboard.mozilla.org/r/254284/#review261286

::: dom/events/DataTransfer.cpp:1553
(Diff revision 1)
> +    if (item.data().type() == IPCDataTransferData::TnsString) {
> +      const nsString& data = item.data().get_nsString();
> +      variant->SetAsAString(data);
> +    } else if (item.data().type() == IPCDataTransferData::TShmem) {
> +      auto data = item.data().get_Shmem();
> +      variant->SetAsACString(nsDependentCString(data.get<char>(), data.Size<char>()));

Do I have to call DeallocShmem here?

::: dom/events/DataTransfer.cpp:1563
(Diff revision 1)
> +    } else {
> +      continue;
> +    }
> +
> +    // We should hide this data from content if we have a file, and we aren't a file.
> +    bool hidden = XRE_IsContentProcess() ? hasFiles

I'm not 100% sure if this is right
Attachment #8989236 - Flags: review?(amarchesini) → review?(enndeakin)
Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

https://reviewboard.mozilla.org/r/254284/#review262964

::: dom/events/Clipboard.cpp:105
(Diff revision 2)
> +}
> +
> +
> +/* static */
> +void
> +Clipboard::HandleFilledDataTransfer(RefPtr<DataTransfer> aDataTransfer, RefPtr<Promise> aPromise,

Put each argument on a separate line since these lines are very long

::: dom/events/DataTransfer.cpp:1554
(Diff revision 2)
> +                                         RefPtr<DataTransfer> aDataTransfer)
> +{
> +  bool hasFiles = false;
> +  for (uint32_t j = 0; j < aIpcDataTransfer.items().Length() && !hasFiles; ++j) {
> +    if (aIpcDataTransfer.items()[j].data().type() == IPCDataTransferData::TIPCBlob) {
> +      hasFiles = true;

I'd find it clearer if you used 'break' here rather than checking hasFiles in the condition.

::: dom/events/DataTransfer.cpp:1577
(Diff revision 2)
> +      continue;
> +    }
> +
> +    // We should hide this data from content if we have a file, and we aren't a file.
> +    bool hidden = XRE_IsContentProcess() ? hasFiles
> +                  && item.data().type() != IPCDataTransferData::TIPCBlob : false;

Why this specific check? Maybe it doesn't matter. You only call ConvertFromIPCDataTransfer from the content process, so maybe that should be asserted at the beginning of the method, and the condition removed here.

::: dom/ipc/ContentParent.cpp:2724
(Diff revision 2)
>    DataTransfer::GetExternalClipboardFormats(aWhichClipboard, aPlainTextOnly, aTypes);
>    return IPC_OK();
>  }
>  
> +void
> +ContentParent::GetDataFromClipboard(nsTArray<nsCString>& aTypes,

This function should just return the result.

::: dom/ipc/ContentParent.cpp:2753
(Diff revision 2)
> +  clipboard->GetData(trans, aWhichClipboard);
> +
> +  // Fill IPC data transfer
> +  nsContentUtils::TransferableToIPCTransferable(trans, aDataTransfer,
> +                                                true, nullptr, this);
> +  return;

No need for 'return' here.

::: dom/ipc/ContentParent.cpp:2757
(Diff revision 2)
> +ContentParent::RecvGetClipboardAsync(const int32_t& aWhichClipboard, const bool& aPlainTextOnly,
> +                      GetClipboardAsyncResolver&& aResolver)

Fix up the indenting and put each argument on a separate line
What part of this are you looking for me to review?
Flags: needinfo?(agakhokidze)
Oops, sorry I should have specified. Neil suggested that I get an IPC peer to look at the IPC changes. So perhaps, ContentParent.cpp, ContentParent.h and PContent.ipdl ? Thanks!!
Flags: needinfo?(agakhokidze)
(In reply to Anny Gakhokidze [:annyG] from comment #3)
> Do I have to call DeallocShmem here?

It does look like you should, but I'm just guessing based on seeing the other places that DeallocShmem is called.
Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

https://reviewboard.mozilla.org/r/254284/#review263242

This looks reasonable to me. I assume you still need whatever existing sync API in some cases?

::: dom/events/DataTransfer.cpp:1553
(Diff revision 4)
> +DataTransfer::ConvertFromIPCDataTransfer(const IPCDataTransfer& aIpcDataTransfer,
> +                                         RefPtr<DataTransfer> aDataTransfer)
> +{
> +  // Verify this method is only called from content process
> +  MOZ_ASSERT(XRE_IsContentProcess());
> +  

nit: trailing whitespace

::: dom/events/DataTransfer.cpp:1569
(Diff revision 4)
> +    RefPtr<nsVariantCC> variant = new nsVariantCC();
> +    if (item.data().type() == IPCDataTransferData::TnsString) {
> +      const nsString& data = item.data().get_nsString();
> +      variant->SetAsAString(data);
> +    } else if (item.data().type() == IPCDataTransferData::TShmem) {
> +      auto data = item.data().get_Shmem();

As you pointed out, you probably need to dealloc the shmem in here somewhere.

::: dom/ipc/ContentParent.h:1094
(Diff revision 4)
>  
>  private:
> +  // Get data from clipboard and fill out IPC data transfer
> +  nsresult
> +  GetDataFromClipboard(nsTArray<nsCString>& aTypes,
> +                                    const int32_t& aWhichClipboard,

These should be lined up with the open paren in GetDataFromClipboard (or maybe they are and this is just getting squished down weirdly in my browser window).
Attachment #8989236 - Flags: review?(continuation) → review+
Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

https://reviewboard.mozilla.org/r/254284/#review263242

Yes, that's in another patch which hasn't landed yet, it's still in review.
Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

https://reviewboard.mozilla.org/r/254284/#review264900


Code analysis found 3 defects in this patch:
 - 3 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/events/DataTransfer.cpp:1549
(Diff revision 6)
>  }
>  
> +/* static */
> +void
> +DataTransfer::ConvertFromIPCDataTransfer(const IPCDataTransfer& aIpcDataTransfer,
> +                                         RefPtr<DataTransfer> aDataTransfer)

Warning: The parameter 'aDataTransfer' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                                         RefPtr<DataTransfer> aDataTransfer)
                                         ~~~~~~               ^
                                         const               &

::: dom/events/DataTransfer.cpp:1549
(Diff revision 6)
>  }
>  
> +/* static */
> +void
> +DataTransfer::ConvertFromIPCDataTransfer(const IPCDataTransfer& aIpcDataTransfer,
> +                                         RefPtr<DataTransfer> aDataTransfer)

Warning: The parameter 'aDataTransfer' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                                         RefPtr<DataTransfer> aDataTransfer)
                                         ~~~~~~               ^
                                         const               &

::: dom/events/DataTransfer.cpp:1549
(Diff revision 6)
>  }
>  
> +/* static */
> +void
> +DataTransfer::ConvertFromIPCDataTransfer(const IPCDataTransfer& aIpcDataTransfer,
> +                                         RefPtr<DataTransfer> aDataTransfer)

Warning: The parameter 'aDataTransfer' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                                         RefPtr<DataTransfer> aDataTransfer)
                                         ~~~~~~               ^
                                         const               &
Attachment #8989236 - Flags: review?(enndeakin)
Depends on: 1461465
Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

https://reviewboard.mozilla.org/r/254284/#review262966

::: dom/events/Clipboard.cpp:145
(Diff revisions 2 - 7)
> +         * because it was already filled. This means that if we call
> +         * DataTransfer::GetData on a data transfer that contains items in the
> +         * desired format then those items will already contain data and
> +         * there won't be an unnecessary call to DataTransferItem::FillInExternalData
> +         */
>          aDataTransfer->GetData(NS_LITERAL_STRING(kTextMime), str, aSubjectPrincipal, ier);

GetData will bail out fairly early on when the contains no data, so you should be ok to just call GetData unconditionally.
Attachment #8989236 - Flags: review?(enndeakin) → review+
Attachment #8999078 - Flags: review?(enndeakin)
Attachment #8989236 - Flags: review+ → review?(enndeakin)
Andrew and Neil, I have mixed up uploading of my patches and as a result it thinks that  Part 2 is the original patch and Part 1 is the new part. The only thing changed in original patch, which is now patch 1 is that I addressed Neil's comment 16. Will you kindly please r+ it as before? My apologies for the inconvenience 

And Neil, I have attached the chrome tests in Part 2, will you please review that? Thanks!!
Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

https://reviewboard.mozilla.org/r/254284/#review269196


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/events/Clipboard.cpp:106
(Diff revision 9)
> +
> +
> +/* static */
> +void
> +Clipboard::ProcessDataTransfer(RefPtr<DataTransfer> aDataTransfer,
> +                               RefPtr<Promise> aPromise,

Warning: The parameter 'aPromise' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                               RefPtr<Promise> aPromise,
                               ~~~~~~          ^
                               const          &
Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

https://reviewboard.mozilla.org/r/254284/#review269198


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/events/Clipboard.cpp:106
(Diff revision 12)
> +
> +
> +/* static */
> +void
> +Clipboard::ProcessDataTransfer(RefPtr<DataTransfer> aDataTransfer,
> +                               RefPtr<Promise> aPromise,

Warning: The parameter 'aPromise' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                               RefPtr<Promise> aPromise,
                               ~~~~~~          ^
                               const          &
Comment on attachment 8999078 [details]
Bug 1472338 - Part 1: Read data from clipboard asynchronously,

https://reviewboard.mozilla.org/r/261666/#review269200


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/events/Clipboard.cpp:106
(Diff revision 1)
> +
> +
> +/* static */
> +void
> +Clipboard::ProcessDataTransfer(RefPtr<DataTransfer> aDataTransfer,
> +                               RefPtr<Promise> aPromise,

Warning: The parameter 'aPromise' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

                               RefPtr<Promise> aPromise,
                               ~~~~~~          ^
                               const          &
Attachment #8989236 - Flags: review?(enndeakin) → review+
Comment on attachment 8999078 [details]
Bug 1472338 - Part 1: Read data from clipboard asynchronously,

https://reviewboard.mozilla.org/r/261666/#review269222
Attachment #8999078 - Flags: review?(enndeakin) → review+
Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

https://reviewboard.mozilla.org/r/254284/#review269224
Attachment #8989236 - Flags: review+
Comment on attachment 8999078 [details]
Bug 1472338 - Part 1: Read data from clipboard asynchronously,

https://reviewboard.mozilla.org/r/261666/#review269368

Sorry for the delay, I was on PTO.
Attachment #8999078 - Flags: review?(continuation) → review+
Blocks: 1619251
Component: DOM: Events → DOM: Copy & Paste and Drag & Drop

annyG: are you still working on this? Note that we're considering to expose readText to the web, see bug 1578321.

Flags: needinfo?(agakhokidze)
See Also: → 1578321

Unfortunately, I haven't had time to clean up and rebase the patches and get them to a state where someone would review them again. However, since Fission is winding down, I could possibly find time to look at them again in a month or two, unless someone else wanted to work on them in the meantime.

Flags: needinfo?(agakhokidze)
Assignee: agakhokidze → nobody
See Also: → 1744524
Blocks: 1744524
See Also: 1744524
Assignee: nobody → mbrodesser
Depends on: 1755481
See Also: → 1755863

Limiting the scope of this bug to clipboard.readText(), because clipboard.read() seems to require significantly more work and making only readText async allows to unblock bug 1744524 and prevent the already old patches from bitrotting more.

Summary: Improve performance of Clipboard::Read and Clipboard::ReadText by reading data from clipboard asynchronously → Improve performance of `lipboard.readText()` by reading data from clipboard asynchronously
Summary: Improve performance of `lipboard.readText()` by reading data from clipboard asynchronously → Improve performance of `clipboard.readText()` by reading data from clipboard asynchronously
See Also: → 666254
See Also: 6662541375022
Keywords: perf
Depends on: 1756164
Attachment #9263747 - Attachment description: WIP: Bug 1472338: part 1) Add Chrome tests for the async Clipboard API → Bug 1472338: part 1) Add Chrome tests for the async Clipboard API. r=mccr8!,NeilDeakin
Attachment #9264845 - Attachment description: Bug 1472338: part 2) Change `clipboard.readText()` to read from the clipboard asynchronously when called from content. r=mccr8!,NeilDeakin → Bug 1472338: part 2) Change `clipboard.readText()` to read from the clipboard asynchronously when called from content. r=mccr8,NeilDeakin!
Pushed by mbrodesser@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/80498424f3a8
part 1) Add Chrome tests for the async Clipboard API. r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/7e08fd5b9010
part 2) Change `clipboard.readText()` to read from the clipboard asynchronously when called from content. r=NeilDeakin,mccr8
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → 99 Branch
You need to log in before you can comment on or make changes to this bug.