Closed Bug 592442 Opened 10 years ago Closed 9 years ago

WebConsole: Completion throws error if closing more bracket are closed then opened

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(blocking2.0 final+)

VERIFIED FIXED
Firefox 4.0b8
Tracking Status
blocking2.0 --- final+

People

(Reporter: julian.viereck, Assigned: msucan)

References

Details

(Whiteboard: [kd4b7][patchclean:1103][Web-Console-Testday])

Attachments

(1 file, 2 obsolete files)

Typing something to the WebConsole input field that has more closing brackets then opening ones causes an error (e.g. `document.getElementById)`):

Error: (void 0) is undefined
Source File: resource://gre/modules/HUDService.jsm
Line: 3328
Attached patch Patch v1.0 (obsolete) — Splinter Review
Attachment #470906 - Flags: feedback?(ddahl)
Attachment #470906 - Flags: feedback?(ddahl) → feedback+
Assignee: nobody → pwalton
Whiteboard: [kd4b6]
Severity: normal → blocker
Attached patch Proposed patch, version 2. (obsolete) — Splinter Review
New version of the patch rebases and splits out the test to follow the format of bug 581069.
Attachment #470906 - Attachment is obsolete: true
Attachment #472052 - Flags: feedback?(ddahl)
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Whiteboard: [kd4b6] → [kd4b7]
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
Blocks: devtools4b8
Comment on attachment 472052 [details] [diff] [review]
Proposed patch, version 2.

Looks good: one nit is the use of the var "c" which i am sure was julian's doing. We should at least file a followup (post-fx4) bug to change that to a name that is understandable.
Attachment #472052 - Flags: feedback?(ddahl) → feedback+
Attachment #472052 - Flags: review?(sdwilsh)
blocking2.0: --- → ?
blocking2.0: ? → final+
Reassigning to Mihai to see if this is still a problem and if the patch is still working.
Assignee: pwalton → mihai.sucan
Comment on attachment 472052 [details] [diff] [review]
Proposed patch, version 2.

>+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_592442_closing_brackets.js
You did not use the boilerplate [1] to generate your license block because the formatting is wrong, but regardless, the license policy [2] encourages the use of the public domain license [3] for test cases.

[1] http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c
[2] http://www.mozilla.org/MPL/license-policy.html
[3] http://www.mozilla.org/MPL/boilerplate-1.1/pd-c

>+  executeSoon(function() {
>+  let error = false;
>+  try {
>+    jsterm.propertyProvider(jsterm.sandbox.window, "document.getElementById)");
>+  } catch (ex) {
>+    error = true;
>+  }
>+
>+  ok(!error, "no error was thrown when an extraneous bracket was inserted");
>+
>+  HUDService.deactivateHUDForContext(gBrowser.selectedTab);
>+  finish();
>+  });
weird indentation

r=sdwilsh with these changes
Attachment #472052 - Flags: review?(sdwilsh) → review+
(In reply to comment #7)
> Comment on attachment 472052 [details] [diff] [review]
> Proposed patch, version 2.
> 
> >+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_592442_closing_brackets.js
> You did not use the boilerplate [1] to generate your license block because the
> formatting is wrong, but regardless, the license policy [2] encourages the use
> of the public domain license [3] for test cases.
> 
> [1] http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c
> [2] http://www.mozilla.org/MPL/license-policy.html
> [3] http://www.mozilla.org/MPL/boilerplate-1.1/pd-c

I believe this is an old patch that did not follow the newer license block guidelines.

> >+  executeSoon(function() {
> >+  let error = false;
> >+  try {
> >+    jsterm.propertyProvider(jsterm.sandbox.window, "document.getElementById)");
> >+  } catch (ex) {
> >+    error = true;
> >+  }
> >+
> >+  ok(!error, "no error was thrown when an extraneous bracket was inserted");
> >+
> >+  HUDService.deactivateHUDForContext(gBrowser.selectedTab);
> >+  finish();
> >+  });
> weird indentation

Fixed this.

Thanks for the review+! Will attach the updated patch.
Attached patch Patch update 3Splinter Review
Updated patch.

Changes:
- rebased on top of the latest mozilla-central.
- changed the test code to use the new style of writing tests (after the big tests split).
- changed the test code license as requested.
- fixed indentation.
- changed from using jsterm.propertyProvider() to jsterm.complete() in the test code. propertyProvider() is too "specific", I'd say. complete() is the method invoked by user interaction, and this method also calls propertyProvider() and does all things needed to perform automatic complete.

I believe this is ready for checkin.
Attachment #472052 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [kd4b7] → [kd4b7][patchclean:1103]
http://hg.mozilla.org/mozilla-central/rev/bc33b57acf78
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
Status: RESOLVED → VERIFIED
Whiteboard: [kd4b7][patchclean:1103] → [kd4b7][patchclean:1103][Web-Console-Testday]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.