Last Comment Bug 664191 - Find a consistent name for GCLI's Request[s]View and Report
: Find a consistent name for GCLI's Request[s]View and Report
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7]
: J. Ryan Stinnett [:jryans] (use ni?)
Depends on:
  Show dependency treegraph
Reported: 2011-06-14 10:25 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2011-06-21 02:49 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-06-14 10:25:49 PDT

Comment 1 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-06-15 16:40:00 PDT
By "find a more consistent name" you mean that you want to rename the RequestView and RequestsView constructors to be... better?

I am not sure what these are "requesting". The doc comments say "Adds a row to the CLI output display" and "A wrapper for a set of rows|command outputs". The doc comments describe to me what they do, but the names don't.

I suggest:

CommandOutputView and CommandOutputListView.

Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-06-16 03:53:37 PDT

In the Bespin days, Request was a sensible name, but refactoring broke the name. RequestView(s) came from those days.

I was thinking of using ReportView and ReportViews; Mihai complained about the name Report, but I didn't much like his suggestions. But I like your suggestions better.

Do you want to take this?

It's not just request_view.js that will need changing - there's also canon which has a Report and ReportList which will need changing to CommandOutput and there's a globalReportList which needs to change to commandOutputList (I think - it's not really global, it's just per instance of GCLI)

And that affects HUDService.jsm, so you'll need to be able to build FF with the GCLI patches ...
Comment 3 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-06-16 07:53:39 PDT
Comment 4 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-06-16 16:28:11 PDT
Here is what I am thinking right now:

RequestView -> CommandOutputView
RequestsView -> CommandOutputListView

and then I am not as sure with this last piece:

ReportList -> CommandOutput[Controller|Manager]

It seems to me we are in agreement with the first two, do you have an opinion on what ReportList should be changed to?
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-06-17 00:47:14 PDT
Was thinking CommandOutputList for symmetry with CommandOutputListView?
Comment 6 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-06-17 13:43:00 PDT
The thing is, it isn't really a list. Yes, it has a list in the implementation, but none of its public operations involve listy things. Its just a wrapper around an event and manages/controls access to those events.

So that's why I like "CommandOutputManager".

I am going to just go ahead and continue so that I am being productive; we can always come back and change it later.
Comment 7 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-06-17 13:56:00 PDT
I buy the CommandOutputManager argument. Done.
Comment 8 Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] 2011-06-17 16:54:42 PDT
Pull request on GitHub:
Comment 9 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-06-21 02:49:02 PDT
Marking verified because there is no UI proof that the bug is fixed. The proof is in the code.

Note You need to log in before you can comment on or make changes to this bug.