Last Comment Bug 593460 - Tests must properly close the Inspector
: Tests must properly close the Inspector
Status: VERIFIED FIXED
[fixed-in-devtools][merged-to-mozilla...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 6
Assigned To: Mihai Sucan [:msucan]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on:
Blocks: 592320 653528
  Show dependency treegraph
 
Reported: 2010-09-03 13:10 PDT by Mihai Sucan [:msucan]
Modified: 2011-07-20 06:45 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (5.62 KB, patch)
2010-09-03 13:13 PDT, Mihai Sucan [:msucan]
rcampbell: feedback+
Details | Diff | Splinter Review
updated fix (5.57 KB, patch)
2010-09-07 03:09 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
updated patch (5.58 KB, patch)
2010-09-16 10:56 PDT, Mihai Sucan [:msucan]
sdwilsh: review+
Details | Diff | Splinter Review
[checked-in][in-devtools] patch update 2 (5.79 KB, patch)
2011-05-04 08:15 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review

Description Mihai Sucan [:msucan] 2010-09-03 13:10:45 PDT
Some Inspector tests wrongly call the Inspector.closeInspectorUI() method. They should call closeInspectorUI(true).
Comment 1 Mihai Sucan [:msucan] 2010-09-03 13:13:04 PDT
Created attachment 471959 [details] [diff] [review]
proposed fix

Proposed fix.
Comment 2 Rob Campbell [:rc] (:robcee) 2010-09-06 06:20:10 PDT
Comment on attachment 471959 [details] [diff] [review]
proposed fix

yes!
Comment 3 Mihai Sucan [:msucan] 2010-09-06 07:33:25 PDT
Comment on attachment 471959 [details] [diff] [review]
proposed fix

Asking for review to fix issues in Inspector tests.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-09-06 16:26:44 PDT
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] 2010-09-07 03:09:43 PDT
Created attachment 472591 [details] [diff] [review]
updated fix

Thanks Gavin! Patch updated accordingly.
Comment 6 Rob Campbell [:rc] (:robcee) 2010-09-16 09:18:32 PDT
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] 2010-09-16 10:56:24 PDT
Created attachment 475906 [details] [diff] [review]
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 2011-05-02 13:13:03 PDT
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] 2011-05-04 08:15:30 PDT
Created attachment 530020 [details] [diff] [review]
[checked-in][in-devtools] patch update 2

Updated the patch to address the review comments.

Thank you Shawn!
Comment 10 Rob Campbell [:rc] (:robcee) 2011-05-04 08:48:26 PDT
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) 2011-05-09 12:51:38 PDT
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] 2011-07-20 06:45:58 PDT
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.

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