Closed Bug 1155643 Opened 5 years ago Closed 5 years ago

Consolidate nsIDebug and nsIDebug2 interfaces

Categories

(Core :: XPCOM, defect)

defect
Not set

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)

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
i would like to work on this bug. Can you assign me the bug
Assignee: nobody → sahukariganesh2
Attached patch bug-1155643-fix (obsolete) — Splinter Review
Removed nsIDebug.idl, moved all it's methods to nsIDebug2.idl. Took care of all dependencies of nsIDebug.
Attachment #8598048 - Flags: review?(paolo.mozmail)
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+
Removed the trailing whitespace in nsIDdebug2.idl
Attachment #8602510 - Flags: review?(paolo.mozmail)
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+
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
Try build has been failed for some test cases, but there are bugs raised for those.
Thanks!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/90e184f46b30
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.