Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

Issue 53053002: [Android WebView] Add a legacy quirk for 'user-scalable=no' case (Closed)

Created:
5 years, 6 months ago by mnaganov (inactive)
Modified:
5 years, 6 months ago
CC:
blink-reviews, jamesr, dglazkov+blink, abarth-chromium, android-webview-reviews_chromium.org
Visibility:
Public.

Description

[Android WebView] Add a legacy quirk for 'user-scalable=no' case In legacy Android WebView implementation, having 'user-scalable=no' in the viewport meta tag ignores scale set via 'initial-scale' and also affects wide viewport quirk behavior. BUG=312691 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161448

Patch Set 1 #

Patch Set 2 : Move the setting into WebSettingsImpl #

Total comments: 2

Patch Set 3 : Rewritten according to aelias@' proposal #

Total comments: 4

Patch Set 4 : Moved min/max scale reset under the quirk condition, rebased #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+105 lines, -6 lines) Patch
M Source/web/PageScaleConstraintsSet.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/web/PageScaleConstraintsSet.cpp View 1 2 3 2 chunks +12 lines, -2 lines 1 comment Download
M Source/web/WebSettingsImpl.h View 1 2 3 3 chunks +6 lines, -0 lines 0 comments Download
M Source/web/WebSettingsImpl.cpp View 1 2 3 2 chunks +6 lines, -0 lines 0 comments Download
M Source/web/WebViewImpl.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/web/tests/WebFrameTest.cpp View 1 2 3 1 chunk +68 lines, -0 lines 0 comments Download
A Source/web/tests/data/viewport-2x-initial-scale-non-user-scalable.html View 1 chunk +8 lines, -0 lines 0 comments Download
A + Source/web/tests/data/viewport-initial-scale-and-user-scalable-no.html View 1 chunk +2 lines, -2 lines 0 comments Download
M public/web/WebSettings.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
mnaganov (inactive)
Hello, Alexandre! Please take a look! This only affects the legacy compatibility mode.
5 years, 6 months ago (2013-10-30 18:27:04 UTC) #1
mnaganov (inactive)
aelias@: Ping!
5 years, 6 months ago (2013-11-01 07:56:42 UTC) #2
aelias_OOO_until_Jul13
See my comment on the bug.
5 years, 6 months ago (2013-11-01 20:12:36 UTC) #3
aelias_OOO_until_Jul13
https://codereview.chromium.org/53053002/diff/40001/Source/web/PageScaleConstraintsSet.cpp File Source/web/PageScaleConstraintsSet.cpp (right): https://codereview.chromium.org/53053002/diff/40001/Source/web/PageScaleConstraintsSet.cpp#newcode176 Source/web/PageScaleConstraintsSet.cpp:176: The logic inside wideViewportQuirkEnabled is too tangled. Could you ...
5 years, 6 months ago (2013-11-01 23:58:00 UTC) #4
mnaganov (inactive)
Thanks, Alexandre! https://codereview.chromium.org/53053002/diff/40001/Source/web/PageScaleConstraintsSet.cpp File Source/web/PageScaleConstraintsSet.cpp (right): https://codereview.chromium.org/53053002/diff/40001/Source/web/PageScaleConstraintsSet.cpp#newcode176 Source/web/PageScaleConstraintsSet.cpp:176: On 2013/11/01 23:58:00, aelias wrote: > The ...
5 years, 6 months ago (2013-11-05 11:01:37 UTC) #5
aelias_OOO_until_Jul13
lgtm. Adding abarth@ for new setting in public/web/ https://codereview.chromium.org/53053002/diff/110001/Source/web/PageScaleConstraintsSet.cpp File Source/web/PageScaleConstraintsSet.cpp (right): https://codereview.chromium.org/53053002/diff/110001/Source/web/PageScaleConstraintsSet.cpp#newcode183 Source/web/PageScaleConstraintsSet.cpp:183: if ...
5 years, 6 months ago (2013-11-05 19:39:09 UTC) #6
eseidel
lgtm https://codereview.chromium.org/53053002/diff/110001/Source/web/PageScaleConstraintsSet.h File Source/web/PageScaleConstraintsSet.h (right): https://codereview.chromium.org/53053002/diff/110001/Source/web/PageScaleConstraintsSet.h#newcode52 Source/web/PageScaleConstraintsSet.h:52: void adjustForAndroidWebViewQuirks(const WebCore::ViewportDescription&, WebCore::IntSize viewSize, int layoutFallbackWidth, float ...
5 years, 6 months ago (2013-11-05 20:09:53 UTC) #7
mnaganov (inactive)
Thanks Alexandre and Eric! https://codereview.chromium.org/53053002/diff/110001/Source/web/PageScaleConstraintsSet.cpp File Source/web/PageScaleConstraintsSet.cpp (right): https://codereview.chromium.org/53053002/diff/110001/Source/web/PageScaleConstraintsSet.cpp#newcode183 Source/web/PageScaleConstraintsSet.cpp:183: if (!description.userZoom) { On 2013/11/05 ...
5 years, 6 months ago (2013-11-06 09:53:06 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnaganov@chromium.org/53053002/210001
5 years, 6 months ago (2013-11-06 09:54:00 UTC) #9
commit-bot: I haz the power
Change committed as 161448
5 years, 6 months ago (2013-11-06 11:00:46 UTC) #10
aelias_OOO_until_Jul13
https://codereview.chromium.org/53053002/diff/210001/Source/web/PageScaleConstraintsSet.cpp File Source/web/PageScaleConstraintsSet.cpp (right): https://codereview.chromium.org/53053002/diff/210001/Source/web/PageScaleConstraintsSet.cpp#newcode189 Source/web/PageScaleConstraintsSet.cpp:189: m_pageDefinedConstraints.minimumScale = m_pageDefinedConstraints.initialScale; Hmm, I don't think this should've ...
5 years, 6 months ago (2013-11-06 20:45:25 UTC) #11
mnaganov (inactive)
On 2013/11/06 20:45:25, aelias wrote: > https://codereview.chromium.org/53053002/diff/210001/Source/web/PageScaleConstraintsSet.cpp > File Source/web/PageScaleConstraintsSet.cpp (right): > > https://codereview.chromium.org/53053002/diff/210001/Source/web/PageScaleConstraintsSet.cpp#newcode189 > ...
5 years, 6 months ago (2013-11-06 20:54:20 UTC) #12
aelias_OOO_until_Jul13
5 years, 6 months ago (2013-11-06 20:55:10 UTC) #13
Message was sent while issue was closed.
OK, thanks!  Not that big a deal, but maybe it could be folded into another
related patch.

Powered by Google App Engine
This is Rietveld 408576698