Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2262323002:
Update viewport_to_dip_scale on bound compositor host (Closed)
|
Created:
4 years, 4 months ago by malaykeshav Modified:
4 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate viewport_to_dip_scale on bound compositor host
Fix for bug that caused the viewport_to_dip_scale not being updated
when a new compositor host was being instantiated.
BUG=633806
COMPONENT=Pepper, Chrome OS
Committed: https://crrev.com/a9af1f7989315eda1c55b490146945c341374d54
Cr-Commit-Position: refs/heads/master@{#413972}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 17 (10 generated)
The CQ bit was checked by malaykeshav@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...
malaykeshav@chromium.org changed reviewers: + oshima@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Update viewport_to_dip_scale on bound compositor host Fix for bug that caused the viewport_to_dip_scale not being updated when a new compositor host was being instantiated. BUG=633806 COMPONENT=Pepper, Chrome OS ========== to ========== Update viewport_to_dip_scale on bound compositor host Fix for bug that caused the viewport_to_dip_scale not being updated when a new compositor host was being instantiated. BUG=633806 COMPONENT=Pepper, Chrome OS ==========
oshima@chromium.org changed reviewers: + bbudge@chromium.org
https://codereview.chromium.org/2262323002/diff/1/content/renderer/pepper/pep... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/2262323002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_plugin_instance_impl.cc:2352: if (compositor->BindToInstance(this)) { +bbudge@ (owner) I feel it's better to do this in BindToInstance, but I'd follow the owner's recommendation.
lgtm https://codereview.chromium.org/2262323002/diff/1/content/renderer/pepper/pep... File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): https://codereview.chromium.org/2262323002/diff/1/content/renderer/pepper/pep... content/renderer/pepper/pepper_plugin_instance_impl.cc:2352: if (compositor->BindToInstance(this)) { On 2016/08/23 22:25:49, oshima wrote: > +bbudge@ (owner) > > I feel it's better to do this in BindToInstance, but I'd follow the owner's > recommendation. I might not understand the comment, but this seems like the only way, since the compositor might not have a viewport_to_dip_scale_ value set. Also, this class makes another call to set_viewport_to_dip_scale when handling view changes: https://cs.chromium.org/chromium/src/content/renderer/pepper/pepper_plugin_in...
The CQ bit was checked by malaykeshav@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/23 at 23:32:39, bbudge wrote: > lgtm > > https://codereview.chromium.org/2262323002/diff/1/content/renderer/pepper/pep... > File content/renderer/pepper/pepper_plugin_instance_impl.cc (right): > > https://codereview.chromium.org/2262323002/diff/1/content/renderer/pepper/pep... > content/renderer/pepper/pepper_plugin_instance_impl.cc:2352: if (compositor->BindToInstance(this)) { > On 2016/08/23 22:25:49, oshima wrote: > > +bbudge@ (owner) > > > > I feel it's better to do this in BindToInstance, but I'd follow the owner's > > recommendation. > > I might not understand the comment, but this seems like the only way, since the compositor might not have a viewport_to_dip_scale_ value set. Also, this class makes another call to set_viewport_to_dip_scale when handling view changes: > > https://cs.chromium.org/chromium/src/content/renderer/pepper/pepper_plugin_in... I think BindToInstance https://cs.chromium.org/chromium/src/content/renderer/pepper/pepper_graphics_... should also work and would be a safer alternative. In case that is required I can create another patch.
Message was sent while issue was closed.
Description was changed from ========== Update viewport_to_dip_scale on bound compositor host Fix for bug that caused the viewport_to_dip_scale not being updated when a new compositor host was being instantiated. BUG=633806 COMPONENT=Pepper, Chrome OS ========== to ========== Update viewport_to_dip_scale on bound compositor host Fix for bug that caused the viewport_to_dip_scale not being updated when a new compositor host was being instantiated. BUG=633806 COMPONENT=Pepper, Chrome OS ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Update viewport_to_dip_scale on bound compositor host Fix for bug that caused the viewport_to_dip_scale not being updated when a new compositor host was being instantiated. BUG=633806 COMPONENT=Pepper, Chrome OS ========== to ========== Update viewport_to_dip_scale on bound compositor host Fix for bug that caused the viewport_to_dip_scale not being updated when a new compositor host was being instantiated. BUG=633806 COMPONENT=Pepper, Chrome OS Committed: https://crrev.com/a9af1f7989315eda1c55b490146945c341374d54 Cr-Commit-Position: refs/heads/master@{#413972} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/a9af1f7989315eda1c55b490146945c341374d54 Cr-Commit-Position: refs/heads/master@{#413972} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
