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

Status: RESOLVED FIXED

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

Comment 1

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

Improve performance of Clipboard::Read and Clipboard::ReadText by
reading data from clipboard asynchronously.

Review commit: https://reviewboard.mozilla.org/r/254284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254284/

Comment 2

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254284/diff/1-2/

Comment 3

[Anny Gakhokidze [:annyG]]

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

Comment 4

[Neil Deakin]

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

Comment 5

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254284/diff/2-3/

Comment 6

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254284/diff/3-4/

Comment 7

[Andrew McCreight [:mccr8]]

What part of this are you looking for me to review?

Comment 8

[Anny Gakhokidze [:annyG]]

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!!

Comment 9

[Andrew McCreight [:mccr8]]

(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 10

[Andrew McCreight [:mccr8]]

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).

Comment 11

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254284/diff/4-5/

Comment 12

[Anny Gakhokidze [:annyG]]

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 13

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254284/diff/5-6/

Comment 14

[Code Review Bot [:reviewbot]]

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               &

Comment 15

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254284/diff/6-7/

Comment 16

[Neil Deakin]

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.

Comment 17

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254284/diff/7-8/

Comment 18

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254284/diff/8-9/

Comment 19

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254284/diff/9-10/

Comment 20

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254284/diff/10-11/

Comment 21

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254284/diff/11-12/

Comment 22

[Anny Gakhokidze [:annyG]]

Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254284/diff/12-13/

Comment 23

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1472338 - Part 1: Read data from clipboard asynchronously,

Improve performance of Clipboard::Read and Clipboard::ReadText by
reading data from clipboard asynchronously.

Review commit: https://reviewboard.mozilla.org/r/261666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261666/

Comment 24

[Anny Gakhokidze [:annyG]]

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 25

[Code Review Bot [:reviewbot]]

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 26

[Code Review Bot [:reviewbot]]

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 27

[Code Review Bot [:reviewbot]]

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          &

Comment 28

[Neil Deakin]

Comment on attachment 8999078 [details]
Bug 1472338 - Part 1: Read data from clipboard asynchronously,

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

Comment 29

[Neil Deakin]

Comment on attachment 8989236 [details]
Bug 1472338 - Part 2: Add chrome tests for navigator.clipboard* APIs,

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

Comment 30

[Andrew McCreight [:mccr8]]

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.

Comment 31

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

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

Comment 32

[Anny Gakhokidze [:annyG]]

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.

Comment 33

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1472338: part 1) Add Chrome tests for the async Clipboard API. r=mccr8!,NeilDeakin

Comment 34

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

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.

Comment 35

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

Attachment: Bug 1472338: part 2) Change `clipboard.readText()` to read from the clipboard asynchronously when called from content. r=mccr8,NeilDeakin!

This revives an old patch, see the bug ticket.

Depends on D138660

Comment 36

[Pulsebot]

Pushed by [e-mail]:
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

Comment 37

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/80498424f3a8
https://hg.mozilla.org/mozilla-central/rev/7e08fd5b9010