Closed Bug 1031064 Opened 10 years ago Closed 6 years ago

Add main thread I/O reporting to about:telemetry

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: bugzilla, Unassigned, Mentored)

References

Details

Attachments

(1 file, 4 obsolete files)

We already expose nsITelemetry.fileIOReports but it would be more convenient to be able to view this information in about:telemetry.
I'm almost done with the UI changes required. However, I do not have any real data available to test the changes. i.e. Telemetry.fileIOReports is an empty object. I put a breakpoint in  TelemetryIOInterposeObserver::Observe and got two calls during startup, but both had empty filenames.

Although I've tested with dummy data, I'd feel more confident if I am able to see it with real one. Is there some setting I need to enable? I'm running Nightly and telemetry is already enabled.
Flags: needinfo?(aklotz)
Which platform are you using? Telemetry.fileIOReports has the most through instrumentation on Windows. I'd suggest testing there if at all possible.
Flags: needinfo?(aklotz)
Hi Aaron,

Thanks for the reply. My main development environment is Linux. As you suggested, I was able to get the data for testing from a Windows box.

I took some liberty in the UI design and added features for sorting and filtering based on stages.

From your status, I see that you are on PTO till July 21st, so I am not expecting a review till then. I'm just posting the patch so that I will not forget it later.

Thanks.
Attachment #8456738 - Flags: review?(aklotz)
Fixed some minor issues that I uncovered while reviewing the code.
Attachment #8456738 - Attachment is obsolete: true
Attachment #8456738 - Flags: review?(aklotz)
Attachment #8459414 - Flags: review?(aklotz)
Comment on attachment 8459414 [details] [diff] [review]
Rev 2. Add main thread file i/o reporting to about:telemetry

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

This looks awesome!

One additional suggestion: Any chance you could add something that optionally filters the table to display only the top n results?

::: toolkit/content/aboutTelemetry.js
@@ +824,5 @@
> +
> +  fileReportStageNames: [
> +    bundle.GetStringFromName("fileIOStageStart"),
> +    bundle.GetStringFromName("fileIOStageNormal"),
> +    bundle.GetStringFromName("fileIOStageShutdown"),

I think we can omit fileIOStageShutdown from the stage dropdown since that can never be populated at the same time that about:telemetry can run.

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties
@@ +60,5 @@
> +fileIOFile = File
> +
> +fileIOStage = Stage
> +
> +fileIOTotalTime = Total Time

Can you include the units (milliseconds) in this heading, please?
Attachment #8459414 - Flags: review?(aklotz) → feedback+
Thanks for the feedback Aaron!.

> One additional suggestion: Any chance you could add something that
> optionally filters the table to display only the top n results?

I have added a row. number column. I believe this is more useful as it would solve your use case as well as provide the total count of the files. But I'm willing to add the filter if you think it is still needed.

> I think we can omit fileIOStageShutdown from the stage dropdown since that
> can never be populated at the same time that about:telemetry can run.

I had assumed that the data was cumulative. I have removed it now.

> Can you include the units (milliseconds) in this heading, please?
Done.
Attachment #8459414 - Attachment is obsolete: true
Attachment #8462951 - Flags: review?(aklotz)
Comment on attachment 8462951 [details] [diff] [review]
Rev 3. Add main thread file i/o reporting to about:telemetry

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

A couple of issues with this one:

* When View Stage is set to "All," I can only change the sort order of the file column but not any of the other columns.
* The sort order indicator is displayed and changes its direction when I click on the "No." header, but only when viewing a stage != "All"
Attachment #8462951 - Flags: review?(aklotz) → feedback+
> * When View Stage is set to "All," I can only change the sort order of the
> file column but not any of the other columns.
This is intentional. I could not figure out an intuitive way to sort by the other columns since each file may have data from multiple stages. This is also the reason for allowing to choose a particular stage.

> * The sort order indicator is displayed and changes its direction when I
> click on the "No." header, but only when viewing a stage != "All"
Fixed it in this patch.
Attachment #8462951 - Attachment is obsolete: true
Attachment #8466007 - Flags: review?(aklotz)
Comment on attachment 8466007 [details] [diff] [review]
Rev 4. Add main thread file i/o reporting to about:telemetry

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

Just a few style nits. I didn't write this code though so I'm going to send it to Vladan for the review.

::: toolkit/content/aboutTelemetry.js
@@ +909,5 @@
> +      aReport.sort(function sortstr(a, b) {
> +        return dir * a[0].localeCompare(b[0]);
> +      });
> +    }
> +    else {

Style nit: please write as
} else {

@@ +962,5 @@
> +    this.fileReportHeaders.forEach((aTitle, i) => {
> +      let allStages = aStage == -1;
> +
> +      // Skip the 'Stage' column if not displaying all stages
> +      if (!allStages && i == this.fileIOStageIdx) return;

Nit: put the return in a block

@@ +968,5 @@
> +      let cell = this.appendCell(header, "th", aTitle  + "\t");
> +
> +      // Don't allow sorting by Row.No. and when showing all stages
> +      // allow sorting only by file name
> +      if (i==this.fileIORowNumIdx || (allStages && i != this.fileIOFileIdx)) return;

Same here

@@ +974,5 @@
> +      cell.addEventListener("click", e => this.onSortByColumnHandler(aReport, e));
> +      cell.classList.add("sortable");
> +
> +      // Display the sort icon only for the selected column
> +      if (!allStages && i != sort.index) return;

and here

@@ +1011,5 @@
> +    let sortedReports = this.sortReport(filteredReps, aStage, aSort);
> +
> +    let even = false; // for row color.
> +    for (let [i, [file, stages]] in Iterator(sortedReports)) {
> +

Nit: there's a lot of blank lines when blocks are opened and closed. Could you condense those, please?

@@ +1033,5 @@
> +          this.addStage(aTable, fileRow, this.fileReportStageNames[j], stageData, even);
> +          even = !even;
> +          fileRow = document.createElement("tr");
> +        }
> +

eg. Here

@@ +1039,5 @@
> +        this.appendCell(fileRow, "td", rowNum);
> +        this.addStage(aTable, fileRow, file, stages[aStage], even);
> +        even = !even;
> +      }
> +

here

@@ +1041,5 @@
> +        even = !even;
> +      }
> +
> +    }
> +

