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

Issue 2432153005: Always evaluate media features to true without MediaValues. (Closed)

Created:
4 years, 2 months ago by rune
Modified:
4 years, 2 months ago
CC:
chromium-reviews, kenneth.christiansen, Yoav Weiss, blink-reviews-style_chromium.org, blink-reviews-css, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Always evaluate media features to true without MediaValues. Having a constructor taking bool made it possible to construct a MediaQueryEvaluator passing a pointer to an object of an arbitrary type as the pointer was converted to a bool without a warning. By closer inspection, the use of the m_expectedResult value had two purposes. One was to return true for matching media type ignoring the rest of the media query. The other cases were for testing purposes where there was no media rules to match, so the result didn't matter. Since there are no useful applications for returning false for media queries containing expressions in addition to type, we can safely return true for all query expression when no MediaValues object is present. There is one place we change the behavior. The StyleResolver constructor has a fallback evaluator when we have no FrameView. That should never happen, though, and it would yield incorrect results regardless of whether we would always return true or false for media query expressions. R=timloh@chromium.org,meade@chromium.org Committed: https://crrev.com/1be8b135bdccee366e212865d42db161e11137e4 Cr-Commit-Position: refs/heads/master@{#426752}

Patch Set 1 #

Patch Set 2 : DCHECK(view). #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -27 lines) Patch
M third_party/WebKit/Source/core/css/MediaQueryEvaluator.h View 2 chunks +5 lines, -11 lines 0 comments Download
M third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp View 2 chunks +3 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp View 1 1 chunk +4 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/dom/StyleElement.cpp View 1 chunk +2 lines, -2 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 24 (16 generated)
rune
ptal
4 years, 2 months ago (2016-10-20 08:01:42 UTC) #5
rune
ptal
4 years, 2 months ago (2016-10-20 08:01:42 UTC) #6
rune
ptal
4 years, 2 months ago (2016-10-20 08:01:42 UTC) #7
meade_UTC10
lgtm This is fine, but could you please update the CL description with a bug ...
4 years, 2 months ago (2016-10-20 08:28:31 UTC) #8
rune
On 2016/10/20 08:28:31, Eddy wrote: > lgtm > > This is fine, but could you ...
4 years, 2 months ago (2016-10-21 07:32:01 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2432153005/20001
4 years, 2 months ago (2016-10-21 08:33:17 UTC) #20
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 2 months ago (2016-10-21 08:45:14 UTC) #22
commit-bot: I haz the power
4 years, 2 months ago (2016-10-21 13:28:15 UTC) #24
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/1be8b135bdccee366e212865d42db161e11137e4
Cr-Commit-Position: refs/heads/master@{#426752}

Powered by Google App Engine
This is Rietveld 408576698