Closed Bug 1728610 Opened 3 years ago Closed 3 years ago

Use logger in urlbar to report errors, warnings, and debug messages instead of Cu.reportError

Categories

(Firefox :: Address Bar, task, P1)

Product:
Firefox
Component:
Address Bar
Type:
task
Points:
2

Tracking

()

Status:
RESOLVED FIXED
Milestone:
95 Branch
Tracking Flags:
Tracking Status
firefox94 + fixed
firefox95 --- fixed

People

(Reporter: adw, Assigned: tanjubrunostar0, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 1 obsolete file)

Bug 1728610 - changed all instance of Cu.reportError with logger.error or this.logger.error where appropriate. r?mak
48 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-release+
Details | Review

We use Cu.reportError in some places, including UrlbarProviderQuickSuggest, but using a logger would let us log additional helpful info. This is from Marco's review comment (D123707, bug 1727668):

why is this provider not just using this.logger.error and friends? That may allow us to troubleshoot issues more easily, especially if we also log .warn and .debug messages... If there's an issue with using the provider logger we should try to resolve that and try to make larger use of it.

There is also a discussion ongoing among tech leads about unifying logging for Firefox Frontend. We're not there yet though, we could start improving logging for the urlbar and then it may be a simple in-place replacement.

https://searchfox.org/mozilla-central/search?q=reportError&path=urlbar&case=false&regexp=false when we have a logger available we should use its error method instead of reportError, for example in UrlbarProvider* and UrlbarProvidersManager

Mentor: mak
Keywords: good-first-bug
Flags: needinfo?(mak)

Cu.reportError should be replaced with logger.error or this.logger.error, depending on what the module has available.
If in the module there's no logger or this.logger, Cu.reportError should stay unchanged.
I think, but didn't double check, the following modules have a logger: UrlbarMuxerUnifiedComplete.jsm, UrlbarProviderExtension.jsm, UrlbarProviderOmnibox.jsm, UrlbarProviderOpenTabs.jsm, UrlbarProviderPlaces.jsm, UrlbarProviderPreloadedSites.jsm, UrlbarProviderQuickSuggest.jsm, UrlbarProviderSearchSuggestions.jsm, UrlbarProviderSearchTips.jsm, UrlbarProviderTabToSearch.jsm, UrlbarProviderTopSites.jsm, UrlbarProvidersManager.jsm

Tests are in browser/components/urlbar and can be run using ./mach test

Flags: needinfo?(mak)

there are about 69 Occurrences of Cu.reportError. You said to check the modules and understanding when the module has a this.logger available right?

(In reply to Marco Bonardo [:mak] from comment #2)

https://searchfox.org/mozilla-central/search?q=reportError&path=urlbar&case=false&regexp=false when we have a logger available we should use its error method instead of reportError, for example in UrlbarProvider* and UrlbarProvidersManager

Flags: needinfo?(mak)

(In reply to Marco Bonardo [:mak] from comment #3)

Cu.reportError should be replaced with logger.error or this.logger.error, depending on what the module has available.
If in the module there's no logger or this.logger, Cu.reportError should stay unchanged.
I think, but didn't double check, the following modules have a logger: UrlbarMuxerUnifiedComplete.jsm, UrlbarProviderExtension.jsm, UrlbarProviderOmnibox.jsm, UrlbarProviderOpenTabs.jsm, UrlbarProviderPlaces.jsm, UrlbarProviderPreloadedSites.jsm, UrlbarProviderQuickSuggest.jsm, UrlbarProviderSearchSuggestions.jsm, UrlbarProviderSearchTips.jsm, UrlbarProviderTabToSearch.jsm, UrlbarProviderTopSites.jsm, UrlbarProvidersManager.jsm

Tests are in browser/components/urlbar and can be run using ./mach test

thanks. so to edit the code. i need to access those modules from my local version of the source code and make the changes there right (just to be sure)

If you followed the tutorials in https://firefox-source-docs.mozilla.org/setup/index.html you should have the code locally, so you can go through it using your favorite editor, and study the code a bit to get more comfortable with it.
Build your own list of modules then compare it to my list in comment 3, did you find more or less modules? We can then look into those.
You can use https://searchfox.org/mozilla-central/source/ to search online through the codebase, and limit the search to the path browser/components/urlbar in the top right path filter.

Flags: needinfo?(mak)

I have made changes to some of the files. I don't know how to proceed.

you've mentioned tests but I've seen several test files in the directory but I don't know which one to use and how to use it.

Flags: needinfo?(mak)

It appears many of the modules do not have logger (e.g UrlbarProviderOpenTabs.jsm, UrlbarProviderPlaces.jsm, UrlbarProviderPreloadedSites.jsm, UrlbarProviderQuickSuggest.jsm). But I have updated all those with logger (Just 6 do after cross-checking the results of modules with reportError occurrences: https://searchfox.org/mozilla-central/search?q=reportError&path=urlbar and those of logger occurrences: https://searchfox.org/mozilla-central/search?q=logger&path=urlbar&case=false&regexp=false

Now how do I proceed?

Assignee: nobody → tanjubrunostar0
Status: NEW → ASSIGNED

Hi. I have submitted a patch. please review the changes I've made so far and let me know if I'm on the right path.

Flags: needinfo?(mak)

Hello, you should keep updating the same revision, not post multiple revisions.
Please check the documentation at https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html#to-update-a-submitted-patch

In practice, you make modifications on top of the original patch, then hg commit --amend those changes to merge them into the original revision, finally you submit the updated revision with moz-phab. This allows the reviewer to check the history of changes and the evolution of the revision across reviews.
Note Phabricator doesn't have something like the squash feature of github, you must squash your revisions manually, by amending changes to an existing revision where necessary.
Once you end up in the current state (multiple revisions on top of each other while you should only have one) you can still fix the problem by using the rebase command. hg rebase -s source -d dest --collapse will collapse dest and all the child revisions into a single revision on top of source.
For example if you have:
rev2
rev1
central
you would hg rebase -s rev1 -d central --collapse and you'd obtain:
rev1+2
central

Once you have merged the revisions, just push them again using moz-phab. The quick reference should have enough information to do that, but feel free to ask if you find difficulties.

Last thing, when you ask for review, the reviewer automatically gets notified about it, you don't need to also set the Request information from field, that field is only necessary when you post a question in a bugzilla comment and need an answer to proceed.

Flags: needinfo?(mak)

oh I get it. thanks for the directions

hello. I've been making changes and using "commit --amend" but it doesn't update phabricator. what can do?

Flags: needinfo?(mak)
Attachment #9245623 - Attachment is obsolete: true

I answered on Phabricator, thank you for squashing the changes

Flags: needinfo?(mak)

Is there another thing to do concerning this bug?

Flags: needinfo?(mak)
Attachment #9245462 - Attachment description: Bug 1728610 - changed all instance of Cu.reportError with logger.error or this.logger.error where appropriate. r?Marco Bonardo <mbonardo@mozilla.com> → Bug 1728610 - changed all instance of Cu.reportError with logger.error or this.logger.error where appropriate. r?mak
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/7713d58b648e
changed all instance of Cu.reportError with logger.error or this.logger.error where appropriate. r=mak

https://hg.mozilla.org/mozilla-central/rev/7713d58b648e

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox95: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch
Flags: needinfo?(mak)
Flags: qe-verify-
Flags: in-testsuite-

[Tracking Requested - why for this release]: We need this for the Firefox Suggest preferences redesign targeting 95/94.

tracking-firefox94: --- → ?

Comment on attachment 9245462 [details]
Bug 1728610 - changed all instance of Cu.reportError with logger.error or this.logger.error where appropriate. r?mak

Beta/Release Uplift Approval Request

  • User impact if declined: We need this for the Firefox Suggest preferences redesign targeting 95/94.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Please see uplift spreadsheet: https://docs.google.com/spreadsheets/d/1LavihS-VOPFYEyum7mrx6FKXmuQeHi9xQHfGNSxjnoY/edit?usp=sharing
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This revision changes uses of Cu.reportError() to use logs instead. These are mainly code paths that are hit when errors occur. Some of the code paths may be hit during automated tests but most are probably not. Regardless, there are no user-facing changes.
  • String changes made/needed:
Attachment #9245462 - Flags: approval-mozilla-release?

Comment on attachment 9245462 [details]
Bug 1728610 - changed all instance of Cu.reportError with logger.error or this.logger.error where appropriate. r?mak

Approved for 94.0.2.

Attachment #9245462 - Flags: approval-mozilla-release? → approval-mozilla-release+

Changing the priority to p1 as the bug is tracked by a release manager for the current release.
See What Do You Triage for more information

Priority: P3 → P1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: