Closed
Bug 1155643
Opened 10 years ago
Closed 10 years ago
Consolidate nsIDebug and nsIDebug2 interfaces
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: Paolo, Assigned: sahukariganesh2, Mentored)
Details
(Keywords: addon-compat, dev-doc-needed, Whiteboard: [good next bug][lang=C++])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Paolo
:
review+
|
Details | Diff | Splinter Review |
This is a good next bug to get familiar with how XPCOM and XPConnect works.
Until recently, many XPCOM interfaces in the code base were present in different versions, because the set of methods they defined used to be "frozen" to provide binary compatibility.
Now that binary compatibility isn't a requirement anymore, interfaces have been consolidated over time. nsIDebug and nsIDebug2 are a simple example of one interface that still needs to be consolidated. Since many consumers reference nsIDebug2, we should probably keep it and remove nsIDebug, as we've done in other similar cases.
The files involved can be found from this search:
https://dxr.mozilla.org/mozilla-central/search?q=nsIDebug&=mozilla-central&redirect=true
These are the required steps:
* Move the methods of nsIDebug.idl into nsIDebug2.idl
* Remove nsIDebug.idl, and the inheritance reference from nsIDebug2
* Adjust all the build system and registration references
* Adjust the call sites that use nsIDebug to use nsIDebug2
Assignee | ||
Comment 1•10 years ago
|
||
i would like to work on this bug. Can you assign me the bug
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → sahukariganesh2
Assignee | ||
Comment 2•10 years ago
|
||
Removed nsIDebug.idl, moved all it's methods to nsIDebug2.idl. Took care of all dependencies of nsIDebug.
Attachment #8598048 -
Flags: review?(paolo.mozmail)
Reporter | ||
Comment 3•10 years ago
|
||
Comment on attachment 8598048 [details] [diff] [review]
bug-1155643-fix
Review of attachment 8598048 [details] [diff] [review]:
-----------------------------------------------------------------
Hi Ganesh, sorry for the unusual delay on this review. This looks great and I've started a tryserver build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1baa0b32f214
::: xpcom/base/nsDebugImpl.h
@@ +36,5 @@
> +{ /* cb6cdb94-e417-4601-b4a5-f991bf41453d */ \
> + 0xcb6cdb94, \
> + 0xe417, \
> + 0x4601, \
> + {0xb4, 0xa5, 0xf9, 0x91, 0xbf, 0x41, 0x45, 0x3d} \
As a side note, if I remember correctly, the CID of the component doesn't need to be updated when the interfaces change. This is because it refers to a concrete component that, over time, may change and implement different interfaces. However, changing the CID here doesn't hurt, so let's keep the new one. The IID change is definitely the important one, since the methods available on the interface changed.
::: xpcom/base/nsIDebug2.idl
@@ +48,5 @@
> + *
> + */
> + void assertion(in string aStr,
> + in string aExpr,
> + in string aFile,
nit: while you're copying the code, you may also remove the trailing whitespace that was incorrectly present in the original file.
Attachment #8598048 -
Flags: review?(paolo.mozmail) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Removed the trailing whitespace in nsIDdebug2.idl
Attachment #8602510 -
Flags: review?(paolo.mozmail)
Reporter | ||
Comment 5•10 years ago
|
||
Comment on attachment 8602510 [details] [diff] [review]
bug-1155643-fix-v2.patch
I didn't enable the unit tests in the previous try run by mistake, so I have to run it again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c953fed42bf
There were x64 build failures in the previous one, but they might actually be infrastructure problems so let's see how it goes.
If there are no test failures related to this patch, feel free to set the "checkin-needed" keyword, so that the patch can be checked in even if I'm not around.
Attachment #8602510 -
Flags: review?(paolo.mozmail) → review+
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8598048 [details] [diff] [review]
bug-1155643-fix
You can also make older versions as "obsolete" when uploading a new one, this makes it clear which attachment is the most recent, especially useful when someone else is handling the checkin.
Attachment #8598048 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
Try build has been failed for some test cases, but there are bugs raised for those.
Keywords: checkin-needed
Comment 10•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•