Closed
Bug 593460
Opened 14 years ago
Closed 14 years ago
Tests must properly close the Inspector
Categories
(DevTools :: General, defect)
DevTools
General
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 6
People
(Reporter: msucan, Assigned: msucan)
References
Details
(Whiteboard: [fixed-in-devtools][merged-to-mozilla-central])
Attachments
(1 file, 3 obsolete files)
5.79 KB,
patch
|
Details | Diff | Splinter Review |
Some Inspector tests wrongly call the Inspector.closeInspectorUI() method. They should call closeInspectorUI(true).
Assignee | ||
Comment 1•14 years ago
|
||
Proposed fix.
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attachment #471959 -
Flags: feedback?(rcampbell)
Comment 2•14 years ago
|
||
Comment on attachment 471959 [details] [diff] [review]
proposed fix
yes!
Attachment #471959 -
Flags: feedback?(rcampbell) → feedback+
Assignee | ||
Comment 3•14 years ago
|
||
Comment on attachment 471959 [details] [diff] [review]
proposed fix
Asking for review to fix issues in Inspector tests.
Attachment #471959 -
Flags: review?(gavin.sharp)
Comment 4•14 years ago
|
||
closeInspectorUI(true) seems to be the common case. Why not flip it around so that the default-no argument behavior clears the store?
Assignee | ||
Comment 5•14 years ago
|
||
Thanks Gavin! Patch updated accordingly.
Attachment #471959 -
Attachment is obsolete: true
Attachment #472591 -
Flags: review?(gavin.sharp)
Attachment #471959 -
Flags: review?(gavin.sharp)
Updated•14 years ago
|
Blocks: devtools924
Comment 6•14 years ago
|
||
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?
Assignee | ||
Comment 7•14 years ago
|
||
Updated patch according to the feedback from Robert, also rebased the code on top of the latest mozilla-central. Thanks!
Attachment #472591 -
Attachment is obsolete: true
Attachment #475906 -
Flags: review?(gavin.sharp)
Attachment #472591 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [patchclean:0916]
Updated•14 years ago
|
No longer blocks: devtools924
Comment 8•14 years ago
|
||
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.
Attachment #475906 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 9•14 years ago
|
||
Updated the patch to address the review comments.
Thank you Shawn!
Attachment #475906 -
Attachment is obsolete: true
Updated•14 years ago
|
Whiteboard: [patchclean:0916] → [patchclean:0916][fixed-in-devtools
Updated•14 years ago
|
Whiteboard: [patchclean:0916][fixed-in-devtools → [patchclean:0916][fixed-in-devtools]
Comment 10•14 years ago
|
||
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
Attachment #530020 -
Attachment description: patch update 2 → [in-devtools] patch update 2
Updated•14 years ago
|
Whiteboard: [patchclean:0916][fixed-in-devtools] → [patchclean:0916][fixed-in-devtools][merge-m-c]
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [patchclean:0916][fixed-in-devtools][merge-m-c] → [fixed-in-devtools][merged-to-mozilla-central]
Target Milestone: --- → Firefox 6
Comment 11•14 years ago
|
||
Comment on attachment 530020 [details] [diff] [review]
[checked-in][in-devtools] patch update 2
http://hg.mozilla.org/mozilla-central/rev/5c711a695cb7
Attachment #530020 -
Attachment description: [in-devtools] patch update 2 → [checked-in][in-devtools] patch update 2
Comment 12•13 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•