While using the inspector on B2G: console.warn: Tried to use rawNode on a remote connection

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

Trunk
Firefox 28
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)

Comment 1

4 years ago
I put that warning in to help identify code that was expecting raw nodes and not dealing well with them.  Now there are two places .rawNode is used:

a) The markupview checks for local nodes because we don't do tag renaming remotely yet
b) The computed-view checks for local nodes when clicking on a link to give view-source extra context if possible.

These are both well-behaved, so I see two possibilities:

1) Just remove the warning.
2) Add an isLocal() thing to the front that well-behaved code can use to protect itself from the warning.
This doesn't seem like a critical fix to me for App Manager v1.  Though it's probably quick to fix too... but there are many other things to do.
No longer blocks: 912913
This also happens when debugging Thunderbird. As mentioned, not really critical though.
Blocks: 876636
(Assignee)

Comment 4

4 years ago
Created attachment 825317 [details] [diff] [review]
v1
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #825317 - Flags: review?(bgrinstead)
(Assignee)

Comment 5

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=6ee75940f7cb
Comment on attachment 825317 [details] [diff] [review]
v1

Review of attachment 825317 [details] [diff] [review]:
-----------------------------------------------------------------

It will be great to not see the warnings anymore! Here are a couple of other places I found rawNode() accessed that should be updated as well: 

* https://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/computed-view.js#1220
* https://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/selection.js#107

::: browser/devtools/inspector/selection.js
@@ +210,5 @@
>      }
>  
>      // As long as there are still tools going around
>      // accessing node.rawNode, this needs to stay.
> +    if (!node.doesUseRemoteConnection_toBeDeprecated()) {

This logic is actually subtly different than before.  Even in the case of a non-remote node, there is still a chance that rawNode() will return null.  Before, it would skip this condition and continue into the rest of the function, but now it will return false all the time after entering.

I'm guessing that the instance where this is the case it may have ended up returning false anyway, but just something to be aware of - may want to do as you did before with: 

let rawNode = null;
if (node.doesUseRemoteConnection_toBeDeprecated()) {
    rawNode = node.rawNode();
}
if (rawNode) ...

::: toolkit/devtools/server/actors/inspector.js
@@ +563,5 @@
> +   *
> +   * This will, one day, be removed. External code should
> +   * not need to know if the target is remote or not.
> +   */
> +  doesUseRemoteConnection_toBeDeprecated: function() {

Maybe we could rename / invert this to isLocal() to be consistent with the WalkerFront: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/selection.js#199.  Or maybe rename that method to match this one (it is only accessed in one place for the walker).
Attachment #825317 - Flags: review?(bgrinstead)
(Assignee)

Comment 7

4 years ago
Created attachment 827233 [details] [diff] [review]
v2
Attachment #825317 - Attachment is obsolete: true
Attachment #827233 - Flags: review?(bgrinstead)
Comment on attachment 827233 [details] [diff] [review]
v2

Review of attachment 827233 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r+ with a green try

::: browser/devtools/styleinspector/computed-view.js
@@ +1216,5 @@
>        return;
>      }
>  
>      let contentDoc = null;
> +    if (node.isLocal_toBeDeprecated()) {

Is node defined here?  Thinking this should be this.tree.viewedElement.isLocal_toBeDeprecated.

::: toolkit/devtools/server/actors/inspector.js
@@ +558,5 @@
>    },
>  
>    /**
> +   * Do we use a local target?
> +   * Usefull to know if a rawNode is available or not.

s/Usefull/Useful
Attachment #827233 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 9

4 years ago
Created attachment 827565 [details] [diff] [review]
v2.1

Addressed comments. https://tbpl.mozilla.org/?tree=Try&rev=eb1f689862bb
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Attachment #827233 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/0d3837a1ce9d
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0d3837a1ce9d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.