578462 - code cleanup: fix typos, remove unused variables, and a bit more

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Mihai Sucan [:msucan]]

User-Agent:       Opera/9.80 (X11; Linux x86_64; U; fr) Presto/2.6.30 Version/10.60
Build Identifier: 

This I am attaching a proposed patch that is based on the feedback I provided last week.

Reproducible: Always

Steps to Reproduce:
no repro steps

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed patch

This is the proposed patch, based on the feedback I provided last week.  This patch includes some more stuff I was able to catch.

Below is the list of changes, in patch source code order, to help with the reviewing:


- Removed the HUDService.getOutputNodeById() method, which was a duplicate of 
the getHeadsUpDisplay() method. Changed the code to only use the 
getHeadsUpDisplay() method.


- Minor adjustment for HUDService.getFilterState().


- In HUDService.unregisterDisplay() I changed the outputNode to be retrieved by 
the getHeadsUpDisplay() method.


- In HUDService.unregisterDisplay():
  delete displays[aId];
  is the same as
  delete this._headsUpDisplays[aId];


- The HUDService.getFilterTextBox() method is unused. However, the 
HUDService.getFilterStringByHUDId() method can use it. Updated the code 
accordingly.


- Fixed HUDService.updateFilterText().


- Removed the HUDService.logConsoleMessage() method - it is unused. The 
nsIConsoleService observer (HUDConsoleObserver) uses the 
HUDService.reportConsoleServiceMessage() method.


- Removed the unused hud variable in HUDService.logMessage().


- Adjusted the switch statement in HUDService.logMessage().


- Fixed a typo in the comment of the HUDService.reportConsoleServiceMessage() 
  method. From "recieved" to "received".

Fixed the same typo for the reportConsoleServiceContentScriptError() method as 
well.


- Removed the unnecessary switch in the HUDService.registerApplicationHooks() 
  method.


- Fixed a typo in the description of the HUDService.httpTransactions property.  
From "trasactions" to "transactions".


- Removed the unused loggedNode variable in the httpObserver() function of the 
HUDService.


- In the HUDService.logConsoleActivity() method there was the following line:

var filterState = this.getFilterState(hudId, msgLogLevel);

Where msgLogLevel is the localized string of the message logLevel. This was an 
error, because the filter state check never worked. Fixed now.


- Made some adjustments to the HUDService.logConsoleActivity() method. Using try 
{ ... } catch (ex) { ...} clauses, as far as I know, are slow - they should only 
be used when needed.


- Fixed a typo in the HUDService.logConsoleActivity() method. From "becasue" to 
"because".


- Removed the unused variables in the HUDService.logActivity() method.


- Fixed a typo in the HUDService.getDisplayByLoadGroup() method comment, at the 
end of the function:

  // TODO: also need to check parent loadGroup(s) incase of iframe activity?;
  // see bug 568643

Changed to "in case".


- Removed the HeadsUpDisplay.XULFactory() method, in favour to makeXULNode().


- Removed the unused hudBox variable in the HeadsUpDisplay object constructor.


- Removed the unused jsTermParentNode property from the HeadsUpDisplay object.  
It was defined in the makeHUDNodes() method:

this.jsTermParentNode = outerWrap;


- Minor adjustment in HeadsUpDisplay.makeButton().


- Removed the @returns object for the constructor of HUDConsole objects. The 
function itself does not actually return anything. It's only used as a 
constructor.


- Fixed a bug in the HUDConsole.sendToHUDService() method. The message object 
that is constructed did not reference the correct message property. Instead of 
this.hud.message, it should point to this.message.


- The NodeFactory() function was defined twice. Fixed.


- Removed the duplication of ELEMENT_NS_URI in the NodeFactory() function.


- Removed the @returns void for the constructor of ConsoleEntry objects.

I did the same for the JSTerm and ConsoleDOMListeners constructors.


- Fixed a typo in the JSTerm.codeInputString() getter. From "conver" to 
"convert".


