Closed Bug 862024 Opened 7 years ago Closed 7 years ago

Warning about replaced window.console API shows when content scripts do not change the object

Categories

(DevTools :: Console, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: msucan, Assigned: msucan)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
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.
Attached patch proposed fix (obsolete) — Splinter Review
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)
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.
(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?
(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?
Attached patch updated patchSplinter Review
Changed to use the Target API will-navigate and navigate events.
Attachment #738063 - Attachment is obsolete: true
Attachment #738414 - Flags: review?(past)
(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 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+
Thanks for the review!

Landed:
https://hg.mozilla.org/integration/fx-team/rev/4112b098c118

(last try push was green)
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4112b098c118
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.