Open Bug 1472338 Opened 2 years ago Updated 9 months ago

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

Categories

(Core :: DOM: Events, enhancement, P3)

enhancement

Tracking

()

People

(Reporter: annyG, Assigned: annyG)

References

(Blocks 1 open bug)

Details

Attachments

(2 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
You need to log in before you can comment on or make changes to this bug.