Show warnings in start up in about:debugging

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: Developer Tools
P5
normal
RESOLVED FIXED
7 months ago
a month ago

People

(Reporter: andym, Assigned: mstriemer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

7 months ago
It might be nice to show warnings that occur in extension startup in the about:debugging page. Currently we just show failures. I can't think of a good example of this yet, so this might not be worth doing.

Comment 1

7 months ago
(In reply to Andy McKay [:andym] from comment #0)
> It might be nice to show warnings that occur in extension startup in the
> about:debugging page. Currently we just show failures. I can't think of a
> good example of this yet, so this might not be worth doing.

One kind of warnings that could be nice to show are the ones related to unrecognized manifest properties. (they are not considered errors and the addon installs just fine, but sometimes they can be an existent property with a typo).
(Reporter)

Updated

7 months ago
Assignee: nobody → mstriemer
Priority: -- → P5
Whiteboard: triaged
Comment hidden (mozreview-request)

Comment 3

2 months ago
mozreview-review
Comment on attachment 8882420 [details]
Bug 1330732 - Show install warnings in about:debugging

https://reviewboard.mozilla.org/r/153550/#review159278

I reviewed the parts related to devtools, looks good! R+ for them.

Can you ask for review from an addon/extensions peer for the /toolkit parts?

::: devtools/client/aboutdebugging/components/addons/panel.js:47
(Diff revision 1)
>      };
>    },
>  
>    componentDidMount() {
>      AddonManager.addAddonListener(this);
> +    Management.on("startup", this.updateAddonsList);

It is not obvious why we need to listen to Management's "startup" event here. Can you add a comment to explain the reason / use cases where this is needed?

::: devtools/client/aboutdebugging/components/addons/target.js:88
(Diff revision 1)
> +  return null;
> -  }
> +}
>  
> -  return [
> -    dom.div({ className: "addons-tip" },
> -      dom.span({ className: "addons-web-ext-tip" },
> +function infoMessages(target) {
> +  const messages = [];
> +  if (isTemporaryID(target.addonID)) {

I was confused when seeing we were using the classname "addons-web-ext-tip" here. This apparently comes from Bug 1326230 , which was not reviewed by a DevTools peer. 

For the record, we should not use classnames which are semantically confusing.

::: devtools/client/aboutdebugging/test/head.js:12
(Diff revision 1)
>     getServiceWorkerList, getTabList, openPanel, waitForInitialAddonList,
>     waitForServiceWorkerRegistered, unregisterServiceWorker,
>     waitForDelayedStartupFinished, setupTestAboutDebuggingWebExtension,
>     waitForServiceWorkerActivation, enableServiceWorkerDebugging,
>     getServiceWorkerContainer, promiseAddonEvent, installAddonWithManager, getAddonByID,
> -   tearDownAddon */
> +   tearDownAddon, waitForInstallMessages */

I think we should go ahead and replace this huge exported comment by 
> /* eslint no-unused-vars: [2, {"vars": "local"}] */

The current list is too big to be actually properly maintained.
Attachment #8882420 - Flags: review?(jdescottes) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a month ago
Andrew, do you mind looking at the toolkit changes or redirecting to someone you think would be a good reviewer?

Comment 6

a month ago
mozreview-review
Comment on attachment 8882420 [details]
Bug 1330732 - Show install warnings in about:debugging

https://reviewboard.mozilla.org/r/153550/#review160340

Hm, I was going to say Luca should take a look at this as I think `DebugUtils` is primarily about connecting the debugger to extensions which is quite different form whats being added here even though they both use the term "debugging".  But he's on PTO for a couple of weeks, so lets go with this as-is and it can be moved around later if he feels strongly.
Attachment #8882420 - Flags: review?(aswan) → review+
(Assignee)

Updated

a month ago
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
(Assignee)

Updated

a month ago
Keywords: checkin-needed

Comment 8

a month ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1d1b993bfea6
Show install warnings in about:debugging r=aswan,jdescottes
Keywords: checkin-needed
Backed out for failing chrome's test_chrome_ext_contentscript_unrecognizedprop_warning.html and test_chrome_ext_eventpage_warning.html:

https://hg.mozilla.org/integration/autoland/rev/c1b069f4bd131be3c0cfd6f68fa81edbf6c3b858

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1d1b993bfea622ab659a157620c7da0de8393063&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=114934254&repo=autoland

[task 2017-07-17T18:28:44.287220Z] 18:28:44     INFO - Extension loaded
[task 2017-07-17T18:28:44.288637Z] 18:28:44     INFO - monitorConsole | [0] did not match {"message":"1500316123464\taddons.webextension.<unknown>\tWARN\tLoading extension 'null': Error processing content_scripts.0.unrecognized_property: An unexpected property was found in the WebExtension manifest.","errorMessage":null,"sourceName":null,"sourceLine":null,"lineNumber":null,"columnNumber":null,"category":null,"windowID":null,"isScriptError":false,"isWarning":false,"isException":false,"isStrict":false}
[task 2017-07-17T18:28:44.289352Z] 18:28:44     INFO - monitorConsole | [0] did not match {"message":"1500316123476\taddons.webextension.{f6235b9c-36f6-454d-b8e9-c21df0f62099}\tWARN\tLoading extension '{f6235b9c-36f6-454d-b8e9-c21df0f62099}': Error processing content_scripts.0.unrecognized_property: An unexpected property was found in the WebExtension manifest.","errorMessage":null,"sourceName":null,"sourceLine":null,"lineNumber":null,"columnNumber":null,"category":null,"windowID":null,"isScriptError":false,"isWarning":false,"isException":false,"isStrict":false}
[task 2017-07-17T18:28:44.289473Z] 18:28:44     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_chrome_ext_contentscript_unrecognizedprop_warning.html | content-script-loaded 
[task 2017-07-17T18:28:44.289985Z] 18:28:44     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_chrome_ext_contentscript_unrecognizedprop_warning.html | test result correct 
[task 2017-07-17T18:28:44.290064Z] 18:28:44     INFO - test page loaded
[task 2017-07-17T18:28:44.290176Z] 18:28:44     INFO - Buffered messages finished
[task 2017-07-17T18:28:44.292861Z] 18:28:44     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_contentscript_unrecognizedprop_warning.html | monitorConsole | number of messages [{message:/Reading manifest: Error processing content_scripts.*.unrecognized_property: An unexpected property was found/}] - got +0, expected 1
Flags: needinfo?(mstriemer)
Also applies to browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_simple.js | monitorConsole | number of messages [{message:/Reading manifest: Error processing browser_action.unrecognized_property: An unexpected property was found/}] - got +0, expected 1
https://treeherder.mozilla.org/logviewer.html#?job_id=114935359&repo=autoland
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a month ago
Tests are passing now. The warnings were missing the "Reading manifest:" prefix.
Flags: needinfo?(mstriemer)
Keywords: checkin-needed

Comment 13

a month ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/004315dd0d8f
Show install warnings in about:debugging r=aswan,jdescottes
Keywords: checkin-needed

Comment 14

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/004315dd0d8f
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.