Closed
Bug 862024
Opened 12 years ago
Closed 12 years ago
Warning about replaced window.console API shows when content scripts do not change the object
Categories
(DevTools :: Console, defect)
DevTools
Console
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 23
People
(Reporter: msucan, Assigned: msucan)
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
7.22 KB,
patch
|
past
:
review+
|
Details | Diff | Splinter Review |
STR:
1. load http://people.mozilla.org/~tglek/dashboard/
2. open the web console.
3. see warning:
> The Web Console logging API (console.log, console.info, console.warn, console.error)
> has been disabled by a script on this page.
This warning shows for no obvious reason.
Assignee | ||
Comment 1•12 years ago
|
||
Problem identified - this is related to the latest Target.jsm changed, navPayload and such.
Patch here, 3 lines fix:
https://github.com/mihaisucan/mozilla-patch-queue/blob/master/patches-webconsole/bug-862024
Tomorrow I'll add a test and submit for review.
Assignee | ||
Comment 2•12 years ago
|
||
This is a regression caused by the recent Target API changes. This patch includes a fix and a test.
Try push: https://tbpl.mozilla.org/?tree=Try&rev=463328323d7e
Thanks!
Attachment #738063 -
Flags: review?(past)
Comment 3•12 years ago
|
||
Can you explain why the console can't rely on the target events for navigation, but has to use the protocol directly? This works for the debugger, so I must be missing something.
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Panos Astithas [:past] from comment #3)
> Can you explain why the console can't rely on the target events for
> navigation, but has to use the protocol directly? This works for the
> debugger, so I must be missing something.
This is something I wanted to ping you about yesterday. I wasn't sure which is the preferred way: target will-navigate/navigate events or the debugger protocol tabNavigated?
The problem at hand is that the web console needs the payload information, and that's not a recommended approach per Target.jsm comments (it's only there for non-remotable tools).
Why should we use the Target API's abstraction here? Do we plan to support other non-remotable targets?
Comment 5•12 years ago
|
||
(In reply to Mihai Sucan [:msucan] from comment #4)
> Why should we use the Target API's abstraction here? Do we plan to support
> other non-remotable targets?
The idea is to reduce code duplication and simplify tools, by moving common code in the target so that it gets reused by all of them. This way the style editor and the page inspector (and scratchpad if it's ever moved inside the toolbox) won't have to know about the navigation details of the remote protocol.
I think the problem is that I forgot to copy nativeConsoleAPI when constructing the event object in onRemoteTabNavigated. Does doing that fix this bug?
Updated•12 years ago
|
Attachment #738063 -
Flags: review?(past)
Assignee | ||
Comment 6•12 years ago
|
||
Changed to use the Target API will-navigate and navigate events.
Attachment #738063 -
Attachment is obsolete: true
Attachment #738414 -
Flags: review?(past)
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Panos Astithas [:past] from comment #5)
> (In reply to Mihai Sucan [:msucan] from comment #4)
> > Why should we use the Target API's abstraction here? Do we plan to support
> > other non-remotable targets?
>
> The idea is to reduce code duplication and simplify tools, by moving common
> code in the target so that it gets reused by all of them. This way the style
> editor and the page inspector (and scratchpad if it's ever moved inside the
> toolbox) won't have to know about the navigation details of the remote
> protocol.
As discussed on IRC I don't see any net benefit for this will-navigate/navigate abstraction on top of tabNavigated. I'd prefer will-navigate/navigate to be removed once all tools are remotable.
> I think the problem is that I forgot to copy nativeConsoleAPI when
> constructing the event object in onRemoteTabNavigated. Does doing that fix
> this bug?
Yes. I updated the patch accordingly. I don't feel strong about which way we do things.
Thanks!
Comment 8•12 years ago
|
||
Comment on attachment 738414 [details] [diff] [review]
updated patch
Review of attachment 738414 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks, and sorry for the breakage!
Attachment #738414 -
Flags: review?(past) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Thanks for the review!
Landed:
https://hg.mozilla.org/integration/fx-team/rev/4112b098c118
(last try push was green)
Whiteboard: [fixed-in-fx-team]
Updated•12 years ago
|
Keywords: regression
Comment 10•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•