Last Comment Bug 739151 - Inspector 3D view (Tilt) doesn't support framesets
: Inspector 3D view (Tilt) doesn't support framesets
[tilt][good first bug][mentor=vporof@...
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Sankha Narayan Guria [:sankha]
: Patrick Brosset <:pbro>
Depends on:
  Show dependency treegraph
Reported: 2012-03-26 00:43 PDT by Victor Porof [:vporof][:vp]
Modified: 2012-07-19 07:40 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Tilt now supports frames (5.82 KB, patch)
2012-07-03 08:42 PDT, Sankha Narayan Guria [:sankha]
no flags Details | Diff | Splinter Review
Updated the tests with nested frames (8.80 KB, patch)
2012-07-10 22:28 PDT, Sankha Narayan Guria [:sankha]
vporof: review+
rcampbell: review+
Details | Diff | Splinter Review

Description Victor Porof [:vporof][:vp] 2012-03-26 00:43:41 PDT
For example, open this page in Tilt: - 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] 2012-03-26 15:34:17 PDT
Added a whiteboard note to make this bug findable in
Comment 2 Sankha Narayan Guria [:sankha] 2012-06-29 07:53:07 PDT
Can you please assign me this bug? I would like to work on it.
Comment 3 Victor Porof [:vporof][:vp] 2012-07-02 08:04:04 PDT
Hey, thanks for the interest!
Let me know if you need any additional help.
Comment 4 Sankha Narayan Guria [:sankha] 2012-07-03 08:42:01 PDT
Created attachment 638741 [details] [diff] [review]
Tilt now supports frames
Comment 5 Victor Porof [:vporof][:vp] 2012-07-03 09:36:32 PDT
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] 2012-07-10 22:28:03 PDT
Created attachment 640924 [details] [diff] [review]
Updated the tests with nested frames
Comment 7 Victor Porof [:vporof][:vp] 2012-07-13 08:02:46 PDT
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:
Comment 8 Victor Porof [:vporof][:vp] 2012-07-13 08:03:15 PDT
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) 2012-07-17 12:27:51 PDT
Comment on attachment 640924 [details] [diff] [review]
Updated the tests with nested frames

you bet!

thanks for the patch! :D
Comment 10 Panos Astithas [:past] 2012-07-18 07:10:48 PDT
Comment 11 Tim Taubert [:ttaubert] 2012-07-19 07:40:39 PDT

Note You need to log in before you can comment on or make changes to this bug.