Last Comment Bug 777339 - Use prlog for focusmanager logging
: Use prlog for focusmanager logging
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Neil Deakin
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-25 06:48 PDT by Neil Deakin
Modified: 2012-07-31 06:10 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
switch logging (29.22 KB, patch)
2012-07-25 06:48 PDT, Neil Deakin
bugs: review-
Details | Diff | Splinter Review
switch logging, patch 2 (35.31 KB, patch)
2012-07-25 07:58 PDT, Neil Deakin
bugs: review+
Details | Diff | Splinter Review

Description Neil Deakin 2012-07-25 06:48:34 PDT
Created attachment 645749 [details] [diff] [review]
switch logging

The current logging requires enabling a macro. Better would be to use prlogging to enable logging without rebuilding.
Comment 1 Olli Pettay [:smaug] 2012-07-25 07:04:14 PDT
Comment on attachment 645749 [details] [diff] [review]
switch logging


>+#ifdef PR_LOGGING
>+
>+// Two types of focus pr logging are available:
>+//   'Focus' for normal focus manager calls
>+//   'FocusNavigation' for tab and document navigation
>+PRLogModuleInfo* gFocusLog;
>+PRLogModuleInfo* gFocusNavigationLog;
>+
>+#define LOGFOCUS(args) PR_LOG(gFocusLog, 4, args)
>+#define LOGFOCUSNAVIGATION(args) PR_LOG(gFocusNavigationLog, 4, args)
>+
>+#define LOGTAG(log, format, content)                            \
>+  {                                                             \
>+    nsAutoString tag(NS_LITERAL_STRING("(none)"));              \
>+    if (content)                                                \
>+      content->Tag()->ToString(tag);                            \
if (expr) {
  stmt;
}
And you could just use Tag()->ToUTF8String()


>+#ifdef PR_LOGGING
>+  LOGFOCUS(("<<MoveFocus begin Type: %d Flags: %x>>", aType, aFlags));
>+
>+  if (PR_LOG_TEST(gFocusLog, PR_LOG_DEBUG)) {
>+    nsCOMPtr<nsPIDOMWindow> focusedWindow = mFocusedWindow;
>+    if (focusedWindow) {
>+      nsCOMPtr<nsIDocument> doc = do_QueryInterface(focusedWindow->GetExtantDocument());
No need for this QI anymore.
There is GetExtantDoc(). And doc could be nsIDocument*


>+#ifdef PR_LOGGING
>+  if (PR_LOG_TEST(gFocusLog, PR_LOG_DEBUG)) {
>+    LOGFOCUS(("Window %p Raised [Currently: %p %p]", aWindow, mActiveWindow.get(), mFocusedWindow.get()));
>+    nsCAutoString spec;
>+    nsCOMPtr<nsIDocument> doc = do_QueryInterface(window->GetExtantDocument());
Ditto

>+#ifdef PR_LOGGING
>+  if (PR_LOG_TEST(gFocusLog, PR_LOG_DEBUG)) {
>+    LOGFOCUS(("Window %p Lowered [Currently: %p %p]", aWindow, mActiveWindow.get(), mFocusedWindow.get()));
>+    nsCAutoString spec;
>+    nsCOMPtr<nsIDocument> doc = do_QueryInterface(window->GetExtantDocument());
And here. And elsewhere...

>     if (doc) {
>       doc->GetDocumentURI()->GetSpec(spec);
>-      printf(" [%p] Active Window: %s", mActiveWindow.get(), spec.get());
>+      LOGFOCUS(("  Lowered Window: %s", spec.get()));
>+    }
>+    if (mActiveWindow) {
>+      doc = do_QueryInterface(mActiveWindow->GetExtantDocument());
>+      if (doc) {
>+        doc->GetDocumentURI()->GetSpec(spec);
>+        LOGFOCUS(("  Active Window: %s", spec.get()));
>+      }
Hmm, GetDocumentURI can return null as the method name indicates.
Comment 2 Neil Deakin 2012-07-25 07:58:08 PDT
Created attachment 645764 [details] [diff] [review]
switch logging, patch 2
Comment 3 Olli Pettay [:smaug] 2012-07-25 08:17:54 PDT
Comment on attachment 645764 [details] [diff] [review]
switch logging, patch 2


>+#ifdef PR_LOGGING
>+  LOGFOCUS(("<<MoveFocus begin Type: %d Flags: %x>>", aType, aFlags));
>+
>+  if (PR_LOG_TEST(gFocusLog, PR_LOG_DEBUG)) {
>+    nsCOMPtr<nsPIDOMWindow> focusedWindow = mFocusedWindow;
Hmm, there shouldn't be need for this to be nsCOMPtr



>@@ -690,17 +709,17 @@ nsFocusManager::WindowRaised(nsIDOMWindo
>     baseWindow->SetVisibility(true);
>   }
> 
>   // inform the DOM window that it has activated, so that the active attribute
>   // is updated on the window
>   window->ActivateOrDeactivate(true);
> 
>   // send activate event
>-  nsCOMPtr<nsIDocument> document = do_QueryInterface(window->GetExtantDocument());
>+  nsIDocument* document = window->GetExtantDoc();
>   nsContentUtils::DispatchTrustedEvent(document,
Perhaps just
nsContentUtils::DispatchTrustedEvent(window->GetExtantDoc(),


>   // send deactivate event
>-  nsCOMPtr<nsIDocument> document = do_QueryInterface(window->GetExtantDocument());
>+  nsIDocument* document = window->GetExtantDoc();
>   nsContentUtils::DispatchTrustedEvent(document,
Ditto
Comment 4 Ed Morley [:emorley] 2012-07-31 06:10:13 PDT
https://hg.mozilla.org/mozilla-central/rev/de293780ba8a

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