Closed
Bug 612405
Opened 14 years ago
Closed 14 years ago
Devise a better way to determine the web console is a 'native' console object
Categories
(DevTools :: General, defect, P3)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 4.0b8
People
(Reporter: ddahl, Assigned: Gavin)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
ddahl
:
review+
Dolske
:
approval2.0+
|
Details | Diff | Splinter Review |
We want to notify the enduser if the console object has been paved over. We currently do that by checking if the console is instanceof HUDConsole. With the landing of the lazy console and integration patches ( bug 587734 ) we seem to not be able to check instanceof when the object is a nsIDOMGlobalPropertyInitializer. The current, cheezy approach is to do:
if (!(aContentWindow.wrappedJSObject.console.classID == CLASS_ID_VALUE)) {
this.logWarningAboutReplacedAPI(hudId);
}
Of course the classID is not enumerable from content pages, a developer who would like to fool this check would have to figure it out from the HUDConsole.jsm source code. Still a pretty weak check though.
Comment 1•14 years ago
|
||
Is this bug to fix cheezyness or does this mean that the "console has been paved over" message won't get displayed after the lazy console lands? (the priority of the latter is higher than the former, to be sure!)
Reporter | ||
Comment 2•14 years ago
|
||
(In reply to comment #1)
> Is this bug to fix cheezyness or does this mean that the "console has been
> paved over" message won't get displayed after the lazy console lands? (the
> priority of the latter is higher than the former, to be sure!)
The message will get displayed - it is just a less than meaningful check if it truly is "our" console.
Assignee | ||
Comment 3•14 years ago
|
||
Right - as landed it could theoretically be replaced by the page in a way that we wouldn't detect, but the page would need to go through efforts to "hide" the replacement from us, so it isn't a huge issue in practice.
Assignee: nobody → gavin.sharp
Assignee | ||
Comment 4•14 years ago
|
||
This is what I had in mind, but it doesn't work. Will need to find a better way.
Comment 5•14 years ago
|
||
so the .classID check didn't work for this? Disappointing.
Any idea why the instanceof check isn't working in your version, gavin?
Reporter | ||
Comment 6•14 years ago
|
||
I tried:
if (!(consoleObject instanceof Ci.nsISupports)) {
as well. we should ask jst why this does not work.
Comment 7•14 years ago
|
||
Why not make |console| a a getter/setter prop on the window and have the setter display the message then overwrite |console|?
Assignee | ||
Comment 8•14 years ago
|
||
We want to continue making use of the "JavaScript Global Property" functionality, rather than having to define setters on every window global like we used to, so that's not a feasible solution.
Fixing bug 613315 and using getGlobalForObject seems like it could work, though.
Assignee | ||
Comment 9•14 years ago
|
||
Actually, it works fine now even without that. I also changed the test so that it actually tests the warning (rather than calling logWarningAboutReplacedAPI directly...), and also covers the other case (making sure the warning doesn't appear when it shouldn't). I also simplified testLogEntry() a bit.
Attachment #491076 -
Attachment is obsolete: true
Reporter | ||
Comment 10•14 years ago
|
||
Comment on attachment 492198 [details] [diff] [review]
patch
sweet. Cu.getGlobalForObject is pretty damn useful!
Assignee | ||
Comment 12•14 years ago
|
||
Attachment #492198 -
Attachment is obsolete: true
Attachment #494253 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #494253 -
Flags: review? → review?(ddahl)
Reporter | ||
Updated•14 years ago
|
Attachment #494253 -
Flags: review?(ddahl) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #494253 -
Flags: approval2.0?
Assignee | ||
Comment 13•14 years ago
|
||
Requesting approval because this is well tested (in addition to simplifying a bunch of existing tests), and removes cruft from a newly-added web-exposed API (good to do now lest people somehow manage to start depending on it).
Updated•14 years ago
|
Attachment #494253 -
Flags: approval2.0? → approval2.0+
Updated•14 years ago
|
Keywords: dev-doc-needed
Updated•14 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 14•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•