Closed Bug 859756 Opened 11 years ago Closed 11 years ago

[browserconsole] Missing nsIConsoleMessages from Browser Console

Categories

(DevTools :: Console, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: rcampbell, Assigned: msucan)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 7 obsolete files)

STR:
1. Open the Browser Console after startup.
2. Open the Error Console
3. Compare

Expected:

Contents should be the same.

Actual:

Some items are missing, e.g., 

Could not read chrome manifest 'file:///Users/rob/Projects/FXTEAM/fx-team-working/obj-x86_64-apple-darwin12.3.0/dist/Nightly.app/Contents/MacOS/browser/extensions/%7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D/chrome.manifest'.

OpenGL LayerManager Initialized Succesfully.
Version: 2.1 INTEL-8.10.44
Vendor: Intel Inc.
Renderer: Intel HD Graphics 4000 OpenGL Engine
FBO Texture Target: TEXTURE_2D

etc.
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Summary: [browser console] Missing nsIConsoleMessages from Browser Console → [browserconsole] Missing nsIConsoleMessages from Browser Console
Attached patch working patch - no tests yet (obsolete) — Splinter Review
This patch adds support for displaying nsIConsoleMessages in the Browser Console.

I also added a |timeStamp| property to all nsIConsoleMessages - otherwise they break sorting for cached messages in the console, and I find it's odd that they don't have any timestamp.
As briefly discussed on IRC this is how I monkey-patched* nsIConsoleMessage to add timeStamp, category and window ID properties. I also needed to do some changes to allow JS scripts to create new instances of nsIConsoleMessages, similar to how we can do it for nsIScriptErrors.

Please provide your feedback/review. Thanks!


* monkey-patching because I feel really small in the Gecko codebase, but I really enjoy the learning experience. Please let me know how I can improve the patch.
Attachment #747588 - Attachment is obsolete: true
Attachment #749450 - Flags: review?(bzbarsky)
Attached patch part 2 - browser console changes (obsolete) — Splinter Review
This patch changes the Web/Browser Console to accept nsIConsoleMessages just like nsIScriptErrors. We will display nsIConsoleMessages that belong to a specific window in the Web Console. In the Browser Console we show all such messages, irrespective of the window ID.

I was torn about reusing the "pageError" protocol packet or not. In the end I decided to add a "logMessage" packet. Thoughts / preferences?

I have added tests in toolkit/ and in browser/.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=54c951390b3b
Attachment #749453 - Flags: review?(rcampbell)
Comment on attachment 749450 [details] [diff] [review]
part 1 - changes to nsIConsoleMessage

>+++ b/js/xpconnect/src/XPCModule.h

Why are the changes to this file needed?  I don't see anyone creating nsConsoleMessage by this new contract in this patch, and in any case adding component registration here for a component over in xpcom is not right.

>+++ b/js/xpconnect/src/nsScriptError.cpp
> nsScriptError::Init(const nsAString& message,
>+                       const char *category,
>+                       uint64_t innerWindowID)

Fix the indent, please.

>+  return InitWithWindowID(message, NS_LITERAL_STRING(""), NS_LITERAL_STRING(""), 0, 0, 0, category, 0);

Why drop the innerWindowID?

Also, EmptyString() for the empty strings.

>+++ b/layout/build/Makefile.in
>+		   -I$(topsrcdir)/xpcom/base \

Why is this change needed?

>+++ b/xpcom/base/Makefile.in
>+		-I$(topsrcdir)/dom/base \

Likewise.

>+++ b/xpcom/base/nsConsoleMessage.cpp

It's too bad nsScriptError doesn't inherit from this class.  The code non-sharing makes me sad.

> nsConsoleMessage::nsConsoleMessage(const PRUnichar *message) 
>+  nsAutoString m;
>+  m.Assign(message);
>+  Init(m, nullptr, 0);

This is going to make two copies of the message, in many cases.  Can you at least use an nsString for 'm' to prevent that?

