Avoid clobbering state in viewport tag initialization.

third_party/WebKit/Source/core/dom/Document.cpp

Index: third_party/WebKit/Source/core/dom/Document.cpp
diff --git a/third_party/WebKit/Source/core/dom/Document.cpp b/third_party/WebKit/Source/core/dom/Document.cpp
index 3a08cfb7e1398a606ce73915bb0c1b314137e08a..6b96158fa69447b81bbae0d5dcb0afa76c50b4fa 100644
--- a/third_party/WebKit/Source/core/dom/Document.cpp
+++ b/third_party/WebKit/Source/core/dom/Document.cpp
@@ -3150,7 +3150,7 @@ void Document::maybeHandleHttpRefresh(const String& content, HttpRefreshType htt
     m_frame->navigationScheduler().scheduleRedirect(delay, refreshURL);
 }
 
-bool Document::shouldMergeWithLegacyDescription(ViewportDescription::Type origin)
+bool Document::shouldMergeWithLegacyDescription(ViewportDescription::Type origin) const
 {
     return settings() && settings()->viewportMetaMergeContentQuirk() && m_legacyViewportDescription.isMetaViewportType() && m_legacyViewportDescription.type == origin;
 }
@@ -3160,37 +3160,35 @@ void Document::setViewportDescription(const ViewportDescription& viewportDescrip
     if (viewportDescription == m_viewportDescription)

[dgozman, 2016/05/09 17:36:19]

This check looks strange now (and was before). Why do we only compare with
|m_viewportDescription| if we can set |m_legacyViewportDescription| instead? I
think it's safe to remove it.

[aelias_OOO_until_Jul13, 2016/05/09 23:56:02]

Agreed it's weird now.  I'd like to preserve a safeguard against unnecessary
calls to updateViewportDescription() though which could do expensive stuff like
layouts, so I split this into two early-returns, one for legacy and one for not.

         return;
 
-    if (settings() && !settings()->viewportMetaEnabled() && viewportDescription.isLegacyViewportType())
-        return;
-
     // The UA-defined min-width is used by the processing of legacy meta tags.
     if (!viewportDescription.isSpecifiedByAuthor())
         m_viewportDefaultMinWidth = viewportDescription.minWidth;

[dgozman, 2016/05/09 17:36:19]

Should we do something similar to |m_viewportDefaultMinWidth|? It could possibly
mess up "request desktop" scenario, couldn't it?

[aelias_OOO_until_Jul13, 2016/05/09 23:56:02]

I don't think there's a risk of bugs with this one since it's basically just a
fixed constant of 980 in all cases, but anyway I moved it within the non-legacy
block which is the only place where it may appear.

 
     if (viewportDescription.isLegacyViewportType()) {
         m_legacyViewportDescription = viewportDescription;
-
-        // When no author style for @viewport is present, and a meta tag for defining
-        // the viewport is, apply the meta tag viewport instead of the UA styles.
-        if (m_viewportDescription.type == ViewportDescription::AuthorStyleSheet)
-            return;
-        m_viewportDescription = viewportDescription;
     } else {
-        // If the legacy viewport tag has higher priority than the cascaded @viewport
-        // descriptors, use the values from the legacy tag.
-        if (!shouldOverrideLegacyDescription(viewportDescription.type))
-            m_viewportDescription = m_legacyViewportDescription;
-        else
-            m_viewportDescription = viewportDescription;
+        m_viewportDescription = viewportDescription;
     }
 
     updateViewportDescription();
 }
 
+ViewportDescription Document::viewportDescription() const
+{
+    ViewportDescription appliedViewportDescription = m_viewportDescription;
+    bool viewportMetaEnabled = settings() && settings()->viewportMetaEnabled();
+    if (m_legacyViewportDescription.type != ViewportDescription::UserAgentStyleSheet && viewportMetaEnabled)
+        appliedViewportDescription = m_legacyViewportDescription;
+    if (shouldOverrideLegacyDescription(m_viewportDescription.type))
+        appliedViewportDescription = m_viewportDescription;
+
+    return appliedViewportDescription;
+}
+
 void Document::updateViewportDescription()
 {
     if (frame() && frame()->isMainFrame()) {
-        frameHost()->chromeClient().dispatchViewportPropertiesDidChange(m_viewportDescription);
+        frameHost()->chromeClient().dispatchViewportPropertiesDidChange(viewportDescription());
     }
 }