1755863 - Improve performance of `Clipboard::Read` by reading data from clipboard asynchronously

Status: RESOLVED FIXED

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

Description

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

Implementation advice:

• There's a WPT for read()ing a ClipboardItem with two types from the content process.
• Some to-be-written async version of FillAllExternalData will have to be called.
• Converting IPCDataTransfer objects to DataTransfer objects might need to cover what https://searchfox.org/mozilla-central/rev/68a5327697ec43aa55b458c504c4b313c9c80528/dom/ipc/ContentChild.cpp#3206-3241 covers too. https://searchfox.org/mozilla-central/rev/211649f071259c4c733b4cafa94c44481c5caacc/dom/base/nsContentUtils.cpp#7528 could be relevant too.

Comment 1

[Edgar Chen [:edgar]]

Attachment: WIP: Bug 1755863 - Part 1: Add async API for getting matching data flavors on nsIClipboard;

Comment 2

[Edgar Chen [:edgar]]

Attachment: Bug 1755863 - Part 1: Add async API for getting data on nsIClipboard; r?NeilDeakin

Depends on D145377

Comment 3

[Edgar Chen [:edgar]]

Attachment: Bug 1755863 - Part 3-2: Use async nsClipboard API to implement clipboard.read(); r?NeilDeakin!,nika!

Depends on D145378

Comment 4

[Edgar Chen [:edgar]]

Attachment: Bug 1755863 - Part 4: Use async nsClipboard API to implement clipboard.readText(); r?NeilDeakin!,mbrodesser!

Depends on D145379

Comment 5

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18903553f8c55bfe7a64e912fe4f8aa89f4829ab

Comment 6

[Edgar Chen [:edgar]]

Hi Neil, in this bug, we would like to use async IPC to get the clipboard data from OS for clipboard.read() (we have done a similar thing for clipboard.readText() in bug 1472338). To achieve that, I introduce some async API in nsIClipboard, and make web Clipboard implementation use those nsIClipboard API directly without going through DataTransfer. Would you mind taking a look? It would be appreciated if you could give some feedback on the WIP patches.

Comment 7

[Neil Deakin]

I can take a look in more detail if you like, but some initial feedback:

1. The widget changes in the first patch don't seem particularly necessary. The apis that get formats/flavours aren't asynchronous in any os, that I know of, nor should it have any performance issue, so I think we only need to use a asynchronous api here to wait for the format info to pass across our content to parent process boundary and can just use the existing clipboard methods in the parent process.
2. Is the code in the third patch in ReadHelper intended to be limited to specific formats? Is this api limited in what formats it supports?
3. We shouldn't be calling clipboard->AsyncGetData in the ClipboardItem constructor -- it should be called in ClipboardItem::GetType.

The general design looks ok though.

Comment 8

[Edgar Chen [:edgar]]

(In reply to Neil Deakin from comment #7)

I can take a look in more detail if you like, but some initial feedback:

1. The widget changes in the first patch don't seem particularly necessary. The apis that get formats/flavours aren't asynchronous in any os, that I know of, nor should it have any performance issue, so I think we only need to use a asynchronous api here to wait for the format info to pass across our content to parent process boundary and can just use the existing clipboard methods in the parent process.

But if we only use the asynchronous API in content process, which means we need to call different nsIClipboard API on parent process and content process in DOM clipboard code, which I would like to avoid doing so.

2. Is the code in the third patch in ReadHelper intended to be limited to specific formats? Is this api limited in what formats it supports?

Yes, we intended to do that for now, Blink and Webkit only support those mime types on clipboard API, too. We could support more later if needed.

3. We shouldn't be calling clipboard->AsyncGetData in the ClipboardItem constructor -- it should be called in ClipboardItem::GetType.

Actually, this is what we expect. Spec also defines the system clipboard data is read when read() is called, https://w3c.github.io/clipboard-apis/#dom-clipboard-read.

Comment 9

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #8)

(In reply to Neil Deakin from comment #7)

1. The widget changes in the first patch don't seem particularly necessary. The apis that get formats/flavours aren't asynchronous in any os, that I know of, nor should it have any performance issue, so I think we only need to use a asynchronous api here to wait for the format info to pass across our content to parent process boundary and can just use the existing clipboard methods in the parent process.

But if we only use the asynchronous API in content process, which means we need to call different nsIClipboard API on parent process and content process in DOM clipboard code, which I would like to avoid doing so.

On a second thought, we could use the existing API to get formats/flavors first, we could do that later if we figure out it is necessary.

Comment 10

[Edgar Chen [:edgar]]

(rebase over bug 1744524)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4d339e24dd49d9e9bcc70a8399aa5fd4e97f1c5

Comment 11

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b2b4766637df8f9a3831b72b3e17d0ed910a9c1

Comment 12

[Edgar Chen [:edgar]]

Attachment: Bug 1755863 - Part 3-1: Make ItemEntry a reference counted object; r?NeilDeakin,nika

Depends on D145378

Comment 13

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4584b7de93b8f6247140cab2b9475f57ed93d671

Comment 14

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=21c25ed4939bb8121e0522e753e42aed9d5316a0

Comment 15

[Edgar Chen [:edgar]]

Attachment: Bug 1755863 - Part 2: Add async API/IPC for getting matching data flavors from clipboard; r?nika!

Depends on D145378

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65e1dba3f3ec
Part 1: Add async API for getting data on nsIClipboard; r=geckoview-reviewers,NeilDeakin,m_kato,nika
https://hg.mozilla.org/integration/autoland/rev/b318f0bf426e
Part 2: Add async API/IPC for getting matching data flavors from clipboard; r=nika,geckoview-reviewers,m_kato
https://hg.mozilla.org/integration/autoland/rev/1c15d67cce05
Part 3-1: Make ItemEntry a reference counted object; r=nika
https://hg.mozilla.org/integration/autoland/rev/4e8b144ba280
Part 3-2: Use async nsClipboard API to implement clipboard.read(); r=NeilDeakin,nika
https://hg.mozilla.org/integration/autoland/rev/2102746fdbcb
Part 4: Use async nsClipboard API to implement clipboard.readText(); r=NeilDeakin,mbrodesser

Comment 17

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/65e1dba3f3ec
https://hg.mozilla.org/mozilla-central/rev/b318f0bf426e
https://hg.mozilla.org/mozilla-central/rev/1c15d67cce05
https://hg.mozilla.org/mozilla-central/rev/4e8b144ba280
https://hg.mozilla.org/mozilla-central/rev/2102746fdbcb