768469 - [gcli] implement a "mdn" command

Status: VERIFIED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Paul Rouget [:paul]]

Attachment: screenshot

> mdn transform-origin

Should show the summary of the MDN doc and let the user open the full documentation in a new tab.

Comment 1

[Paul Rouget [:paul]]

Attachment: scratchpad prototype

Comment 2

[Andres Hernandez [:andreshm]]

Attachment: Progress patch

I have some questions:

1. The command is only for CSS properties (https://developer.mozilla.org/en-US/docs/CSS/property) or should we allow also XUL, DOM or other mdn elements?

> mdn toolbar (https://developer.mozilla.org/en-US/docs/XUL/toolbar)

2. The mdn pages don't have a #pageText element. So, it's ok to look for the #Summary element and use the following <p> elements?
<h2 id="Summary">Summary</h2>
<p>The <code>transform-origin</code> CSS property lets you modify the origin for transformations of an element. For example, the transform-origin of the <code>rotate()</code> function is the centre of rotation. (This property is applied by first translating the element by the negated value of the property, then applying the element's transform, then translating by the property value.)</p>
<p>Not explicitely set values are reset to their corresponding values.</p>

3. While testing, clicking on the link usually crash Firefox, any idea how can I solve this? 
I see the following on console:
JavaScript strict warning: resource:///modules/devtools/Promise.jsm, line 227: reference to undefined property this._id
WARNING: NS_ENSURE_TRUE(!mHasOrHasHadOwner || mOwner) failed: file ../../../dist/include/nsDOMEventTargetHelper.h, line 99
###!!! ABORT: aRelevantLinkVisited should only be set when we have a separate style: 'aRulesIfVisited || !aRelevantLinkVisited', file

Comment 3

[Paul Rouget [:paul]]

Hi Andres. Thanks for looking at this.
The mdn command should not be CSS only. So we need to fix bug 768456 first.

Comment 4

[Joe Walker [:jwalker]]

Comment on attachment 658309 [details] [diff] [review]
Progress patch

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

I've taken a look over the patch and it looks good to me. It looks like Les might have some suggestions about the 'I'm feeling lucky' (i.e. give me the best match you've got) in here: https://gist.github.com/3307756
Does that help?

Comment 5

[Andres Hernandez [:andreshm]]

Attachment: Progress patch v2

Updated patch based on Les solution. 

I think we can take the locale from Services.prefs.getCharPref("general.useragent.locale");

Btw, I found that nightly crashes when adding the html anchor <a>. Without it works fine. 

I'll wait for bug 768456.

Comment 6

[Joe Walker [:jwalker]]

New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"

Comment 7

[desiradaniel2007]

I've forked that gist [:walker] mentioned and here's what came out till now: https://gist.github.com/dannydes/6962839 There have not been any testing yet.

I am still unsure about DOM/Web API commands. Should they be in the same command or in a different one?

This is the first time I am submitting a patch and need some guidance on the coding style.

Comment 8

[Joe Walker [:jwalker]]

(In reply to desiradaniel2007 from comment #7)
> I've forked that gist [:walker] mentioned and here's what came out till now:
> https://gist.github.com/dannydes/6962839 There have not been any testing yet.
> 
> I am still unsure about DOM/Web API commands. Should they be in the same
> command or in a different one?
> 
> This is the first time I am submitting a patch and need some guidance on the
> coding style.

I suggest the best thing is to vote on bug 768456, getting that done will make this job *much* easier.

Comment 9

[Joe Walker [:jwalker]]

Resetting priorities because these P* levels are very out of date.
Sorry for the bug spam. Filter on Lobster Thermidor

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Andres Hernandez [:andreshm] from comment #2)
> ###!!! ABORT: aRelevantLinkVisited should only be set when we have a
> separate style: 'aRulesIfVisited || !aRelevantLinkVisited', file

This assertion has been removed in bug 575675.

Comment 11

[jre33p]

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

Comment 12

[Loic]

 [e-mail], could you stop playing with the flags or your account will be disabled, thanks.

Comment 13

[shivang nagaria]

I would like to work on this bug. Can you please assign this work to me.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to shivang nagaria from comment #13)
> I would like to work on this bug. Can you please assign this work to me.

Typically we'll wait until you've attached your first patch before assigning the bug.  You're welcome to get started!  I'd recommend checking out the DevTools hacking guide[1] to help you get started.

As for advice on what to do here, Patrick, would you be a good mentor for this based on reviewing the MDN tooltip bug?  I wasn't sure.

[1]: https://wiki.mozilla.org/DevTools/Hacking

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 658641 [details] [diff] [review]
Progress patch v2

Marking as obsolete now, it's been a very long while.

Comment 16

[Patrick Brosset <:pbro>]

I've marked the 2 very old patches on this bug as obsolete.
Feel free to get started and ask any questions you might have (you can also find us on IRC #devtools for usually quicker answers).
In terms of code, this will require a new GCLI command file to be written.
Most commands are in toolkit/devtools/gcli/commands, you should take a look at them to get some ideas about how they're written.
In order to actually call MDN to get a response, there's a lot of code that's been written for this already (this was done in bug 980006), so it should be relatively easy to reuse this code. Take a look at browser/devtools/shared/widgets/MdnDocsWidget.js

Comment 17

[Johan K. Jensen :johankj]

How should it be determined what to show based on a query?
Using the MDN Search API (as I’ve seen mentioned in bug 769572#c36)?
If so, should there be any exceptions when e.g. querying for "display" returns Web/SVG/Attribute/display rather than the (most likely) expected Web/CSS/display?
Even searching for "css display" will return Glossary/CSS rather than a more useful result.

Should the command have an argument for the topic? E.g. `mdn css background-color` or `mdn js setTimeout`?

When showing the information, should the Summary-section be the only thing or are there other parts of the MDN document which should be displayed? The MdnDocsWidget also shows the Syntax-section for CSS. Are there other sections which could/should be displayed? (Examples? Attributes?) Or does MDN contain too many topics for it to be united?

Comment 18

[Patrick Brosset <:pbro>]

Sorry for the delay here. Hopefully some answers below.

(In reply to Johan K. Jensen from comment #17)
> How should it be determined what to show based on a query?
> Using the MDN Search API (as I’ve seen mentioned in bug 769572#c36)?
> If so, should there be any exceptions when e.g. querying for "display"
> returns Web/SVG/Attribute/display rather than the (most likely) expected
> Web/CSS/display?
> Even searching for "css display" will return Glossary/CSS rather than a more
> useful result.
> 
> Should the command have an argument for the topic? E.g. `mdn css
> background-color` or `mdn js setTimeout`?
> 
> When showing the information, should the Summary-section be the only thing
> or are there other parts of the MDN document which should be displayed? The
> MdnDocsWidget also shows the Syntax-section for CSS. Are there other
> sections which could/should be displayed? (Examples? Attributes?) Or does
> MDN contain too many topics for it to be united?

MDN is a wiki and as such contains a large amount of documentation that's inconsistent in style and content. Along with the work that has been done in bug 980006, there's also been some work done on MDN to make sure the pages for CSS properties shared a common structure. So, yeah, I do think we should have an argument to the command (e.g. 'mdn css color') because various categories will have various output formats. Also because css is probably the only category we can be sure to have a properly structured output that can be parsed by the code in MdnDocsWidget.

Why don't we start with just this? Creating a command that can only search for css properties for now and that reuses most of the code in MdnDocsWidget to display a structured output?

We could create a more general command that attempted at displaying the content of the top result for any query, but if we did this, there would be not much added value over opening a new browser tab and searching mdn there.

Comment 19

[shivang nagaria]

Attachment: Basic work is done, just need to reformat the response.

Please review the patch and let me know if any improvement can be done.
Meanwhile, I'll try to format the response.

Comment 20

[Tim Nguyen :ntim]

Comment on attachment 8668942 [details] [diff] [review]
Basic work is done, just need to reformat the response.

Please only set one of the 2 flags.

Comment 21

[shivang nagaria]

I need some help in displaying the summary. I was successful in getting the response, but I not able to display it nicely. I did find something here[1] ,but I can't understand it. Please explain it.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/source/lib/gcli/commands/help.js#333-367

Comment 22

[Patrick Brosset <:pbro>]

Comment on attachment 8668942 [details] [diff] [review]
Basic work is done, just need to reformat the response.

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

That's a good start. I have made some comments which hopefully should help you progress. Let me know if you need more help on this.

::: devtools/shared/gcli/commands/mdn.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const { Cc, Ci, Cu, CC } = require("chrome");

None of these are really used, please remove this import.

@@ +4,5 @@
> +
> +"use strict";
> +
> +const { Cc, Ci, Cu, CC } = require("chrome");
> +const { Services } = require("resource://gre/modules/Services.jsm");

Services isn't used either, you can also remove this import.

@@ +6,5 @@
> +
> +const { Cc, Ci, Cu, CC } = require("chrome");
> +const { Services } = require("resource://gre/modules/Services.jsm");
> +const l10n = require("gcli/l10n");
> +const dirService = Cc["@mozilla.org/file/directory_service;1"]

And dirService is also not used, you can get rid of this too.

@@ +30,5 @@
> +      }
> +    ],
> +    returnType: "string",
> +    exec: function(args, context) {
> +      var cssSummary = null;

Please use 'let' instead of 'var' here.

@@ +33,5 @@
> +    exec: function(args, context) {
> +      var cssSummary = null;
> +      return getCssDocs(args.property).then(function(result) {
> +        var jsonString = JSON.stringify(result.valueOf());
> +	JSON.parse(jsonString, function(key, value) {

Why stringifying then parsring?
My understanding of the getCssDocs function is that the promise it returns resolves to an object that contains 2 properties: summary and syntax.
Summary is a string that you could display straight away, and syntax too, but if you wanted to highlight its syntax, you could use appendSyntaxHighlightedCSS which the MdnDocsWidget module also exports. Just like the the MDN tooltip does. See this: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/MdnDocsWidget.js#335

As for displaying the output on screen, you should take a look at the cookie command for example, because it does something similar: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/commands/cookie.js#142

::: toolkit/locales/en-US/chrome/global/devtools/gclicommands.properties
@@ +1586,5 @@
> +# LOCALIZATION NOTE (mdnDesc, mdnCssDesc, mdnCssProp) These strings describe the 'mdn' commands and
> +# all available parameters.
> +mdnDesc=Make use of mdn wiki pages.
> +mdnCssDesc=Read the documentation of specified css property.
> +mdnCssProp=Property Name

Can you write a separate comment for each of these strings? You can simply copy other occurrences of *Desc in the same file and change the right part.

Comment 23

[Patrick Brosset <:pbro>]

I believe my last comment should help but in case it's not enough, let me forward the needinfo for comment 21 to Joe who knows much more than me about displaying glci command outputs.

Comment 24

[Joe Walker [:jwalker]]

Hmm. The easiest thing to do is to just return a string, and you're done.

You can provide internal formatting using a view, see the cookie command that Patrick pointed you at for that. The docs on the templating thing are here: https://github.com/joewalker/domtemplate

You could in theory return HTML, but we would need to make sure there wasn't an XSS attack where someone edited an MDN article and then got their code injected into some chrome content, so we'd prefer to avoid that avenue if possible.

Comment 25

[shivang nagaria]

Attachment: Formatting also done.

Please review the patch and let me know if any improvement can be done.
I will also attach an screen-shot.

Thanks

Comment 26

[shivang nagaria]

Attachment: Scrren-shot of the work.

Comment 27

[Patrick Brosset <:pbro>]

Comment on attachment 8673727 [details] [diff] [review]
Formatting also done.

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

Nice progress. There are still a number of things to work on, but you're definitely getting there.

::: devtools/shared/gcli/commands/mdn.js
@@ +16,5 @@
> +    item: "command",
> +    runAt: "client",
> +    name: "mdn css",
> +    description: l10n.lookup("mdnCssDesc"),
> +    returnType: "cssDes",

What does cssDes mean here? I know it's just a string and you can pick anything as long as the converter below uses the same, but maybe choose something more self-explanatory, like cssPropertyOutput?

@@ +29,5 @@
> +    exec: function(args, context) {
> +      let cssSummary = [];
> +      return getCssDocs(args.property).then(function(result) {
> +        let jsonString = JSON.stringify(result.valueOf());
> +	JSON.parse(jsonString, function(key, value) {

I asked this in comment 22, what is the purpose of valueOf, JSON.stringify and JSON.parse here? Why can't you just use result?

@@ +34,5 @@
> +	  if (key === "summary") {
> +	    cssSummary.push({
> +	      summary: value,
> +	      property: args.property,
> +	      url: "https://developer.mozilla.org/en-US/docs/Web/css/" + args.property

Please reuse PAGE_LINK_URL from MdnDocsWidget.js instead.
In order to be able to do this, you'll need to modify that file so it 'exports' that variable:

var PAGE_LINK_URL = exports.PAGE_LINK_URL = "https://developer.mozilla.org/docs/Web/CSS/";
instead of
var PAGE_LINK_URL = "https://developer.mozilla.org/docs/Web/CSS/";

This way, the URL is only in one place and cna be changed only there if it ever needs to change.

@@ +39,5 @@
> +	    });
> +  	  }
> +	});
> +	return cssSummary;
> +	}, function(error) { 

nit: indentation is off here.

@@ +43,5 @@
> +	}, function(error) { 
> +	     cssSummary.push({
> +	       summary: error,
> +	       property: args.property,
> +	       url: "https://developer.mozilla.org/en-US/docs/Web/css/" + args.property

Same comment about reusing PAGE_LINK_URL here.

@@ +54,5 @@
> +  {
> +    item: "converter",
> +    from: "cssDes",
> +    to: "view",
> +    exec: function(CssDesList, context) {

CssDesList should not be capitalized, and also, can you choose a more self-explanatory name here too?

@@ +55,5 @@
> +    item: "converter",
> +    from: "cssDes",
> +    to: "view",
> +    exec: function(CssDesList, context) {
> +      if (CssDesList[0].summary === 404) {

Isn't summary a String? You're comparing it to a number here.

@@ +58,5 @@
> +    exec: function(CssDesList, context) {
> +      if (CssDesList[0].summary === 404) {
> +	return context.createView({
> +	  html:
> +	    '<p>' + 'CSS property: ' + CssDesList[0].property + ' does not exist.' + '</p>'

You seem to only ever be interested in the first item of this array. Can the array have more than 1 item? Can it have 0 items? Maybe you should be assigning a local variable to make the code simpler:

let propertyData = cssDesList[0];

@@ +66,5 @@
> +	html: 
> +	  '<div>' + 
> +	  '<p style="float: right">' + CssDesList[0].url + '</p>' +
> +	  '<p style="font-weight: bold; display: inline">' + CssDesList[0].property + '</p>' +
> +	  '<h2>Summary</h2>' + '<p>' + CssDesList[0].summary + '</p>' +

The context.createView function works with a template languages that avoids this kind of string concatenation.
This would work for instance:

html: 
  '<div>' +
  '<p style="float: right">${url}</p>' +
  '<p style="font-weight: bold; display: inline">${name}</p>' +
  '<h2>Summary</h2><p>${summary}</p>' +
  '</div>',
data: {
  url: cssDesList[0].url,
  name: cssDesList[0].property,
  summary: cssDesList[0].summary
}

*But* now we have ES2015 template strings, so we could use this too (plus they work multiline too):

html: 
 `<div>
    <p style="float: right">${CssDesList[0].url}</p>
    <p style="font-weight: bold; display: inline">${CssDesList[0].property}</p>
    <h2>Summary</h2>
    <p>${CssDesList[0].summary}</p>
  </div>`

2 other comments about this code:
- please use L10N for all strings displayed to the user. So here, Summary should also be localized in the .properties file
- please don't use inline CSS styles, please use CSS classes instead and add the rules and properties to devtools\client\commandline\commandline.css

Comment 28

[shivang nagaria]

Thanks for review.I'll submit the next patch soon and can you please this bug to me ?

Comment 29

[Joe Walker [:jwalker]]

> *But* now we have ES2015 template strings, so we could use this too (plus they work multiline too):

True, but template strings are an XSS hole where using normal strings with domtemplate isn't. domtemplate does an innerHtml on the whole thing, and then walks the DOM looking for ${} markers, filling them out as it goes.

Net result - the browser does XSS escaping for normal JS strings with domtemplate, but nothing does it for template strings.

Comment 30

[shivang nagaria]

Attachment: bug_768469_3.diff

Comment 31

[shivang nagaria]

The response coming from the css docs pages are in `json`, and the `JSON.parse` takes string as input that's why I used this function, and converting the response into string.

Comment 32

[Patrick Brosset <:pbro>]

(In reply to shivang nagaria from comment #31)
> The response coming from the css docs pages are in `json`, and the
> `JSON.parse` takes string as input that's why I used this function, and
> converting the response into string.
The result of the getCssDocs method is a javascript object. It's transmitted over the wire as a json string, yes, but once it gets back it's already transformed as a usable javascript object.
For instance, if I run this simplified test code locally:

  let {devtools} = Cu.import("resource://devtools/shared/Loader.jsm", {});
  let {getCssDocs} = devtools.require("devtools/client/shared/widgets/MdnDocsWidget");
  getCssDocs("border").then(result => {
    console.log(result)
  });

I get an actual javascript object in the console that looks like this:

Object { summary: "The border CSS property is a shorth…", syntax: "border: 1px; border: 2px dotted; bo…" }

And I could just use result.summary or result.syntax directly.
What I find weird in your code is that before using JSON.parse, you do a JSON.stringify. You should get rid of

Comment 33

[Patrick Brosset <:pbro>]

Comment on attachment 8703434 [details] [diff] [review]
bug_768469_3.diff

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

::: devtools/shared/gcli/commands/mdn.js
@@ +29,5 @@
> +    exec: function(args, context) {
> +      let cssSummary = [];
> +      return getCssDocs(args.property).then(
> +	function(result) {
> +          let jsonString = JSON.stringify(result);

You should remove this line.

@@ +31,5 @@
> +      return getCssDocs(args.property).then(
> +	function(result) {
> +          let jsonString = JSON.stringify(result);
> +
> +	  JSON.parse(jsonString, function(key, value) {

And then, you don't need to parse the string again.
Something like this should work better:


getCssDocs(args.property).then(result => {
  cssSummary.push({
    summary: result.summary,
    ...
  });
});

Also, why do you push to an array? Can there ever be more than one entries in this array?

::: devtools/shared/locales/en-US/gclicommands.properties
@@ +1594,5 @@
> +# LOCALIZATION NOTE (mdnCssDesc) A very short string used to describe the
> +# result of the 'mdn css' commmand.
> +mdnCssDesc=Provide the summary of specified css property.
> +# LOCALIZATION NOTE (mdnCssProp) String use to describe the parameter used
> +# in the 'mdn css' command.

Please make this comment (and the 2 following ones) unique. For now they are exactly the same: "String use to describe the parameter used in the 'mdn css' command."
For instance here, you could write something like this instead:
"String used to describe the 'property name' parameter used in the 'mdn css' command."

@@ +1599,5 @@
> +mdnCssProp=Property Name
> +# LOCALIZATION NOTE (cssPropStr) String use to describe the parameter used
> +# in the 'mdn css' command.
> +cssPropStr=Css Property
> +# LOCALIZATION NOTE (cssSummeryStr) String use to describe the parameter used

Typo here, cssSummeryStr should be cssSummaryStr

Comment 34

[shivang nagaria]

Attachment: bug_768469_4.diff

I can't show the error if the user type any random words with this command.

Comment 35

[Patrick Brosset <:pbro>]

Sorry for the delay. I plan on taking a look at your patch Monday next week.
Clearning the NI? flag, the review flag will be enough.

Comment 36

[Patrick Brosset <:pbro>]

Comment on attachment 8707061 [details] [diff] [review]
bug_768469_4.diff

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

::: devtools/shared/gcli/commands/mdn.js
@@ +26,5 @@
> +	description: l10n.lookup("mdnCssProp")
> +      }
> +    ],
> +    exec: function(args, context) {
> +      return getCssDocs(args.property).then(result => 

The getCssDocs function returns a promise that *rejects* when the css property can not be found. So you need to define a rejection handler if you want to be notified of 404 and other types of errors. See the jsdoc of the getCssDocs function here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/MdnDocsWidget.js#188

You can define a rejection handler like this:

return getCssDocs(args.property).then(result => {
  return {
    summary: result.summary,
    url: PAGE_LINK_URL + args.property,
    property: args.property
  };
}, error => {
  return {
    error: error,
    property: args.property
  };
});

This way, you're catching the error, and sending a different object to your converter when it occurs.

@@ +47,5 @@
> +
> +      let cssPropStr = l10n.lookup("cssPropStr");
> +      let cssSummaryStr = l10n.lookup("cssSummaryStr");
> +	
> +      if (cssSummary == 404) {

Remove this whole block here, cssSummary is never going to be equal to the number 404.
Instead, you should look at the cssDoc object (which is the object your command returns) and have an `if (cssDoc.error) {}`.
If the object contains an error, then you should create a view that displays the error.
If there's no error, then display the normal view.

Comment 37

[shivang nagaria]

Attachment: bug_768469_5.diff

Please review and let me know what are the improvements can be done.
Can you also give me some bugs, I really want to contribute more. Thanks!

Comment 38

[Tim Nguyen :ntim]

(In reply to shivang nagaria from comment #37)
> Created attachment 8709592 [details] [diff] [review]
> bug_768469_5.diff
> 
> Please review and let me know what are the improvements can be done.
> Can you also give me some bugs, I really want to contribute more. Thanks!

This patch seems to miss a file.

Comment 39

[shivang nagaria]

Attachment: bug_768469_6.diff

Sorry for the wrong patch

Comment 40

[Patrick Brosset <:pbro>]

Comment on attachment 8709853 [details] [diff] [review]
bug_768469_6.diff

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

Almost there. I think this looks pretty good.
I've made a few suggestions to make the code easier to read and more in line with our coding standards.

::: devtools/shared/gcli/commands/mdn.js
@@ +28,5 @@
> +    ],
> +    exec: function(args, context) {
> +      return getCssDocs(args.property).then(result =>
> +	  ({
> +	    summary: result.summary,

Why not pass the whole result object here? Maybe we want to only display the summary for now, but we might want to add more info later.
So maybe:

return getCssDocs(args.property).then(result => {
  return {
    data: result,
    url: PAGE_LINK_URL + args.property,
    property: args.property
  };
}, error => {
  return {
    error,
    property: args.property
  };
});

@@ +46,5 @@
> +    exec: function(cssDoc, context) {
> +      let cssProperty = cssDoc.property;
> +
> +      let cssPropStr = l10n.lookup("cssPropStr");
> +      let cssSummaryStr = l10n.lookup("cssSummaryStr");

These 2 variable names are a bit confusing because they look very much like the cssProperty variable defined before.
So just reading their names, it's hard to know what value they hold. Does 'Str' mean that they are string values, or localized strings.
Also, they are just used in one place, so I think you can get rid of them altogether, and just use 'l10n.lookup(...)' below when creating the HTML.

@@ +48,5 @@
> +
> +      let cssPropStr = l10n.lookup("cssPropStr");
> +      let cssSummaryStr = l10n.lookup("cssSummaryStr");
> +
> +      if (cssDoc.error) {

This block should come first in the exec function. This way we can check first if there's an error and return early.
This would also make the function easier to read.

@@ +52,5 @@
> +      if (cssDoc.error) {
> +	// The css property specified doesn't exist.
> +	return context.createView({
> +	  html:
> +	    '<p>' + cssPropStr + ': ' + cssProperty + ' does not exist.' + '</p>'

This needs to be localized too in gclicommands.properties.
Instead of using cssPropStr here and concatenating various strings, you should instead create a new string

mdnCssPropertyNotFound=MDN documentation for the CSS property %1$S was not found

Which you can then simply use like this:

"<p>" + l10n.lookupFormat("mdnCssPropertyNotFound", [cssProperty]) + "</p>";

Also, please use only double-quotes for javascript strings.

::: devtools/shared/locales/en-US/gclicommands.properties
@@ +1613,5 @@
>  folderOpenDirResult=Opened %1$S
>  
> +# LOCALIZATION NOTE (mdnDesc) A very short string used to describe the
> +# use of 'mdn' command.
> +mdnDesc=Make use of mdn wiki pages.

Maybe rephrase to:
Retrieve documentation from MDN

@@ +1616,5 @@
> +# use of 'mdn' command.
> +mdnDesc=Make use of mdn wiki pages.
> +# LOCALIZATION NOTE (mdnCssDesc) A very short string used to describe the
> +# result of the 'mdn css' commmand.
> +mdnCssDesc=Provide the summary of specified css property.

Maybe better as:
Retrieve documentation about a given CSS property name

@@ +1619,5 @@
> +# result of the 'mdn css' commmand.
> +mdnCssDesc=Provide the summary of specified css property.
> +# LOCALIZATION NOTE (mdnCssProp) String used to describe the 'property name'
> +# parameter used in the 'mdn css' command.
> +mdnCssProp=Property Name

No need to capitalize Name here:
Property name

@@ +1622,5 @@
> +# parameter used in the 'mdn css' command.
> +mdnCssProp=Property Name
> +# LOCALIZATION NOTE (cssPropStr) String used to label the 'css property name'
> +# in the result of the 'mdn css' command.
> +cssPropStr=Css Property

No need to capitalize Property here. However CSS should be all caps:
CSS property

Comment 41

[shivang nagaria]

Attachment: bug_768469_7.diff

Comment 42

[Patrick Brosset <:pbro>]

Comment on attachment 8711324 [details] [diff] [review]
bug_768469_7.diff

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

Thanks for this new patch. It looks good. Almost ready to go in I think.
I only have a few minor comments about:
- the formatting of the file is off in places, you can easily check this by using eslint locally: https://wiki.mozilla.org/DevTools/CodingStandards
- the link to MDN should be clickable (to fix this, it's a bit complex however, just using a <a href> element won't work for security reasons, so you'd have to add an event listener with javascript, which means changing the "view" return type to "dom"

Instead of listing all these minor points here and asking you to re-upload a patch, I'll upload a corrected patch here.

Comment 43

[Patrick Brosset <:pbro>]

Attachment: Bug_768469_-_[gcli]_implement_a__mdn__command._r_p.diff

Here's a new version of your patch with all the comments I had about your last patch addressed.
Please take a look at it and let me know if you need any explanation about any of the changes.
I've kept the author name unchanged since you wrote 99% of the patch, I only corrected the minor remaining problems.

Comment 44

[Patrick Brosset <:pbro>]

Pending try push to make sure all tests still pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=20b0fbfeaee6&group_state=expanded

Comment 45

[Patrick Brosset <:pbro>]

Alright, try is green-enough to confirm there's no regression now. Let's ask for this patch to be checked in!
Thanks Shivang for your work on this! This should be in FF47 if everything goes well.

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1c89f7306a4d

Comment 47

[shivang nagaria]

Thanks Patrick for great help.
I have few question about the patch:

1.Why did you use "dom" in place of "view" (line no. 42 in mdn.js file) ?
2.Why are 4 tests failed, what's the reason ?

Also can you give me some similar bugs to work on, I'd love to work.

Comment 48

[Greg Tatum [:gregtatum]]

I believe this triggered an error in the mochitests that I just ran into. Here's the output:

console.error:
  Failed to load module devtools/shared/gcli/commands/mdn: Module `devtools/shared/gcli/commands/mdn` is not found at resource://devtools/shared/gcli/commands/mdn.js

The `devtools/shared/gcli/commands/mdn` was not added to the `devtools/shared/gcli/commands/mdn/moz.build` file.

Comment 49

[Patrick Brosset <:pbro>]

(In reply to shivang nagaria from comment #47)
> Thanks Patrick for great help.
> I have few question about the patch:
> 
> 1.Why did you use "dom" in place of "view" (line no. 42 in mdn.js file) ?
In order to make the link clickable (to open the mdn page in a new tab), I had to resort to using javascript. For security reasons, I couldn't just use a <a href="..."> link because the iframe that the gcli output is loaded in is in a sandbox.
Because I'm using javascript to register the event listener on the url dom element, I had to create DOM manually too, and so, that's why the converter now returns "dom" instead of "view". "view" is a special output type that gcli knows how to handle and transform to DOM, but you can instead give it real DOM elements too.
> 2.Why are 4 tests failed, what's the reason ?
The tests that are failing on the TRY push are all unrelated. We sometimes (often) have what's called intermittent oranges: tests that only fail some of the times, under certain circumstances. We're trying to fix those as we go and make sure they don't fail too often, but we do have them, and it means that TRY pushes are rarely (never) all green.
> Also can you give me some similar bugs to work on, I'd love to work.
Awesome! Thanks for your interest.
One way is to browse bugs on http://firefox-dev.tools/ (you can select "easy bugs only" and then all, one or more tools from the sidebar).
There seems to be an icon-related easy bug on the performance tool, if that's something interesting to you: bug 1224660
Also bug 1224819 is mentored, someone started but never finished it, it might be a good idea to take over from them.

Comment 50

[Patrick Brosset <:pbro>]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #48)
> I believe this triggered an error in the mochitests that I just ran into.
> Here's the output:
> 
> console.error:
>   Failed to load module devtools/shared/gcli/commands/mdn: Module
> `devtools/shared/gcli/commands/mdn` is not found at
> resource://devtools/shared/gcli/commands/mdn.js
> 
> The `devtools/shared/gcli/commands/mdn` was not added to the
> `devtools/shared/gcli/commands/mdn/moz.build` file.
I've just pushed a fix to fx-team in bug 1243682. I was able to reproduce locally (on my other computer) and this fixed it.
Sorry about that.
Somehow, this never was a problem when I tested the patch locally. My suspicion is that because we're able to reload devtools files from source, this somehow allowed the new command to be loaded even though it was never declared in the build config file.
Because the way we reload files is that we map the entire devtools/ top-level directory to our local check-out, and our module paths match the file-system path. So gcli was able, locally, to just load the command this way.

Alex: do you think this is what has happened? Is there a way we can prevent this in the future? As a reviewer, I often don't think about checking the corresponding moz.build, jar.mn files and if it somehow works when I test, this might slip by, unnoticed.

Comment 51

[Alexandre Poirot [:ochameau]]

Hum. We could craft some script to check that but I have no idea how/when it is going to be run.
But in general, this used to fail when running tests.
Was this feature covered by a test?

Comment 52

[Patrick Brosset <:pbro>]

(In reply to Alexandre Poirot [:ochameau] from comment #51)
> Hum. We could craft some script to check that but I have no idea how/when it
> is going to be run.
> But in general, this used to fail when running tests.
> Was this feature covered by a test?
Right, a test that tried to run the command would have caught this.
No new test was added because: most of what the command does is call getCssDocs, which is already tested somewhere else. And all other command tests I see seem pretty pointless to me because they usually only test the validity of gcli's autocompletion mechanism, which isn't going to differ based on the command.
But I guess I missed one important point when reviewing: that we should at least have 1 test that tries to simply run the command.

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1c89f7306a4d

Comment 54

[Will Bamberg [:wbamberg]]

I've added the command to the docs page for GCLI: https://developer.mozilla.org/en-US/docs/Tools/GCLI#Commands.

Comment 56

[Khalid Syfullah Zaman [:khalid32]]

Since there's no 'Developer Toolbar' on reported build, I have reproduced this bug on Nightly 46.0a1 (2016-01-13) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Nightly 48.0a1!
Build ID: 20160329030246
User Agent: Mozilla/5.0 (X11; Linux i686; rv:48.0) Gecko/20100101 Firefox/48.0

Also the bug's fix is now verified on Latest Developer Edition 47.0a2!
Build ID: 20160328004018
User Agent: Mozilla/5.0 (X11; Linux i686; rv:47.0) Gecko/20100101 Firefox/47.0