684848 - Gather graphics info/failures in endurance shared module

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Add remaining details from about:support to the endurance report.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Gather graphics info/failures in endurance shared module. v1.0

Initial version of patch. Much of this code is based on http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.js

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 560254 [details] [diff] [review]
Gather graphics info/failures in endurance shared module. v1.0

>+  graphics = new graphics.GraphicsManager();
>+  frame.events.fireEvent('graphicsInfo', graphics.info);
>+  frame.events.fireEvent('graphicsFailures', graphics.failures);

Can we have a single event for info and failures? We would end-up in a single property under system_information in any way.

>+function GraphicsManager() {

Basically it's not a manager. We are only getting information and can't manage anything. Just use Graphics?

>+  this._info = [];
>+  this._failures = [];

Those two lines should be on top of the gather method.

>+    try {
>+      // nsIGfxInfo is currently only implemented on Windows
>+      this._gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo);
>+    } catch(e) {}

A .getService call has only to happen once in a module. Make it global as what we have in other modules.

>+    if (this._gfxInfo) {
[..]
>+      if (mozmill.isWindows) {

This if condition is not necessary because we also check for _gfxInfo which is only available on Windows.

>+        var version = Cc["@mozilla.org/system-info;1"]
>+                      .getService(Ci.nsIPropertyBag2)
>+                      .getProperty("version");
>+        var isWindowsVistaOrHigher = (parseFloat(version) >= 6.0);
>+        if (isWindowsVistaOrHigher) {
>+          var d2dEnabled = "false";
>+          try {
>+            d2dEnabled = this._gfxInfo.D2DEnabled;
>+          } catch(e) {}
>+          this._pushFeatureInfo("direct2DEnabled", this._gfxInfo.FEATURE_DIRECT2D, d2dEnabled);

If we are using external code please make sure to put a comment at the top that you have taken it from somewhere else. Not sure about the coding style yet, if we should adapt it to our own style or not. I would still prefer it. Right now it's kinda hard to read with all those missing blank lines and not-hanging if/else or try/catch blocks.

>+      if (mozmill.isWindows) {

Same as above.

>+      this._pushFeatureInfo("webglRenderer", webglfeature, webglenabled, webglrenderer);

4th parameter of the method says 'message'. How does it correlate to webglrenderer which is a boolean?

>+      if (mozmill.isWindows) {
>+        var feature = this._gfxInfo.FEATURE_DIRECT3D_9_LAYERS;
>+      } else {
>+        var feature = this._gfxInfo.FEATURE_OPENGL_LAYERS;
>+      }

We should better check for this._gfxInfo in the if condition.

>+  _errorMessageForFeature : function GraphicsManager_errorMessageForFeature(feature) {

Please define the action: _getErrorMessageForFeature()

>+  _pushInfo : function GraphicsManager_pushInfo(name, value) {
>+    if (value) {
>+      this._info.push({"label": this._bundle.GetStringFromName(name),
>+                       "value": value});

We should not only push entries which value is true. Instead we should push everything.

>+  _pushFeatureInfo : function GraphicsManager_pushFeatureInfo(name, feature, isEnabled, message) {
>+    message = message || isEnabled;
>+    if (!isEnabled) {
>+      var errorMessage = this._errorMessageForFeature(feature);
>+      if (errorMessage)
>+        message = errorMessage;
>+    }
>+    this._pushInfo(name, message);

The 2nd parameter of _pushInfo is a value for the information 'name'. Pushing in a message here, is quite confusing. Can you please elaborate?

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Gather graphics info/failures in endurance shared module. v1.1

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Comment on attachment 560254 [details] [diff] [review]
> Gather graphics info/failures in endurance shared module. v1.0
> 
> >+  graphics = new graphics.GraphicsManager();
> >+  frame.events.fireEvent('graphicsInfo', graphics.info);
> >+  frame.events.fireEvent('graphicsFailures', graphics.failures);
> 
> Can we have a single event for info and failures? We would end-up in a
> single property under system_information in any way.

Done.

> >+function GraphicsManager() {
> 
> Basically it's not a manager. We are only getting information and can't
> manage anything. Just use Graphics?

Done.

> >+  this._info = [];
> >+  this._failures = [];
> 
> Those two lines should be on top of the gather method.

Done.

> >+    try {
> >+      // nsIGfxInfo is currently only implemented on Windows
> >+      this._gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo);
> >+    } catch(e) {}
> 
> A .getService call has only to happen once in a module. Make it global as
> what we have in other modules.

Done.

> >+    if (this._gfxInfo) {
> [..]
> >+      if (mozmill.isWindows) {
> 
> This if condition is not necessary because we also check for _gfxInfo which
> is only available on Windows.

This is from the mozilla-central code, and I'm guessing that it's because nsIGfxInfo is *currently* only implemented on Windows. See: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.js#234

> >+        var version = Cc["@mozilla.org/system-info;1"]
> >+                      .getService(Ci.nsIPropertyBag2)
> >+                      .getProperty("version");
> >+        var isWindowsVistaOrHigher = (parseFloat(version) >= 6.0);
> >+        if (isWindowsVistaOrHigher) {
> >+          var d2dEnabled = "false";
> >+          try {
> >+            d2dEnabled = this._gfxInfo.D2DEnabled;
> >+          } catch(e) {}
> >+          this._pushFeatureInfo("direct2DEnabled", this._gfxInfo.FEATURE_DIRECT2D, d2dEnabled);
> 
> If we are using external code please make sure to put a comment at the top
> that you have taken it from somewhere else. Not sure about the coding style
> yet, if we should adapt it to our own style or not. I would still prefer it.
> Right now it's kinda hard to read with all those missing blank lines and
> not-hanging if/else or try/catch blocks.

Comment added as @see annotation. Please provide feedback on whether this is the correct place.

> >+      if (mozmill.isWindows) {
> 
> Same as above.

See my response above.

> >+      this._pushFeatureInfo("webglRenderer", webglfeature, webglenabled, webglrenderer);
> 
> 4th parameter of the method says 'message'. How does it correlate to
> webglrenderer which is a boolean?

I agree this is odd. It's taken from http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.js#305. See my final comment about refactoring.

> >+      if (mozmill.isWindows) {
> >+        var feature = this._gfxInfo.FEATURE_DIRECT3D_9_LAYERS;
> >+      } else {
> >+        var feature = this._gfxInfo.FEATURE_OPENGL_LAYERS;
> >+      }
> 
> We should better check for this._gfxInfo in the if condition.

See my response above.

> >+  _errorMessageForFeature : function GraphicsManager_errorMessageForFeature(feature) {
> 
> Please define the action: _getErrorMessageForFeature()

Done.

> >+  _pushInfo : function GraphicsManager_pushInfo(name, value) {
> >+    if (value) {
> >+      this._info.push({"label": this._bundle.GetStringFromName(name),
> >+                       "value": value});
> 
> We should not only push entries which value is true. Instead we should push
> everything.

Are you sure? This creates a more verbose report, although we can style these reports to be more compact. See http://mozmill.blargon7.com/#/endurance/report/64cceae8f12ef60cd4077db5e8004ffb as an example of a report with everything.

> >+  _pushFeatureInfo : function GraphicsManager_pushFeatureInfo(name, feature, isEnabled, message) {
> >+    message = message || isEnabled;
> >+    if (!isEnabled) {
> >+      var errorMessage = this._errorMessageForFeature(feature);
> >+      if (errorMessage)
> >+        message = errorMessage;
> >+    }
> >+    this._pushInfo(name, message);
> 
> The 2nd parameter of _pushInfo is a value for the information 'name'.
> Pushing in a message here, is quite confusing. Can you please elaborate?

Again, I agree this is odd, and from http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.js#209

Initially I was reluctant to distance from the original code, as future improvements to aboutSupport.js should be reflected in this new shared module. I'm happy to distance the two if that's what you feel is appropriate here.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 561165 [details] [diff] [review]
Gather graphics info/failures in endurance shared module. v1.1

(In reply to Dave Hunt (:davehunt) from comment #3)
> > >+    if (this._gfxInfo) {
> > [..]
> > >+      if (mozmill.isWindows) {
> > 
> > This if condition is not necessary because we also check for _gfxInfo which
> > is only available on Windows.
> 
> This is from the mozilla-central code, and I'm guessing that it's because
> nsIGfxInfo is *currently* only implemented on Windows. See:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.
> js#234

Right. But I talk about the second if condition here. It doesn't make sense to have it here, because _gfxInfo is already checked and this variable is null on non-Windows systems. So please remove any mozmill.isWindows checks.

>+function Graphics() {
>+  this._bundle = Services.strings.createBundle("chrome://global/locale/aboutSupport.properties");
>+
>+  try {
>+    // nsIGfxInfo is currently only implemented on Windows
>+    this._gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo);
>+  } catch(e) {}

As mentioned in the last review. This has to be a global variable. Also please initialize it lazily.

>+  /**
>+   * Gather graphics info and failures
>+   * @see http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.js
>+   */

Please add more content here so it's clear that we have c&p'ed the code from Firefox. Our licenses are the same so a license reference shouldn't be necessary.

> > >+      this._pushFeatureInfo("webglRenderer", webglfeature, webglenabled, webglrenderer);
> > 
> > 4th parameter of the method says 'message'. How does it correlate to
> > webglrenderer which is a boolean?
> 
> I agree this is odd. It's taken from
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.
> js#305. See my final comment about refactoring.

So lets leave it in as it is. We could improve the code for Firefox instead, if wanted. Nothing important for now.

>+  /**
>+   * Get the error message for a particular feature
>+   */
>+  _getErrorMessageForFeature : function Graphics_getErrorMessageForFeature(feature) {

Because that's also a method in the original code we should probably leave the old naming and remove 'get'. Otherwise it's a too big hassle to update the code.

> > >+  _pushInfo : function GraphicsManager_pushInfo(name, value) {
> > >+    if (value) {
> > >+      this._info.push({"label": this._bundle.GetStringFromName(name),
> > >+                       "value": value});
> > 
> > We should not only push entries which value is true. Instead we should push
> > everything.
> 
> Are you sure? This creates a more verbose report, although we can style
> these reports to be more compact. See
> http://mozmill.blargon7.com/#/endurance/report/
> 64cceae8f12ef60cd4077db5e8004ffb as an example of a report with everything.

That's fine. It makes it way easier for us in the dashboard code to compare different reports. We only should fallback to 'Unknown' in the dashboard for those entries and not display empty strings. But that's not part of this patch.

>+  _pushFeatureInfo : function Graphics_pushFeatureInfo(name, feature, isEnabled, message) {
>
> > The 2nd parameter of _pushInfo is a value for the information 'name'.
> > Pushing in a message here, is quite confusing. Can you please elaborate?
> 
> Again, I agree this is odd, and from
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.
> js#209 

Leave it as it is.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Comment on attachment 561165 [details] [diff] [review] [diff] [details] [review]
> Gather graphics info/failures in endurance shared module. v1.1
> 
> (In reply to Dave Hunt (:davehunt) from comment #3)
> > > >+    if (this._gfxInfo) {
> > > [..]
> > > >+      if (mozmill.isWindows) {
> > > 
> > > This if condition is not necessary because we also check for _gfxInfo which
> > > is only available on Windows.
> > 
> > This is from the mozilla-central code, and I'm guessing that it's because
> > nsIGfxInfo is *currently* only implemented on Windows. See:
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.
> > js#234
> 
> Right. But I talk about the second if condition here. It doesn't make sense
> to have it here, because _gfxInfo is already checked and this variable is
> null on non-Windows systems. So please remove any mozmill.isWindows checks.

I'm also talking about the second if condition. Again, this is from the mozilla-central, which also does both of these checks. If the nsIGfxInfo is ever implemented on non-Windows then would we need this second check? That's my only guess for why the two checks are there in mozilla-central.

> >+function Graphics() {
> >+  this._bundle = Services.strings.createBundle("chrome://global/locale/aboutSupport.properties");
> >+
> >+  try {
> >+    // nsIGfxInfo is currently only implemented on Windows
> >+    this._gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo);
> >+  } catch(e) {}
> 
> As mentioned in the last review. This has to be a global variable. Also
> please initialize it lazily.

Sure. Could you point me to an example?

> >+  /**
> >+   * Gather graphics info and failures
> >+   * @see http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.js
> >+   */
> 
> Please add more content here so it's clear that we have c&p'ed the code from
> Firefox. Our licenses are the same so a license reference shouldn't be
> necessary.

What would you like? An overview of the functionality copied, or more specifics such as methods, etc? Do you want me to keep the @see annotation or reference the source another way?

> >+  /**
> >+   * Get the error message for a particular feature
> >+   */
> >+  _getErrorMessageForFeature : function Graphics_getErrorMessageForFeature(feature) {
> 
> Because that's also a method in the original code we should probably leave
> the old naming and remove 'get'. Otherwise it's a too big hassle to update
> the code.

To be clear, you want me to revert this to 'errorMessageForFeature'?

> > > >+  _pushInfo : function GraphicsManager_pushInfo(name, value) {
> > > >+    if (value) {
> > > >+      this._info.push({"label": this._bundle.GetStringFromName(name),
> > > >+                       "value": value});
> > > 
> > > We should not only push entries which value is true. Instead we should push
> > > everything.
> > 
> > Are you sure? This creates a more verbose report, although we can style
> > these reports to be more compact. See
> > http://mozmill.blargon7.com/#/endurance/report/
> > 64cceae8f12ef60cd4077db5e8004ffb as an example of a report with everything.
> 
> That's fine. It makes it way easier for us in the dashboard code to compare
> different reports. We only should fallback to 'Unknown' in the dashboard for
> those entries and not display empty strings. But that's not part of this
> patch.

Agreed, I'll consider that when updating the dashboard patch.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Dave Hunt (:davehunt) from comment #5)
> I'm also talking about the second if condition. Again, this is from the
> mozilla-central, which also does both of these checks. If the nsIGfxInfo is
> ever implemented on non-Windows then would we need this second check? That's
> my only guess for why the two checks are there in mozilla-central.

I have now checked the original code and it seems like those entries will only exist on Windows then. So they are fetching extra platform specific entries. We can leave it as it is for now.

> > >+  try {
> > >+    // nsIGfxInfo is currently only implemented on Windows
> > >+    this._gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo);
> > >+  } catch(e) {}
> > 
> > As mentioned in the last review. This has to be a global variable. Also
> > please initialize it lazily.
> 
> Sure. Could you point me to an example?

See the services module of the API refactoring project. There are some examples.

> > Please add more content here so it's clear that we have c&p'ed the code from
> > Firefox. Our licenses are the same so a license reference shouldn't be
> > necessary.
> 
> What would you like? An overview of the functionality copied, or more
> specifics such as methods, etc? Do you want me to keep the @see annotation
> or reference the source another way?

The @see reference is fine and will make it available in our docs too. In general just say that this code has been copied from the Firefox code. Feel free to add lines of code you have had to change, which will make it easier for future updates. Probably the @see has be global for the whole file. Otherwise we would have to reference each single function.

> > Because that's also a method in the original code we should probably leave
> > the old naming and remove 'get'. Otherwise it's a too big hassle to update
> > the code.
> 
> To be clear, you want me to revert this to 'errorMessageForFeature'?

Yes.

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Gather graphics info/failures in endurance shared module. v1.2

(In reply to Henrik Skupin (:whimboo) from comment #6)
> > > >+  try {
> > > >+    // nsIGfxInfo is currently only implemented on Windows
> > > >+    this._gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo);
> > > >+  } catch(e) {}
> > > 
> > > As mentioned in the last review. This has to be a global variable. Also
> > > please initialize it lazily.
> > 
> > Sure. Could you point me to an example?
> 
> See the services module of the API refactoring project. There are some
> examples.

Thanks for your help with this.

> > > Please add more content here so it's clear that we have c&p'ed the code from
> > > Firefox. Our licenses are the same so a license reference shouldn't be
> > > necessary.
> > 
> > What would you like? An overview of the functionality copied, or more
> > specifics such as methods, etc? Do you want me to keep the @see annotation
> > or reference the source another way?
> 
> The @see reference is fine and will make it available in our docs too. In
> general just say that this code has been copied from the Firefox code. Feel
> free to add lines of code you have had to change, which will make it easier
> for future updates. Probably the @see has be global for the whole file.
> Otherwise we would have to reference each single function.

Done.

> > > Because that's also a method in the original code we should probably leave
> > > the old naming and remove 'get'. Otherwise it's a too big hassle to update
> > > the code.
> > 
> > To be clear, you want me to revert this to 'errorMessageForFeature'?
> 
> Yes.

Done.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 563243 [details] [diff] [review]
Gather graphics info/failures in endurance shared module. v1.2

>+/**
>+ * Graphics class
>+ * Much of this code is taken from Firefox's about:support code.
>+ * @see http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.js

Nit: Please add an empty line after the first line so we have a separation.

Otherwise I think we can go ahead and land it. I'm sure you have tested it in detail. r=me with the nit addressed. Please land.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/b5b83d230ba0 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/365e61148e8e (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/26517625ff2f (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/26517625ff2f (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/5b813697a370 (mozilla-2.0)