593460 - Tests must properly close the Inspector

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

Some Inspector tests wrongly call the Inspector.closeInspectorUI() method. They should call closeInspectorUI(true).

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed fix

Proposed fix.

Comment 2

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 471959 [details] [diff] [review]
proposed fix

yes!

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 471959 [details] [diff] [review]
proposed fix

Asking for review to fix issues in Inspector tests.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

closeInspectorUI(true) seems to be the common case. Why not flip it around so that the default-no argument behavior clears the store?

Comment 5

[Mihai Sucan [:msucan]]

Attachment: updated fix

Thanks Gavin! Patch updated accordingly.

Comment 6

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 472591 [details] [diff] [review]
updated fix

+   * @param boolean aDontClearStore tells if you want the store associated to 
+   * the current tab/window to be cleared or not. set this to true to not clear 
+   * the store, or false otherwise
    */
-  closeInspectorUI: function IUI_closeInspectorUI(aClearStore)
+  closeInspectorUI: function IUI_closeInspectorUI(aDontClearStore)

I'd prefer the name didn't have a contracted negative in it (with missing apostrophe!). Maybe change aDontClearStore to aKeepStore?

Comment 7

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch according to the feedback from Robert, also rebased the code on top of the latest mozilla-central. Thanks!

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 475906 [details] [diff] [review]
updated patch

Review of attachment 475906 [details] [diff] [review]:

r=sdwilsh

::: browser/base/content/inspector.js
@@ +739,5 @@
    * tabContainer.TabSelect and others.
    *
+   * @param boolean aKeepStore tells if you want the store associated to the
+   * current tab/window to be cleared or not. set this to true to not clear the
+   * store, or false otherwise.

nit: "tells" should be on a newline.
nit: capitalize after a period.

Comment 9

[Mihai Sucan [:msucan]]

Attachment: [checked-in][in-devtools] patch update 2

Updated the patch to address the review comments.

Thank you Shawn!

Comment 10

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 530020 [details] [diff] [review]
[checked-in][in-devtools] patch update 2

checked into devtools:
http://hg.mozilla.org/projects/devtools/rev/5c711a695cb7

Comment 11

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 530020 [details] [diff] [review]
[checked-in][in-devtools] patch update 2

http://hg.mozilla.org/mozilla-central/rev/5c711a695cb7

Comment 12

[AndreiD[QA]]

The changes made with the patch landed in central (comment 11) are visible in FX6 Beta:
i.e:
 this.closeInspectorUI(); INSTEAD OF this.closeInspectorUI(true);
in inspector.js -> http://hg.mozilla.org/releases/mozilla-beta/file/084847ea02b4/browser/base/content/inspector.js

Setting this as Verified.