here
Attachment #8466007 - Flags: review?(vdjeric)
Attachment #8466007 - Flags: review?(aklotz)
Attachment #8466007 - Flags: feedback+
Fixed issues in code style.
Attachment #8466007 - Attachment is obsolete: true
Attachment #8466007 - Flags: review?(vdjeric)
Attachment #8466429 - Flags: review?(aklotz)
Attachment #8466429 - Flags: review?(aklotz) → review?(vdjeric)
Hi Mukil, sorry for the delay, I'll review your patch tomorrow
Comment on attachment 8466429 [details] [diff] [review]
Rev 5. Add main thread file i/o reporting to about:telemetry

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

This looks amazing :) I applied the patch to my own tree and did a first-pass review. I'll re-review after the changes, but I also want a Firefox front-end person to take a look, cause we're following their coding conventions for this file

UX changes:

- limit the duration column to 1 decimal point of precision
- How hard would it be to sort by any column?
- i don't think we need the "Number" column
- move the "View Stage" selector below the section header and set the default selection to "Normal"

::: toolkit/content/aboutTelemetry.js
@@ +836,5 @@
> +    // Set up the properties to refer to the index of each header column
> +    // by name and store the actual column title in the fileReportHeaders
> +    for (let [i, headerName] in Iterator(this.fileReportHeaders)) {
> +        this[headerName + "Idx"] = i;
> +        this.fileReportHeaders[i] = bundle.GetStringFromName(headerName);

why not just have another member array in FileIOReports, e.g. fileReportHeaderStrings, instead of replacing the field names in the fileReportHeaders array with the locale strings

or maybe just statically load the strings for simplicity:
let FileIOReports = {
 totalTimeString: bundle.GetStringFromName(fileIOTotalTime"),
 ...
}

@@ +852,5 @@
> +      let opt = document.createElement("option");
> +      opt.value = i.toString();
> +      opt.text  = stageName;
> +      stageSelection.add(opt);
> +    });

Couldn't we just do this statically? We'll always have the 2 stages (startup and normal; shutdown won't be shown).. we'd just need to load the right string caption

@@ +888,5 @@
> +   * Function that sorts the file I/O report object according to
> +   * the given sort direction and sort-by column index
> +   *
> +   * @param aReport The unsorted file I/O report object
> +   *

Nit: drop the extra blank line between @param lines :)

