Closed Bug 897960 Opened 11 years ago Closed 11 years ago

walker should support mozbrowser iframes

Categories

(DevTools :: General, defect)

Product:
DevTools
Component:
General
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 26

People

(Reporter: kgrandon, Assigned: paul)

References

Details

Attachments

(6 files)

Screenshot from 2013-09-07 17:43:08.png
(deleted), image/png
Details
Patch A. Fix LayoutHelpers.jsm
(deleted), patch
dcamp
: review+
Details | Diff | Splinter Review
Patch B. helpers.js
(deleted), patch
dcamp
: review+
Details | Diff | Splinter Review
Patch C. s/LayoutHelpers/new LayoutHelpers(win)/g
(deleted), patch
dcamp
: review+
Details | Diff | Splinter Review
Patch D. inspector.js
(deleted), patch
dcamp
: review+
Details | Diff | Splinter Review
Patch E. Fix more tests
(deleted), patch
paul
: review+
Details | Diff | Splinter Review
This is working in the currently released Firefox, but broken in Firefox Nightly. When I open gaia inside of nightly and launch an App, it's difficult to inspect those elements because the inspector will not jump to the right element. My guess is that this might be somehow related to mozbrowser/mozapp iframes.
Could it be your fault? (iframes don't expand in the markupview)
Flags: needinfo?(dcamp)
I meant: Dcamp, could it be your fault? (iframes don't expand in the markupview)
Kevin, Rik, how do you select the element? Right-click -> Inspect?
It doesn't matter, it fails with both methods: "right-click > inspect" or "activate select element with mouse > click"
Could you attach a screenshot?
Anthony just showed me: - gaia with mozbrowser iframes - iframe don't expand in the markup view - breadcrumbs are correctly displayed with the right selection - it works in Firefox 24
Does this only happen for mozbrowser iframes? Do normal iframes work as expected?
Flags: needinfo?(paul)
Normal iframes work as expected.
Flags: needinfo?(paul)
While debugging apps, iframes are empty.
What would help a lot is a reduced test case.
Kevin: Do you have any idea if we're doing something special to insert those iframes that could trip up the devtools?
Flags: needinfo?(kgrandon)
mozbrowser iframes host a window that runs in a different process. We don't support that yet.
Summary: Inspector does not jump to correct element in nightly → walker should support mozbrowser iframes (e10s)
actually, afair, what Anthony showed me was running in a regular Firefox desktop (no multiprocess). So i'm not sure about the e10s part.
Summary: walker should support mozbrowser iframes (e10s) → walker should support mozbrowser iframes
Blocks: 889889
it appears to work when I use the markup view. It might be at the highlighter level.
In markup-view.js, in _ensureVisible, in the while loop, we get this exception: > TypeError: value is not a non-null object Because parentNode() return null: > let parent = node.parentNode(); > if (!container.elt.parentNode) { > let parentContainer = this._containers.get(parent); Where node is the <html> tag.
I figured it out. Patch coming.
Here is the issue: In Gaia code, we use mozbrowser iframes: <iframe mozbrowser>. These iframes work in a self contained way. That means we can do `window.parent` and `window.frameElement`. The solution is to use `nsIDocShell.getSameTypeParentIgnoreBrowserAndAppBoundaries()`. From this, we can get the parent window, but then, 2 problems: 1. it doesn't solve the window.frameElement issue 2. we don't know when to stop going up in the window hierarchy. We don't stop at the tab level, but at the very top window level (browser.xul in Firefox Desktop, and shell.xul in B2G) The coming patch solves these 2 issues this way: For 1, we just get the parent window, go through all the iframes, if one has a contentWindow that matches the window, we found the iframe. There's probably a better way to do that, but not sure how. For 2, we could do some magic trick and guess when to stop (if docshell types are different, if we end-up in a chrome window, ...), but it doesn't sound safe, and I can think of cases where we want to stop earlier (chrome window in a tab). So I introduce a "top window" variable, that says when to stop. This top window has to be defined every time we want to use something similar to window.frameElement and window.parent. Which is essentially in toolkit/LayoutHelpers.jsm and toolkit/inspector.js.
Flags: needinfo?(kgrandon)
Flags: needinfo?(dcamp)
Attached patch Patch A. Fix LayoutHelpers.jsm (deleted) — — Splinter Review
Removed irrelevant code (prettyKey) (moved to another file in another patch). LayoutHelpers is now a class. One instance of LayoutHelpers is bound by a top level window. Implement getFrameElement, getParentWindow and isTopLevelWindow.
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #801258 - Flags: review?(dcamp)
Attached patch Patch B. helpers.js (deleted) — — Splinter Review
We used LayoutHelpers.jsm to put some random helpers in it. Let's get a dedicated file and move there the not-related methods.
Attachment #801259 - Flags: review?(dcamp)
Attachment #801260 - Flags: review?(dcamp)
Attached patch Patch D. inspector.js (deleted) — — Splinter Review
Attachment #801261 - Flags: review?(dcamp)
This also solves bug 889889.
I'm working on a test (not sure how I'll test that...).
Blocks: appmgr_v1
Attachment #801258 - Flags: review?(dcamp) → review+
Attachment #801259 - Flags: review?(dcamp) → review+
Attachment #801260 - Flags: review?(dcamp) → review+
Attachment #801261 - Flags: review?(dcamp) → review+
I'm r+'ing before the tests are written because I want to get testing from the Gaia work week. But we should still get a test.
orange: https://tbpl.mozilla.org/php/getParsedLog.php?id=27562069&tree=Fx-Team Silly me, testing (bc) instead of (oth).
Attached patch Patch E. Fix more tests (deleted) — — Splinter Review
Attachment #801405 - Flags: review?(paul)
Attachment #801405 - Attachment description: Patch D. Fix more tests → Patch E. Fix more tests
Attachment #801405 - Flags: review?(paul) → review+
Whiteboard: [land-in-fx-team]
Whiteboard: [land-in-fx-team]
This is a huge help for us, thank you!
For future reference, it's better if the commit message actually explains what the patch does.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: