|
|
Created:
5 years, 4 months ago by a.renevier Modified:
5 years, 3 months ago Reviewers:
Yoav Weiss, eae, bokan, kenneth.r.christiansen, Timothy Loh, Mike West, pfeldman CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionUpdate css styles after modifying display mode
After setting display mode, css styles should be recalculated.
Before this patch, the new display mode value was taken into account as
soon as MediaQueryList::updateMatches() was executed (for example after
a call to window.matchMedia. But css media rules were not updated.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201417
Patch Set 1 #
Total comments: 8
Patch Set 2 : updated patch #
Total comments: 2
Patch Set 3 : New patch: call styleResolverChanged() from mediaQueryAffectingValueChanged() #Patch Set 4 : #
Created: 5 years, 4 months ago
Messages
Total messages: 31 (10 generated)
a.renevier@samsung.com changed reviewers: + kenneth.r.christiansen@intel.com, mkwst@chromium.org, yoav@yoav.ws
https://codereview.chromium.org/1285703004/diff/1/Source/web/tests/WebViewTes... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/1285703004/diff/1/Source/web/tests/WebViewTes... Source/web/tests/WebViewTest.cpp:2109: URLTestHelpers::registerMockedURLFromBaseURL(WebString::fromUTF8(m_baseURL.c_str()), WebString::fromUTF8("display_mode.html")); You forgot to add this display_mode.html file? https://codereview.chromium.org/1285703004/diff/1/Source/web/tests/WebViewTes... Source/web/tests/WebViewTest.cpp:2113: EXPECT_EQ("regular-ui", content); regular-ui? That is not a valid value for display-mode - you must mean ´browser´ here
mkwst@chromium.org changed reviewers: + eae@chromium.org
I'm not a good reviewer for the right times to recalculate style. +eae@, who might be interested in looking at this or delegating it. https://codereview.chromium.org/1285703004/diff/1/Source/core/frame/FrameView... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1285703004/diff/1/Source/core/frame/FrameView... Source/core/frame/FrameView.cpp:1198: if (m_frame->document()) { Can we do this only if |mode| is different from |m_displayMode|? https://codereview.chromium.org/1285703004/diff/1/Source/web/tests/WebViewTes... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/1285703004/diff/1/Source/web/tests/WebViewTes... Source/web/tests/WebViewTest.cpp:2112: std::string content = webView->mainFrame()->contentAsText(21).utf8(); 21?
https://codereview.chromium.org/1285703004/diff/1/Source/core/frame/FrameView... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1285703004/diff/1/Source/core/frame/FrameView... Source/core/frame/FrameView.cpp:1198: if (m_frame->document()) { On 2015/08/17 15:29:06, Mike West (traveling) wrote: > Can we do this only if |mode| is different from |m_displayMode|? Done. https://codereview.chromium.org/1285703004/diff/1/Source/web/tests/WebViewTes... File Source/web/tests/WebViewTest.cpp (right): https://codereview.chromium.org/1285703004/diff/1/Source/web/tests/WebViewTes... Source/web/tests/WebViewTest.cpp:2109: URLTestHelpers::registerMockedURLFromBaseURL(WebString::fromUTF8(m_baseURL.c_str()), WebString::fromUTF8("display_mode.html")); On 2015/08/17 09:03:36, kenneth.r.christiansen wrote: > You forgot to add this display_mode.html file? Done. https://codereview.chromium.org/1285703004/diff/1/Source/web/tests/WebViewTes... Source/web/tests/WebViewTest.cpp:2112: std::string content = webView->mainFrame()->contentAsText(21).utf8(); On 2015/08/17 15:29:06, Mike West (traveling) wrote: > 21? size of "regular-ui" + "minimal-ui" + 1 https://codereview.chromium.org/1285703004/diff/1/Source/web/tests/WebViewTes... Source/web/tests/WebViewTest.cpp:2113: EXPECT_EQ("regular-ui", content); On 2015/08/17 09:03:36, kenneth.r.christiansen wrote: > regular-ui? That is not a valid value for display-mode - you must mean ´browser´ > here In my test file, I only check if display mode is minimal-ui, in which case it displays minimal-ui. Otherwise, it display regular-ui. Do you think I should change that label to browser?
eae@chromium.org changed reviewers: + timloh@chromium.org
LGTM but I'd like timloh to take a look as well.
https://codereview.chromium.org/1285703004/diff/20001/Source/core/frame/Frame... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1285703004/diff/20001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:1204: m_frame->document()->styleResolverChanged(); Perhaps this call should be inside mediaQueryAffectingValueChanged(), since the other callers all also call it. https://codereview.chromium.org/1285703004/diff/20001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:1208: void FrameView::setMediaType(const AtomicString& mediaType) Can we fix this one too? :-)
On 2015/08/20 00:16:01, a.renevier wrote: lgtm with nit Please also remove the setNeedsRecalcStyleInAllFrames() calls in Page::settingsChanged (SettingsDelegate::FontFamilyChange and SettingsDelegate::MediaQueryChange)
On 2015/08/20 05:39:30, Timothy Loh wrote: > On 2015/08/20 00:16:01, a.renevier wrote: > > lgtm with nit > > Please also remove the setNeedsRecalcStyleInAllFrames() calls in > Page::settingsChanged (SettingsDelegate::FontFamilyChange and > SettingsDelegate::MediaQueryChange) I removed the call after SettingsDelegate::MediaQueryChange. Are you sure I also need to remove it after SettingsDelegate::MediaQueryChange? I don't understand how it is related to the rest of the patch?
On 2015/08/21 17:10:13, a.renevier wrote: > On 2015/08/20 05:39:30, Timothy Loh wrote: > > On 2015/08/20 00:16:01, a.renevier wrote: > > > > lgtm with nit > > > > Please also remove the setNeedsRecalcStyleInAllFrames() calls in > > Page::settingsChanged (SettingsDelegate::FontFamilyChange and > > SettingsDelegate::MediaQueryChange) > > I removed the call after SettingsDelegate::MediaQueryChange. > Are you sure I also need to remove it after SettingsDelegate::MediaQueryChange? > I don't understand how it is related to the rest of the patch? The call is redundant if mediaQueryAffectingValueChanged() is now requesting a style recalc. BTW there's also the call for FontFamilyChange...
On 2015/08/24 01:16:47, Timothy Loh wrote: > On 2015/08/21 17:10:13, a.renevier wrote: > > On 2015/08/20 05:39:30, Timothy Loh wrote: > > > On 2015/08/20 00:16:01, a.renevier wrote: > > > > > > lgtm with nit > > > > > > Please also remove the setNeedsRecalcStyleInAllFrames() calls in > > > Page::settingsChanged (SettingsDelegate::FontFamilyChange and > > > SettingsDelegate::MediaQueryChange) > > > > I removed the call after SettingsDelegate::MediaQueryChange. > > Are you sure I also need to remove it after > SettingsDelegate::MediaQueryChange? > > I don't understand how it is related to the rest of the patch? > > The call is redundant if mediaQueryAffectingValueChanged() is now requesting a > style recalc. BTW there's also the call for FontFamilyChange... Yes sorry, I was confused in my previous message. I understand why I need to remove the call after SettingsDelegate::MediaQueryChange. My last patch has that change. I am still wondering why it's needed after FontFamilyChange: styleEngine().updateGenericFontFamilySettings is being called, and it does not call mediaQueryAffectingValueChanged()
On 2015/08/24 18:04:28, a.renevier wrote: > On 2015/08/24 01:16:47, Timothy Loh wrote: > > On 2015/08/21 17:10:13, a.renevier wrote: > > > On 2015/08/20 05:39:30, Timothy Loh wrote: > > > > On 2015/08/20 00:16:01, a.renevier wrote: > > > > > > > > lgtm with nit > > > > > > > > Please also remove the setNeedsRecalcStyleInAllFrames() calls in > > > > Page::settingsChanged (SettingsDelegate::FontFamilyChange and > > > > SettingsDelegate::MediaQueryChange) > > > > > > I removed the call after SettingsDelegate::MediaQueryChange. > > > Are you sure I also need to remove it after > > SettingsDelegate::MediaQueryChange? > > > I don't understand how it is related to the rest of the patch? > > > > The call is redundant if mediaQueryAffectingValueChanged() is now requesting a > > style recalc. BTW there's also the call for FontFamilyChange... > > Yes sorry, I was confused in my previous message. I understand why I need to > remove the call after SettingsDelegate::MediaQueryChange. My last patch has that > change. > > I am still wondering why it's needed after FontFamilyChange: > styleEngine().updateGenericFontFamilySettings is being called, and it does not > call mediaQueryAffectingValueChanged() Sorry, my mistake, I probably misread it :-). lgtm to land as is
The CQ bit was checked by a.renevier@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/1285703004/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1285703004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1285703004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
a.renevier@samsung.com changed reviewers: + bokan@chromium.org, pfeldman@chromium.org
Oups, I was missing reviewers for Source/Web. Adding bokan and pfeldman
lgtm
The CQ bit was checked by a.renevier@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1285703004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1285703004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by a.renevier@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1285703004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1285703004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201417 |