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

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
Form Manager
P3
enhancement
RESOLVED FIXED
6 months ago
2 months ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

Trunk
mozilla54
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [form autofill:M1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
Summary: [Form Autofill] Add built-in debug logging to ease debuggin → [Form Autofill] Add built-in debug logging to ease debugging
Comment hidden (mozreview-request)

Comment 2

6 months ago
mozreview-review
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

6 months ago
mozreview-review
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
Attachment #8837237 - Flags: review?(schung)
(Assignee)

Comment 4

6 months ago
mozreview-review-reply
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.
(Assignee)

Comment 5

6 months ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 7

6 months ago
mozreview-review
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!
Attachment #8837237 - Flags: review?(schung) → review+

Comment 8

6 months ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/365ed2bc91f6
[Form Autofill] Add built-in debug logging to ease debugging. r=steveck

Comment 9

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/365ed2bc91f6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

2 months ago
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.