New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 4 users

Issue metadata

Status: Fixed
Owner:
NOT IN USE
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

@viewport can cause creating a style resolver twice

Reported by r...@opera.com, Mar 2 2015

Issue description

When creating a style resolver, we now collect all rulesets using whatever viewport we have for evaluating media queries. If we collect @viewport rules which changes the viewport which in turn will cause media rule evaluations to change, we have to recollect all style rules.

Instead, we should collect @viewport rules first, let it affect the viewport, and then collect other rules.

Somewhat related to  crbug.com/332763 

 

Comment 1 by r...@opera.com, Mar 2 2015

Run the attached viewport.html in the inspector timeline. You'll see that from parseHTML we call a recalcStyle, and then immediately afterwards, from layout, we will call recalcStyle once again. That is due to this bug.

Comment 2 by r...@opera.com, Mar 2 2015

viewport.html
161 bytes View Download

Comment 3 by loyso@chromium.org, Jun 3 2015

Owner: loyso@chromium.org

Comment 4 by loyso@chromium.org, Jun 3 2015

Owner: ----
Status: Available

Comment 5 by soonm@google.com, Jul 15 2015

Labels: Hotlist-CodeHealth

Comment 6 by soonm@google.com, Jul 15 2015

Labels: -Hotlist-CodeHealth Performance
Project Member

Comment 7 by sheriffbot@chromium.org, Jul 14 2016

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been available for more than 365 days, and should be re-evaluated. Hotlist-Recharge-Cold label is added for tracking. Please re-triage this issue.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Recharge-Cold
Status: Available (was: Untriaged)
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d0fae7951dff76665a0fbe4c26abb1a94b87072f

commit d0fae7951dff76665a0fbe4c26abb1a94b87072f
Author: rune <rune@opera.com>
Date: Fri Oct 14 08:29:52 2016

Add hasViewportRule() flag to StyleSheetContents.

This is a pre-requisite for collecting viewport rules before generating
the RuleSet. The RuleSet contents depends on media query evaluation,
which in turn depends on viewport size resolution, which means we are
currently may generate the RuleSet, and recalculate style, twice in the
presence of both @media and @viewport.

This CL is split out from [1] which in turn is split out from [2].

[1] https://codereview.chromium.org/2405143003/
[2] https://codereview.chromium.org/1913833002/

R=timloh@chromium.org
BUG= 463098 

Review-Url: https://codereview.chromium.org/2408353003
Cr-Commit-Position: refs/heads/master@{#425273}

[modify] https://crrev.com/d0fae7951dff76665a0fbe4c26abb1a94b87072f/third_party/WebKit/Source/core/css/StyleRule.h
[modify] https://crrev.com/d0fae7951dff76665a0fbe4c26abb1a94b87072f/third_party/WebKit/Source/core/css/StyleSheetContents.cpp
[modify] https://crrev.com/d0fae7951dff76665a0fbe4c26abb1a94b87072f/third_party/WebKit/Source/core/css/StyleSheetContents.h
[modify] https://crrev.com/d0fae7951dff76665a0fbe4c26abb1a94b87072f/third_party/WebKit/Source/core/css/StyleSheetContentsTest.cpp
[modify] https://crrev.com/d0fae7951dff76665a0fbe4c26abb1a94b87072f/third_party/WebKit/Source/core/css/parser/CSSParserImpl.cpp

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b7a7561cf670c265fbeccaa1a279db7e18686b6

commit 4b7a7561cf670c265fbeccaa1a279db7e18686b6
Author: rune <rune@opera.com>
Date: Fri Oct 14 09:57:23 2016

Don't generate RuleSets for viewport UA sheets.

Start collecting UA @viewport rules from the StyleSheetContents instead
of the RuleSet. The reason is that we need to collect viewport rules
before creating the RuleSet in order to use the correct actual viewport
for evaluating media queries. This is split out from [1].

Also introducing a separate MediaQueryEvaluator in the
ViewportStyleResolver which should eventually be based on the initial
viewport and not the actual viewport as described in the CSS Device
Adaptation spec.

[1] https://codereview.chromium.org/2405143003

R=timloh@chromium.org
BUG= 463098 

Review-Url: https://codereview.chromium.org/2410283005
Cr-Commit-Position: refs/heads/master@{#425284}

[modify] https://crrev.com/4b7a7561cf670c265fbeccaa1a279db7e18686b6/third_party/WebKit/Source/core/css/CSSDefaultStyleSheets.cpp
[modify] https://crrev.com/4b7a7561cf670c265fbeccaa1a279db7e18686b6/third_party/WebKit/Source/core/css/CSSDefaultStyleSheets.h
[modify] https://crrev.com/4b7a7561cf670c265fbeccaa1a279db7e18686b6/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp
[modify] https://crrev.com/4b7a7561cf670c265fbeccaa1a279db7e18686b6/third_party/WebKit/Source/core/css/resolver/ViewportStyleResolver.cpp
[modify] https://crrev.com/4b7a7561cf670c265fbeccaa1a279db7e18686b6/third_party/WebKit/Source/core/css/resolver/ViewportStyleResolver.h

Comment 11 by r...@opera.com, Oct 17 2016

Labels: -Pri-2 Pri-3
Owner: r...@opera.com
Status: Started (was: Available)
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 20 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17

commit 70d21a8093c3096a01fa62e6ac7c7bdba3aeca17
Author: rune <rune@opera.com>
Date: Thu Oct 20 07:56:44 2016

Collect @viewport before constructing RuleSets.

- Move ViewportStyleResolver to StyleEngine.

- Only create a ViewportStyleResolver for top level documents.

- Collect @viewport rules via the DocumentStyleSheetCollection.

- Use the initial viewport size for resolving viewport relative
  lengths.

- Introduce initialViewportChanged() and viewportRulesChanged() in
  StyleEngine to trigger re-collection and resolution of the actual
  viewport. These currently trigger an immediate call to updateViewport
  which will later be a part of the document lifecycle phase for
  updating active stylesheets.

This finally fixes issues  332763 ,  455136 , and  463098 .

R=timloh@chromium.org
BUG= 567021 , 463098 , 455136 , 332763 

Review-Url: https://chromiumcodereview.appspot.com/2420413005
Cr-Commit-Position: refs/heads/master@{#426427}

[modify] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/core/css/RuleSet.cpp
[modify] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/core/css/RuleSet.h
[modify] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.cpp
[modify] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/core/css/resolver/ScopedStyleResolver.h
[modify] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp
[modify] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/core/css/resolver/StyleResolver.h
[modify] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/core/css/resolver/ViewportStyleResolver.cpp
[modify] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/core/css/resolver/ViewportStyleResolver.h
[modify] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/core/dom/StyleEngine.cpp
[modify] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/core/dom/StyleEngine.h
[modify] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/core/frame/FrameView.cpp
[modify] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/core/page/Page.cpp
[modify] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[add] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/web/tests/data/viewport-and-media.html
[add] https://crrev.com/70d21a8093c3096a01fa62e6ac7c7bdba3aeca17/third_party/WebKit/Source/web/tests/data/viewport-lengths.html

Comment 13 by r...@opera.com, Oct 21 2016

Status: Fixed (was: Started)

Sign in to add a comment