1470540 - Improve performance of DataTransfer::CacheExternalClipboardFormats by retrieving supported clipboard formats at once

Status: RESOLVED FIXED

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

Comment 1

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1470540 - Improve performance of DataTransfer::CacheExternalClipboardFormats,

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

Comment 2

[Anny Gakhokidze [:annyG]]

Comment on attachment 8987212 [details]
Bug 1470540 - Improve performance of DataTransfer::CacheExternalClipboardFormats,

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

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8987212 [details]
Bug 1470540 - Improve performance of DataTransfer::CacheExternalClipboardFormats,

https://reviewboard.mozilla.org/r/252466/#review259120

This patch must be reviewed by an IPC peer because of the sync IPC call.

::: dom/events/DataTransfer.cpp:617
(Diff revision 2)
> +/* static */
> +void
> +DataTransfer::GetExternalClipboardFormats(const int32_t& aWhichClipboard,
> +                                         const bool& aPlainTextOnly,
> +                                         nsTArray<nsCString>* aResult)
> +{

MOZ_ASSERT(aResult)

::: dom/events/DataTransfer.cpp:624
(Diff revision 2)
> +    do_GetService("@mozilla.org/widget/clipboard;1");
> +  if (!clipboard || aWhichClipboard < 0) {
> +    return;
> +  }
> +
> +  auto typesToCheck = { kUnicodeMime };

no auto here.

::: dom/events/DataTransfer.cpp:629
(Diff revision 2)
> +  auto typesToCheck = { kUnicodeMime };
> +
> +  // If not plain text only, then instead check all the other types
> +  if (!aPlainTextOnly) {
> +    typesToCheck = { kCustomTypesMime, kFileMime, kHTMLMime, kRTFMime,
> +                     kURLMime, kURLDataMime, kUnicodeMime, kPNGImageMime };

this array and the previous one should be static and used here as reference.

::: dom/events/DataTransfer.cpp:634
(Diff revision 2)
> +                     kURLMime, kURLDataMime, kUnicodeMime, kPNGImageMime };
> +  }
> +
> +  for (auto typeToCheck : typesToCheck) {
> +    bool hasType = false;
> +    const char * b[] = {typeToCheck};

better name than 'b'?

::: dom/events/DataTransfer.cpp:635
(Diff revision 2)
> +  }
> +
> +  for (auto typeToCheck : typesToCheck) {
> +    bool hasType = false;
> +    const char * b[] = {typeToCheck};
> +    clipboard->HasDataMatchingFlavors(b, /* number of flavors to check */ 1,

check the return value here.

::: dom/events/DataTransfer.cpp:636
(Diff revision 2)
> +
> +  for (auto typeToCheck : typesToCheck) {
> +    bool hasType = false;
> +    const char * b[] = {typeToCheck};
> +    clipboard->HasDataMatchingFlavors(b, /* number of flavors to check */ 1,
> +                                    aWhichClipboard, &hasType);

indentation

::: dom/events/DataTransfer.cpp:1391
(Diff revision 2)
> +  } else {
> +    GetExternalClipboardFormats(mClipboardType, aPlainTextOnly, &typesArray);
> +  }
> +
>    if (aPlainTextOnly) {
> -    bool supported;
> +    // The only thing that will be in types is kUnicodeMime

well.. in theory MOZ_ASSERT(typeArray.IsEmpty() || typeArray.Length() == 1). right?

::: dom/ipc/ContentParent.cpp:2704
(Diff revision 2)
>    delete [] typesChrs;
>    return IPC_OK();
>  }
>  
>  mozilla::ipc::IPCResult
> +ContentParent::RecvGetExternalClipboardFormats(const int32_t& aWhichClipboard, 

extra space

::: ipc/ipdl/sync-messages.ini:872
(Diff revision 2)
>  description =
>  [PContent::GetClipboard]
>  description =
>  [PContent::ClipboardHasType]
>  description =
> +[PContent::GetExternalClipboardFormats]

you need a r+ from an IPC peer.

Comment 4

[Anny Gakhokidze [:annyG]]

Comment on attachment 8987212 [details]
Bug 1470540 - Improve performance of DataTransfer::CacheExternalClipboardFormats,

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

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8987212 [details]
Bug 1470540 - Improve performance of DataTransfer::CacheExternalClipboardFormats,

https://reviewboard.mozilla.org/r/252466/#review259750

This patch needs a commit message that explains what it is doing, and why. You should also explain why you need a sync message in the commit message, so I can figure out if it is really needed or not. You should also put something in the description field in the sync-message whitelist file.

Comment 6

[Andrew McCreight [:mccr8]]

My high-level comment here is that we really, really want to avoid sync messages because they can cause severe performance problems, and are hard to undo, so this needs some strong justification, or an explanation like maybe you are turning one kind of sync message into another, so there's no increase. Maybe that is all obvious, but I don't know anything about the clipboard.

Comment 7

[Anny Gakhokidze [:annyG]]

Comment on attachment 8987212 [details]
Bug 1470540 - Improve performance of DataTransfer::CacheExternalClipboardFormats,

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

Comment 8

[Anny Gakhokidze [:annyG]]

Andrew, I have added a better commit message and a description to sync-messages whitelist file. Please let me know if the explanation needs to be expanded! Thanks.

Comment 9

[Anny Gakhokidze [:annyG]]

Comment on attachment 8987212 [details]
Bug 1470540 - Improve performance of DataTransfer::CacheExternalClipboardFormats,

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

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8987212 [details]
Bug 1470540 - Improve performance of DataTransfer::CacheExternalClipboardFormats,

https://reviewboard.mozilla.org/r/252464/#review260496

Thanks for the explanation! As a minor nitpick, the commit message should be broken up into multiple lines (around, say, 80 characters wide) rather than being just a single continuous line. Here's an example of a commit message that demonstrates what I mean: https://hg.mozilla.org/mozilla-central/rev/4425a51bc165

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8987212 [details]
Bug 1470540 - Improve performance of DataTransfer::CacheExternalClipboardFormats,

https://reviewboard.mozilla.org/r/252466/#review260498

Comment 12

[Anny Gakhokidze [:annyG]]

Comment on attachment 8987212 [details]
Bug 1470540 - Improve performance of DataTransfer::CacheExternalClipboardFormats,

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

Comment 13

[Anny Gakhokidze [:annyG]]

Comment on attachment 8987212 [details]
Bug 1470540 - Improve performance of DataTransfer::CacheExternalClipboardFormats,

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61dc09155864
Improve performance of DataTransfer::CacheExternalClipboardFormats, r=baku,mccr8

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/61dc09155864