Anny Gakhokidze [:annyG] Reporter
	Description • 6 years ago

      No description provided.

	Anny Gakhokidze [:annyG] Reporter
	Comment 3 • 6 years ago
mozreview-review

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

	Neil Deakin
	Comment 4 • 6 years ago
mozreview-review

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

	Andrew McCreight [:mccr8]
	Comment 7 • 6 years ago

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

	Anny Gakhokidze [:annyG] Reporter
	Comment 8 • 6 years ago

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

	Andrew McCreight [:mccr8]
	Comment 9 • 6 years ago

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

	Andrew McCreight [:mccr8]
	Comment 10 • 6 years ago
mozreview-review

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

	Anny Gakhokidze [:annyG] Reporter
	Comment 12 • 6 years ago
mozreview-review-reply

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.

	Code Review Bot [:reviewbot]
	Comment 14 • 6 years ago
mozreview-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               &

	Neil Deakin
	Comment 16 • 6 years ago
mozreview-review

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.

	Anny Gakhokidze [:annyG] Reporter
	Comment 24 • 6 years ago

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

	Code Review Bot [:reviewbot]
	Comment 25 • 6 years ago
mozreview-review

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          &

	Code Review Bot [:reviewbot]
	Comment 26 • 6 years ago
mozreview-review

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          &

	Code Review Bot [:reviewbot]
	Comment 27 • 6 years ago
mozreview-review

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          &

	Neil Deakin
	Comment 28 • 6 years ago
mozreview-review

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

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

	Neil Deakin
	Comment 29 • 6 years ago
mozreview-review

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

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

	Andrew McCreight [:mccr8]
	Comment 30 • 6 years ago
mozreview-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.

	Pulsebot
	Comment 36 • 2 years ago

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