Allow sending a port message and receive a reply
ClosedPublic

Authored by broulik on Sun, Aug 11, 4:22 PM.

Details

Summary

Instead of manually matching a sent message to a port message, allow adding a serial which is then used to relate a reply back so you just get a neat Promise-based API.
For compatibility, the existing method cannot be changed, as with new extension and older host I will never get a reply, and I cannot tell whether the caller actually asked for the promise or not and if they didn't we would leak the promise resolvers as we never got a reply.

Test Plan

Placed a sendReply call in a handleData

sendPortMessageWithReply("settings", "getSubsystemStatus").then((message) => {
    console.log("got the status", message);
});

Diff Detail

Repository
R856 Plasma Browser Integration
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik created this revision.Sun, Aug 11, 4:22 PM
Restricted Application added a project: Plasma. · View Herald TranscriptSun, Aug 11, 4:22 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Sun, Aug 11, 4:22 PM
broulik updated this revision to Diff 63560.Sun, Aug 11, 6:21 PM
  • Fix if condition
broulik updated this revision to Diff 63595.Mon, Aug 12, 8:33 AM
  • Let plugin access the serial so it can store it and send a reply asynchronously later
fvogt added a comment.Mon, Aug 12, 6:23 PM

IMHO the member variable is really ugly. Messages with and without serial number have to be handled differently anyway, so why not introuce a new handleMessage(event, json, serial) method?

extension/extension-utils.js
65

Add a manual wrap before INT32_MAX, just to be safe

extension/extension.js
108–111

You could make 0 a valid serial, currently -1 and 0 are both used as flag values but only one is really needed

host/abstractbrowserplugin.cpp
66

IMO printing a warning and maybe not doing anything is more fitting here than an assert which only works in debug builds and is then fatal.

broulik updated this revision to Diff 63624.Mon, Aug 12, 8:27 PM
  • address review comments

there's now a handleData overload which takes a serial and returns a QJsonObject. You can just return a QJsonObject to reply immediately or store the serial and call sendReply explicitly later

broulik added inline comments.Mon, Aug 12, 8:54 PM
extension/content-script.js
693 ↗(On Diff #63624)

Don't mind this screwup

broulik added inline comments.Tue, Aug 13, 7:54 AM
host/abstractbrowserplugin.h
56

Why does it say "got hidden" when those two clearly have a different signature?

fvogt added inline comments.Tue, Aug 13, 8:33 AM
host/abstractbrowserplugin.h
56

Because only the overridden overload is visible in the child classes. You can fix this by adding using AbstractBrowserPlugin::handleData; to the plugin classes.

broulik updated this revision to Diff 63644.Tue, Aug 13, 8:55 AM
  • Silence method hidden warning

Didn't add it to settings as I have some follow-up patches that will actually use the other overload, too

broulik updated this revision to Diff 63645.Tue, Aug 13, 9:15 AM
  • Properly support zero as serial
  • Wrap int max properly
broulik updated this revision to Diff 63646.Tue, Aug 13, 9:17 AM
  • We're signed int :)
fvogt added inline comments.Tue, Aug 13, 9:51 AM
extension/extension.js
108–111

Would need to be "replyToSerial" in message now, undefined is not smaller than 0

host/pluginmanager.cpp
132

Might be useful to always reply as ack?

fvogt accepted this revision.Tue, Aug 13, 1:03 PM
This revision is now accepted and ready to land.Tue, Aug 13, 1:03 PM
This revision was automatically updated to reflect the committed changes.