[api-minor] Convert getJSActions to return data in a Map - #21664
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21664 +/- ##
=======================================
Coverage 89.96% 89.96%
=======================================
Files 263 263
Lines 66886 66890 +4
=======================================
+ Hits 60171 60179 +8
+ Misses 6715 6711 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Compared to regular Objects there's a number of advantages to using Maps: - They support proper iteration. - They have a simple way to check for the existence of data. - They have a simple/efficient way to check the number of elements. If this functionality was added today, I cannot imagine that we'd choose an Object for this data. Note also how in the scripting-implementation the `actions` were already converted into a Map, via the `createActionsMap` helper. In the Firefox PDF Viewer sending `Map`s to the scripting-implementation should be fine, since it uses the browser `Cu.cloneInto` functionality; see https://searchfox.org/firefox-main/source/toolkit/components/pdfjs/content/PdfSandbox.sys.mjs However with QuickJS, used by the GENERIC viewer, all data needs to be stringified and unfortunately `JSON.stringify()` doesn't support Maps. Hence we convert Maps to Objects, via a [`replacer` function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#replacer), since the existing `createActionsMap` usage will convert the actions-Objects back to Maps.
248cf4e to
803c9d7
Compare
|
/botio browsertest |
From: Bot.io (Linux m4)ReceivedCommand cmd_browsertest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/50b1eccdace79dc/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_browsertest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/6b92c54280df5bc/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/50b1eccdace79dc/output.txt Total script time: 18.24 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/6b92c54280df5bc/output.txt Total script time: 24.07 mins
|
timvandermeij
left a comment
There was a problem hiding this comment.
Looks good; thank you!
Compared to regular Objects there's a number of advantages to using Maps:
If this functionality was added today, I cannot imagine that we'd choose an Object for this data.
Note also how in the scripting-implementation the
actionswere already converted into a Map, via thecreateActionsMaphelper.In the Firefox PDF Viewer sending
Maps to the scripting-implementation should be fine, since it uses the browserCu.cloneIntofunctionality; see https://searchfox.org/firefox-main/source/toolkit/components/pdfjs/content/PdfSandbox.sys.mjsHowever with QuickJS, used by the GENERIC viewer, all data needs to be stringified and unfortunately
JSON.stringify()doesn't support Maps. Hence we convert Maps to Objects, via areplacerfunction, since the existingcreateActionsMapusage will convert the actions-Objects back to Maps.