Update css styles after modifying display mode

Update 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

[a.renevier, 2015-08-11 17:44:25]

a.renevier@samsung.com changed reviewers:
+ kenneth.r.christiansen@intel.com, mkwst@chromium.org, yoav@yoav.ws

[kenneth.r.christiansen, 2015-08-17 09:03:36]

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

[Mike West, 2015-08-17 15:29:06]

mkwst@chromium.org changed reviewers:
+ eae@chromium.org

[Mike West, 2015-08-17 15:29:07]

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?

[a.renevier, 2015-08-17 17:25:58]

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, 2015-08-17 17:29:18]

eae@chromium.org changed reviewers:
+ timloh@chromium.org

[eae, 2015-08-17 17:29:18]

LGTM but I'd like timloh to take a look as well.

[Timothy Loh, 2015-08-18 04:12:20]

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? :-)

[Timothy Loh, 2015-08-20 05:39:30]

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)

[a.renevier, 2015-08-21 17:10:13]

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?

[Timothy Loh, 2015-08-24 01:16:47]

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...

[a.renevier, 2015-08-24 18:04:28]

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()

[Timothy Loh, 2015-08-25 00:04:22]

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

[a.renevier, 2015-08-26 17:54:52]

The CQ bit was checked by a.renevier@samsung.com

[a.renevier, 2015-08-26 17:54:52]

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: " ")

[commit-bot: I haz the power, 2015-08-26 17:55:09]

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

[commit-bot: I haz the power, 2015-08-26 18:06:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-26 18:06:35]

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, 2015-08-26 22:23:18]

a.renevier@samsung.com changed reviewers:
+ bokan@chromium.org, pfeldman@chromium.org

[a.renevier, 2015-08-26 22:23:47]

Oups, I was missing reviewers for Source/Web. Adding bokan and pfeldman

[pfeldman, 2015-08-27 18:59:09]

lgtm

[a.renevier, 2015-08-27 22:06:42]

The CQ bit was checked by a.renevier@samsung.com

[commit-bot: I haz the power, 2015-08-27 22:06:51]

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

[commit-bot: I haz the power, 2015-08-27 22:25:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-27 22:25:37]

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...)

[a.renevier, 2015-08-28 17:23:14]

The CQ bit was checked by a.renevier@samsung.com

[commit-bot: I haz the power, 2015-08-28 17:23:24]

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

[commit-bot: I haz the power, 2015-08-28 18:24:13]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201417