Improve signature of nsIConsoleService::GetMessageArray

RESOLVED FIXED in mozilla19

Status

()

Core
XPCOM
--
enhancement
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

({addon-compat})

unspecified
mozilla19
addon-compat
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
The current IDL signature is:
>void getMessageArray([array, size_is(count)] out nsIConsoleMessage messages, out PRUint32 count);
This is inconvenient to use from JavaScript:
>var out = {}; // Throwaway references to support 'out' parameters.
>this.mCService.getMessageArray(out, {});
>var messages = out.value;
Instead, the signature should be changed to:
>void getMessageArray([optional] out PRUint32 count, [retval, array, size_is(count)] out nsIConsoleMessage messages);
This allows the JavaScript to say simply:
>var messages = this.mCService.getMessageArray();
(Assignee)

Comment 1

6 years ago
Created attachment 539766 [details] [diff] [review]
Proposed patch

If an extension wanted to be backward compatible, it could write this:
>var out = {}; // Throwaway references to support 'out' parameters.
>var messages = this.mCService.getMessageArray(out, {}) || out.value;
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #539766 - Flags: review?(benjamin)

Comment 2

6 years ago
Comment on attachment 539766 [details] [diff] [review]
Proposed patch

I agree that this is a better signature. I'm not sure it's worth the churn in order to get this better signature now, though. Writeup to dev.platform shortly, but I'm hoping to remove this API and several others in favor of something that looks like DOM events.

Updated

6 years ago
Attachment #539766 - Flags: review?(benjamin) → review-
(Assignee)

Comment 3

5 years ago
Created attachment 674600 [details] [diff] [review]
Updated for bitrot

I changed the forEach in WebConsoleUtils.jsm to a filter because I was touching the code anyway but I can change it back or get separate review on that change.
Attachment #539766 - Attachment is obsolete: true
Attachment #674600 - Flags: review?(benjamin)

Comment 4

5 years ago
Comment on attachment 674600 [details] [diff] [review]
Updated for bitrot

ok. Please talk to jorge about spreading the word to the addon community and adding an addon compat check for whichever release this lands in.
Attachment #674600 - Flags: review?(benjamin) → review+
Keywords: addon-compat
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/06b998c1100d
Sorry, but this had to be backed out for mochitest-other orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b404d3977390

https://tbpl.mozilla.org/php/getParsedLog.php?id=16701647&tree=Mozilla-Inbound

30056 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/devtools/webconsole/test/test_cached_messages.html | number of cached page errors - got 1, expected 2
30068 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/devtools/webconsole/test/test_cached_messages.html | Test timed out.
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3942451 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of AsyncStatement with size 88 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1760 instances of AtomImpl with size 40 bytes each (70400 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 48 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of CalculateFrecencyFunction with size 24 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 900 instances of CompositeDataSourceImpl with size 120 bytes each (108000 bytes total)
(Assignee)

Comment 7

5 years ago
Created attachment 677967 [details] [diff] [review]
Test fix

I couldn't even get the test to work locally without the patch, because it does sneaky things with window.top and doesn't expect the uncaught exception.
Attachment #677967 - Flags: review?(rcampbell)
Attachment #677967 - Flags: review?(mihai.sucan)

Comment 8

5 years ago
Try run for a3794b698647 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a3794b698647
Results (out of 31 total builds):
    success: 30
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/neil@parkwaycc.co.uk-a3794b698647
Comment on attachment 677967 [details] [diff] [review]
Test fix

This looks good to me. Thank you Neil!
Attachment #677967 - Flags: review?(mihai.sucan) → review+
(Assignee)

Updated

5 years ago
Attachment #677967 - Flags: review?(rcampbell)
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c58f4ae031e

Comment 11

5 years ago
Try run for a3794b698647 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a3794b698647
Results (out of 32 total builds):
    success: 30
    warnings: 1
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/neil@parkwaycc.co.uk-a3794b698647
https://hg.mozilla.org/mozilla-central/rev/9c58f4ae031e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.