Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
804783002:
Improve DPI matching & scaling logic. (Closed)
DescriptionImprove DPI matching & scaling logic.
Previously the client would attempt to scale the host desktop to account for the difference between client & host DPI. This could produce poor results in situations in which the client could not control the host dimensions, e.g. if the host was fixed at a very low resolution, but being displayed on a high-resolution display with the same notional DPI then lots of space would be wasted.
This CL implements a new scheme, described in the comment block in remoting.ClientSession.updateDimensions(), that allows for more flexible up-scaling of very small hosts, and better handling of close-to-integer scale factors.
BUG=135089, 366359
Committed: https://crrev.com/301d65d865ed85c01f8243565f3f76b232f3953f
Cr-Commit-Position: refs/heads/master@{#316353}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Reinstate basic DPI matching and snap-to-integer-scale #Patch Set 4 : Improve comments #
Total comments: 4
Patch Set 5 : Unit-tests! #
Total comments: 18
Patch Set 6 : Address review comments #Patch Set 7 : Update non-shrink-to-fit tests #
Total comments: 17
Patch Set 8 : Address review comments #Patch Set 9 : Fix multi-monitor specialization & update tests #
Total comments: 8
Patch Set 10 : Address review comments #Patch Set 11 : Rebase #
Messages
Total messages: 29 (4 generated)
Rebase
Reinstate basic DPI matching and snap-to-integer-scale
Improve comments
wez@chromium.org changed reviewers: + jamiewalch@chromium.org
PTAL For unit-testing purposes I think it'd be best to pull this logic out into a helper function that can be tested independently of the ClientSession itself - any thoughts on where that should live?
I would either create a new file containing just the function you want to test, or make it a static member (ie, not on the prototype) of ClientSession. https://codereview.chromium.org/804783002/diff/60001/remoting/webapp/crd/js/c... File remoting/webapp/crd/js/client_session.js (right): https://codereview.chromium.org/804783002/diff/60001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1268: // b. If host DPI is less than the client's then up-scale accordingly. Does this overrule constraint 3, above? https://codereview.chromium.org/804783002/diff/60001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1270: // cancel-out some or all of the up-scaling from (b). It's not clear what "some or all" means here. Can you be more specific? https://codereview.chromium.org/804783002/diff/60001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1273: // letterboxing. Does "avoid excessive letterboxing" imply that we'll always select the largest integer that gives a plugin size smaller than the window (ie, no scroll-bars)? Edit: No, it seems that's not true, based on step 3. That being the case, I think it needs some clarification. https://codereview.chromium.org/804783002/diff/60001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1277: // then reduce it to that integer factor, to avoid blurring. By "fractionally", do mean "only just"? Given that we're talking about ratios (a.k.a, fractions) below, I would pick a different word to avoid confusion, or (if it's not overkill) be explicit: (1 + epsilon) * k, for some integer k.
PTAL; this patch-set adds some unit-tests. It's currently missing some tests for the close-to-integer-scale-factor case and for the full-screen multi-monitor behaviour. It's all one big test but could reasonably be broken down into several smaller tests; WDYT?
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
This doesn't solve the problem for the scenario I described in crbug.com/454618
https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... File remoting/webapp/crd/js/client_session.js (right): https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1295: isFullscreen, shrinkToFit) { Maybe assert that any value used as a numerator (or even all values, if that makes sense) is not zero. https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1308: // cancel-out some or all of the up-scaling from (b). It's not clear what "cancel-out" means in this context. Some examples would help. https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1342: scale = Math.min(scaleX, scaleY); It's not immediately obvious from reading the code that scale will be >= 1.0. I think an assert would document that nicely. https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1349: Math.min(scale, 1.0 * clientHeight / desktopSize.height); I'm not sure what the "1.0 *" was doing in the original code, but I don't think it's needed. https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1376: if (scaleBlurriness < 1.1) { Optional: I wonder if this would be better expressed as a difference rather than a ratio? In other words, sacrifice up to x pixels horizontally and vertically in order to make the image less blurred. https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/unittest... File remoting/webapp/unittests/client_session_unittest.js (right): https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/unittest... remoting/webapp/unittests/client_session_unittest.js:21: // Client and host w/ same DPI. I think these test cases need descriptions to make it clear why we expect the output that we do (eg. "avoid non-integer scale-factors"). That can either be a comment in this file, or you can split it into multiple tests with appropriate descriptions. https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/unittest... remoting/webapp/unittests/client_session_unittest.js:37: QUnit.deepEqual(pluginSize, size(600, 450)); It took me a while to work out where the 600 came from. Expressing it as 640 * 450 / 480 might be clearer.
https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... File remoting/webapp/crd/js/client_session.js (right): https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1301: // small. I think another goal here should also be to render the screen to match physical screen size of the host. Without it we can just ignore desktopDpi received from the host and still satisfy these goals. https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1325: var hostPixelRatioX = Math.ceil(desktopDpi.x / 96); can this be Math.floor() instead of Math.ceil()? That way we won't be rendering low-DPI image on hi-DPI display without upscaling, while still avoiding blurring when it is possible. IMO it's better to blur the image while upscaling instead of rendering it small and unreadable.
PTAL Sergey, the 150% DPI host to high-DPI client case you described in crbug.com/454618 is covered by the 4th 150% DPI host test-case - notice that the new algorithm results in a 3x up-scale from the original host dimensions, rather than leaving the host teeny-tiny. https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... File remoting/webapp/crd/js/client_session.js (right): https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1295: isFullscreen, shrinkToFit) { On 2015/02/04 23:18:30, Jamie wrote: > Maybe assert that any value used as a numerator (or even all values, if that > makes sense) is not zero. Done. https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1301: // small. On 2015/02/04 23:57:41, Sergey Ulanov wrote: > I think another goal here should also be to render the screen to match physical > screen size of the host. Without it we can just ignore desktopDpi received from > the host and still satisfy these goals. Exactly; the DPI is only included as part of calculating the "ideal" size, below. https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1308: // cancel-out some or all of the up-scaling from (b). On 2015/02/04 23:18:29, Jamie wrote: > It's not clear what "cancel-out" means in this context. Some examples would > help. Done. https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1325: var hostPixelRatioX = Math.ceil(desktopDpi.x / 96); On 2015/02/04 23:57:41, Sergey Ulanov wrote: > can this be Math.floor() instead of Math.ceil()? That way we won't be rendering > low-DPI image on hi-DPI display without upscaling, while still avoiding blurring > when it is possible. IMO it's better to blur the image while upscaling instead > of rendering it small and unreadable. We should actually be able to remove the rounding entirely now that the snap-to-blurrless is implemented further down, but I'd prefer to make that change separately. It's also the case that the larger vs blurry trade-off is subjective, so we should experiment rather than just changing the behaviour in this CL. Finally, there's no trade-off in the case of a host having been resized-to-fit the client - in that case it should end up resized down to dimensions that can be up-scaled by an integer factor at the client. Step #2 performs further up-scaling if the host is still "naturally" smaller than the client after taking DPI into account. https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1342: scale = Math.min(scaleX, scaleY); On 2015/02/04 23:18:30, Jamie wrote: > It's not immediately obvious from reading the code that scale will be >= 1.0. I > think an assert would document that nicely. Done. https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1349: Math.min(scale, 1.0 * clientHeight / desktopSize.height); On 2015/02/04 23:18:30, Jamie wrote: > I'm not sure what the "1.0 *" was doing in the original code, but I don't think > it's needed. D'oh; thanks for spotting that! https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/crd/js/c... remoting/webapp/crd/js/client_session.js:1376: if (scaleBlurriness < 1.1) { On 2015/02/04 23:18:30, Jamie wrote: > Optional: I wonder if this would be better expressed as a difference rather than > a ratio? In other words, sacrifice up to x pixels horizontally and vertically in > order to make the image less blurred. I considered that, but I think a ratio makes more sense; if you consider the difference between 640x480 and 2560x1600, clearly the point at which so much space is being wasted it's better to allow blurring isn't the same - a 16px border around 640x480 is a lot more wasted space than it is around 2560x1600. https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/unittest... File remoting/webapp/unittests/client_session_unittest.js (right): https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/unittest... remoting/webapp/unittests/client_session_unittest.js:21: // Client and host w/ same DPI. On 2015/02/04 23:18:30, Jamie wrote: > I think these test cases need descriptions to make it clear why we expect the > output that we do (eg. "avoid non-integer scale-factors"). That can either be a > comment in this file, or you can split it into multiple tests with appropriate > descriptions. Yeah, I think splitting into a set of distinct tests is preferable; it also makes it easier to understand which case broke, in the case of a failure! https://codereview.chromium.org/804783002/diff/80001/remoting/webapp/unittest... remoting/webapp/unittests/client_session_unittest.js:37: QUnit.deepEqual(pluginSize, size(600, 450)); On 2015/02/04 23:18:30, Jamie wrote: > It took me a while to work out where the 600 came from. Expressing it as 640 * > 450 / 480 might be clearer. Acknowledged.
On 2015/02/06 01:29:43, Wez wrote: > PTAL > > Sergey, the 150% DPI host to high-DPI client case you described in > crbug.com/454618 is covered by the 4th 150% DPI host test-case - notice that the > new algorithm results in a 3x up-scale from the original host dimensions, rather > than leaving the host teeny-tiny. That bug is actually about 125% DPI, or any DPI that's slightly higher than 96. 150% DPI is closer to 200% than 100% and the current behavior is not that bad for that case (the true midpoint between 100% and 200% is about 141% ~= 136 DPI). I'm not insisting on fixing it here, but then I'd argue that crbug.com/454618 is not a dup of crbug.com/366359.
On 2015/02/06 01:56:55, Sergey Ulanov wrote: > On 2015/02/06 01:29:43, Wez wrote: > > PTAL > > > > Sergey, the 150% DPI host to high-DPI client case you described in > > crbug.com/454618 is covered by the 4th 150% DPI host test-case - notice that > the > > new algorithm results in a 3x up-scale from the original host dimensions, > rather > > than leaving the host teeny-tiny. > > That bug is actually about 125% DPI, or any DPI that's slightly higher than 96. > 150% DPI is closer to 200% than 100% and the current behavior is not that bad > for that case (the true midpoint between 100% and 200% is about 141% ~= 136 > DPI). > I'm not insisting on fixing it here, but then I'd argue that crbug.com/454618 is > not a dup of crbug.com/366359. Just to make it clear, the new logic for "Up-scale to avoid excessive letterboxing" doesn't fix the scenario I described in crbug.com/454618. It's only useful when client has much larger screen than the host and doesn't do anything when host and client have comparable _physical_ screen size (i.e. the pixel resolution is much higher on the client)
On 2015/02/06 02:55:19, Sergey Ulanov wrote: > On 2015/02/06 01:56:55, Sergey Ulanov wrote: > > On 2015/02/06 01:29:43, Wez wrote: > > > PTAL > > > > > > Sergey, the 150% DPI host to high-DPI client case you described in > > > crbug.com/454618 is covered by the 4th 150% DPI host test-case - notice that > > the > > > new algorithm results in a 3x up-scale from the original host dimensions, > > rather > > > than leaving the host teeny-tiny. > > > > That bug is actually about 125% DPI, or any DPI that's slightly higher than > 96. > > 150% DPI is closer to 200% than 100% and the current behavior is not that bad > > for that case (the true midpoint between 100% and 200% is about 141% ~= 136 > > DPI). > > I'm not insisting on fixing it here, but then I'd argue that crbug.com/454618 > is > > not a dup of crbug.com/366359. Gotcha; I got my %ages mixed up; will add some tests for 125%. > Just to make it clear, the new logic for "Up-scale to avoid excessive > letterboxing" doesn't fix the scenario I described in crbug.com/454618. It's > only useful when client has much larger screen than the host and doesn't do > anything when host and client have comparable _physical_ screen size (i.e. the > pixel resolution is much higher on the client) Why not? The algorithm will typically pick the largest integer scale factor between the client _device pixel_ resolution and the host resolution, which should typically give a good result.
I'd like clarification on what the output of the function represents before reviewing the rest of the unit tests. https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/client_session.js (right): https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_session.js:1261: // Position the container. I realize your CL doesn't add this, but AFAICT, this block of variables is never used. I think you can delete it. https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_session.js:1291: * @return {{width: number, height: number}} Since the inputs are in a variety of different units, I think you need to be explicit about what the output represents. https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_session.js:1353: // 2. Up-scale to avoid excessive letterboxing, if necessary. If I'm reading this correctly, this block eliminates horizontal or vertical letter-boxing (or both). Is that right? If so, then I think it would be clearer to state that explicitly in place of "avoid excessive letterboxing." https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_session.js:1367: scale = Math.min(scaleFitHeight, scaleFitWidth); I think it would be clearer (at least with the current variable names) not to compute the min for the width and height scale factors. In other words, it would read: scaleFitWidth = <amount to scale in order to fit the width> scaleFitHeight = <amount to scale in order to fit the height> scale = <min of these, but no larger than we've already computed> Math.min can take more than two parameters in JS, so it's also shorter. https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_session.js:1371: // multi-monitor combinations more intelligently. This part of the algorithm is not covered by your description, I don't think. https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/unittes... File remoting/webapp/unittests/client_session_unittest.js (right): https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/unittes... remoting/webapp/unittests/client_session_unittest.js:31: // 3. Client X dimension larger than hosts, Y dimension smaller. I think you've got this the wrong way round, which means that this test and the previous one are basically the same (the result is limited by the x-dimension in both cases).
PTAL https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/client_session.js (right): https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_session.js:1261: // Position the container. On 2015/02/06 23:06:02, Jamie wrote: > I realize your CL doesn't add this, but AFAICT, this block of variables is never > used. I think you can delete it. Done. https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_session.js:1291: * @return {{width: number, height: number}} On 2015/02/06 23:06:02, Jamie wrote: > Since the inputs are in a variety of different units, I think you need to be > explicit about what the output represents. Done. https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_session.js:1353: // 2. Up-scale to avoid excessive letterboxing, if necessary. On 2015/02/06 23:06:02, Jamie wrote: > If I'm reading this correctly, this block eliminates horizontal or vertical > letter-boxing (or both). Is that right? If so, then I think it would be clearer > to state that explicitly in place of "avoid excessive letterboxing." No; letterboxing is the wrong term - you basically end up with large margins around the desktop, both vertically and horizontally, if it's much smaller than the client. To avoid all that wasted space, and having a tiny desktop, but to also avoid introducing blurring, we allow integer up-scaling; there may still be both vertical and horizontal borders afterward. Updated comment to be clearer. https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_session.js:1367: scale = Math.min(scaleFitHeight, scaleFitWidth); On 2015/02/06 23:06:02, Jamie wrote: > I think it would be clearer (at least with the current variable names) not to > compute the min for the width and height scale factors. In other words, it would > read: > > scaleFitWidth = <amount to scale in order to fit the width> > scaleFitHeight = <amount to scale in order to fit the height> > scale = <min of these, but no larger than we've already computed> > > Math.min can take more than two parameters in JS, so it's also shorter. Done. https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_session.js:1371: // multi-monitor combinations more intelligently. On 2015/02/06 23:06:02, Jamie wrote: > This part of the algorithm is not covered by your description, I don't think. Done. https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/unittes... File remoting/webapp/unittests/client_session_unittest.js (right): https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/unittes... remoting/webapp/unittests/client_session_unittest.js:31: // 3. Client X dimension larger than hosts, Y dimension smaller. On 2015/02/06 23:06:02, Jamie wrote: > I think you've got this the wrong way round, which means that this test and the > previous one are basically the same (the result is limited by the x-dimension in > both cases). Typo in description; Y & X got swapped.
https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/client_session.js (right): https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_session.js:1371: // multi-monitor combinations more intelligently. On 2015/02/09 21:32:28, Wez wrote: > On 2015/02/06 23:06:02, Jamie wrote: > > This part of the algorithm is not covered by your description, I don't think. > > Done. Un-done, since that breaks the multi-monitor special-case, below, which relies upon scaleFitWidth & scaleFitHeight.
https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/client_session.js (right): https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_session.js:1367: scale = Math.min(scaleFitHeight, scaleFitWidth); On 2015/02/09 21:32:28, Wez wrote: > On 2015/02/06 23:06:02, Jamie wrote: > > I think it would be clearer (at least with the current variable names) not to > > compute the min for the width and height scale factors. In other words, it > would > > read: > > > > scaleFitWidth = <amount to scale in order to fit the width> > > scaleFitHeight = <amount to scale in order to fit the height> > > scale = <min of these, but no larger than we've already computed> > > > > Math.min can take more than two parameters in JS, so it's also shorter. > > Done. I don't think you've changed this (or is your "un-done" comment meant to be here?) https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/unittes... File remoting/webapp/unittests/client_session_unittest.js (right): https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/unittes... remoting/webapp/unittests/client_session_unittest.js:31: // 3. Client X dimension larger than hosts, Y dimension smaller. On 2015/02/09 21:32:28, Wez wrote: > On 2015/02/06 23:06:02, Jamie wrote: > > I think you've got this the wrong way round, which means that this test and > the > > previous one are basically the same (the result is limited by the x-dimension > in > > both cases). > > Typo in description; Y & X got swapped. Maybe add a test for the case where it's the y-dimension that constrains the result (and perhaps one where the aspect ratio is the same, but the sizes differ)? https://codereview.chromium.org/804783002/diff/160001/remoting/webapp/unittes... File remoting/webapp/unittests/client_session_unittest.js (right): https://codereview.chromium.org/804783002/diff/160001/remoting/webapp/unittes... remoting/webapp/unittests/client_session_unittest.js:19: test('choosePluginSize() handles same-DPI client & host', s/same/low/? (since you have a test for both being high-DPI later on) https://codereview.chromium.org/804783002/diff/160001/remoting/webapp/unittes... remoting/webapp/unittests/client_session_unittest.js:44: QUnit.deepEqual(pluginSize, size(1280, (1280 / 640) * 480)); I realize it's mathematically the same, but if you're testing the "up-scale by an integer amount" code-path then the expected size would be more readable if expressed as (2* 640, 2 * 480). https://codereview.chromium.org/804783002/diff/160001/remoting/webapp/unittes... remoting/webapp/unittests/client_session_unittest.js:67: QUnit.deepEqual(pluginSize, size(640 * 3 / 2.0, 480 * 3 / 2.0)); I don't understand how 3/2 is relevant to these calculations. Is this a copy/paste error from the 150% case, below? If not, please add a clarifying comment.
kelvinp@chromium.org changed reviewers: + kelvinp@chromium.org
https://codereview.chromium.org/804783002/diff/160001/remoting/remoting_webap... File remoting/remoting_webapp_files.gypi (right): https://codereview.chromium.org/804783002/diff/160001/remoting/remoting_webap... remoting/remoting_webapp_files.gypi:181: 'webapp/unittests/client_session_unittest.js', FYI that gary and I are working on a refactoring. Much of the code that in charge of letter boxing are moving into desktop_viewport.js. https://codereview.chromium.org/918783002/ Would be great if you can call the unittest file desktop_viewport_unittest.js
On 2015/02/12 22:47:04, kelvinp wrote: > https://codereview.chromium.org/804783002/diff/160001/remoting/remoting_webap... > File remoting/remoting_webapp_files.gypi (right): > > https://codereview.chromium.org/804783002/diff/160001/remoting/remoting_webap... > remoting/remoting_webapp_files.gypi:181: > 'webapp/unittests/client_session_unittest.js', > FYI that gary and I are working on a refactoring. Much of the code that in > charge of letter boxing are moving into desktop_viewport.js. > https://codereview.chromium.org/918783002/ > Would be great if you can call the unittest file desktop_viewport_unittest.js I think it makes more sense to land this CL and then rebase and land the refactoring; the rebase should take care of moving the relevant code into desktop_viewport unless it's radically different.
Sorry; had to rebase this... https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... File remoting/webapp/crd/js/client_session.js (right): https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... remoting/webapp/crd/js/client_session.js:1367: scale = Math.min(scaleFitHeight, scaleFitWidth); On 2015/02/09 23:52:34, Jamie wrote: > On 2015/02/09 21:32:28, Wez wrote: > > On 2015/02/06 23:06:02, Jamie wrote: > > > I think it would be clearer (at least with the current variable names) not > to > > > compute the min for the width and height scale factors. In other words, it > > would > > > read: > > > > > > scaleFitWidth = <amount to scale in order to fit the width> > > > scaleFitHeight = <amount to scale in order to fit the height> > > > scale = <min of these, but no larger than we've already computed> > > > > > > Math.min can take more than two parameters in JS, so it's also shorter. > > > > Done. > > I don't think you've changed this (or is your "un-done" comment meant to be > here?) Yes, this is what got un-done. https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/unittes... File remoting/webapp/unittests/client_session_unittest.js (right): https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/unittes... remoting/webapp/unittests/client_session_unittest.js:31: // 3. Client X dimension larger than hosts, Y dimension smaller. On 2015/02/09 23:52:34, Jamie wrote: > On 2015/02/09 21:32:28, Wez wrote: > > On 2015/02/06 23:06:02, Jamie wrote: > > > I think you've got this the wrong way round, which means that this test and > > the > > > previous one are basically the same (the result is limited by the > x-dimension > > in > > > both cases). > > > > Typo in description; Y & X got swapped. > > Maybe add a test for the case where it's the y-dimension that constrains the > result Done. > (and perhaps one where the aspect ratio is the same, but the sizes > differ)? Not sure what the purpose of the latter test would be? https://codereview.chromium.org/804783002/diff/160001/remoting/remoting_webap... File remoting/remoting_webapp_files.gypi (right): https://codereview.chromium.org/804783002/diff/160001/remoting/remoting_webap... remoting/remoting_webapp_files.gypi:181: 'webapp/unittests/client_session_unittest.js', On 2015/02/12 22:47:04, kelvinp wrote: > FYI that gary and I are working on a refactoring. Much of the code that in > charge of letter boxing are moving into desktop_viewport.js. > https://codereview.chromium.org/918783002/ > Would be great if you can call the unittest file desktop_viewport_unittest.js The unit-test files all need moving anyway, and I'm about to land this, so let's take care of it after the two CLs have landed in a webapp-tests cleanup CL. https://codereview.chromium.org/804783002/diff/160001/remoting/webapp/unittes... File remoting/webapp/unittests/client_session_unittest.js (right): https://codereview.chromium.org/804783002/diff/160001/remoting/webapp/unittes... remoting/webapp/unittests/client_session_unittest.js:19: test('choosePluginSize() handles same-DPI client & host', On 2015/02/09 23:52:35, Jamie wrote: > s/same/low/? (since you have a test for both being high-DPI later on) Done. https://codereview.chromium.org/804783002/diff/160001/remoting/webapp/unittes... remoting/webapp/unittests/client_session_unittest.js:44: QUnit.deepEqual(pluginSize, size(1280, (1280 / 640) * 480)); On 2015/02/09 23:52:35, Jamie wrote: > I realize it's mathematically the same, but if you're testing the "up-scale by > an integer amount" code-path then the expected size would be more readable if > expressed as (2* 640, 2 * 480). Done. https://codereview.chromium.org/804783002/diff/160001/remoting/webapp/unittes... remoting/webapp/unittests/client_session_unittest.js:67: QUnit.deepEqual(pluginSize, size(640 * 3 / 2.0, 480 * 3 / 2.0)); On 2015/02/09 23:52:35, Jamie wrote: > I don't understand how 3/2 is relevant to these calculations. Is this a > copy/paste error from the 150% case, below? If not, please add a clarifying > comment. Done.
On 2015/02/13 18:59:08, Wez wrote: > Sorry; had to rebase this... > > https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... > File remoting/webapp/crd/js/client_session.js (right): > > https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/crd/js/... > remoting/webapp/crd/js/client_session.js:1367: scale = Math.min(scaleFitHeight, > scaleFitWidth); > On 2015/02/09 23:52:34, Jamie wrote: > > On 2015/02/09 21:32:28, Wez wrote: > > > On 2015/02/06 23:06:02, Jamie wrote: > > > > I think it would be clearer (at least with the current variable names) not > > to > > > > compute the min for the width and height scale factors. In other words, it > > > would > > > > read: > > > > > > > > scaleFitWidth = <amount to scale in order to fit the width> > > > > scaleFitHeight = <amount to scale in order to fit the height> > > > > scale = <min of these, but no larger than we've already computed> > > > > > > > > Math.min can take more than two parameters in JS, so it's also shorter. > > > > > > Done. > > > > I don't think you've changed this (or is your "un-done" comment meant to be > > here?) > > Yes, this is what got un-done. > > https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/unittes... > File remoting/webapp/unittests/client_session_unittest.js (right): > > https://codereview.chromium.org/804783002/diff/120001/remoting/webapp/unittes... > remoting/webapp/unittests/client_session_unittest.js:31: // 3. Client X > dimension larger than hosts, Y dimension smaller. > On 2015/02/09 23:52:34, Jamie wrote: > > On 2015/02/09 21:32:28, Wez wrote: > > > On 2015/02/06 23:06:02, Jamie wrote: > > > > I think you've got this the wrong way round, which means that this test > and > > > the > > > > previous one are basically the same (the result is limited by the > > x-dimension > > > in > > > > both cases). > > > > > > Typo in description; Y & X got swapped. > > > > Maybe add a test for the case where it's the y-dimension that constrains the > > result > > Done. > > > (and perhaps one where the aspect ratio is the same, but the sizes > > differ)? > > Not sure what the purpose of the latter test would be? > > https://codereview.chromium.org/804783002/diff/160001/remoting/remoting_webap... > File remoting/remoting_webapp_files.gypi (right): > > https://codereview.chromium.org/804783002/diff/160001/remoting/remoting_webap... > remoting/remoting_webapp_files.gypi:181: > 'webapp/unittests/client_session_unittest.js', > On 2015/02/12 22:47:04, kelvinp wrote: > > FYI that gary and I are working on a refactoring. Much of the code that in > > charge of letter boxing are moving into desktop_viewport.js. > > https://codereview.chromium.org/918783002/ > > Would be great if you can call the unittest file desktop_viewport_unittest.js > > The unit-test files all need moving anyway, and I'm about to land this, so let's > take care of it after the two CLs have landed in a webapp-tests cleanup CL. > > https://codereview.chromium.org/804783002/diff/160001/remoting/webapp/unittes... > File remoting/webapp/unittests/client_session_unittest.js (right): > > https://codereview.chromium.org/804783002/diff/160001/remoting/webapp/unittes... > remoting/webapp/unittests/client_session_unittest.js:19: > test('choosePluginSize() handles same-DPI client & host', > On 2015/02/09 23:52:35, Jamie wrote: > > s/same/low/? (since you have a test for both being high-DPI later on) > > Done. > > https://codereview.chromium.org/804783002/diff/160001/remoting/webapp/unittes... > remoting/webapp/unittests/client_session_unittest.js:44: > QUnit.deepEqual(pluginSize, size(1280, (1280 / 640) * 480)); > On 2015/02/09 23:52:35, Jamie wrote: > > I realize it's mathematically the same, but if you're testing the "up-scale by > > an integer amount" code-path then the expected size would be more readable if > > expressed as (2* 640, 2 * 480). > > Done. > > https://codereview.chromium.org/804783002/diff/160001/remoting/webapp/unittes... > remoting/webapp/unittests/client_session_unittest.js:67: > QUnit.deepEqual(pluginSize, size(640 * 3 / 2.0, 480 * 3 / 2.0)); > On 2015/02/09 23:52:35, Jamie wrote: > > I don't understand how 3/2 is relevant to these calculations. Is this a > > copy/paste error from the 150% case, below? If not, please add a clarifying > > comment. > > Done. Pingy.
lgtm
The CQ bit was checked by wez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/804783002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/301d65d865ed85c01f8243565f3f76b232f3953f Cr-Commit-Position: refs/heads/master@{#316353} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
