[Chromium] Call restrictScaleFactorToInitialScaleIfNotUserScalable unless/until userScalable is supported directly.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[John Mellor]

Chromium should call restrictScaleFactorToInitialScaleIfNotUserScalable unless/until userScalable is supported directly.

[John Mellor]

Created attachment 151981 [details]
Patch

[John Mellor]

In bug 70609 comment 19 Fady factored out the following line from computeViewportAttributes:

    if (!args.userScalable) result.maximumScale = result.minimumScale = result.initialScale;

into a separate function, restrictScaleFactorToInitialScaleIfNotUserScalable, which wasn't called on Chromium. Instead at least on Android we used to handle userScalable separately. However Android dropped that separate handling in https://gerrit-int.chromium.org/12197 when it synchronized with upstream, and current upstream seems to completely ignore the value of userScalable!

This patch fixes the immediate issue, that we completely ignore user-scalable=no. But I seem to remember there was some subtle reason for not overwriting mininum-scale/maximum-scale with the initial-scale, and that it was better to handle it separately. Fady, can you remember what this was? And do any of you remember why we stopped handling userScalable separately? Was it just a casualty of upstreaming?

[Adam Barth]

Comment on attachment 151981 [details]
Patch

You should be able to write a test for this along the lines of WebFrameTest.CanOverrideMaximumScaleFactor in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/WebViewTest.cpp

[Peter Beverloo]

Comment on attachment 151981 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151981&action=review

> Source/WebKit/chromium/ChangeLog:6
> +        Since https://gerrit-int.chromium.org/12197 the chromium-android port

We shouldn't refer to internal URLs in ChangeLog messages. They're meant to be informative for all parties, and this way only Googlers can get access to the context.

[John Mellor]

(In reply to comment #2)
> But I seem to remember there was some subtle reason for not overwriting mininum-scale/maximum-scale with the initial-scale, and that it was better to handle it separately.

Ah yes - suppose the viewport tag doesn't have a min/max-scale, but does have user-scalable=no. The initial-scale computed at this point assumes that we will use the viewport width as our initial containing block [1]. But if the page has a CSS min-width larger than the viewport width, and the viewport tag didn't specify any scale information, we might later choose to set the initial containing block such that the entire page fits within it, and end up with a smaller initial-scale (I can't remember whether or not this is fully spec-compliant, but OTH some major browsers do it anyway). If we implement user-scalable by overwriting min/max-scale with our initial viewport-based guess at the initial-scale, then if we later do want to adjust the initial-scale, we can no longer tell whether we were banned from doing so (because of an explicit min/max-scale), or we are actually allowed to do so (because there was only a user-scalable directive).

Nevertheless, I still think this patch is useful in the short term, as it's better than not supporting user-scalable at all!

A test would be useful. I probably won't get round to that for a little while.

[1]: http://www.w3.org/TR/2004/CR-CSS21-20040225/visudet.html#containing-block-details

[John Mellor]

(In reply to comment #4)
> (From update of attachment 151981 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151981&action=review
> 
> > Source/WebKit/chromium/ChangeLog:6
> > +        Since https://gerrit-int.chromium.org/12197 the chromium-android port
> 
> We shouldn't refer to internal URLs in ChangeLog messages. They're meant to be informative for all parties, and this way only Googlers can get access to the context.

$ git grep -n 'rdar' -- '*ChangeLog*' | wc -l
13464

I agree in principle, but this is the chromium ChangeLog and there's no publicly accessible equivalent way of referencing our downstream history, so it seems worth keeping this useful piece of context...

[Adam Barth]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 151981 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=151981&action=review
> > 
> > > Source/WebKit/chromium/ChangeLog:6
> > > +        Since https://gerrit-int.chromium.org/12197 the chromium-android port
> > 
> > We shouldn't refer to internal URLs in ChangeLog messages. They're meant to be informative for all parties, and this way only Googlers can get access to the context.
> 
> $ git grep -n 'rdar' -- '*ChangeLog*' | wc -l
> 13464
> 
> I agree in principle, but this is the chromium ChangeLog and there's no publicly accessible equivalent way of referencing our downstream history, so it seems worth keeping this useful piece of context...

As a contributor to the project, I often become sad when my search for the "why" behind a piece of code ends in an rdar link.  I appreciate that you've taken the time to write a descriptive ChangeLog for this patch, which is very helpful and mitigates this issue to a large extent.

Unfortunately, any value that's added by the gerrit-int is only enjoyed by the small number of folks who can follow that link.  If that's a large amount of value, then the majority of the contributors to the project are missing out.  If it's a small amount of value, then it's probably not worth making the rest of the contributors feel sad that they can't follow the link

[Adam Barth]

> A test would be useful. I probably won't get round to that for a little while.

I can try writing one for you.

[John Mellor]

(In reply to comment #8)
> > A test would be useful. I probably won't get round to that for a little while.
> 
> I can try writing one for you.

Thanks Adam!

[Adam Barth]

Turns out we can test this using a simple LayoutTest.

[Adam Barth]

Created attachment 155372 [details]
Patch

[John Mellor]

Comment on attachment 155372 [details]
Patch

Test looks good (though possibly a little implementation-specific?).

[Adam Barth]

> Test looks good (though possibly a little implementation-specific?).

Yeah, that's a general problem with these fast/viewport tests.  However, as far as I can tell, this is the implementation used by the other ports that implement viewport in WebKit.

[John Mellor]

Shall we try to get this landed then?

[Adam Barth]

Sorry, this fell off my radar.

[Adam Barth]

@tony: Would you be willing to review this patch?

[Tony Chang]

Comment on attachment 155372 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155372&action=review

> LayoutTests/fast/viewport/viewport-limits-adjusted-for-no-user-scale-control.html:7
> +                alert(internals.configurationForViewport(document, 1, 320, 480, 320, 352));

Nit: It would be nice if the test said PASS or something so random people gardening can quickly tell if the test is correct or not.

> LayoutTests/fast/viewport/viewport-limits-adjusted-for-no-user-scale.html:7
> +                alert(internals.configurationForViewport(document, 1, 320, 480, 320, 352));

Ditto.

[Adam Barth]

Comment on attachment 155372 [details]
Patch

Thanks!

[Adam Barth]

Created attachment 162320 [details]
Patch for landing

[Adam Barth]

Committed r127704: <http://trac.webkit.org/changeset/127704>