Last Comment Bug 747493 - Craiglist Forums Unusable / pages with framesets not fully visible
: Craiglist Forums Unusable / pages with framesets not fully visible
Status: VERIFIED FIXED
[layout]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: Firefox 16
Assigned To: Timothy Nikkel (:tnikkel)
:
:
Mentors:
https://forums.craigslist.org/?forumI...
: 752575 753014 753351 754144 754833 755092 755580 (view as bug list)
Depends on:
Blocks: 750896 762427 763570
  Show dependency treegraph
 
Reported: 2012-04-20 11:54 PDT by Damon Sicore (:damons)
Modified: 2016-07-29 14:24 PDT (History)
33 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
video of the damage (1.73 MB, video/quicktime)
2012-04-20 12:50 PDT, Damon Sicore (:damons)
no flags Details
testcase (908 bytes, text/html)
2012-04-23 09:48 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Simpler testcase (235 bytes, text/html)
2012-04-27 16:02 PDT, Mats Palmgren (:mats)
no flags Details
wip (8.27 KB, patch)
2012-05-29 15:46 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
patch (8.30 KB, patch)
2012-06-08 14:31 PDT, Timothy Nikkel (:tnikkel)
mats: review+
roc: review+
Details | Diff | Splinter Review
patch (9.58 KB, patch)
2012-06-09 06:54 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
patch (9.56 KB, patch)
2012-06-09 07:28 PDT, Timothy Nikkel (:tnikkel)
mark.finkle: approval‑mozilla‑aurora+
blassey.bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Damon Sicore (:damons) 2012-04-20 11:54:39 PDT
Fennec Native 14.01a, Nightly (2012-04-20)

STR: Visit https://forums.craigslist.org/?forumID=76&areaID=1 to get some support because you are getting a divorce.

Expected results:

Be able to view the entire page, be able to log in, be able to search forums, be able to scroll around.

Actual results:

Galaxy S2:  Only the left frame message tree is visible, no way to zoom out to get the login and search functionality.

Galaxy Nexus: Both frames are visible, but half of the search text boxes are cut off, and you can't zoom out to view the entire page.  Also, the login links in the upper right hand side are inaccessible.
Comment 1 Damon Sicore (:damons) 2012-04-20 12:09:45 PDT
Works perfectly on Chrome on Android.
Comment 2 Damon Sicore (:damons) 2012-04-20 12:50:55 PDT
Created attachment 617070 [details]
video of the damage
Comment 3 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-04-20 15:19:58 PDT
I guess bug 746681 might be a dup of this bug then?
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2012-04-23 08:24:36 PDT
this doesn't look like font inflation, can we try turning it off and seeing if it still reproduces?
Comment 5 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-04-23 09:48:31 PDT
Created attachment 617506 [details]
testcase

You should see a green, yellow and a red block with this testcase

This seems to work in the latest Aurora Fennec Native build 13.0a2 2012-04-23 (if you don't switch orientation at least).

I guess this might be a regression from the Maple branch landing.
Comment 6 Catalin Suciu [:csuciu] 2012-04-24 07:40:44 PDT
This issue doesn't occur on:
Build ID : Nightly (2012-03-14)20120314031139
http://hg.mozilla.org/mozilla-central/rev/c71845b3b2a6

but it occurs on:
Build ID : Nightly (2012-03-15)20120315031154
http://hg.mozilla.org/mozilla-central/rev/d0d13f09be44

Possible range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c71845b3b2a6&tochange=d0d13f09be44
Comment 7 Damon Sicore (:damons) 2012-04-25 09:54:19 PDT
Looks like that range includes a 351 change set merge from maple for OMTC.  Should we also look back in the maple branch pre-merge and narrow a range there?
Comment 8 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-04-25 10:07:37 PDT
This might have just not worked on the Maple branch.
Benoit, Ali, is there anything QA can do more here?
Comment 9 Ali Juma [:ajuma] 2012-04-25 11:25:18 PDT
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #8)
> This might have just not worked on the Maple branch.
> Benoit, Ali, is there anything QA can do more here?

It'd be helpful to check if this works in the earliest usable Maple branch build, and if so, to find a regression range there.
Comment 10 Jet Villegas (:jet) 2012-04-25 14:04:15 PDT
To Mats for a look.
Comment 11 Catalin Suciu [:csuciu] 2012-04-26 03:01:23 PDT
This is what i found on Maple:

good build:
Maple (2012-03-21)20120221040247
http://hg.mozilla.org/projects/maple/rev/d644bee23551

bad build:
Maple (2012-03-22)20120222040208
http://hg.mozilla.org/projects/maple/rev/b520f34d78f1

Possible range:
http://hg.mozilla.org/projects/maple/pushloghtml?fromchange=d644bee23551&tochange=b520f34d78f1
Comment 12 JP Rosevear [:jpr] 2012-04-26 07:35:58 PDT
Adding in people who might be guilty based on regression range.  Pretty sure this is not readability.
Comment 13 :Ehsan Akhgari 2012-04-26 08:19:07 PDT
This looks like one of the viewport changes that we did to me.  Somebody needs to bisect this range, but finding the regression range may not be very useful, as I think this code has changed a lot since then.

kats showed me a bug in rendering overflow:hidden body elements yesterday which looked a bit like this broken-ness.
Comment 14 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-26 12:17:56 PDT
*** Bug 735652 has been marked as a duplicate of this bug. ***
Comment 15 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-26 12:18:04 PDT
*** Bug 748526 has been marked as a duplicate of this bug. ***
Comment 16 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-26 12:23:19 PDT
So I tracked this down and the problem, in a nutshell is this:

When the page body has overflow:hidden set on it, the mContentSize in the FrameMetrics that is received by CompositorParent.cpp contains the clipped page size rather than the full page size. This sort of makes sense, because that represents the size of the layer AIUI, and that's how big the layer is. However, the compositor also needs to know how big the page is without the overflow clipping, so that it can tell Java and let Java update the page size stored there.

The way I would like to fix this is by adding a couple of fields to the FrameMetrics that represent the actual page size without overflow-clipping, and use those in CompositorParent. However I don't know enough about the layout code to know where I would get the relevant values to populate the fields in FrameMetrics.

Note also that the issue from bug 746120 affects this page, and any other frameset/overflow:hidden pages.
Comment 17 :Ehsan Akhgari 2012-04-26 13:07:05 PDT
I'm not sure why you need the unclipped page dimensions anywhere.  Shouldn't Java just use the clipped size?  Or am I missing someting?
Comment 18 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-26 13:49:13 PDT
On my test page, when we first render the page, it has:

CSS viewport as 980x1413
page size as 980x1413
screen size as 720x1038
resolution as 0.7347

If the page does NOT have overflow:hidden, the mContentSize comes out as 720x1038, which gets sent to Java as the page size in device pixels. Java sees that the page is scaled appropriately to fit on the device screen, and all is good.

If the page DOES have overflow:hidden, the mContentSize comes out as 529x763, which gets sent to Java as the page size in device pixels. Java sees the page is too small for the device screen (which is 720x1038) and bumps the resolution to 1.0 so that it "fits". This causes the problems with part of the page being clipped out.

It seems to me that if a page is such that it fits on the screen, then overflow:hidden should basically have no effect. Which means that my test page (which has no significant content inside the body) should have the same values being propagated everywhere with or without overflow:hidden. The first point at which I observe a difference is this mContentSize variable, so I assume that is where the problem lies.

Why does mContentSize report a size of 529x763 instead of 720x1038 when there is an overflow:hidden, with everything else being the same?
Comment 19 :Ehsan Akhgari 2012-04-26 14:15:26 PDT
I talked to kats about it.  It seems to me that mContentSize is computed correctly for the case where we don't have a scroll frame, but maybe the caller is using it to mean the wrong thing.  It's not really clear what's causing the problem at this point.
Comment 20 Mats Palmgren (:mats) 2012-04-27 16:02:18 PDT
Created attachment 619202 [details]
Simpler testcase
Comment 21 Timothy Nikkel (:tnikkel) 2012-04-27 17:17:34 PDT
Frameset documents don't have a root scroll frame. That could break assumptions.
Comment 22 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-27 20:14:28 PDT
I still need an answer to this question:

> Why does mContentSize report a size of 529x763 instead of 720x1038 when there is an
> overflow:hidden, with everything else being the same?
Comment 23 Wesley Johnston (:wesj) 2012-05-01 08:46:29 PDT
*** Bug 743681 has been marked as a duplicate of this bug. ***
Comment 24 Timothy Nikkel (:tnikkel) 2012-05-01 15:10:34 PDT
(In reply to Kartikaya Gupta (:kats) from comment #22)
> I still need an answer to this question:
> 
> > Why does mContentSize report a size of 529x763 instead of 720x1038 when there is an
> > overflow:hidden, with everything else being the same?

The top level document is a frameset? The testcase Mats posted and the URL for this bug point to a top level document being a frameset. If so then that document won't have overflow:hidden, it has no scrollframes or any of that. So it's one of the child documents that has overflow:hidden? If so we would expect mContentSize to be smaller than the whole screen since that isn't taking up the whole screen, only a part of it.
Comment 25 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-02 06:41:01 PDT
(In reply to Timothy Nikkel (:tn) from comment #24)
> The top level document is a frameset? The testcase Mats posted and the URL
> for this bug point to a top level document being a frameset. If so then that
> document won't have overflow:hidden, it has no scrollframes or any of that.

Sorry, I should have mentioned: the page I was testing on was the simplified test case in bug 748526, located at http://people.mozilla.org/~kgupta/bug/748526.html

However, I believe the same issue happens on framesets because the html.css file sets a default overflow on framesets of -moz-hidden-unscrollable, see http://mxr.mozilla.org/mozilla-central/source/layout/style/html.css#686
Comment 26 Timothy Nikkel (:tnikkel) 2012-05-02 11:00:58 PDT
Frameset documents are quite different from all other documents. They don't have scroll frames at all. In normal documents overflow:hidden would generate a scroll frame. What makes you think that your overflow:hidden testcase is having the same issue as a frameset document?
Comment 27 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-02 12:43:25 PDT
Are you sure that overflow:hidden documents generate a scroll frame? When I stick a breakpoint at [1] I see the scrollableFrame coming out as null. Or is that chunk of code not relevant? The same page without overflow:hidden does have a scrollableFrame at that point. In fact that if statement is where the different mContentSize comes from (what I was talking about in comment 18).

As far as I can tell, frameset documents and overflow:hidden documents both do NOT generate a scroll frame, and both have a reported mContentSize that somewhere along the way doesn't take into account the resolution. This results in similar user-visible behaviour on both types of pages, namely the page appears slightly zoomed-in and cropped.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#233
Comment 28 Timothy Nikkel (:tnikkel) 2012-05-02 14:12:34 PDT
overflow:hidden documents definitely do get a scroll frame. But there are a few steps between having a scroll frame and getting to the code you pointed out where things could be different, and that could be the cause of the bug.

The bug for framesets and overflow:hidden might be fixed by the same change, or might require two fixes.

I haven't been able to look into this bug other than reading the comments here so far.
Comment 29 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-07 10:52:29 PDT
*** Bug 752575 has been marked as a duplicate of this bug. ***
Comment 30 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-08 11:31:53 PDT
*** Bug 753014 has been marked as a duplicate of this bug. ***
Comment 31 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-09 07:01:16 PDT
*** Bug 753026 has been marked as a duplicate of this bug. ***
Comment 32 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-09 07:02:22 PDT
Updating title for now since I've been using this bug to track both frameset and overflow:hidden page-truncation. It may be that they are separate issues, as tn says, in which case we can split out another bug if needed.
Comment 33 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-09 14:20:28 PDT
*** Bug 753351 has been marked as a duplicate of this bug. ***
Comment 34 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-09 14:47:37 PDT
:jrmuizel and I looked at this a bit further today. It looks like there are two viewport frames - one for the top-level window and one for the content window. The content window correctly has a CSS width of 980 pixels (which is what we set the CSS viewport width to). The outer viewport frame however has a CSS width of 720 pixels when the page has overflow:hidden, and that's what the compositor is picking up as the page size. Setting the CSS viewport on the top-level window didn't seem to affect anything; we believe that the outer viewport frame is sizing itself based on the NS_RESIZE event that is dispatched (i.e. using window.outerWidth) and that this may or may not be correct.
Comment 35 Jeff Muizelaar [:jrmuizel] 2012-05-10 16:14:20 PDT
Interestingly RecordFrameMetrics records different numbers 720 vs. 980 depending on whether we're taking a screenshot or not.
Comment 36 Timothy Nikkel (:tnikkel) 2012-05-10 20:16:50 PDT
The screenshot path doesn't ask to reuse the widget layers so it should build a fresh set of layers, meaning that the frame metrics recorded aren't used for anything I would think.
Comment 37 Vladimir Vukicevic [:vlad] [:vladv] 2012-05-14 07:32:35 PDT
*** Bug 753069 has been marked as a duplicate of this bug. ***
Comment 38 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-14 09:07:38 PDT
*** Bug 754144 has been marked as a duplicate of this bug. ***
Comment 39 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-14 09:32:58 PDT
This bug is still assigned to me, but I think the ball is in layout's court right now. I don't know what the correct fix for this bug would be.
Comment 40 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-14 11:44:53 PDT
*** Bug 754927 has been marked as a duplicate of this bug. ***
Comment 41 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-14 13:29:57 PDT
*** Bug 754833 has been marked as a duplicate of this bug. ***
Comment 42 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-15 13:30:26 PDT
*** Bug 755092 has been marked as a duplicate of this bug. ***
Comment 43 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-16 07:07:13 PDT
*** Bug 755580 has been marked as a duplicate of this bug. ***
Comment 44 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-16 07:23:53 PDT
The patch on bug 753742 seems to fix the overflow:hidden cases but not the frameset cases. Once that patch lands, I'll undupe the overflow:hidden bugs from this bug and dupe them to bug 753742 instead. Apologies in advance for the resulting bugspam.
Comment 45 Timothy Nikkel (:tnikkel) 2012-05-29 15:46:09 PDT
Created attachment 628129 [details] [diff] [review]
wip

This creates a scroll layer for frameset documents.

After this the width is drawn right, but there are some weird artifacts going on.
Comment 46 Timothy Nikkel (:tnikkel) 2012-06-06 12:30:22 PDT
The artifacts are pretty severe, sometimes not drawing the right half of the page at all.
Comment 47 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-07 10:28:19 PDT
Last update on my side: I was able to reproduce the issue tn was seeing on a droid RAZR but not the Galaxy Nexus. The reason I think stuff wasn't painting with his patch is because it is getting clipped out; at that point in the frame tree the clip rect is the size of the screen and so anything that has CSS coordinates outside that area doesn't get drawn.

IRC conversation extract:

17:13:01 kats: tn: this looks very similar to the clipping problem i was seeing in bug 757362. In nsSubdocumentFrame, the size of the dirtyRect is the size of the screen rather than the size of the CSS viewport. (I think). And so anything that has CSS coordinates that exceed your screen pixel size will be clipped out and not drawn
17:14:11 kats: tn: on the droid razr the screen width is 540x960 pixels so anything that's outside that in CSS coordinates gets clipped out
17:14:35 kats: tn: the galaxy nexus I was testing with originally has 720 width so that right frame doesn't get clipped out and draws fine
17:15:14 kats: tn: that being said i don't know what the correct fix is
17:15:30 tn: kats, that makes total sense, let me see if i can fix that
17:15:33 tn: kats, thanks
Comment 48 Timothy Nikkel (:tnikkel) 2012-06-07 11:03:54 PDT
The fix is to change the dirty region to be the display port at the right place, just need to figure out what the right place is exactly.
Comment 49 Timothy Nikkel (:tnikkel) 2012-06-07 11:04:51 PDT
There are still rendering errors after doing that, but they are less severe.
Comment 50 Jet Villegas (:jet) 2012-06-07 12:05:04 PDT
tn: Can you post up the latest WIP patch and screenshots of the glitches? We may take this fix anyway, depending on the severity of the rendering issues.
Comment 51 Timothy Nikkel (:tnikkel) 2012-06-08 11:04:05 PDT
The patch here is still the latest, the issue from comment 47 is a separate issue, so I'll post that to another bug.

I think we can probably land this patch (once it's cleaned up for lading) and worry about the rendering errors after.
Comment 52 Timothy Nikkel (:tnikkel) 2012-06-08 14:31:03 PDT
Created attachment 631532 [details] [diff] [review]
patch
Comment 53 Mats Palmgren (:mats) 2012-06-08 17:12:57 PDT
Comment on attachment 631532 [details] [diff] [review]
patch

> layout/base/nsDisplayList.cpp
>+nsDisplaySimpleScrollLayer::BuildLayer(nsDisplayListBuilder* aBuilder,
> ...
>+  if (content) {
>+    usingDisplayport = nsLayoutUtils::GetDisplayPort(content, &displayport);
>+  }

I don't think the null-check is necessary, 'nsLayoutUtils::FindIDFor(content)'
would have already crashed if 'content' was null.

(same in nsDisplayScrollLayer::BuildLayer)

+  nsRect viewport = mFrame->GetRect() -
+                    mFrame->GetPosition() +
+                    aBuilder->ToReferenceFrame(mFrame);

You can write the right hand side more efficiently as:
  nsRect(aBuilder->ToReferenceFrame(mFrame), mFrame->GetSize());

(same in nsDisplayScrollLayer::BuildLayer)



> layout/generic/nsSubDocumentFrame.cpp

Your code looks fine - perhaps add a comment why we create a layer
for frameset with a display port.

>  bool addedLayer = false;
>
>   if (subdocRootFrame && parentAPD != subdocAPD) {
>     NS_WARN_IF_FALSE(!addedLayer,
>                      "Two container layers have been added. "
>                       "Performance may suffer.");

Testing 'addedLayer' doesn't seem very useful... perhaps the warning was
left there as documentation though.

Actually, isn't that what we're doing?
(nsDisplaySimpleScrollLayer::BuildLayer creates a container layer)
Is performance OK if you zoom?

BTW, did you consider the order?  right now you may have:
nsDisplayZoom
  nsDisplaySimpleScrollLayer

ie, nsDisplaySimpleScrollLayer's layer may not be the root container layer.

I don't know much about layers though, so someone else should look too.

r=mats, with that reservation
Comment 54 Timothy Nikkel (:tnikkel) 2012-06-08 17:25:35 PDT
(In reply to Mats Palmgren [:mats] from comment #53)
> I don't think the null-check is necessary,
> 'nsLayoutUtils::FindIDFor(content)'
> would have already crashed if 'content' was null.

Ok.

> You can write the right hand side more efficiently as:
>   nsRect(aBuilder->ToReferenceFrame(mFrame), mFrame->GetSize());

Ok.

> > layout/generic/nsSubDocumentFrame.cpp
> 
> Your code looks fine - perhaps add a comment why we create a layer
> for frameset with a display port.

Ok.

> Testing 'addedLayer' doesn't seem very useful... perhaps the warning was
> left there as documentation though.

The code has been moved around a few times since that was created I think. I can look to rationalize it in a different patch.

> Actually, isn't that what we're doing?
> (nsDisplaySimpleScrollLayer::BuildLayer creates a container layer)
> Is performance OK if you zoom?

Yeah, container layers don't kill performance. It's okay to create one or two or more if we need them, but we don't want to create one that we don't need.

> BTW, did you consider the order?  right now you may have:
> nsDisplayZoom
>   nsDisplaySimpleScrollLayer
> 
> ie, nsDisplaySimpleScrollLayer's layer may not be the root container layer.

I did consider that. I think that will be fine due to how the compositor looks for the scrollable layer (breadth first search to find the first scroll layer). And I think it is also the correct way, the scroll layer will be in the child document's app units, so it should be inside the zoom item/layer. Zoom isn't currently used on mobile, so it mostly doesn't matter though.
Comment 55 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-08 21:31:17 PDT
Comment on attachment 631532 [details] [diff] [review]
patch

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

::: layout/base/nsDisplayList.cpp
@@ +2259,5 @@
> +  ViewID scrollId = nsLayoutUtils::FindIDFor(content);
> +
> +  nsRect viewport = mFrame->GetRect() -
> +                    mFrame->GetPosition() +
> +                    aBuilder->ToReferenceFrame(mFrame);

ToReferenceFrame()

@@ +2284,5 @@
> +    // The visible region for the children may be much bigger than the hole we
> +    // are viewing the children from, so that the compositor process has enough
> +    // content to asynchronously pan while content is being refreshed.
> +
> +    nsRegion childVisibleRegion = displayport + aBuilder->ToReferenceFrame(mFrame);

ToReferenceFrame()

::: layout/base/nsDisplayList.h
@@ +2016,5 @@
>  };
>  
>  /**
> + * A display item that has no purpose but to ensure its contents get
> + * their own layer.

... and that FrameMetrics are recorded, if the frame's document's root element has a displayport?
Comment 56 Timothy Nikkel (:tnikkel) 2012-06-09 06:45:02 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #55)
> ToReferenceFrame()

Ok.

> ToReferenceFrame()

Ok.

> ... and that FrameMetrics are recorded, if the frame's document's root
> element has a displayport?

Ah yes, forget to fix this up.
Comment 57 Timothy Nikkel (:tnikkel) 2012-06-09 06:54:39 PDT
Created attachment 631662 [details] [diff] [review]
patch

With review comments addressed.
Comment 58 Timothy Nikkel (:tnikkel) 2012-06-09 07:28:55 PDT
Created attachment 631664 [details] [diff] [review]
patch
Comment 59 Timothy Nikkel (:tnikkel) 2012-06-09 17:28:06 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/18b206c111cd
Comment 60 Ryan VanderMeulen [:RyanVM] 2012-06-10 15:25:50 PDT
https://hg.mozilla.org/mozilla-central/rev/18b206c111cd
Comment 61 Timothy Nikkel (:tnikkel) 2012-06-11 10:48:01 PDT
Frameset documents still need work after this bug. Bug 757362, comment 11 applies to them as well, and bug 763570.
Comment 62 Jet Villegas (:jet) 2012-06-11 11:57:39 PDT
[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: lost data when framesets are used on a page
Testing completed (on m-c, etc.): on mozilla-central for less than 1 day
Risk to taking this patch (and alternatives if risky):  moderate (patch affects frameset docs only)
String or UUID changes made by this patch: none
Comment 63 Kevin Brosnan [:kbrosnan] 2012-06-11 14:43:03 PDT
This looks good on today's trunk build (2012-06-10). Checked duplicate bugs all seem to be resolved or no longer reproducing. Google reader is giving us a mobile site now (no framesets there afik).
Comment 64 Timothy Nikkel (:tnikkel) 2012-06-11 15:12:52 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/5b923ee207e0
Comment 65 Timothy Nikkel (:tnikkel) 2012-06-11 16:32:09 PDT
Back out for simple compile error due to difference between beta and central
https://hg.mozilla.org/releases/mozilla-beta/rev/2a03198e95a1
Comment 66 Timothy Nikkel (:tnikkel) 2012-06-11 16:32:27 PDT
Landed with that fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/0d744168d314
Comment 67 Timothy Nikkel (:tnikkel) 2012-06-12 09:38:36 PDT
Comment on attachment 631664 [details] [diff] [review]
patch

We have this on central and beta right now. Doesn't that imply we automatically want it on aurora?
Comment 68 Timothy Nikkel (:tnikkel) 2012-06-12 13:24:30 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/4223ec8761b3
Comment 69 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-15 15:01:23 PDT
Note for posterity: this patch was backed out on m-c as part of bug 763570 as we had a fix that was cleaner and fixed other fallout as well. Leaving this bug closed because bug 763570 doesn't break this behaviour, just fixes it differently.

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