Last Comment Bug 692795 - Firebug should be able to suppress web console prompted by scratchpad
: Firebug should be able to suppress web console prompted by scratchpad
Status: RESOLVED FIXED
[needs-testing]
: qawanted
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: x86 Windows Vista
: P3 normal (vote)
: Firefox 12
Assigned To: Mihai Sucan [:msucan]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 690552
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-07 08:31 PDT by Jan Honza Odvarko [:Honza]
Modified: 2012-01-12 12:33 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (3.36 KB, patch)
2011-10-07 08:56 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review

Description Jan Honza Odvarko [:Honza] 2011-10-07 08:31:38 PDT
When there is an error in Scratchpad, the build-in console pop-up.

Firebug (or any other dev tool/Console) should be able to register itself as the default Console.

Thoughts?

Honza
Comment 1 Mihai Sucan [:msucan] 2011-10-07 08:56:55 PDT
Created attachment 565551 [details] [diff] [review]
proposed patch

My thoughts in the shape of a quick patch. All tests pass.

Honza: You have here what you need: you can override the opening of the Web Console from Scratchpad, and you can override the context menu element inspection menu item. Firebug can change the chromeWindow.Devtools.* methods as needed. Add prefs to allow default tools to work.

Please note this is a quick patch! for fancier stuff, we can open separate bugs, for other features. I think this is a good start - better than to have two "Inspect element" menu items.

Comments welcome!
Comment 2 Jan Honza Odvarko [:Honza] 2011-10-12 05:52:29 PDT
(In reply to Mihai Sucan [:msucan] from comment #1)
> Honza: You have here what you need: you can override the opening of the Web
> Console from Scratchpad, and you can override the context menu element
> inspection menu item. Firebug can change the chromeWindow.Devtools.* methods
> as needed. Add prefs to allow default tools to work.
Yep I have tested the patch and works for me.

> Please note this is a quick patch! for fancier stuff, we can open separate
> bugs, for other features. I think this is a good start - better than to have
> two "Inspect element" menu items.
Exactly

Just one question, I am not sure what are the rules here, but wouldn't the Devtools related code (and so, even e.g. the Scratchpad object) deserve to be in another file? (while still accessible from browser.xul scope) The browser.js is already pretty huge pile of code.

Honza
Comment 3 Mihai Sucan [:msucan] 2011-10-12 06:05:47 PDT
(In reply to Jan Honza Odvarko from comment #2)
> (In reply to Mihai Sucan [:msucan] from comment #1)
> > Honza: You have here what you need: you can override the opening of the Web
> > Console from Scratchpad, and you can override the context menu element
> > inspection menu item. Firebug can change the chromeWindow.Devtools.* methods
> > as needed. Add prefs to allow default tools to work.
> Yep I have tested the patch and works for me.

Great!

> > Please note this is a quick patch! for fancier stuff, we can open separate
> > bugs, for other features. I think this is a good start - better than to have
> > two "Inspect element" menu items.
> Exactly
> 
> Just one question, I am not sure what are the rules here, but wouldn't the
> Devtools related code (and so, even e.g. the Scratchpad object) deserve to
> be in another file? (while still accessible from browser.xul scope) The
> browser.js is already pretty huge pile of code.

That's really up to the browser peer reviewing the patch. It makes sense to, some day, move our browser.js devtools code into a browser-devtools.js that would be #included at the top of browser.js.

Worth filing a follow up bug report?
Comment 4 Rob Campbell [:rc] (:robcee) 2012-01-10 09:15:41 PST
we need to retest this on nightly to see if the recent fix for the Error messages in Scratchpad bug 690552.
Comment 5 Maniac Vlad Florin (:vladmaniac) 2012-01-12 10:35:16 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #4)
> we need to retest this on nightly to see if the recent fix for the Error
> messages in Scratchpad bug 690552.

Tested the exception throwing mechanism with the latest Nightly and its a pass. 

Tested all major exceptions to be thrown correctly (syntax errors, illegal statements) in Scratchpad. 

From the Qa side you guys have a green light for closing the bug
Comment 6 Rob Campbell [:rc] (:robcee) 2012-01-12 12:32:33 PST
Awesome, thank you!
Comment 7 Rob Campbell [:rc] (:robcee) 2012-01-12 12:33:16 PST
Comment on attachment 565551 [details] [diff] [review]
proposed patch

cancelling feedback request, no longer needed.

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