Last Comment Bug 664695 - Improve signature of nsIConsoleService::GetMessageArray
: Improve signature of nsIConsoleService::GetMessageArray
Status: RESOLVED FIXED
: addon-compat
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla19
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-16 04:36 PDT by neil@parkwaycc.co.uk
Modified: 2014-08-27 21:12 PDT (History)
5 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch (8.80 KB, patch)
2011-06-16 04:39 PDT, neil@parkwaycc.co.uk
benjamin: review-
Details | Diff | Splinter Review
Updated for bitrot (10.43 KB, patch)
2012-10-24 02:23 PDT, neil@parkwaycc.co.uk
benjamin: review+
Details | Diff | Splinter Review
Test fix (11.86 KB, patch)
2012-11-02 18:26 PDT, neil@parkwaycc.co.uk
mihai.sucan: review+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2011-06-16 04:36:27 PDT
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();
Comment 1 neil@parkwaycc.co.uk 2011-06-16 04:39:17 PDT
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;
Comment 2 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-06-17 09:52:14 PDT
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.
Comment 3 neil@parkwaycc.co.uk 2012-10-24 02:23:59 PDT
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.
Comment 4 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-11-02 08:10:11 PDT
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.
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-11-02 14:12:27 PDT
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)
Comment 7 neil@parkwaycc.co.uk 2012-11-02 18:26:20 PDT
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.
Comment 8 Mozilla RelEng Bot 2012-11-02 23:15:31 PDT
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 9 Mihai Sucan [:msucan] 2012-11-03 03:19:18 PDT
Comment on attachment 677967 [details] [diff] [review]
Test fix

This looks good to me. Thank you Neil!
Comment 11 Mozilla RelEng Bot 2012-11-03 12:45:30 PDT
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

Note You need to log in before you can comment on or make changes to this bug.