@@ +1070,5 @@
> +  appendCell: function FileIOReport_appendCell(aRowElement, aCellType, aCellText, aRowSpan) {
> +    let cellElement = document.createElement(aCellType);
> +
> +    if (aRowSpan) {
> +        cellElement.rowSpan = aRowSpan;

indent two spaces

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.dtd
@@ +48,5 @@
>    Histograms Collected by Add-ons
>  ">
>  
> +<!ENTITY aboutTelemetry.fileIOSection "
> +  Main thread file I/O reports

Capitalize first letter of each word for consistency with rest of page

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties
@@ +62,5 @@
> +fileIOFile = File
> +
> +fileIOStage = Stage
> +
> +fileIOTotalTime = Total Time (milliseconds)

Total Time (ms) <-- shorter label to go with less precision in the data

@@ +78,5 @@
> +fileIOStageStart = Start
> +
> +fileIOStageNormal = Normal
> +
> +fileIOStageShutdown = Shutdown

How can we ever have shutdown data?
Attachment #8466429 - Flags: review?(vdjeric)
Comment on attachment 8466429 [details] [diff] [review]
Rev 5. Add main thread file i/o reporting to about:telemetry

Tim, can you take a look at the coding style for the JS and CSS? Or suggest someone else from frontend?
Attachment #8466429 - Flags: feedback?(ttaubert)
Hi Vlad, thanks for the feedback.

There are a few points on which I need more information before I start making the changes.

> - How hard would it be to sort by any column?
I'm sorry, could you elaborate? It *is* currently possible to sort by any column (expect Row. No) when either the 'Start' or 'Normal' stages are displayed. When 'All' stages are displayed, I could not figure out a way to intuitively sort by columns other than file name. If this is what you are referring to, I'm afraid I can't think of any way to do this. Please let me know if you have any ideas.

> - i don't think we need the "Number" column
I just want to confirm this again since in comment 5, Aaron mentioned it would be nice to have some way of filtering the top N files for each stage/attribute (like no. of reads, writes etc). I thought it would be more useful to display the rank so that it would also serve as a total count. Are you suggesting that we should go with Aaron's approach or remove this feature completely?

For the following points, I've given the reasons for why the approach was chosen. If you think they should be changed in the next patch, please let me know.

> ::: toolkit/content/aboutTelemetry.js
> @@ +836,5 @@
> > +    // Set up the properties to refer to the index of each header column
> > +    // by name and store the actual column title in the fileReportHeaders
> > +    for (let [i, headerName] in Iterator(this.fileReportHeaders)) {
> > +        this[headerName + "Idx"] = i;
> > +        this.fileReportHeaders[i] = bundle.GetStringFromName(headerName);
> 
> why not just have another member array in FileIOReports, e.g.
> fileReportHeaderStrings, instead of replacing the field names in the
> fileReportHeaders array with the locale strings
> 
> or maybe just statically load the strings for simplicity:
> let FileIOReports = {
>  totalTimeString: bundle.GetStringFromName(fileIOTotalTime"),
>  ...
> }

The code for adding the table header is just a loop over the header strings, so I think using a seperate member for each column would not be good. 

The initial version had an array 'fileReportHeaders' with just the locale strings (much like the fileReportStageNames array). But the problem was that the indices of the members were hard coded in few places and when I added a new 'row. no' column, it was really hard to change the code. It would make the code more readable to have a property for each column name (e.g. this.fileIOStageIdx refers to the stage name column) in the array so I can replace the hard coded indices.

As to why the strings are replaced, I agree with you and I will change it in the next patch so that the keys for the bundle strings are in a local array rather than a member.

> @@ +852,5 @@
> > +      let opt = document.createElement("option");
> > +      opt.value = i.toString();
> > +      opt.text  = stageName;
> > +      stageSelection.add(opt);
> > +    });
> 
> Couldn't we just do this statically? We'll always have the 2 stages (startup
> and normal; shutdown won't be shown).. we'd just need to load the right
> string caption

The problem was, the stage names are used in the JS code (to display in the table when 'All' stages are shown) and needed to be present in the .properties file. Creating the drop-down statically would mean that the stage names need to be present in the .dtd files too. I chose to avoid the dupication and create them from JS itself.
Comment on attachment 8466429 [details] [diff] [review]
Rev 5. Add main thread file i/o reporting to about:telemetry

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

JS is looking good with the few nits below fixed. Gijs, can you please take a look at the CSS? My CSS skillz aren't that great.

::: toolkit/content/aboutTelemetry.js
@@ +831,5 @@
> +
> +  /**
> +   * Get the data and start rendering with default sort settings
> +   */
> +  startRender: function FileIOReports_startRender() {

nit: we don't need to name our functions, our JS engine is good enough to figure it out and print nice stack traces :)

@@ +834,5 @@
> +   */
> +  startRender: function FileIOReports_startRender() {
> +    // Set up the properties to refer to the index of each header column
> +    // by name and store the actual column title in the fileReportHeaders
> +    for (let [i, headerName] in Iterator(this.fileReportHeaders)) {

Iterator() is deprecated. for (let headerName of this.fileReportHeaders) would be better. For the index you could count manually. Or just use this.fileReportHeaders.forEach((headerName, i) => { ... });

@@ +850,5 @@
> +    let stageSelection = document.getElementById("view-stage-select");
> +    this.fileReportStageNames.forEach((stageName, i) => {
> +      let opt = document.createElement("option");
> +      opt.value = i.toString();
> +      opt.text  = stageName;

Nit: please don't align on "=".

@@ +907,5 @@
> +      let col = aSort.index - this.fileIOTotalTimeIdx;
> +      aReport.sort(function sortnum(a, b) {
> +        let num1 = a[1][aStage][col];
> +        let num2 = b[1][aStage][col];
> +        return dir * (num1 < num2 ? -1 : (num1 > num2 ? 1 : 0));

return dir * (num1 - num2);

or

return dir * (a[1][aStage][col] - b[1][aStage][col]);

@@ +910,5 @@
> +        let num2 = b[1][aStage][col];
> +        return dir * (num1 < num2 ? -1 : (num1 > num2 ? 1 : 0));
> +      });
> +    }
> +    return aReport;

Returning the input param doesn't really make sense if the function modifies the array in-place, no?

@@ +948,5 @@
> +  renderTableHeader: function FileIOReport_renderTableHeader(aTable, aReport, aStage, sort) {
> +    let header = document.createElement("tr");
> +
> +    this.fileReportHeaders.forEach((aTitle, i) => {
> +      let allStages = aStage == -1;

This should be outside the loop.

@@ +959,5 @@
> +      let cell = this.appendCell(header, "th", aTitle  + "\t");
> +
> +      // Don't allow sorting by Row.No. and when showing all stages
> +      // allow sorting only by file name
> +      if (i==this.fileIORowNumIdx || (allStages && i != this.fileIOFileIdx)) {

Nit: i == this.

@@ +970,5 @@
> +      // Display the sort icon only for the selected column
> +      if (!allStages && i != sort.index) {
> +          return;
> +      }
> +      cell.classList.add(sort.dir);

Maybe:

if (allStages || i == sort.index) {
  cell.classList.add(sort.dir);
}

@@ +995,5 @@
> +
> +    // Include a row only if either all stages are shown or the selected stage
> +    // has some stats
> +    let filteredReps = [[k, v]
> +                        for each ([k,v] in Iterator(aReport))

Please replace Iterator() here as well.

@@ +1001,5 @@
> +                       ];
> +    let sortedReports = this.sortReport(filteredReps, aStage, aSort);
> +    let even = false; // for row color.
> +
> +    for (let [i, [file, stages]] in Iterator(sortedReports)) {

Please replace Iterator() here as well.

@@ +1003,5 @@
> +    let even = false; // for row color.
> +
> +    for (let [i, [file, stages]] in Iterator(sortedReports)) {
> +      let fileRow = document.createElement("tr");
> +      let rowNum = (i + 1).toString() + "\t";

Nit: we can get rid of the .toString() here.

@@ +1007,5 @@
> +      let rowNum = (i + 1).toString() + "\t";
> +
> +      if (allStages) {
> +        // number of stages with non-null data
> +        let nStages  = stages.filter((x) => { return !!x }).length;

Nit: let nStages = stages.filter(x => x).length;

@@ +1008,5 @@
> +
> +      if (allStages) {
> +        // number of stages with non-null data
> +        let nStages  = stages.filter((x) => { return !!x }).length;
> +        let rspan = (nStages).toString(); //row span

Do we need the .toString() here really?

@@ +1016,5 @@
> +        this.appendCell(fileRow, "td", file + "\t", rspan);
> +
> +        // The first stage will be in the row with filename + row no.
> +        // the remaining stages will be in their own row.
> +        for (let [j, stageData] in Iterator(stages)) {

Iterator()

@@ +1017,5 @@
> +
> +        // The first stage will be in the row with filename + row no.
> +        // the remaining stages will be in their own row.
> +        for (let [j, stageData] in Iterator(stages)) {
> +          if (!stageData) continue;

Please put continue on its on line and wrap it in brackets.

@@ +1047,5 @@
> +   *
> +   * @param aIsEven Used to decide the class name for styling the table row
> +   */
> +  addStage: function FileReport_addStage(aTable, aRow, aPrecedCol, aStageReport, aIsEven) {
> +    if (aIsEven) aRow.classList.add("even");

Wrap in brackets and make that three lines, please.

@@ +1051,5 @@
> +    if (aIsEven) aRow.classList.add("even");
> +
> +    this.appendCell(aRow, "td", aPrecedCol + "\t");
> +
> +    for (let [i, stat] in  Iterator(aStageReport)) {

Iterator()
Attachment #8466429 - Flags: feedback?(ttaubert)
Attachment #8466429 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8466429 - Flags: feedback+
Comment on attachment 8466429 [details] [diff] [review]
Rev 5. Add main thread file i/o reporting to about:telemetry

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

CSS seems fine generally, I left some nits. Tentative f+, but the issues I've highlighted would be better off fixed. :-)

::: toolkit/content/aboutTelemetry.css
@@ +190,5 @@
>  }
> +
> +#file-io-section table {
> +  width: 70%;
> +  border-spacing: 0px;

Nit: this doesn't seem to do anything.

@@ +196,5 @@
> +}
> +
> +#file-io-section  td, th {
> +  border: 1px solid #ddd;
> +  border-spacing: 0px;

Ditto.

@@ +205,5 @@
> +  background-color: #ddd;
> +}
> +
> +#file-io-section tr {
> +  height: 3.0em;

Nit: 3em

@@ +217,5 @@
> +  content: '▼';
> +}
> +
> +#file-io-section .sort-asc:after {
> +  content: '▲';

I ran this locally on OS X 10.9, and somehow the "down" arrow is much bigger than the up arrow. I actually can't figure out why, but we should ideally fix it...

@@ +221,5 @@
> +  content: '▲';
> +}
> +
> +#file-io-section .sortable:hover {
> +  color: #101010;

My eyesight must not be as good as I would like, because I can't tell the difference between #000000 and #101010... might just leave this out? Or did you mean for this value to more starkly contrast with #000000 ?
Attachment #8466429 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Hi Tim,
Thanks for the feedback. I'll submit a new patch with the nits fixed.

Hi Gijs,
Thanks for the feedback. I'm not sure what to do about this:

> I ran this locally on OS X 10.9, and somehow the "down" arrow is much bigger than the up arrow. I
> actually can't figure out why, but we should ideally fix it...

On my linux machine with the latest nightly, both arrows have the same size. Is this is a bug in the OS X version of firefox? 

> My eyesight must not be as good as I would like, because I can't tell the difference between #000000
> and #101010... might just leave this out? Or did you mean for this value to more starkly contrast
> with #000000 ?

Perhaps this is because of the age of my monitor :). I'll change it to a darker color instead.


Vlad,
In comment #14, I had raised some queries regarding your feedback. I'm still blocked on those changes. Once you provide the answers, I'll submit a new patch with Tim's & Gijs's feedback incorporated.

Thanks,
Mukilan
Flags: needinfo?(vdjeric)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Mukilan Thiyagarajan from comment #17)
> Hi Gijs,
> Thanks for the feedback. I'm not sure what to do about this:
> 
> > I ran this locally on OS X 10.9, and somehow the "down" arrow is much bigger than the up arrow. I
> > actually can't figure out why, but we should ideally fix it...
> 
> On my linux machine with the latest nightly, both arrows have the same size.
> Is this is a bug in the OS X version of firefox? 

It's more likely to be a font issue of sorts... I don't really know what the issue is here. Minimal testcase:

data:text/html,<meta charset="utf-8"><style>p{font:message-box}</style><p>▼▲

John, can you help understand what's going on here?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jdaggett)
(In reply to :Gijs Kruitbosch from comment #18)
> It's more likely to be a font issue of sorts... I don't really know what the
> issue is here. Minimal testcase:
> 
> data:text/html,<meta charset="utf-8"><style>p{font:message-box}</style><p>▼▲
> 
> John, can you help understand what's going on here?

You're pulling characters from different fonts here. The OSX default font Lucida Grande has glyphs for the down (U+25BC) and right triangles but lacks one for the up triangle (U+25B2). With Gecko system fallback we pick up the glyph from Menlo but Safari picks it up from Hiragino Kaku Gothic ProN.  Compare the two URL's below.

data:text/html,<meta charset="utf-8"><style>p{font-family:lucida grande,menlo}</style><p>▼▲

data:text/html,<meta charset="utf-8"><style>p{font-family:lucida grande,hiragino kaku gothic pron}</style><p>▼▲

I'll try to address this on bug 1055931.
Flags: needinfo?(jdaggett)
Depends on: 1055931
Comment on attachment 8466429 [details] [diff] [review]
Rev 5. Add main thread file i/o reporting to about:telemetry

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

New style comments:

- It looks like your CSS changes have added borders to table headings in the other about:telemetry sections. Can you fix this?
- When I click on a table heading to sort, the column expands horizontally to fit the addition of an up or down arrow in the the table heading. Can we just add some extra padding to each column heading so that columns don't resize themselves after being clicked on?

Answers to your questions:

> > - i don't think we need the "Number" column
> I just want to confirm this again since in comment 5, Aaron mentioned it
> would be nice to have some way of filtering the top N files for each
> stage/attribute (like no. of reads, writes etc). I thought it would be more
> useful to display the rank so that it would also serve as a total count. Are
> you suggesting that we should go with Aaron's approach or remove this
> feature completely?

I think in comment 5, Aaron was suggesting a checkbox to limit the number of rows displayed at one time, because often there were too many main-thread I/O reports. We recently added a 50ms threshold for recording main thread I/O in bug 1030487, so I don't think we'll have too many rows anymore. So we no longer need the "No." column.

> > - How hard would it be to sort by any column?
> I'm sorry, could you elaborate? It *is* currently possible to sort by any
> column (expect Row. No) when either the 'Start' or 'Normal' stages are
> displayed. When 'All' stages are displayed, I could not figure out a way to
> intuitively sort by columns other than file name. If this is what you are
> referring to, I'm afraid I can't think of any way to do this. Please let me
> know if you have any ideas.

That's right, I'd like to sort by any column in the "All" view.

What do you mean by "intuitively"? If you're saying it's unclear how to implement it, I'd suggest combining the data from the start + normal stages (e.g. firstStage.concat(secondStage)), and then just feed the result to the sort + render functions. 

> The code for adding the table header is just a loop over the header strings,
> so I think using a seperate member for each column would not be good. 

Ok

> The initial version had an array 'fileReportHeaders' with just the locale
> strings (much like the fileReportStageNames array). But the problem was that
> the indices of the members were hard coded in few places and when I added a
> new 'row. no' column, it was really hard to change the code. It would make
> the code more readable to have a property for each column name (e.g.
> this.fileIOStageIdx refers to the stage name column) in the array so I can
> replace the hard coded indices.

I agree hard-coded indices are bad.

However, you don't really need the "..Idx" properties. If you don't modify fileReportHeaders (and store the bundle strings in a different array), you can just refer to the the columns by those names.

if (aSort.index == this.fileIOFileIdx) { ...

vs

if (this.fileReportHeaders[aSort.index] == "fileIOFile")

^^ Admittedly, this is less readable, so it's your call.


> As to why the strings are replaced, I agree with you and I will change it in
> the next patch so that the keys for the bundle strings are in a local array
> rather than a member.

Great

> The problem was, the stage names are used in the JS code (to display in the
> table when 'All' stages are shown) and needed to be present in the
> .properties file. Creating the drop-down statically would mean that the
> stage names need to be present in the .dtd files too. I chose to avoid the
> dupication and create them from JS itself.

Ok. I think we can still improve by getting rid of the fileReportStageNames array, but still creating the dropdown from JS code.

Also, I would also suggest simplifying the code by dropping some of the special-case logic for the "All" view, e.g. it's ok to show the "Stage" column in all the views.
Flags: needinfo?(vdjeric)
File I/O Reports aren't really used, and may be retired.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: