Closed Bug 1330732 Opened 3 years ago Closed 2 years ago

Show warnings in start up in about:debugging

Categories

(WebExtensions :: Developer Tools, defect, P5)

defect

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: andy+bugzilla, Assigned: mstriemer)

References

(Blocks 1 open bug)

Details

(Whiteboard: triaged)

Attachments

(1 file)

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.
(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).
Assignee: nobody → mstriemer
Priority: -- → P5
Whiteboard: triaged
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+
Andrew, do you mind looking at the toolkit changes or redirecting to someone you think would be a good reviewer?
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+
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
Keywords: checkin-needed
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
Tests are passing now. The warnings were missing the "Reading manifest:" prefix.
Flags: needinfo?(mstriemer)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/004315dd0d8f
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Duplicate of this bug: 1404055
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.