Issue 11759023: [Android WebView] Implement WebSettings.{get|set}UseWideViewport

Issue 11759023: [Android WebView] Implement WebSettings.{get|set}UseWideViewport (Closed)

Created:
7 years, 11 months ago by mnaganov (inactive)

Modified:
7 years, 11 months ago

Reviewers:
jcivelli, darin (slow to review), Jay Civelli, benm (inactive)

CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

[Android WebView] Implement WebSettings.{get|set}UseWideViewport The setting is mapped directly onto WebKit's WebSettings.viewportEnabled R=benm@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176103

Patch Set 1 #

Patch Set 2 : Updated #

Total comments: 8

Patch Set 3 : Comments addressed #

Total comments: 2

Patch Set 4 : Moved preference setting code #

Patch Set 5 : Rebase and fix tests that were expecting UseWideViewPort=true #

Created: 7 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+187 lines, -4 lines)			Patch
M	android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnScaleChangedTest.java	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java	View	1 2 3 4	4 chunks	+125 lines, -0 lines	0 comments	Download
M	android_webview/javatests/src/org/chromium/android_webview/test/ContentViewZoomTest.java	View	1 2 3 4	5 chunks	+5 lines, -0 lines	0 comments	Download
M	content/browser/android/content_settings.cc	View		4 chunks	+12 lines, -0 lines	0 comments	Download
M	content/browser/web_contents/web_contents_impl.cc	View	1 2 3 4	1 chunk	+2 lines, -0 lines	0 comments	Download
M	content/public/android/java/src/org/chromium/content/browser/ContentSettings.java	View	1 2 3	2 chunks	+35 lines, -0 lines	0 comments	Download
M	content/public/android/java/src/org/chromium/content/common/DeviceInfo.java	View		1 chunk	+1 line, -1 line	0 comments	Download
M	content/public/common/common_param_traits_macros.h	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	content/renderer/render_view_impl.cc	View	1 2 3 4	1 chunk	+1 line, -3 lines	0 comments	Download
M	webkit/glue/webpreferences.h	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	webkit/glue/webpreferences.cc	View	1 2 3	2 chunks	+3 lines, -0 lines	0 comments	Download

Messages

Total messages: 14 (0 generated)

Expand Messages | Collapse Messages

mnaganov (inactive)

Please note that this depends on https://bugs.webkit.org/show_bug.cgi?id=106021

7 years, 11 months ago (2013-01-04 17:23:32 UTC) #1

mnaganov (inactive)

The WebKit patch has been r+'d. Please take a look at this one.

7 years, 11 months ago (2013-01-08 10:53:23 UTC) #2

benm (inactive)

lgtm, just nits on the test. https://codereview.chromium.org/11759023/diff/3001/android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java File android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java (right): https://codereview.chromium.org/11759023/diff/3001/android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java#newcode1059 android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1059: // current (and ...

7 years, 11 months ago (2013-01-08 13:54:04 UTC) #3

mnaganov (inactive)

Thanks, Ben! https://codereview.chromium.org/11759023/diff/3001/android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java File android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java (right): https://codereview.chromium.org/11759023/diff/3001/android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java#newcode1059 android_webview/javatests/src/org/chromium/android_webview/test/AwSettingsTest.java:1059: // current (and upcoming) device screen widths ...

7 years, 11 months ago (2013-01-08 16:42:42 UTC) #4

mnaganov (inactive)

Need an owner's review: darin@ for content/public/common, content/renderer and webkit/glue jcivelli@ for content/public/android and content/browser/android ...

7 years, 11 months ago (2013-01-08 16:48:54 UTC) #5

darin (slow to review)

https://codereview.chromium.org/11759023/diff/8001/content/renderer/render_view_impl.cc File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/11759023/diff/8001/content/renderer/render_view_impl.cc#newcode3220 content/renderer/render_view_impl.cc:3220: webkit_preferences_.viewport_enabled = true; It seems a little odd to ...

7 years, 11 months ago (2013-01-08 17:10:42 UTC) #6

mnaganov (inactive)

7 years, 11 months ago (2013-01-08 18:14:44 UTC) #7

darin (slow to review)

On Tue, Jan 8, 2013 at 10:14 AM, <mnaganov@chromium.org> wrote: > > https://codereview.chromium.**org/11759023/diff/8001/** > content/renderer/render_view_**impl.cc<https://codereview.chromium.org/11759023/diff/8001/content/renderer/render_view_impl.cc> ...

7 years, 11 months ago (2013-01-08 19:14:24 UTC) #8

mnaganov (inactive)

On 2013/01/08 19:14:24, darin wrote: > On Tue, Jan 8, 2013 at 10:14 AM, <mailto:mnaganov@chromium.org> ...

7 years, 11 months ago (2013-01-09 17:00:44 UTC) #9

On 2013/01/08 19:14:24, darin wrote:
> On Tue, Jan 8, 2013 at 10:14 AM, <mailto:mnaganov@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/11759023/diff/8001/**
> >
>
content/renderer/render_view_**impl.cc<https://codereview.chromium.org/11759023/diff/8001/content/renderer/render_view_impl.cc>
> > File content/renderer/render_view_**impl.cc (right):
> >
> > https://codereview.chromium.**org/11759023/diff/8001/**
> >
>
content/renderer/render_view_**impl.cc#newcode3220<https://codereview.chromium.org/11759023/diff/8001/content/renderer/render_view_impl.cc#newcode3220>
> > content/renderer/render_view_**impl.cc:3220:
> > webkit_preferences_.viewport_**enabled = true;
> > On 2013/01/08 17:10:42, darin wrote:
> >
> >> It seems a little odd to be setting a WebPreferences member here in
> >>
> > the
> >
> >> RenderViewImpl.  Normally, we initialize WebPreferences in the
> >>
> > browser-side, and
> >
> >> then send those all down together when initializing a RenderViewImpl.
> >>
> > Maybe
> >
> >> this change suggests that we shouldn't be checking kEnableViewport in
> >>
> > the
> >
> >> renderer, but instead checking that in the browser?
> >>
> >
> > Yes, it does look odd to me as well. But as you can see,
> > 'setViewportEnabled' works together with other settings and even flags
> > that are set directly on a WebView (enableFixedLayoutMode,
> > setFixedLayoutSize), so we will need to put more things into
> > WebPreferences and perhaps, move fixed layout parameters into
> > WebSettings. I can do that first, prior to this patch.
> >
> 
> OK
>

It seems that moving all viewport and fixed layout-related settings into
WebSettings doesn't seem to be feasible. For example, when enabling fixed layout
and setting fixed layout size, WebView simply delegates to WebCore's ScrollView,
which doesn't seem to use settings.

I moved the code that sets the value of the 'viewport_enabled' preference into
WebContentsImpl to follow the general pattern, but RenderViewImpl still
continues to process command-line switches to set the remaining flags.

darin@ - does this look better to you now?
 
> 
> >
> >
>
https://codereview.chromium.**org/11759023/%3Chttps://codereview.chromium.org...>
> >

mnaganov (inactive)

Thanks, Darin! Jay, can you take a look at Android parts, please?

7 years, 11 months ago (2013-01-09 18:03:41 UTC) #11

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mnaganov@chromium.org/11759023/18003

7 years, 11 months ago (2013-01-10 13:04:15 UTC) #13

Message was sent while issue was closed.

Change committed as 176103

Expand Messages | Collapse Messages