Last Comment Bug 567872 - Implement the basic layout of the progress element
: Implement the basic layout of the progress element
Status: RESOLVED FIXED
: html5
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla6
Assigned To: Mounir Lamouri (:mounir)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 514437 569216
Blocks: 633207
  Show dependency treegraph
 
Reported: 2010-05-24 15:16 PDT by Mounir Lamouri (:mounir)
Modified: 2011-05-10 07:10 PDT (History)
18 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch - WIP (13.89 KB, patch)
2010-05-26 09:34 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch - WIP (14.66 KB, patch)
2010-05-28 08:49 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch - WIP (19.89 KB, patch)
2010-06-01 17:16 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch - WIP (21.88 KB, patch)
2010-06-02 17:20 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch - WIP (25.12 KB, patch)
2010-06-04 10:14 PDT, Mounir Lamouri (:mounir)
dbaron: feedback-
Details | Diff | Splinter Review
WIP Patch (19.72 KB, patch)
2011-02-01 10:19 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1 (19.72 KB, patch)
2011-02-10 08:15 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1 (34.75 KB, patch)
2011-02-10 08:16 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.1 (35.28 KB, patch)
2011-03-01 06:46 PST, Mounir Lamouri (:mounir)
roc: review+
dbaron: review-
Details | Diff | Splinter Review
Inter diff (v1.1 to v1.2) (10.68 KB, patch)
2011-05-04 09:11 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.2 (37.35 KB, patch)
2011-05-04 09:12 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.3 (37.21 KB, patch)
2011-05-04 15:50 PDT, Mounir Lamouri (:mounir)
dbaron: review+
Details | Diff | Splinter Review
Inter diff (3.89 KB, patch)
2011-05-06 02:39 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2010-05-24 15:16:05 PDT
This bug is about the layout/rendering of the HTML5 progress element (bug 514437 for the content).
Comment 1 Mounir Lamouri (:mounir) 2010-05-26 09:34:01 PDT
Created attachment 447550 [details] [diff] [review]
Patch - WIP

This patch is still WIP. Actually, I have a weird issue: when I am (un)zooming, the progress bar doesn't update correctly. If I call nsBlockFrame::Reflow() _after_ updating the bar, the bar takes 100% of the width. If I do that before (like in the patch), the bar size is really approximative. The next reflow re-set the bar to the correct size.
Robert, would you have any idea of what I'm doing wrong here ?
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-05-26 15:54:27 PDT
This is weird since you're reflowing the child div using normal block reflow and then overriding its position manually.

I suggest not inheriting from nsBlockFrame and just writing your own implementation of Reflow that sizes and positions the child. nsGfxButtonControlFrame would be a good example to follow (its reflow method is in nsHTMLButtonControlFrame::Reflow).
Comment 3 d 2010-05-28 05:59:32 PDT
Are you using the OS's native progress bars for styling, or have you come up with your own?
Comment 4 Mounir Lamouri (:mounir) 2010-05-28 06:03:32 PDT
(In reply to comment #3)
> Are you using the OS's native progress bars for styling, or have you come up
> with your own?

This is using the OS native progress bar like xul:progressmeter. IOW, it's using -moz-appearance: progressbar; and -moz-appearance: progresschunk;.
It's working great with GTK. It looks like there is a bug on Windows (at least 7) and I have to check on MacOS X.
Comment 5 Mounir Lamouri (:mounir) 2010-05-28 08:49:57 PDT
Created attachment 448018 [details] [diff] [review]
Patch - WIP

This should use the native rendering for MacOS X, Windows XP, Windows 7 and GNU/Linux (GTK). And there is a default rendering in case of there is no native rendering for the platform.

Some issues specific to platform:
 - on windows, there is a bug in progresschunk (see bug 568825), I had to add "overflow: hidden;" to prevent that.
 - on MacOS X, the progress bar mechanism is quite ugly: when an element use "-moz-appearance: progressbar;", the widget look at @value, @max and @mode to know how to show the progressbar. So there is no need for an anonymous element, that's why there is a lot of #IFDEF MOZ_WIDGET_COCOA in the patch. This is also breaking RTL on MacOS X. In addition, the MacOS X widget has a fixed height... IOW, it's a pain.

Another issue is for the 'undeterminated state'. MacOS has a native look I could use but there is nothing on GTK and I'm wondering what should be done for Windows. I don't like the way it's done in xul:progressmeter.

So, roc, could you give me your feedback on this patch ? I do not ask for a review because I don't know how I should manage the issues mentioned before, there is no tests atm and there other stuff (see TODO's in the code).
Comment 6 Mounir Lamouri (:mounir) 2010-05-28 09:50:33 PDT
CCing mstange to have his opinion about the MacOS widget.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-05-28 15:38:35 PDT
You're still inheriting from nsBlockFrame, and I still think you should be inheriting from nsHTMLContainerFrame instead, instead of calling nsBlockFrame::Reflow and then overriding the child frame's position.

But we also need to think about supporting author styling here ...

Maybe instead of the Cocoa #ifdefs we could just make moz-appearance:progresschunk draw nothing?
Comment 8 Mounir Lamouri (:mounir) 2010-05-28 16:11:17 PDT
(In reply to comment #7)
> You're still inheriting from nsBlockFrame, and I still think you should be
> inheriting from nsHTMLContainerFrame instead, instead of calling
> nsBlockFrame::Reflow and then overriding the child frame's position.

Hmmm, ok.

> But we also need to think about supporting author styling here ...

I think a pseudo-element like ::-moz-progress-bar to be able to style the anonymous div would be great. However, I've not be able to do that for ::-moz-placeholder. IIRC, Boris told me the our style system isn't really ready for that kind of pseudo-element. (CCing Boris so he may say more)
Do you see any other issue related to author styling ?

> Maybe instead of the Cocoa #ifdefs we could just make
> moz-appearance:progresschunk draw nothing?

I may try but that would make us create an anonymous div for nothing. Do you think it worths it ? And I suppose the change would have to be done in the css with %if or in C++ in the frame code, right ?
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-05-28 16:19:44 PDT
(In reply to comment #8)
> I think a pseudo-element like ::-moz-progress-bar to be able to style the
> anonymous div would be great. However, I've not be able to do that for
> ::-moz-placeholder. IIRC, Boris told me the our style system isn't really ready
> for that kind of pseudo-element. (CCing Boris so he may say more)
> Do you see any other issue related to author styling ?

Not right now. We really need a spec for author styling though since it will constrain how we want to implement these controls.

> > Maybe instead of the Cocoa #ifdefs we could just make
> > moz-appearance:progresschunk draw nothing?
> 
> I may try but that would make us create an anonymous div for nothing. Do you
> think it worths it ?

Yes, I think that's fine. We'll need the anonymous div for author styling on Mac, anyway.

> And I suppose the change would have to be done in the css
> with %if or in C++ in the frame code, right ?

No, we should just do it in nsNativeThemeCocoa. ThemeSupportsWidget should return true for progresschunk, but then drawing the progresschunk should do nothing.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2010-05-28 18:50:23 PDT
I'm trying to recall what the issue was with the pseudo-element in the moz-placeholder case, other than the fact that the frame constructor didn't know it needed to create the different style context....  Do you remember what problems you ran into?

It would be good to have a description of exactly what we want to do with pseudo-elements for form controls in general; then we can see what changes are needed to support it.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-05-28 23:26:14 PDT
We want to map style rules for a pseudo-element to apply to a particular anonymous child of the base element. I guess that means the nsStyleContext for the pseudoelement gets assigned to the base element's frame. What particular issues do we need clarification on?
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2010-05-28 23:33:26 PDT
Is it always an anonymous child, or can it be a deeper anonymous descendant?
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-05-29 02:00:09 PDT
Good question. I don't know if we'll need non-child descendants.
Comment 14 Mounir Lamouri (:mounir) 2010-05-29 02:24:01 PDT
Maybe we will for elements like <input type='number'> where we there will be a <div> containing the two buttons. The buttons are not direct descendants but we may want to style them.
Comment 15 Tantek Çelik 2010-05-29 10:03:17 PDT
Agreed with volkmar. One example of this is the ::value pseudo-element:

http://www.w3.org/TR/css3-ui/#pseudo-value

We'll likely have to make up some number of -moz- prefixed pseudo-elements to enable styling of the pieces of a form element like the two buttons that volkmar refers to.

Documenting some thoughts on this here: https://wiki.mozilla.org/Tantek-Mozilla-projects#Styling_HTML5_forms_elements
Comment 16 Mounir Lamouri (:mounir) 2010-06-01 09:07:14 PDT
(In reply to comment #10)
> I'm trying to recall what the issue was with the pseudo-element in the
> moz-placeholder case, other than the fact that the frame constructor didn't
> know it needed to create the different style context....  Do you remember what
> problems you ran into?

I remember now. The issue was we wanted to have -moz-placeholder limited to only some CSS properties (like first-letter) so we were not able to use -moz-placeholder to set the default style of the placeholder (like setting visibility: hidden). Maybe we could use -moz-progressbar to style the progress bar as we would not run into that kind of issue. I will try that.
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2010-06-01 09:14:29 PDT
If we're going to need random non-child descendants stylable as pseudo-elements, then the "parent to inherit from" code will need to be changed to support that.  We will also need to figure out a way to pass the relevant information out via the native anonymous content API so the frame ctor can create the right style contexts to start with.
Comment 18 Mounir Lamouri (:mounir) 2010-06-01 17:16:45 PDT
Created attachment 448650 [details] [diff] [review]
Patch - WIP

nsProgressFrame is now inheriting from nsHTMLContainerFrame and redefines it own Reflow method. There is also a -moz-progress-bar pseudo element working.
I will do some cleaning and tweaks tomorrow. Until now, all feedbacks are welcome.
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-01 22:18:32 PDT
We probably need to do something to make ReResolveStyleContext work properly when we use these kinds of pseudo-elements. Boris or dbaron would tell you.

What's the goal of extrawidth?

You should size and position the child by setting up the right state and calling ReflowChild, instead of doing it last.
Comment 20 Mounir Lamouri (:mounir) 2010-06-02 17:20:43 PDT
Created attachment 448903 [details] [diff] [review]
Patch - WIP

(In reply to comment #19)
> We probably need to do something to make ReResolveStyleContext work properly
> when we use these kinds of pseudo-elements. Boris or dbaron would tell you.
> 
> What's the goal of extrawidth?

None.

> You should size and position the child by setting up the right state and
> calling ReflowChild, instead of doing it last.

I did that in this patch. I've also moved make the reflow more clean by creating a new function for the child.

Maybe we could introduce a new pseudo-class to access the progress elements with position == -1. It could be named :indeterminated. It may help authors to style the progress bar when it's in the indeterminated state. What's your opinion Tantek ?
Comment 21 Tantek Çelik 2010-06-02 17:49:14 PDT
> Maybe we could introduce a new pseudo-class to access the progress elements
with position == -1. It could be named :indeterminated. It may help authors to
style the progress bar when it's in the indeterminated state. What's your
opinion Tantek ?

There's an existing similarly-named pseudo-class that captures this semantic, albeit for other inputs, that we could reasonably re-use

:indeterminate

http://www.w3.org/TR/css3-ui/#pseudo-classes

http://www.w3.org/TR/2001/CR-css3-selectors-20011113/#UIstates

http://www.w3.org/TR/html5/interactive-elements.html#selector-indeterminate

We should implement that and the file bugs against CSS3-UI + HTML5 indicating that progress element with unknown progress (-1) is another valid use of :indeterminate.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2010-06-02 18:29:07 PDT
> We should implement that

It's already implemented in general.  You'd just need IntrinsicState() for <progress> to set the right bit as needed (and you would need to send ContentStatesChanged as needed, of course).

I agree that reusing :indeterminate here makes sense.
Comment 23 Mounir Lamouri (:mounir) 2010-06-04 10:14:02 PDT
Created attachment 449286 [details] [diff] [review]
Patch - WIP

Few questions with this patch:
- Boris and David, what should be needed for |ReResolveStyleContext| ?
- I don't think GetMinWidth() should be needed as the child frame size will depend on the container and the widgets should accept any size in every platform (even if that should be checked carefully) ;
- margin and padding on the child frame do not behave as expected (margin do not interact with the container but with the other element and padding behaves like I would expect margin to). I tried to work on that but it doesn't sound really trivial. Roc, do you have any pointer/code to look at ?

Except these list and two platform-specific issues (MacOS widget height is limited to 15 and Windows widget has a small bug), I think the patch is ready.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-05 15:04:37 PDT
dbaron may have a better idea about the margin/padding problem.
Comment 25 David Baron :dbaron: ⌚️UTC-10 2010-06-16 16:29:28 PDT
Do you have testcases I can look at?
Comment 26 Mounir Lamouri (:mounir) 2010-06-16 16:33:27 PDT
(In reply to comment #25)
> Do you have testcases I can look at?

Unfortunately, I lost my test cases with all my data from my HD last week.
However, I think you can use this experimental build of Firefox (it supports the progress element):
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mlamouri@mozilla.com-16a2c53aab0d/

Do you want me to write you some test cases considering you are using this version or you may have time to play with it ?
Comment 27 David Baron :dbaron: ⌚️UTC-10 2010-06-16 17:10:26 PDT
I just wanted to see what the markup looked like to understand what it was that you're implementing.

I noticed it might be better to implement nsIAnonymousContentCreator::CreateFrameFor rather than overriding SetInitialChildList.  But that's unlikely to be related to the margins problem.

It seems like nsProgressFrame might have more code than it really needs to have.  (There's a lot of reflow code there to go through looking for offset errors that would be causing margins to break, but I'm wondering if that code is necessary.)
Would it be sufficient to inherit from nsBlockFrame (which also corresponds better to the display:inline-block that you have), but then also override the frame type of the *child* frame, and in that child frame class, override nsIFrame::ComputeAutoSize (but nothing else) to deal with the width computation?  Then you could avoid overriding any Reflow methods at all, I think.
Comment 28 David Baron :dbaron: ⌚️UTC-10 2010-06-20 10:47:39 PDT
Comment on attachment 449286 [details] [diff] [review]
Patch - WIP

If you want additional feedback beyond the previous comment, please let me know.
Comment 29 Mounir Lamouri (:mounir) 2011-02-01 10:19:34 PST
Created attachment 508807 [details] [diff] [review]
WIP Patch

I removed the indeterminate state and the pseudo-element code. I will do that in other patches.
I had a look at |ComputeAutoSize| and I don't get how this could help me given that I want to set the size of the child's frame, not the frame itself.
Comment 30 Mounir Lamouri (:mounir) 2011-02-10 08:15:38 PST
Created attachment 511397 [details] [diff] [review]
Patch v1

I chose to split this in multiple bugs. This bug will only resolve the basic layout. I will open a bug for :indeterminate pseudo-class and ::progress-bar pseudo-element.

Not that the tests are disabled on MacOS because the widget is showing the progress depending on the element's value. I might need to open a bug for that if it's not following the specs.
Comment 31 Mounir Lamouri (:mounir) 2011-02-10 08:16:51 PST
Created attachment 511398 [details] [diff] [review]
Patch v1

It's better with the tests.
Comment 32 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-02-28 14:47:01 PST
+  nsAutoString classValue;
+  classValue.AppendLiteral("progress-bar");
+  rv = mBarDiv->SetAttr(kNameSpaceID_None, nsGkAtoms::_class, classValue,
+                        PR_FALSE);
+  NS_ENSURE_SUCCESS(rv, rv);

Why do we need to set a class here?

+  // If the state in indeterminated, the progress bar will be hidden.

indeterminate

+    xoffset = aReflowState.ComputedWidth() - availSize.width +
+              aReflowState.mComputedBorderPadding.right;

Shouldn't this be .left? In which case you can just += here.

Shouldn't progress bars have some intrinsic size?

+    NS_MAX(yoffset, 0);

This does nothing.

What about dbaron's comment #27? Especially the last part. It would be nice to be able to avoid implementing Reflow.
Comment 33 Mounir Lamouri (:mounir) 2011-02-28 17:32:15 PST
(In reply to comment #32)
> +  nsAutoString classValue;
> +  classValue.AppendLiteral("progress-bar");
> +  rv = mBarDiv->SetAttr(kNameSpaceID_None, nsGkAtoms::_class, classValue,
> +                        PR_FALSE);
> +  NS_ENSURE_SUCCESS(rv, rv);
> 
> Why do we need to set a class here?

Indeed, progress > div in the css is enough. I don't need the class.

> +    xoffset = aReflowState.ComputedWidth() - availSize.width +
> +              aReflowState.mComputedBorderPadding.right;
> 
> Shouldn't this be .left? In which case you can just += here.

Indeed. I've added two tests with margin/padding right/left/top/bottom having different values.

> Shouldn't progress bars have some intrinsic size?

What the intrinsic size represents isn't really clear to me so I'm not sure.

> +    NS_MAX(yoffset, 0);
> 
> This does nothing.

Oups, removed.

> What about dbaron's comment #27? Especially the last part. It would be nice to
> be able to avoid implementing Reflow.

I thought creating a new frame class just to set the size wasn't ideal (and it was more interesting for me to implement Reflow). If you think we should prevent implementing Reflow at any cost, I can try to implement a nsProgressBarChildFrame class.

By the way, bug 633209 is cleaning a lot the Reflow implementation.
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-02-28 17:40:46 PST
(In reply to comment #33)
> What the intrinsic size represents isn't really clear to me so I'm not sure.

If someone writes just

<p><progress value="0" max="100"></progress>

It should get its intrinsic width and height. What do you want them to be?
Comment 35 Mounir Lamouri (:mounir) 2011-02-28 17:42:36 PST
(In reply to comment #34)
> (In reply to comment #33)
> > What the intrinsic size represents isn't really clear to me so I'm not sure.
> 
> If someone writes just
> 
> <p><progress value="0" max="100"></progress>
> 
> It should get its intrinsic width and height. What do you want them to be?

It's set in forms.css: height is 1em and width is 10em.
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2011-02-28 18:09:47 PST
What if the site explicitly styles as "height: auto; width: auto"?
Comment 37 Mounir Lamouri (:mounir) 2011-02-28 18:31:29 PST
I guess I should override ComputeAutoSize, right?
Comment 38 Boris Zbarsky [:bz] (still a bit busy) 2011-02-28 18:48:04 PST
Probably, yes.
Comment 39 Mounir Lamouri (:mounir) 2011-03-01 06:46:25 PST
Created attachment 515905 [details] [diff] [review]
Patch v1.1

With review comments applied including auto size.
Comment 40 David Baron :dbaron: ⌚️UTC-10 2011-04-27 15:40:37 PDT
Comment on attachment 515905 [details] [diff] [review]
Patch v1.1

In both DestroyFrom and Reflow, you should assert !GetPrevContinuation()
before the calls to RegUnRegAccessKey.

I think it would be nice if we could make progress frames a reflow
root so that reflow of their bar frame would be fast, but I don't think
we can be sure that the native theme code won't cause overflow.

Up to Reflow.

In Reflow, what causes barFrame to be null?  Given your current code,
I think that needs an assertion and probably even a failure nsresult.
Otherwise, I think you need to properly fill in aStatus and
aDesiredSize.


In ReflowBarFrame:

You really shouldn't be using the same nsHTMLReflowMetrics for both
parent and child... though you fix that in bug 633209.  If they land
together I guess that's ok.  But I'd prefer if you roll the changes to
ReflowBarFrame from that patch into this one; they'd make things much
easier to understand.  (It's relatively easy to do by just copying bits
of patch files around in your patch queue... just copy the relevant
chunks from one patch to the *end* of the chunks for that file in the
other patch, and then apply and refresh both patches.  Of course...
commit to your patch queue first so you have a backup in case you mess
up.)

I think your use of the availSize parameter to nsHTMLReflowState is
somewhat bogus; I'd rather just do the "normal thing" here since it
doesn't really do anything useful for you anyway, as far as I can tell.
That is, just pass
  nsSize availSize(aReflowState.ComputedWidth(), NS_UNCONSTRAINEDSIZE);
for the aAvailableSize parameter to the reflow state constructor, and
keep the later modifications you make to it in other variables.

I'm also not confident your interaction of |position| with margins,
padding, and border seems correct, but I'm also not confident it's
wrong.  Could you explain the intent in the patch (preferably once
the bug 633209 ReflowBarFrame changes are rolled in)?

>+  // If the state in indeterminate, the progress bar will be hidden.

What causes this?


In nsProgressFrame::AttributeChanged, again, I don't like the !barFrame
case.  If it's really extreme, it should be asserting; if it's not, it
should be continuing on to nsHTMLContainerFrame::AttributeChanged.

nsProgressFrame.h:

IsFrameOfType should be public.

html.css:

If you have to rewrap this, make it one selector per line while you're
there.

With those changes, this looks good, but I'd like to look at the
revised patch.  I'm really sorry for taking this long to get to this.
Comment 41 Mounir Lamouri (:mounir) 2011-05-04 09:09:58 PDT
(In reply to comment #40)
> I'm also not confident your interaction of |position| with margins,
> padding, and border seems correct, but I'm also not confident it's
> wrong.  Could you explain the intent in the patch (preferably once
> the bug 633209 ReflowBarFrame changes are rolled in)?

It would be hard to explain every line given that I wrote this patch a few months ago. Though, everything should be tested and if you thing something should be changed, I'm fine to do it as long as I can find a test (and it doesn't break the current ones).

> >+  // If the state in indeterminate, the progress bar will be hidden.
> 
> What causes this?

The comment was completely wrong. I think I have another patch fixing it but let's do that here.
Comment 42 Mounir Lamouri (:mounir) 2011-05-04 09:11:04 PDT
Created attachment 530036 [details] [diff] [review]
Inter diff (v1.1 to v1.2)

r=roc
Comment 43 Mounir Lamouri (:mounir) 2011-05-04 09:12:19 PDT
Created attachment 530038 [details] [diff] [review]
Patch v1.2
Comment 44 Mounir Lamouri (:mounir) 2011-05-04 15:50:56 PDT
Created attachment 530184 [details] [diff] [review]
Patch v1.3

xOffset doesn't depend on RTL anymore. See bug 633209 review comments.
Comment 45 Mounir Lamouri (:mounir) 2011-05-04 15:51:22 PDT
I will write a test for that before landing the patch.
Comment 46 David Baron :dbaron: ⌚️UTC-10 2011-05-05 16:03:06 PDT
Comment on attachment 530184 [details] [diff] [review]
Patch v1.3

NS_ASSERTION(!GetPrevContinuation(), "Should be called for primary frame!");

The assertion text should probably say "nsProgressFrame should not have continuations; if it does we need to call RegUnregAccessKey only for the first".

(twice)

>++  // If the state in indeterminate, the progress bar will be shown with width=100%.

wrap at less than 80 chars

In Reflow, you don't need the |availSize| variable anymore; you don't
use availSize.height and you can merge availSize.width and width, so just
call it width and drop availSize.

>+  if (position >= 0.0f) {

This should be 0.0, not 0.0f (merge in that piece of one of the higher
patches).


r=dbaron with that
Comment 47 Mounir Lamouri (:mounir) 2011-05-06 02:39:10 PDT
Created attachment 530583 [details] [diff] [review]
Inter diff

Applying review comments.

r=roc,dbaron
Comment 48 Mounir Lamouri (:mounir) 2011-05-09 05:34:27 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/6d42dd8b3d0d
Comment 49 Mounir Lamouri (:mounir) 2011-05-09 05:35:24 PDT
I meant:
http://hg.mozilla.org/mozilla-central/rev/a8b8078beea2
Comment 50 Shawn Wilsher :sdwilsh 2011-05-09 16:10:59 PDT
Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.
Comment 51 Mounir Lamouri (:mounir) 2011-05-10 06:55:28 PDT
The regression wasn't caused by these patches. Re-landed:
http://hg.mozilla.org/mozilla-central/rev/97e11f940f25

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