1339515 - [Form Autofill] Add built-in debug logging to ease debugging

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Matthew N. [:MattN]]

We should add built-in logging wherever it may be useful for debugging bug reports or during development. It can be controlled by a preference so it's it's not verbose by default.

Comment 1

[Matthew N. [:MattN]]

Attachment: Bug 1339515 - [Form Autofill] Add built-in debug logging to ease debugging.

Review commit: https://reviewboard.mozilla.org/r/112412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112412/

Comment 2

[Steve Chung [:steveck]]

Comment on attachment 8837237 [details]
Bug 1339515 - [Form Autofill] Add built-in debug logging to ease debugging.

https://reviewboard.mozilla.org/r/112412/#review113962

::: browser/extensions/formautofill/FormAutofillHandler.jsm:22
(Diff revision 1)
> +Cu.import("resource://formautofill/FormAutofillUtils.jsm");
> +
>  XPCOMUtils.defineLazyModuleGetter(this, "FormAutofillHeuristics",
>                                    "resource://formautofill/FormAutofillHeuristics.jsm");
>  
> +this.log = null;

I could understand the log's scope in FormAutofillContent, but I'm not quite sure the scope of the log. The scope of this here seems the content process scope. You probably want to avoid the unnecessary log instance duplication in each handler instance,  but I'm not sure the process scope log would work correctly between different objects. The process scpoe log is reseted in different objects while script loaded, and prefix of the log should be determined by the last loaded script. Maybe we can just have a log instance per process and message prefix is set manually while calling the debug method?

Comment 3

[Steve Chung [:steveck]]

Comment on attachment 8837237 [details]
Bug 1339515 - [Form Autofill] Add built-in debug logging to ease debugging.

https://reviewboard.mozilla.org/r/112412/#review113970

Some xpcshell tests seems broken, I guess the scpoe of the log might be the problem

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8837237 [details]
Bug 1339515 - [Form Autofill] Add built-in debug logging to ease debugging.

https://reviewboard.mozilla.org/r/112412/#review113962

> I could understand the log's scope in FormAutofillContent, but I'm not quite sure the scope of the log. The scope of this here seems the content process scope. You probably want to avoid the unnecessary log instance duplication in each handler instance,  but I'm not sure the process scope log would work correctly between different objects. The process scpoe log is reseted in different objects while script loaded, and prefix of the log should be determined by the last loaded script. Maybe we can just have a log instance per process and message prefix is set manually while calling the debug method?

Each JSM has its own scope. The problem we were seeing with frame scripts doesn't apply here.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8837237 [details]
Bug 1339515 - [Form Autofill] Add built-in debug logging to ease debugging.

https://reviewboard.mozilla.org/r/112412/#review113970

I will look at the tests.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8837237 [details]
Bug 1339515 - [Form Autofill] Add built-in debug logging to ease debugging.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112412/diff/1-2/

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8837237 [details]
Bug 1339515 - [Form Autofill] Add built-in debug logging to ease debugging.

https://reviewboard.mozilla.org/r/112412/#review114074

LGTM, thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/365ed2bc91f6
[Form Autofill] Add built-in debug logging to ease debugging. r=steveck

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/365ed2bc91f6