>+nsConsoleMessage::Init(const nsAString& message,
>+      if (outer)
>+	mOuterWindowID = outer->WindowID();

Please fix the indentation.  And I assume you have tabs in there.... you don't want that. Spaces only.

You presumably need to set things like mInnerWindowID and mOuterWindowID in the nsConsoleMessage constructor.  Also mIsFromPrivateWindow and mTimeStamp.

>+nsConsoleMessage::GetMessageMoz(PRUnichar **result)

This used to return mMessage. Why do something different now?  That looks wrong to me.

>+nsConsoleMessage::ToString(nsACString& /*UTF8*/ aResult)
>+    aResult.Assign(ToNewUTF8String(mMessage));

This leaks.  If you just want to return mMessage, then do:

  CopyUTF16toUTF8(mMessage, aResult);

>+++ b/xpcom/base/nsIConsoleMessage.idl
>+    void Init(in AString message, [optional] in string category, [optional] in unsigned long long innerWindowID);

Why uppercase 'I'?  Seems like you want "init" instead.

Or is the problem that you have an "init" on nsIScriptError already?  In that case, I would prefer "initMessage" here or something.
Attachment #749450 - Flags: review?(bzbarsky) → review-
Attachment #749453 - Flags: review?(rcampbell) → review+
Thanks for the quick review!

(In reply to Boris Zbarsky (:bz) from comment #4)
> Comment on attachment 749450 [details] [diff] [review]
> part 1 - changes to nsIConsoleMessage
> 
> >+++ b/js/xpconnect/src/XPCModule.h
> 
> Why are the changes to this file needed?  I don't see anyone creating
> nsConsoleMessage by this new contract in this patch, and in any case adding
> component registration here for a component over in xpcom is not right.

nsIConsoleMessages are created in part 2 patch from JS. should we not allow that? I guess we can add optional arguments to nsIConsoleService.logStringMessage() that take the category and the window ID.


> >+++ b/js/xpconnect/src/nsScriptError.cpp
> > nsScriptError::Init(const nsAString& message,
> >+                       const char *category,
> >+                       uint64_t innerWindowID)
> 
> Fix the indent, please.
>
> >+  return InitWithWindowID(message, NS_LITERAL_STRING(""), NS_LITERAL_STRING(""), 0, 0, 0, category, 0);
> 
> Why drop the innerWindowID?
> 
> Also, EmptyString() for the empty strings.

Fixed.

> >+++ b/layout/build/Makefile.in
> >+		   -I$(topsrcdir)/xpcom/base \
> 
> Why is this change needed?
> 
> >+++ b/xpcom/base/Makefile.in
> >+		-I$(topsrcdir)/dom/base \
> 
> Likewise.

I was getting "nsConsoleMessage.h" include file not found errors throughout these folders. All these changes were needed once I added nsIConsoleMessage contractid in XPCModule.h. I didn't find a better place. Can you suggest a better approach?


> >+++ b/xpcom/base/nsConsoleMessage.cpp
> 
> It's too bad nsScriptError doesn't inherit from this class.  The code
> non-sharing makes me sad.

Good point. I tried to change nsScriptError to inherit from nsConsoleMessage but then I get errors about ambiguous inheritance. I'm missing something here.

> > nsConsoleMessage::nsConsoleMessage(const PRUnichar *message) 
> >+  nsAutoString m;
> >+  m.Assign(message);
> >+  Init(m, nullptr, 0);
> 
> This is going to make two copies of the message, in many cases.  Can you at
> least use an nsString for 'm' to prevent that?

Fixed.

> >+nsConsoleMessage::Init(const nsAString& message,
> >+      if (outer)
> >+	mOuterWindowID = outer->WindowID();
> 
> Please fix the indentation.  And I assume you have tabs in there.... you
> don't want that. Spaces only.
> 
> You presumably need to set things like mInnerWindowID and mOuterWindowID in
> the nsConsoleMessage constructor.  Also mIsFromPrivateWindow and mTimeStamp.

Fixed.

> >+nsConsoleMessage::GetMessageMoz(PRUnichar **result)
> 
> This used to return mMessage. Why do something different now?  That looks
> wrong to me.

Fixed.

> >+nsConsoleMessage::ToString(nsACString& /*UTF8*/ aResult)
> >+    aResult.Assign(ToNewUTF8String(mMessage));
> 
> This leaks.  If you just want to return mMessage, then do:
> 
>   CopyUTF16toUTF8(mMessage, aResult);

Fixed.

> >+++ b/xpcom/base/nsIConsoleMessage.idl
> >+    void Init(in AString message, [optional] in string category, [optional] in unsigned long long innerWindowID);
> 
> Why uppercase 'I'?  Seems like you want "init" instead.

Fixed.

Will update the patch.
> nsIConsoleMessages are created in part 2 patch from JS. should we not allow that? 

Ah, that should be fine.  But in that case, please add to the right module!

> I didn't find a better place. Can you suggest a better approach?

nsConsoleMessage is in xpcom, so how about putting its registration in XPCOMModule.inc?

> I tried to change nsScriptError to inherit from nsConsoleMessage

Please don't.  It's sad that it does not, but not sad enough to warrant the pain involved in that change.
Updated the patch based on review comments. I didn't fix the includes / component registration, and the nsScriptError still doesn't inherit from nsConsoleMessage.

Can you please suggest the changes I need to make / point me to docs I should read about this stuff? Or some code I can look at. Also, I don't want my lack of experience with this code to take too much of your time by helping me. If needed, we can ping someone more experienced than myself with platform code to help us with this work.

Thank you!
Attachment #749450 - Attachment is obsolete: true
Attachment #750029 - Flags: feedback?(bzbarsky)
Rebased the patch.
Attachment #749453 - Attachment is obsolete: true
(In reply to Boris Zbarsky (:bz) from comment #6)
> > nsIConsoleMessages are created in part 2 patch from JS. should we not allow that? 
> 
> Ah, that should be fine.  But in that case, please add to the right module!
> 
> > I didn't find a better place. Can you suggest a better approach?
> 
> nsConsoleMessage is in xpcom, so how about putting its registration in
> XPCOMModule.inc?

I didn't find said file. Thanks for pointing it to me. Will look into it. Thanks!

> > I tried to change nsScriptError to inherit from nsConsoleMessage
> 
> Please don't.  It's sad that it does not, but not sad enough to warrant the
> pain involved in that change.

Ah, ok. I saw it leads me down a path of pain... I thought it's caused by my noobism.
Fixed the component registration - this makes more sense. Thanks for pointing me to the right file (in my mxr searches I didn't find it).
Attachment #750029 - Attachment is obsolete: true
Attachment #750029 - Flags: feedback?(bzbarsky)
Attachment #750059 - Flags: review?(bzbarsky)
New try push: https://tbpl.mozilla.org/?tree=Try&rev=53e0a55a9717

(yesterday's try push was broken by infrastructure problems.)
Comment on attachment 750059 [details] [diff] [review]
part 1 - changes to nsIConsoleMessage v0.3

Yep, looks good!
Attachment #750059 - Flags: review?(bzbarsky) → review+
Had build failures in my try push. clang on my system allowed nsIScriptError to override the Init() virtual function from nsIConsoleMessage, but not gcc on try servers. I have renamed the new nsIConsoleMessage method to InitMessage().

Try push looks green: https://tbpl.mozilla.org/?tree=Try&rev=d57096658451
Attachment #750059 - Attachment is obsolete: true
dev-doc-needed for the new properties in nsIConsoleMessage: timeStamp, category, innerWindowID, outerWindowID and isFromPrivateWindow. Also note the new initMessage() method and contract ID for nsIConsoleMessage: "@mozilla.org/consolemessage;1".
Keywords: dev-doc-needed
Blocks: 602006
https://hg.mozilla.org/mozilla-central/rev/7ac5779a426c
https://hg.mozilla.org/mozilla-central/rev/073791f33ab7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Backed out for causing intermittent Windows build failures.
https://hg.mozilla.org/mozilla-central/rev/ecdfb8bb501e

https://tbpl.mozilla.org/php/getParsedLog.php?id=23090070&tree=Mozilla-Inbound
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 24 → ---
The problem is presumably that the nsGlobalWindow include is not OK in XPCOM, because XPCOM can get built before that header is exported?
(In reply to Boris Zbarsky (:bz) from comment #18)
> The problem is presumably that the nsGlobalWindow include is not OK in
> XPCOM, because XPCOM can get built before that header is exported?

How can we fix this issue? If you believe the fix would require too many changes we can take out the window IDs and just keep timestamp and category.
The only reason you need nsGlobalWindiw is to call GetInnnerWindowWithId, right?

Can you use the nsIWindowMediator function instead?  Not sure if that would be exported early enough...  Benjamin?
Flags: needinfo?(benjamin)
Ping? I would like to land this patch and the dependencies.
It appears to me that the primary "problem" here is that nsGlobalWindow.h is not exported at all, so naturally you wouldn't be able to include it. Everyone who currently includes it is using LOCAL_INCLUDES to get at it, e.g. http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/Makefile.in#68

Other than it seems weird for any of this to be in XPCOM, it seems like whatever function you use ought to be in a public header, not a DOM-private one. Maybe we should move the entire consoleservice IDL+impl to somewhere in devtools?
Flags: needinfo?(benjamin)
Blocks: 875427
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #22)
> It appears to me that the primary "problem" here is that nsGlobalWindow.h is
> not exported at all, so naturally you wouldn't be able to include it.
> Everyone who currently includes it is using LOCAL_INCLUDES to get at it,
> e.g.
> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/Makefile.in#68

Can we fix this issue?

> Other than it seems weird for any of this to be in XPCOM, it seems like
> whatever function you use ought to be in a public header, not a DOM-private
> one. Maybe we should move the entire consoleservice IDL+impl to somewhere in
> devtools?

Should we do that here? My primary goal in this bug is to get nsIConsoleMessages displayed in the Browser Console. For that I needed the |timeStamp| property. While I was there I also added the |category| and window ID properties because this was something we were interested to do other times for the web console. If these changes require a lot of other work to happen we should probably split it into two bugs.

Thank you!
As discussed on IRC I've updated the patch to only add the timeStamp property to nsIConsoleMessages, to avoid the problems with window IDs. I also removed |category| because it is not needed to get nsIConsoleMessages displayed in the browser console.

Thanks!
Attachment #750978 - Attachment is obsolete: true
Rebased the patch and updated tests to match changes in "part 1".

Try push: https://tbpl.mozilla.org/?tree=Try&rev=9182fade429f

If it turns out to be green I will land these patches and bug 602006.
Attachment #750030 - Attachment is obsolete: true
Try push was green. Landed the two patches:

https://hg.mozilla.org/integration/fx-team/rev/a6c12acfa421
https://hg.mozilla.org/integration/fx-team/rev/ea16ea097418
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a6c12acfa421
https://hg.mozilla.org/mozilla-central/rev/ea16ea097418
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Depends on: 883501
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: