Bug 78772 - flex-wrap:nowrap should be flex-wrap:none
Summary: flex-wrap:nowrap should be flex-wrap:none
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 62048
  Show dependency treegraph
 
Reported: 2012-02-15 19:18 PST by Ojan Vafai
Modified: 2012-10-14 22:12 PDT (History)
7 users (show)

See Also:


Attachments
ProposedPatch (17.32 KB, patch)
2012-02-16 16:54 PST, Joe Thomas
ojan: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch-with-testcases-updated (29.08 KB, patch)
2012-02-16 20:44 PST, Joe Thomas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2012-02-15 19:18:09 PST
The spec changed to be consistent with text-wrap instead of white-space. http://dev.w3.org/csswg/css3-flexbox/#flex-wrap0
Comment 1 Joe Thomas 2012-02-16 16:54:04 PST
Created attachment 127470 [details]
ProposedPatch
Comment 2 Ojan Vafai 2012-02-16 16:56:35 PST
Comment on attachment 127470 [details]
ProposedPatch

View in context: https://bugs.webkit.org/attachment.cgi?id=127470&action=review

Thanks for the fix!

> Source/WebCore/ChangeLog:10
> +        Modified the existing test case.

FYI: In the future you don't need to include this description. You can just delete the no new tests line. In cases where there actually isn't test coverage *and* you're not adding tests, then you do need an explanation of why tests aren't needed.
Comment 3 Joe Thomas 2012-02-16 16:58:44 PST
(In reply to comment #2)
> (From update of attachment 127470 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127470&action=review
> 
> Thanks for the fix!
> 
> > Source/WebCore/ChangeLog:10
> > +        Modified the existing test case.
> 
> FYI: In the future you don't need to include this description. You can just delete the no new tests line. In cases where there actually isn't test coverage *and* you're not adding tests, then you do need an explanation of why tests aren't needed.

Sure. Thanks for the review.
Comment 4 noel gordon 2012-02-16 19:16:31 PST
From http://queues.webkit.org/results/11536390, do we expect these tests to change, or was it just noise?

Regressions: Unexpected text diff mismatch : (3)
  fast/css/getComputedStyle/computed-style-without-renderer.html = TEXT
  fast/css/getComputedStyle/computed-style.html = TEXT
  svg/css/getComputedStyle-basic.xhtml = TEXT
Comment 5 WebKit Review Bot 2012-02-16 19:42:47 PST
Comment on attachment 127470 [details]
ProposedPatch

Rejecting attachment 127470 [details] from commit-queue.

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
Full output: http://queues.webkit.org/results/11535402
Comment 6 WebKit Review Bot 2012-02-16 20:21:54 PST
Comment on attachment 127470 [details]
ProposedPatch

Attachment 127470 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11543026

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
Comment 7 Joe Thomas 2012-02-16 20:22:50 PST
(In reply to comment #4)
> From http://queues.webkit.org/results/11536390, do we expect these tests to change, or was it just noise?
> 
> Regressions: Unexpected text diff mismatch : (3)
>   fast/css/getComputedStyle/computed-style-without-renderer.html = TEXT
>   fast/css/getComputedStyle/computed-style.html = TEXT
>   svg/css/getComputedStyle-basic.xhtml = TEXT

Yes these test cases needs to be updated. I will submit another patch soon.
Comment 8 Joe Thomas 2012-02-16 20:44:05 PST
Created attachment 127511 [details]
Patch-with-testcases-updated

Updated the failing test cases.
Comment 9 WebKit Review Bot 2012-02-17 11:56:58 PST
Comment on attachment 127511 [details]
Patch-with-testcases-updated

Clearing flags on attachment: 127511

Committed r108102: <http://trac.webkit.org/changeset/108102>
Comment 10 WebKit Review Bot 2012-02-17 11:57:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 noel gordon 2012-02-17 13:33:20 PST
(In reply to comment #7)
> Yes these test cases needs to be updated. I will submit another patch soon.

Thanks Joe.  Odd that we need platform-specific results for dumpAsText() tests.