Last Comment Bug 706193 - footer text on nytimes.com inflated too much
: footer text on nytimes.com inflated too much
Status: VERIFIED FIXED
readability, [font-inflation: list]
: mobile
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: mozilla14
Assigned To: David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
:
Mentors:
: 711759 713241 714080 718501 719685 720567 729961 735609 (view as bug list)
Depends on: 743105 743817 747857 768942
Blocks: 707195 font-inflation 711759 713241 714080 719685 729961
  Show dependency treegraph
 
Reported: 2011-11-29 11:27 PST by Madhava Enros [:madhava]
Modified: 2012-06-27 09:39 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta+
11+


Attachments
Add a preference for the threshold at which we have enough text within a BFC to use font size inflation. (, patch 1) (15.86 KB, patch)
2012-04-12 15:21 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
roc: review+
Details | Diff | Splinter Review
Add a font inflation data structure per block formatting context. (, patch 2) (10.47 KB, patch)
2012-04-12 15:21 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
Build font data structure by walking the necessary text. (, patch 3) (13.99 KB, patch)
2012-04-12 15:21 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
Use the font inflation data to disable font inflation for small pieces of text. (, patch 4) (3.46 KB, patch)
2012-04-12 15:21 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
roc: review+
Details | Diff | Splinter Review
Add tests for font.size.inflation.lineThreshold preference and the associated threshold behavior. (, patch 5) (23.72 KB, patch)
2012-04-12 15:22 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
roc: review+
Details | Diff | Splinter Review
Add a font inflation data structure per block formatting context. (, patch 2) (17.21 KB, patch)
2012-04-13 16:21 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
roc: review+
Details | Diff | Splinter Review
Build font data structure by walking the necessary text. (, patch 3) (14.19 KB, patch)
2012-04-13 16:22 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
Build font data structure by walking the necessary text. (, patch 3) (16.98 KB, patch)
2012-04-13 16:38 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
Build font data structure by walking the necessary text. (, patch 3) (16.99 KB, patch)
2012-04-13 17:50 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
Add a font inflation data structure per block formatting context. (, patch 2) (19.24 KB, patch)
2012-04-15 17:59 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
roc: review+
Details | Diff | Splinter Review
Build font data structure by walking the necessary text. (, patch 3) (19.25 KB, patch)
2012-04-15 18:01 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
roc: review+
Details | Diff | Splinter Review

Description Madhava Enros [:madhava] 2011-11-29 11:27:40 PST
As seen here: http://www.flickr.com/photos/madhava_work/6426486801/
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-01 14:52:58 PST
Feel free to dupe this to another font inflation bug
Comment 2 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-12-07 14:50:55 PST
In some sense, this is inflation doing what it's supposed to do.  That said, this is text that's intended to fit on one line, so it doesn't really need inflation.  I think iOS Safari has a character threshold for its inflation, so you need at least a certain size run of text in the same style before it triggers inflation.  That said, this is probably more than enough text to trigger that.  I'm curious if there's some other basis we could use to determine that it's ok not to inflate the text here.
Comment 3 Scott Johnson (:jwir3) 2011-12-08 14:46:10 PST
I actually don't think this is too big. Instead, I'm more interested in the nytimes news articles being smaller (i.e. not inflated). Any idea why these //weren't// inflated?

We could probably detect a situation like this, because these appear to be list elements. So, we could maybe detect a horizontally-laid out list, and suppress the inflation there?
Comment 4 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-12-08 20:33:47 PST
(In reply to Scott Johnson (:jwir3) from comment #3)
> I actually don't think this is too big. Instead, I'm more interested in the
> nytimes news articles being smaller (i.e. not inflated). Any idea why these
> //weren't// inflated?

Why what exactly isn't inflated?

> We could probably detect a situation like this, because these appear to be
> list elements. So, we could maybe detect a horizontally-laid out list, and
> suppress the inflation there?

Detect how?
Comment 5 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-12-29 09:02:37 PST
Basically, to fix this, we need to detect the situation where the text wouldn't be wrapped if it is not inflated.  I'd suggest that we may actually want to allow 2 lines of text.

I think this is in turn going to require caching information about font inflation (as will bug 706609).  I'm thinking that I want to use 2 frame state bits:
 a. one to say whether something is a container for font size inflation
 b. the other to say whether the nearest ancestor-or-self container gets inflated

Then, for each container, we'll have the chance to do a small amount of work (scanning text and its styles) to figure out (b) above, probably at frame-construction time.  One issue here is that this can't actually just be per-container -- the text scanning would have to run across containers, since we want:
  <p>A lot of text</p>
  <p>Just a few words</p>
to yield the same inflation for the two paragraphs, not small text for the one-liner.

I suspect that, when doing this scanning, it may be sufficient to look at the intrinsic widths of the text and (for finding contiguous styles) at the styles of the block.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-12-29 12:26:32 PST
> I suspect that, when doing this scanning, it may be sufficient to look at
> the intrinsic widths of the text and (for finding contiguous styles) at the
> styles of the block.

Actually, I don't think so, because this needs to handle parent/child relationships well.  So I think it needs to look at the styles of the contiguous pieces of text.
Comment 7 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-01-19 14:07:34 PST
(In reply to David Baron [:dbaron] from comment #6)
> > I suspect that, when doing this scanning, it may be sufficient to look at
> > the intrinsic widths of the text and (for finding contiguous styles) at the
> > styles of the block.
> 
> Actually, I don't think so, because this needs to handle parent/child
> relationships well.  So I think it needs to look at the styles of the
> contiguous pieces of text.

Except we probably need to look at more than text -- need to look at things like form controls, which can be inflated.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-01-26 14:47:01 PST
Brief status update:  this is the next thing I'm planning to work on, but it requires a bit of research and investigation to figure out what to do and then how to do it (neither of which I know precisely), so there's basically zero chance I'm going to get it done before I leave for vacation and a CSS WG meeting.
Comment 9 Scott Johnson (:jwir3) 2012-02-01 13:10:12 PST
David, 

Since you're on vacation, if you'd like, I can start on some of these things (e.g. adding the frame state bits and seeing if I can work through getting some of this set at frame construction time), and then we can get back together about it after you're vacation and CSS WG meetings.
Comment 10 Scott Johnson (:jwir3) 2012-02-01 15:40:42 PST
(In reply to Scott Johnson (:jwir3) from comment #9)
> (e.g. adding the frame state bits and seeing if I can work through getting
> some of this set at frame construction time)

Ah. I didn't realize that you had already added that as part of bug 706609. I'll look into seeing if I can utilize it for this (and possibly other) cases.
Comment 11 Matt Brubeck (:mbrubeck) 2012-02-07 12:24:53 PST
*** Bug 720567 has been marked as a duplicate of this bug. ***
Comment 12 JP Rosevear [:jpr] 2012-02-23 19:27:23 PST
Beta blockers.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-02-24 17:43:58 PST
OK, to fix both this and bug 707195, I'm starting to think that maybe the right approach is to:

 * scan all of the text in each BFC, and for each font size used, gather:
   + the largest width at which that font size was used
   + the amount of text at that font size

Then, to fix bug 706193, we wouldn't do inflation at all when there isn't a sufficient amount of text to merit it.

To fix bug 707195, we'd keep the inflation ratio consistent within the BFC, and for each font size apply the largest width of it and the font sizes smaller than it.


I still need to work out how to do dynamic updating.
Comment 14 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-03-02 16:04:14 PST
(In reply to David Baron [:dbaron] from comment #13)
> OK, to fix both this and bug 707195, I'm starting to think that maybe the
> right approach is to:
> 
>  * scan all of the text in each BFC, and for each font size used, gather:
>    + the largest width at which that font size was used

Except I don't see how we'd know the widths at the appropriate time (now that I'm trying to implement it).

>    + the amount of text at that font size
> 
> Then, to fix bug 706193, we wouldn't do inflation at all when there isn't a
> sufficient amount of text to merit it.
> 
> To fix bug 707195, we'd keep the inflation ratio consistent within the BFC,
> and for each font size apply the largest width of it and the font sizes
> smaller than it.

I'm inclined to give up on trying to fix bug 707195 (i.e., wontfix it) and try to approach this bug alone.
Comment 15 Scott Johnson (:jwir3) 2012-03-20 10:06:33 PDT
*** Bug 718501 has been marked as a duplicate of this bug. ***
Comment 16 Scott Johnson (:jwir3) 2012-03-26 15:00:17 PDT
*** Bug 713241 has been marked as a duplicate of this bug. ***
Comment 17 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-03-30 10:47:59 PDT
*** Bug 711759 has been marked as a duplicate of this bug. ***
Comment 18 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-03-30 10:50:06 PDT
*** Bug 714080 has been marked as a duplicate of this bug. ***
Comment 19 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-03-30 10:50:13 PDT
*** Bug 719685 has been marked as a duplicate of this bug. ***
Comment 20 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-03-30 10:50:25 PDT
*** Bug 729961 has been marked as a duplicate of this bug. ***
Comment 21 JP Rosevear [:jpr] 2012-04-05 08:08:19 PDT
David, do you have a status update here?
Comment 22 Kevin Brosnan [:kbrosnan] 2012-04-09 09:43:51 PDT
*** Bug 735609 has been marked as a duplicate of this bug. ***
Comment 23 JP Rosevear [:jpr] 2012-04-09 11:17:54 PDT
Looks like the dep is ready to land.
Comment 24 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-12 15:21:16 PDT
Created attachment 614586 [details] [diff] [review]
Add a preference for the threshold at which we have enough text within a BFC to use font size inflation.  (, patch 1)
Comment 25 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-12 15:21:28 PDT
Created attachment 614587 [details] [diff] [review]
Add a font inflation data structure per block formatting context.  (, patch 2)
Comment 26 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-12 15:21:44 PDT
Created attachment 614588 [details] [diff] [review]
Build font data structure by walking the necessary text.  (, patch 3)
Comment 27 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-12 15:21:58 PDT
Created attachment 614589 [details] [diff] [review]
Use the font inflation data to disable font inflation for small pieces of text.  (, patch 4)
Comment 28 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-12 15:22:13 PDT
Created attachment 614590 [details] [diff] [review]
Add tests for font.size.inflation.lineThreshold preference and the associated threshold behavior.  (, patch 5)
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-13 03:53:11 PDT
Comment on attachment 614587 [details] [diff] [review]
Add a font inflation data structure per block formatting context.  (, patch 2)

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

::: layout/generic/nsFrame.cpp
@@ +495,5 @@
> +  NS_ASSERTION(aFrame->GetStateBits() & NS_FRAME_FONT_INFLATION_CONTAINER,
> +               "should only call this on containers");
> +  nsIAtom *fType = aFrame->GetType();
> +  return (aFrame->IsFrameOfType(nsIFrame::eBlockFrame) &&
> +          (aFrame->GetStateBits() & NS_BLOCK_FLOAT_MGR)) ||

Do we need the IsFrameOfType check here?

@@ +499,5 @@
> +          (aFrame->GetStateBits() & NS_BLOCK_FLOAT_MGR)) ||
> +         aFrame->IsFrameOfType(nsIFrame::eXULBox) ||
> +         // For cells, NS_BLOCK_FLOAT_MGR is set on the anon box inside,
> +         // which isn't an inflation container at all (since its parent
> +         // has the same content node).

Rearrange this expression so that we only call GetType if we need it, and so that we check everything else before doing IsFrameOfType calls.

Maybe it would be better to simply have a new IsFrameOfType bit here?
Comment 30 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-13 06:15:40 PDT
Comment on attachment 614588 [details] [diff] [review]
Build font data structure by walking the necessary text.  (, patch 3)

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

::: layout/generic/nsFrame.cpp
@@ +3572,5 @@
>      CoordNeedsRecalc(metrics->mAscent);
>    }
> +
> +  if (GetStateBits() & NS_FRAME_FONT_INFLATION_FLOW_ROOT) {
> +    nsFontInflationData::MarkFontInflationDataDirty(this);

Why do we need to do this since you fully recreate font inflation data on every reflow anyway?
Comment 31 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-13 08:57:57 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> Do we need the IsFrameOfType check here?

Yes, since NS_BLOCK_FLOAT_MGR is a class-private bit.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> Why do we need to do this since you fully recreate font inflation data on
> every reflow anyway?

Where do I fully recreate them on every reflow?
Comment 32 Mark Finkle (:mfinkle) (use needinfo?) 2012-04-13 10:39:41 PDT
Given the code changes, we'd like to see this land before beta and we still thinks this blocks.
Comment 33 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-13 12:08:34 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> Rearrange this expression so that we only call GetType if we need it, and so
> that we check everything else before doing IsFrameOfType calls.
> 
> Maybe it would be better to simply have a new IsFrameOfType bit here?

I think if we're worried about performance, it makes more sense to just spread the setting-the-bit logic out and put it in the various constructors of various frame classes as needed (e.g., with the !GetParent() in nsFrame, with an is-inflation-container check and the space manager check in nsBlockFrame, and with only an is-inflation-container check in nsBoxFrame and nsLeafBoxFrame, nsHTMLScrollFrame, nsXULScrollFrame, and nsTableCellFrame).  Does that sound ok to you?
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-13 15:56:01 PDT
(In reply to David Baron [:dbaron] from comment #31)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> > Why do we need to do this since you fully recreate font inflation data on
> > every reflow anyway?
> 
> Where do I fully recreate them on every reflow?

When we do this in nsHTMLReflowState::Init?

if (frame->GetStateBits() & NS_FRAME_FONT_INFLATION_FLOW_ROOT) {
  // Destroy and re-create our font inflation data.
  nsFontInflationData::CreateFontInflationDataFor(*this);
}
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-13 15:56:51 PDT
(In reply to David Baron [:dbaron] from comment #33)
> I think if we're worried about performance, it makes more sense to just
> spread the setting-the-bit logic out and put it in the various constructors
> of various frame classes as needed (e.g., with the !GetParent() in nsFrame,
> with an is-inflation-container check and the space manager check in
> nsBlockFrame, and with only an is-inflation-container check in nsBoxFrame
> and nsLeafBoxFrame, nsHTMLScrollFrame, nsXULScrollFrame, and
> nsTableCellFrame).  Does that sound ok to you?

Yes!
Comment 36 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-13 16:01:29 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)
> (In reply to David Baron [:dbaron] from comment #31)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> > > Why do we need to do this since you fully recreate font inflation data on
> > > every reflow anyway?
> > 
> > Where do I fully recreate them on every reflow?
> 
> When we do this in nsHTMLReflowState::Init?
> 
> if (frame->GetStateBits() & NS_FRAME_FONT_INFLATION_FLOW_ROOT) {
>   // Destroy and re-create our font inflation data.
>   nsFontInflationData::CreateFontInflationDataFor(*this);
> }

Hmmm.  I should remove that.
Comment 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-13 16:06:30 PDT
Comment on attachment 614588 [details] [diff] [review]
Build font data structure by walking the necessary text.  (, patch 3)

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

::: layout/generic/nsFontInflationData.cpp
@@ +146,5 @@
> +  }
> +
> +  PRUint32 len = frames.Length();
> +  nsHTMLReflowState *reflowStates =
> +    static_cast<nsHTMLReflowState*>(malloc(sizeof(nsHTMLReflowState) * len));

moz_xmalloc?

@@ +192,5 @@
> +  }
> +
> +  nsIFrame *nca = NearestCommonAncestorFirstInFlow(firstInflatableDescendant,
> +                                                   lastInflatableDescendant,
> +                                                   bfc);

How important is this? How often is nca not equal to bfc?

@@ +229,5 @@
> +        // Goes in a different set of inflation data.
> +        continue;
> +      }
> +
> +      if (kid->GetType() == nsGkAtoms::textFrame) {

This doesn't handle XUL textboxes but I guess that's OK.

@@ +265,5 @@
> +                prevWS = true;
> +              } else {
> +                ++len;
> +                prevWS = false;
> +              }

Move this out to something in nsTextFrameUtils I think ... say ComputeApproximateLengthWithWhitespaceCompression(nsStyleText*, nsIContent*)?
Comment 38 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-13 16:21:47 PDT
Created attachment 614946 [details] [diff] [review]
Add a font inflation data structure per block formatting context.  (, patch 2)
Comment 39 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-13 16:22:18 PDT
Created attachment 614947 [details] [diff] [review]
Build font data structure by walking the necessary text.  (, patch 3)
Comment 40 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-13 16:23:33 PDT
Comment on attachment 614947 [details] [diff] [review]
Build font data structure by walking the necessary text.  (, patch 3)

oops, sorry, raced with comment 37
Comment 41 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-13 16:25:20 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> @@ +192,5 @@
> > +  }
> > +
> > +  nsIFrame *nca = NearestCommonAncestorFirstInFlow(firstInflatableDescendant,
> > +                                                   lastInflatableDescendant,
> > +                                                   bfc);
> 
> How important is this? How often is nca not equal to bfc?

I think at the top level of the page it can be kinda important, since pages are often "fixed width", but the fixed width is unlikely to be on the root and may not be on the body either.
Comment 42 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-13 16:38:33 PDT
Created attachment 614954 [details] [diff] [review]
Build font data structure by walking the necessary text.  (, patch 3)
Comment 43 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-13 17:23:07 PDT
Comment on attachment 614954 [details] [diff] [review]
Build font data structure by walking the necessary text.  (, patch 3)

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

::: layout/generic/nsFontInflationData.cpp
@@ +131,5 @@
> +
> +  while (aFrame1 != aFrame2) {
> +    aFrame1 = aFrame1->GetParent()->GetFirstInFlow();
> +    aFrame2 = aFrame2->GetParent()->GetFirstInFlow();
> +  }

Wouldn't it be simpler and faster to build two nsAutoTArrays of ancestor nsIFrame*s the way nsLayoutUtils::DoCompareTreePosition does?

::: layout/generic/nsHTMLReflowState.cpp
@@ +318,5 @@
> +
> +  if (frame->GetStateBits() & NS_FRAME_FONT_INFLATION_FLOW_ROOT) {
> +    // Destroy and re-create our font inflation data.
> +    nsFontInflationData::EnsureFontInflationDataFor(*this);
> +  }

Fix comment, since you're not destroying and recreating the data! In fact, why do we need this here? Can't we just call EnsureFontInflationDataFor from ShouldInflateFontsForContainer?
Comment 44 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-13 17:50:57 PDT
Created attachment 614974 [details] [diff] [review]
Build font data structure by walking the necessary text.  (, patch 3)
Comment 45 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-13 17:54:13 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43)
> why do we need this here? Can't we just call EnsureFontInflationDataFor from
> ShouldInflateFontsForContainer?

No, because it needs container widths.  (I think it's avoidable, but doing so would be pretty complicated.)
Comment 46 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-13 21:28:19 PDT
Comment on attachment 614974 [details] [diff] [review]
Build font data structure by walking the necessary text.  (, patch 3)

Turns out I have some try server orange to deal with related to the reflow state setup in patch 3, though.
Comment 47 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-13 23:01:07 PDT
What ensures that font inflation data is recomputed if some ancestor container width changes, during a resize reflow?
Comment 48 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-14 12:20:12 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47)
> What ensures that font inflation data is recomputed if some ancestor
> container width changes, during a resize reflow?

Right.  That's why I had that code there.

I should probably dirty the width information and the text size information separately.
Comment 49 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-15 17:59:30 PDT
Created attachment 615213 [details] [diff] [review]
Add a font inflation data structure per block formatting context. (, patch 2)

The changes to this patch are pretty minor, to fix some assertions during crashtests/reftests.  The most straightforward way to fix them (and I'm not 100% sure I've got them all, but I think it gets the first 10 or so that were hit, with 2 changes) was to make the following two changes which I think are both correct:

 * make any floating or abs-pos elements that are inflation containers also be flow roots (unfortunately, I couldn't find a point during frame initialization when NS_FRAME_OUT_OF_FLOW is set reliably, so I just used disp->IsFloating() || disp->IsAbsolutelyPositioned().

 * make SVG outer frames and foreignObject frames always be both inflation containers and flow roots

This avoids hitting some assertions related to reflow state construction, which these things expect to be done in peculiar ways.
Comment 50 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-15 18:01:41 PDT
Created attachment 615214 [details] [diff] [review]
Build font data structure by walking the necessary text. (, patch 3)

This has some pretty major changes to address the comments.  Mainly:

 * width recomputation and text rescanning are now separate (addressing comment 48, which was a mistake in addressing comment 36)

 * I added the optimization to stop scanning text when we cross the threshold, which will avoid scanning all of large swaths of text
Comment 51 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-15 19:36:05 PDT
Comment on attachment 615214 [details] [diff] [review]
Build font data structure by walking the necessary text. (, patch 3)

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

::: layout/generic/nsFontInflationData.cpp
@@ +196,5 @@
> +
> +  nsIFrame *firstInflatableDescendant =
> +             FindEdgeInflatableFrameIn(bfc, eFromStart),
> +           *lastInflatableDescendant =
> +             FindEdgeInflatableFrameIn(bfc, eFromEnd);

May as well move this call to the if (firstInflatableDescendant) path.
Comment 52 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-16 00:20:21 PDT
https://tbpl.mozilla.org/?tree=Try&rev=e62c45257f01 has a few more failures I need to deal with (a bunch of assertions that are a regression from the SVG change in comment 49, I think because it was missing one additional piece needed), and a crash that looks like a null dereference so probably ought to be easy.  I'll look more closely in the morning.
Comment 53 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-16 12:05:15 PDT
Two small changes to fix those failures:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/34274b741da6
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/2858f41baab4
and a new try run:
https://tbpl.mozilla.org/?tree=Try&rev=2038675d9c88
Comment 56 Andreea Pod 2012-05-28 04:43:46 PDT
The footer text on nytimes.com now looks good. Marking this as verified fixed on build: Firefox 15.0a1 (2012-05-27)
Device: LG Optimus 2X (Android 2.2.2)

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