- Fixed a typo in the JSTerm.openPropertyPanel() method. From "destroyes" to 
"destroys".


- Adjusted the switch statement in the JSTerm.keyDown() event handler.


- Fixed a typo: ConsolEntry in a comment, instead of ConsoleEntry.


- Adjusted the switch statement in the HUDConsoleObserver.observe() method.


- Fixed the function name for the following methods:

ConsoleStorage.createDisplay()
ConsoleStorage.sequenceId()
HUDService.registerApplicationHooks()
HUDService.sequenceId()
HUDService.startHTTPObservation()
HUDService.unregisterActiveContext()
HUDService.windowInitializer()
JSTerm.createSandbox()


...

That's all! I hope it's not too much. Another idea would be to break this patch into multiple reports, if desired. Just let me know how you propose we should do this.

Note that this should apply cleanly to the latest code of the default branch, and that it has no regressions, according to the tests. I tried to keep things minimal.


Thanks!

Comment 2

[David Dahl :ddahl]

This kind of work is really best left until after a quickly-approaching release is released. I do like what you are doing, but we need to wait on it until we ship.

Comment 3

[David Dahl :ddahl]

Comment on attachment 457149 [details] [diff] [review]
proposed patch

let's rebase after a later release, either a future beta or Fx4

Comment 4

[Kevin Dangoor]

There's been a lot of other work going on around the console (the new logging infrastructure and the API change that consolidates everything through one path). We should strive to either resurrect this and finish it off or to drop it for now.

Comment 5

[David Dahl :ddahl]

We should shelve this for now as it is no doubt bit rotten and is more "elective" work instead of fixing existing bugs. I like bugs like this, but after a release is a better time.

Comment 6

[Mihai Sucan [:msucan]]

After the release, I can update the patch, so no worries.

Comment 7

[Kevin Dangoor]

This bug is ancient. Can we close it?

Comment 8

[Mihai Sucan [:msucan]]

Sure, you can close it. Some of the things in the patch are probably still valid, but we have smaller bug reports about precise things to clean up.

Comment 9

[David Dahl :ddahl]

Mihai: do you know which of this list is still a problem? would you mind making bugs for them?

Comment 10

[Mihai Sucan [:msucan]]

David: checked.

Things like:

-  unregisterActiveContext: function HS_deregisterActiveContext(aContextDOMId)
+  unregisterActiveContext: function HS_unregisterActiveContext(aContextDOMId)

... still remain valid. Not worth separate bug reports.

If you want I can "rebase" the patch (basically redo it). I expect there would be fewer changes. Or ... we just close the bug and be done with it. :)

The idea of a general code-quality "audit" ... that would fix typos, remove unused variables and methods, etc ... would perhaps not be bad, but it doesn't have high priority at all.

Comment 11

[David Dahl :ddahl]

(In reply to comment #10)
> David: checked.
> 
> Things like:
> 
> -  unregisterActiveContext: function HS_deregisterActiveContext(aContextDOMId)
> +  unregisterActiveContext: function HS_unregisterActiveContext(aContextDOMId)
> 
> ... still remain valid. Not worth separate bug reports.

right.

> 
> If you want I can "rebase" the patch (basically redo it). I expect there would
> be fewer changes.
> 
please rebase at your leisure

> The idea of a general code-quality "audit" ... that would fix typos, remove
> unused variables and methods, etc ... would perhaps not be bad, but it doesn't
> have high priority at all.

Actually it does have a highish priority as it is low-hanging fruit and will make things more maintainable going forward

Comment 12

[Mihai Sucan [:msucan]]

David: looked into rebasing this patch now. I can do it, but lots of cleanup is already in bug 592469.

I think we need to get that reviewed first and checked-in. Once we do that, we can do the remaining work. If we don't take things in a proper order we'll end up doings lots of work several times - rebasing between patches and so forth.

Putting bug 592469 as a dependency. Maybe we should ping Gavin about it.

Comment 13

[Mihai Sucan [:msucan]]

This is now generally fixed. There was a lot of code cleanup in the Web Console and more will come.