Closed Bug 1470540 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
DOM: Events
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: annyG, Assigned: annyG)

Details

Attachments

(1 file)

Bug 1470540 - Improve performance of DataTransfer::CacheExternalClipboardFormats,
59 bytes, text/x-review-board-request
baku
: review+
mccr8
: review+
Details 
      No description provided.
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.
Attachment #8987212 - Flags: review?(amarchesini) → review+
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.
Attachment #8987212 - Flags: review?(continuation) → review-
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.
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 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 on attachment 8987212 [details]
Bug 1470540 - Improve performance of DataTransfer::CacheExternalClipboardFormats,

https://reviewboard.mozilla.org/r/252466/#review260498
Attachment #8987212 - Flags: review?(continuation) → review+
Pushed by nlayzell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/61dc09155864
Improve performance of DataTransfer::CacheExternalClipboardFormats, r=baku,mccr8
https://hg.mozilla.org/mozilla-central/rev/61dc09155864
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: