760099 - add logging for events/test_docload.xul

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[Marco Zehe (:MarcoZ)]

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 (:asurkov)]

(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)]

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.

Comment 4

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e65e082823d7

Comment 5

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/e65e082823d7