739151 - Inspector 3D view (Tilt) doesn't support framesets

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Victor Porof [:vporof][:vp]]

For example, open this page in Tilt: http://users.dsic.upv.es/~jsilva/wwv2012/ - It's much too flat :) Granted, it's hard to still find pages with framesets in them, but still, it would be nice to support them.

I think TiltUtils.jsm is the place to start. Specifically, the traverse() function doesn't handle framesets (or frames) at all, only iframes. Testing could be done by extending browser_tilt_utils05.js (it would be nice to add testing in that html string for both framesets, frames and iframes).

Comment 1

[Victor Porof [:vporof][:vp]]

Added a whiteboard note to make this bug findable in http://is.gd/cS47Rb

Comment 2

[Sankha Narayan Guria [:sankha]]

Can you please assign me this bug? I would like to work on it.

Comment 3

[Victor Porof [:vporof][:vp]]

Hey, thanks for the interest!
Let me know if you need any additional help.

Comment 4

[Sankha Narayan Guria [:sankha]]

Attachment: Tilt now supports frames

Comment 5

[Victor Porof [:vporof][:vp]]

Comment on attachment 638741 [details] [diff] [review]
Tilt now supports frames

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

Thanks for the patch! Works nicely!

The reason we're creating a top level iframe in browser_tilt_utils05.js is just to have some DOM we'll parse later, which is given by the html string for the iframe source. The test you added doesn't address the actual content that will eventually be displayed in Tilt, but makes the visualized document container a frame instead of iframe.

For the test, please revert back to using an iframe as a container, like in the utils05.js test, and add some more intricate frameset(s) + frames in the html source string. Please make sure that you're handling interesting cases like multiple frames, iframes, maybe a couple of framesets as well. You can also remove all the extra nodes that are already tested in utils05.js, like script and head.

Comment 6

[Sankha Narayan Guria [:sankha]]

Attachment: Updated the tests with nested frames

Comment 7

[Victor Porof [:vporof][:vp]]

Comment on attachment 640924 [details] [diff] [review]
Updated the tests with nested frames

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

This is an awesome patch! Thanks!
Definitely r+ for a great and comprehensive test.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ac47d6d7876f

Comment 8

[Victor Porof [:vporof][:vp]]

Comment on attachment 640924 [details] [diff] [review]
Updated the tests with nested frames

Rob, please take a look so we can land this.

Comment 9

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 640924 [details] [diff] [review]
Updated the tests with nested frames

you bet!

thanks for the patch! :D

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/564ed4fde8ab

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/564ed4fde8ab