Julian Descottes [:jdescottes] Reporter
	Description • 7 years ago

I was recently testing the MDN docs widget for a change I made on the HTML Tooltip and I saw that it was unable to load any information for most of the usual properties. In my short test, only the -moz-* properties were getting some content to display.

The MDN Docs widget was disabled by default in Bug 1352801. I would suggest removing the code now as it seems the "backend" changed on MDN side and we had very low usage anyway.

	Patrick Brosset <:pbro>
	Comment 1 • 7 years ago

Good call, let's just remove the code now. If we ever need to see how it worked again, we can consult the hg history.

I think this would be a good first bug for someone who wants to get started with DevTools development. And removing unused code is always pretty satisfying I think :)

Julian, I'll set you as a mentor on this one. Please re-assign the role to me if you don't have time right now.

To get started, you'll need to first go through our contribution guide: http://docs.firefox-dev.tools/
Here are a few pointers for files/code to be removed. Maybe there's more, but it's a start:

devtools\client\shared\widgets\mdn-docs.css
devtools\client\shared\widgets\MdnDocsWidget.js
docsTooltip.visitMDN and docsTooltip.loadDocsError in devtools\client\locales\en-US\inspector.properties
several test files: devtools\client\shared\test\doc_mdn-css-* and devtools\client\shared\test\browser_mdn-docs-*
etc.

	Julian Descottes [:jdescottes] Reporter
	Comment 2 • 7 years ago

Thanks Patrick!

Short summary about the MDN Docs tooltip. This feature used to be turned on by default and added a context menu item in the inspector's rule view "Show MDN Docs". Clicking on the item would open a popup with MDN documentation. As said in the summary, the feature is now off by default, and when enabled barely works probably due to changes on MDN side.

As Patrick mentioned this bug is about removing source code and test files related to this feature.

For source-code, a good start is to look for the usage of the preference that drives the feature: devtools.inspector.mdnDocsTooltip.enabled [1]. From there you can start removing the code that handles the preference and see which files are now useless and can be deleted. 

For test files, on top of the ones mentioned in the prev. comment I think we also have : devtools/client/inspector/rules/test/browser_rules_context-menu-show-mdn-docs-0*.js

[1] http://searchfox.org/mozilla-central/search?q=devtools.inspector.mdnDocsTooltip.enabled&path=

	Julian Descottes [:jdescottes] Reporter
	Comment 3 • 7 years ago

Assigning to :Towkir as discussed on IRC! Thanks for your interest in this bug, let me know if you have questions.

	[:Towkir] Ahmed Assignee
	Comment 4 • 7 years ago

Hi, please have a look if I have covered everything or removed anything necessary.

	Julian Descottes [:jdescottes] Reporter
	Comment 5 • 7 years ago

Comment on attachment 8914120 [details] [diff] [review]
mdn-docs-removal.patch

Review of attachment 8914120 [details] [diff] [review]:
-----------------------------------------------------------------

With the patch attached here Firefox is not building. When you remove a test file such as devtools/client/inspector/rules/test/browser_rules_context-menu-show-mdn-docs-01.js, it also needs to be removed from the corresponding browser.ini file which lists the file (here that would be devtools/client/inspector/rules/test/browser.ini).

A simple search for "mdn-docs" in devtools still gives me matches in:
- devtools/client/inspector/rules/test/browser.ini
- devtools/client/jar.mn
- devtools/client/shared/test/browser.ini
- devtools/client/shared/test/browser_mdn-docs-01.js (<-- this file and all the similar ones should also be deleted)
- devtools/client/themes/tooltips.css

You need a working development environment to work on this bug and you should only submit a patch if Firefox builds.

	[:Towkir] Ahmed Assignee
	Comment 6 • 7 years ago

I thought I should remove this[0] mention of MdnDocsWidget.js in this comment, as the file exists no more. 

[0] https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/test/head.js#279

	Julian Descottes [:jdescottes] Reporter
	Comment 7 • 7 years ago

Comment on attachment 8914317 [details] [diff] [review]
mdn-docs-removal.patch

Review of attachment 8914317 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the new patch, it's much better! Some more cleanup left though.

There are still some references to MDN docs left in the tree:
- devtools/client/inspector/shared/style-inspector-menu.js (method _onShowMdnDocs)
- devtools/client/inspector/shared/tooltips-overlay.js (everything related to cssDocs)
- devtools/client/shared/widgets/tooltip/CssDocsTooltip.js ( file should be deleted)

The support files used in the tests can also be removed:
- devtools/client/shared/test/doc_mdn-css-basic-testing.html
- devtools/client/shared/test/doc_mdn-css-no-summary-or-syntax.html
- devtools/client/shared/test/doc_mdn-css-no-summary.html
- devtools/client/shared/test/doc_mdn-css-no-syntax.html
- devtools/client/shared/test/doc_mdn-css-syntax-old-style.html

Finally there's another part of DevTools which uses the MDN docs widget: GCLI (command line for devtools). We should also remove the "mdn" command from there.
- remove devtools/shared/gcli/commands/mdn.js
- remove the reference to mdn.js in devtools/shared/gcli/commands/index.js
- same in devtools/shared/gcli/commands/moz.build

(In reply to [:Towkir] Ahmed from comment #6)
> Created attachment 8914317 [details] [diff] [review]
> mdn-docs-removal.patch
> 
> I thought I should remove this[0] mention of MdnDocsWidget.js in this
> comment, as the file exists no more. 
> 
> [0]
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/test/
> head.js#279

The whole method checkCssSyntaxHighlighterOutput should be removed as it was only used in the tests that are removed here.

	[:Towkir] Ahmed Assignee
	Comment 8 • 7 years ago

Here it is.

	[:Towkir] Ahmed Assignee
	Comment 9 • 7 years ago

Sorry, forgot to update the borwser.ini and moz.build again :(
Here is the updated patch.

	Julian Descottes [:jdescottes] Reporter
	Comment 10 • 7 years ago

Comment on attachment 8914379 [details] [diff] [review]
mdn-docs-removal.patch

Review of attachment 8914379 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Still some references to cleanup:
- in devtools/shared/locales/en-US/gclicommands.properties
- in devtools/client/themes/commandline.css
- remove devtools/client/shared/test/doc_mdn-docs-01.html

I will push your changeset to try in the meantime. Almost there!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bb93b6437355208d586bdc01d823d1a1efa17b5

	[:Towkir] Ahmed Assignee
	Comment 11 • 7 years ago

Ah, okay.

	Julian Descottes [:jdescottes] Reporter
	Comment 12 • 7 years ago

Comment on attachment 8914631 [details] [diff] [review]
mdn-docs-removal.patch

Review of attachment 8914631 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good Towkir, thanks! 
One final nit: there is a JS doc left to be removed, after that we should be good to go.

Try with your last patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f692a625d0188a0e6ceb5268c9913770a277463

::: devtools/client/shared/test/head.js
@@ +273,3 @@
>   * @param {Node} parent
>   * The DOM node whose children are the output of the syntax highlighter.
>   */

Please remove all the JS documentation that describes this method too.

	[:Towkir] Ahmed Assignee
	Comment 13 • 7 years ago

Nit done.

	Pulsebot
	Comment 14 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/acaf22b66faf
Remove MDN Docs widget. r=jdescottes

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 15 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/acaf22b66faf

	Chris Mills [:cmills]
	Comment 16 • 7 years ago

I added a note to the Fx58 rel notes mentioning this:

https://developer.mozilla.org/en-US/Firefox/Releases/58#Developer_Tools

I looked through the dev tools docs, and only found one mention of it on here (which I removed):

https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS

Let me know if you need anything else, docs-wise. Thanks!

	Julian Descottes [:jdescottes] Reporter
	Comment 17 • 7 years ago

Thanks! For the record I couldn't find any other reference to this on MDN.

	Daniele "Mte90" Scasciafratte
	Comment 18 • 7 years ago

Very sad that this feature was removed!
I know that many frontend developers and newbie use a lot and they use Firefox instead others for this feature.
Maybe before remove this kind of feature (also if broken) it will be better to ask to the community on discourse to listen the users :-/

	Julian Descottes [:jdescottes] Reporter
	Comment 19 • 7 years ago

(In reply to Daniele "Mte90" Scasciafratte from comment #18)
> Very sad that this feature was removed!
> I know that many frontend developers and newbie use a lot and they use
> Firefox instead others for this feature.
> Maybe before remove this kind of feature (also if broken) it will be better
> to ask to the community on discourse to listen the users :-/

This bug was about removing code that was no longer working due to APIs that changed on MDN side.
You can have a look at the conversation in https://bugzilla.mozilla.org/show_bug.cgi?id=1201600. Usage statistics indicated that the feature was used around 4000 times per year. That's extremely low if you consider the devtools' user base.

I think it still should have been announced on the mailing list, we should try to be more vocal when removing features. However the decision to keep the feature and maintain the code both on DevTools and MDN side must also be driven by the actual usage. In this case 4000 hits/year really feels too low to justify the maintenance cost.

	Daniele "Mte90" Scasciafratte
	Comment 20 • 7 years ago

I can understand the issue about the numbers but is a feature that has no so much life and maybe we can try to promote better.
After all is something that you need to know for use it and many people that use devtools doesn't catch easily this kind of stuff.

	Julian Descottes [:jdescottes] Reporter
	Comment 21 • 7 years ago

In general we want to add more links from DevTools to MDN. For instance the links that show up in the Console when there is a JS error have really been successful, because they're discoverable, not buried inside of a context menu.

Having links from the inspector to MDN is definitely something we want to keep working on. For instance showing a link when a user enters an invalid value for a CSS property. Easier to discover, more likely to be of interest for the user etc...

The general idea of linking to MDN is definitely not going away :) But the MDN Docs tooltip is going away, hoping to replace it with a better solution.

Regarding promoting features better, we could always communicate more to make people aware of a given feature. But in the end if it's not discoverable and used, there is something wrong with the feature itself.

	Sebastian Zartner [:sebo]
	Comment 22 • 7 years ago

I would have liked to say that you could use the 'mdn css' command of the Developer Toolbar (available via Ctrl+F2) instead, though unfortunately that feature is currently also broken. Therefore I've created bug 1416233.

Having said that, the GCLI is also not very discoverable. So I agree with Julian that other kinds of integrations with MDN should be created. I even remember discussions about a separate Documentation panel, though I can't find a related bug number.

Sebastian