Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(13)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 639783002: Media query values should not be 0 when overflowed (Closed)

Created:
6 years, 2 months ago by Kyungtae Kim
Modified:
5 years, 4 months ago
Reviewers:
Yoav Weiss, rune
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, dglazkov+blink, ed+blinkwatch_opera.com, rune+blink
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Media query values should not be 0 when overflowed roundForImpreciseConversion function for CSSPrimitiveValue makes overflowed value as 0, but it's not a valid result when the value is used for min/max condition for media query. So, for media query values, the overflowed value should become the numeric_limits. TEST=fast/media/media-query-overflow-value.html BUG=375574

Patch Set 1 #

Total comments: 1

Patch Set 2 : rebase #

Total comments: 3

Patch Set 3 : fix nits #

Patch Set 4 : fix nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+49 lines, -0 lines) Patch
A LayoutTests/fast/media/media-query-overflow-value.html View 1 2 3 1 chunk +38 lines, -0 lines 0 comments Download
A LayoutTests/fast/media/media-query-overflow-value-expected.txt View 1 chunk +11 lines, -0 lines 0 comments Download

Messages

Total messages: 16 (1 generated)
Kyungtae Kim
PTAL
6 years, 2 months ago (2014-10-08 11:39:31 UTC) #1
rune
https://codereview.chromium.org/639783002/diff/1/Source/core/css/MediaValues.h File Source/core/css/MediaValues.h (right): https://codereview.chromium.org/639783002/diff/1/Source/core/css/MediaValues.h#newcode46 Source/core/css/MediaValues.h:46: result = static_cast<T>(tempResult); There is a clampTo templated method ...
6 years, 2 months ago (2014-10-08 12:26:39 UTC) #3
Yoav Weiss
On 2014/10/08 12:26:39, rune wrote: > https://codereview.chromium.org/639783002/diff/1/Source/core/css/MediaValues.h > File Source/core/css/MediaValues.h (right): > > https://codereview.chromium.org/639783002/diff/1/Source/core/css/MediaValues.h#newcode46 > ...
6 years, 2 months ago (2014-10-08 12:32:19 UTC) #4
Yoav Weiss
> > I've just committed https://codereview.chromium.org/633643003/patch/40001/50001 > this morning, which should do what you're trying ...
6 years, 2 months ago (2014-10-08 12:34:35 UTC) #5
Kyungtae Kim
On 2014/10/08 12:34:35, Yoav Weiss wrote: > > > > I've just committed > https://codereview.chromium.org/633643003/patch/40001/50001 ...
6 years, 2 months ago (2014-10-10 04:52:39 UTC) #6
Yoav Weiss
Looks good, just a few style nits. https://codereview.chromium.org/639783002/diff/20001/LayoutTests/fast/media/media-query-overflow-value.html File LayoutTests/fast/media/media-query-overflow-value.html (right): https://codereview.chromium.org/639783002/diff/20001/LayoutTests/fast/media/media-query-overflow-value.html#newcode3 LayoutTests/fast/media/media-query-overflow-value.html:3: <head> nit: ...
6 years, 2 months ago (2014-10-10 07:33:49 UTC) #7
rune
The clampTo is still not entirely correct, ref my examples with px with decimal points, ...
6 years, 2 months ago (2014-10-10 07:52:55 UTC) #8
Yoav Weiss
On 2014/10/10 07:52:55, rune wrote: > The clampTo is still not entirely correct, ref my ...
6 years, 2 months ago (2014-10-10 07:55:56 UTC) #9
Yoav Weiss
On 2014/10/10 07:52:55, rune wrote: > The clampTo is still not entirely correct, ref my ...
6 years, 2 months ago (2014-10-10 08:24:08 UTC) #10
rune
Using the js-test.js framework, this test can be made much simpler: <!DOCTYPE html> <script src="../../resources/js-test.js"></script> ...
6 years, 2 months ago (2014-10-17 09:23:38 UTC) #11
rwlbuis
@rune, @yoav the issue is marked as WontFix, so we don't need this testcase?
5 years, 4 months ago (2015-08-12 19:53:13 UTC) #12
Yoav Weiss
On 2015/08/12 19:53:13, rwlbuis wrote: > @rune, @yoav the issue is marked as WontFix, so ...
5 years, 4 months ago (2015-08-13 06:06:25 UTC) #13
rwlbuis
On 2015/08/13 06:06:25, Yoav Weiss (OOO until 15th) wrote: > On 2015/08/12 19:53:13, rwlbuis wrote: ...
5 years, 4 months ago (2015-08-13 14:37:53 UTC) #14
rwlbuis
On 2015/08/13 14:37:53, rwlbuis wrote: > On 2015/08/13 06:06:25, Yoav Weiss (OOO until 15th) wrote: ...
5 years, 4 months ago (2015-08-13 15:33:29 UTC) #15
Laszlo Gombos
5 years, 4 months ago (2015-08-13 19:39:34 UTC) #16 
On 2015/08/13 15:33:29, rwlbuis wrote:

> I put it here because I can't upload to this CL:
> 
> https://codereview.chromium.org/1292553002/

Closing this one than. Thanks.

Powered by Google App Engine
This is Rietveld 408576698