Last Comment Bug 760099 - add logging for events/test_docload.xul
: add logging for events/test_docload.xul
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 759670
  Show dependency treegraph
 
Reported: 2012-05-31 07:15 PDT by alexander :surkov
Modified: 2012-06-02 12:24 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (17.32 KB, patch)
2012-05-31 07:15 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-05-31 07:15:21 PDT
Created attachment 628718 [details] [diff] [review]
patch
Comment 1 Marco Zehe (:MarcoZ) on PTO until August 15 2012-05-31 07:52:27 PDT
Comment on attachment 628718 [details] [diff] [review]
patch

Some drive-by nits I found:

>- * Log the accesisble object address (4 spaces indent).
>+ * Log the accesisble object address as message entry (4 spaces indent).

Correct spelling of "accessible" while you're here.

>+function enableLogging(aModules)
>+{
>+  gAccRetrieval.setLogging(aModules);
>+}
>+function disableLogging()
>+{
>+  gAccRetrieval.setLogging("");
>+}
>+

Blank line between the two functions.
Comment 2 alexander :surkov 2012-05-31 08:53:35 PDT
(In reply to Marco Zehe (:MarcoZ) from comment #1)
> >+function enableLogging(aModules)
> >+{
> >+  gAccRetrieval.setLogging(aModules);
> >+}
> >+function disableLogging()
> >+{
> >+  gAccRetrieval.setLogging("");
> >+}
> >+
> 
> Blank line between the two functions.

they share the same comment so I keep them together intentionally.
Comment 3 Trevor Saunders (:tbsaunde) 2012-05-31 21:01:13 PDT
Comment on attachment 628718 [details] [diff] [review]
patch

>+  if (type == nsISelectionController::SELECTION_NORMAL ||
>+      type == nsISelectionController::SELECTION_SPELLCHECK) {
>+
>+    bool isNormalSelection =
>+      (type == nsISelectionController::SELECTION_NORMAL);
>+
>+    bool isIgnored = !aDocument || !aDocument->IsContentLoaded();
>+    printf("\nSelection changed, selection type: %s, notification %s\n",
>+           (isNormalSelection ? "normal" : "spellcheck"),
>+           (isIgnored ? "ignored" : "pending"));
>+  } else {
>+    bool isIgnored = !aDocument || !aDocument->IsContentLoaded();
>+    printf("\nSelection changed, selection type: unknown, notification %s\n",
>+           (isIgnored ? "ignored" : "pending"));
>+  }

it seems like this might be simpler if we did if type == normal ... else if type == spellcheck ... else ...

>+#ifdef DEBUG
>+  if (eventCount > 0) {
>+    if (logging::IsEnabled(logging::eEvents)) {
>+      logging::MsgBegin("EVENTS", "events processing");
>+      logging::Address("document", mDocument);
>+      logging::MsgEnd();
>+    }
>+  }
>+#endif

I'd probably make that one if, and put a blank line after the ifdef to seperate it from real code.

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