|
|
Created:
4 years, 3 months ago by Peter Kasting Modified:
4 years, 3 months ago CC:
chromium-reviews, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake cr.icon.getSupportedScaleFactors slightly closer to the .cc version.
This should only affect Android, and should, I think, have little if any visible
effect, since if the browser side can't satisfy the high-DPI version of the
request it will automatically fall back to the low version anyway.
BUG=none
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/53e7734771c2f9a2273b53f80902957c771df258
Cr-Commit-Position: refs/heads/master@{#416387}
Patch Set 1 #
Messages
Total messages: 32 (14 generated)
Description was changed from ========== Make cr.icon.getSupportedScaleFactors slightly closer to the .cc version. This should only affect Android, and should, I think, have little if any visible effect, since if the browser side can't satisfy the high-DPI version of the request it will automatically fall back to the low version anyway. BUG=none TEST=none ========== to ========== Make cr.icon.getSupportedScaleFactors slightly closer to the .cc version. This should only affect Android, and should, I think, have little if any visible effect, since if the browser side can't satisfy the high-DPI version of the request it will automatically fall back to the low version anyway. BUG=none TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by pkasting@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pkasting@chromium.org changed reviewers: + oshima@chromium.org
Do you think this is worth doing? I am not fixing any particular bug. I happened to be messing with this code, and I noticed the JS was way out of sync with ResourceBundle::InitSharedInstance(). This CL makes it slightly more in-sync. I was frightened of doing anything more drastic. I don't have an Android device, so I don't really know how to test for certain whether this is safe. It seems like this CL has downside risk without clear upside gain, except for making the two functions at least a little closer. So I'm not sure whether to proceed. I'm hoping you have a better idea.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/26 05:09:39, Peter Kasting wrote: > Do you think this is worth doing? > > I am not fixing any particular bug. I happened to be messing with this code, > and I noticed the JS was way out of sync with > ResourceBundle::InitSharedInstance(). This CL makes it slightly more in-sync. > I was frightened of doing anything more drastic. > > I don't have an Android device, so I don't really know how to test for certain > whether this is safe. It seems like this CL has downside risk without clear > upside gain, except for making the two functions at least a little closer. So > I'm not sure whether to proceed. I'm hoping you have a better idea. Thank you for cleaning up! I think this is good and harmless on Android (as Android only supports single DPR), but it's probably safer to exclude dsf=1 for android as well, and do the same on ResourceBundle side.
On 2016/08/26 21:36:19, oshima wrote: > On 2016/08/26 05:09:39, Peter Kasting wrote: > > Do you think this is worth doing? > > > > I am not fixing any particular bug. I happened to be messing with this code, > > and I noticed the JS was way out of sync with > > ResourceBundle::InitSharedInstance(). This CL makes it slightly more in-sync. > > > I was frightened of doing anything more drastic. > > > > I don't have an Android device, so I don't really know how to test for certain > > whether this is safe. It seems like this CL has downside risk without clear > > upside gain, except for making the two functions at least a little closer. So > > I'm not sure whether to proceed. I'm hoping you have a better idea. > > Thank you for cleaning up! I think this is good and harmless on Android (as > Android only supports single DPR), > but it's probably safer to exclude dsf=1 for android as well, and do the same on > ResourceBundle side. Hmm. You mean changing ResourceBundle::InitSharedInstance() to move the 100P item from the top into the "not Android or ios" case and then removing the "if !100P" condition from the end of the Android block? That feels risky to me. It means anywhere in C++ code that asks for a 1x resource will fail, won't it? What is the advantage of going this route over the opposite direction (that the current patch implements)?
(Still waiting for your reply before proceeding further here)
On 2016/09/01 22:13:31, Peter Kasting wrote: > (Still waiting for your reply before proceeding further here) Oh sorry, I missed the mail. lgtm this one, and I can update the cc side. Android does not support multiple scales (only current one), but you'r right that it can be risky, so let's change it separately and see if it sticks.
On 2016/09/02 00:32:44, oshima wrote: > On 2016/09/01 22:13:31, Peter Kasting wrote: > > (Still waiting for your reply before proceeding further here) > > Oh sorry, I missed the mail. > > lgtm this one, and I can update the cc side. Android does not support multiple > scales (only current one), but you'r right that it can be risky, so let's change > it separately and see if it sticks. Hmm, I want to make sure I'm clear. Before this change, the Chrome side uses two scale factors for Android (100 and whatever the closest supported scale is to what it thinks the DSF is) and the JS side uses one (the exact current DSF). This change would make the JS side more like Chrome today by adding 100 for Android. Your proposal was to change the Chrome side to remove 100. It seems like these move in opposite directions, so I'm not sure why we'd do both. Is landing this change really what you want?
slgtm Let's land this first to keep the logic in sync. I'll then remove 1.0f from android later. (probably after M55 branch cut) Rationale: * keeping the logic in sync is good. * Android does not support multiple scales right now, and almost most android devices are 2 or up so adding 1.0 does not make much sense. * However, adding 1.0 here is less risky than removing 1.0 in c++, so this is good choice now. * If removing 1.0 caused a problem, we can revert that CL only. * Android will support multiple scale factors, but the implementation will be different (just adding 1.0f isn't the right way anyway)
On 2016/09/02 13:43:39, oshima wrote: > slgtm > > Let's land this first to keep the logic in sync. > > I'll then remove 1.0f from android later. (probably after M55 branch cut) I meant from android js (above) /c++ (RS) code.
The CQ bit was checked by pkasting@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
pkasting@chromium.org changed reviewers: + tommycli@chromium.org
+tommycli for OWNERS
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
lgtm on the basis of oshima's lgtm
The CQ bit was checked by pkasting@chromium.org
also no objections. lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by pkasting@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Make cr.icon.getSupportedScaleFactors slightly closer to the .cc version. This should only affect Android, and should, I think, have little if any visible effect, since if the browser side can't satisfy the high-DPI version of the request it will automatically fall back to the low version anyway. BUG=none TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Make cr.icon.getSupportedScaleFactors slightly closer to the .cc version. This should only affect Android, and should, I think, have little if any visible effect, since if the browser side can't satisfy the high-DPI version of the request it will automatically fall back to the low version anyway. BUG=none TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/53e7734771c2f9a2273b53f80902957c771df258 Cr-Commit-Position: refs/heads/master@{#416387} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/53e7734771c2f9a2273b53f80902957c771df258 Cr-Commit-Position: refs/heads/master@{#416387} |