Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2472133002:
Pass min/max page scale to compositor from WebFrameWidget. (Closed)
DescriptionPass min/max page scale to compositor from WebFrameWidget.
Remove a FIXME by plumbing the min/max page scale factor from the
WebView into the WebFrameWidget's layerTreeView.
This CL is in preparation for plumbing the page scale into the
layer tree for subframes, in order that the raster scale can be
set properly. The page scale for subframes should remain a 1.f.
BUG=645917
Committed: https://crrev.com/4d4ff5f9774061cd48d7941596e2c58cd1b5dbe1
Cr-Commit-Position: refs/heads/master@{#429624}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add comment. #
Depends on Patchset: Messages
Total messages: 22 (11 generated)
The CQ bit was checked by wjmaclean@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...
Description was changed from ========== Pass min/max page scale to compositor from WebFrameWidget. Remove a FIXME by plumbing the min/max page scale factor from the WebView into the WebFrameWidget's layerTreeView. BUG=none ========== to ========== Pass min/max page scale to compositor from WebFrameWidget. Remove a FIXME by plumbing the min/max page scale factor from the WebView into the WebFrameWidget's layerTreeView. BUG=none ==========
wjmaclean@chromium.org changed reviewers: + bokan@chromium.org
bokan@ - one-liner, PTAL?
https://codereview.chromium.org/2472133002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): https://codereview.chromium.org/2472133002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:265: 1, view()->minimumPageScaleFactor(), view()->maximumPageScaleFactor()); Is it ok that we'll be clobbering the page scale factor with 1 here?
Description was changed from ========== Pass min/max page scale to compositor from WebFrameWidget. Remove a FIXME by plumbing the min/max page scale factor from the WebView into the WebFrameWidget's layerTreeView. BUG=none ========== to ========== Pass min/max page scale to compositor from WebFrameWidget. Remove a FIXME by plumbing the min/max page scale factor from the WebView into the WebFrameWidget's layerTreeView. BUG=645917 ==========
On 2016/11/03 14:55:50, bokan wrote: > https://codereview.chromium.org/2472133002/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): > > https://codereview.chromium.org/2472133002/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:265: 1, > view()->minimumPageScaleFactor(), view()->maximumPageScaleFactor()); > Is it ok that we'll be clobbering the page scale factor with 1 here? Yes. Subframes don't set a PageScaleFactor ever (it's only on the main frame). So this value always was 1.
On 2016/11/03 15:02:05, wjmaclean wrote: > On 2016/11/03 14:55:50, bokan wrote: > > > https://codereview.chromium.org/2472133002/diff/1/third_party/WebKit/Source/w... > > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): > > > > > https://codereview.chromium.org/2472133002/diff/1/third_party/WebKit/Source/w... > > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:265: 1, > > view()->minimumPageScaleFactor(), view()->maximumPageScaleFactor()); > > Is it ok that we'll be clobbering the page scale factor with 1 here? > > Yes. Subframes don't set a PageScaleFactor ever (it's only on the main frame). > So this value always was 1. In that case, why do they need the min/max values?
On 2016/11/03 15:08:40, bokan wrote: > On 2016/11/03 15:02:05, wjmaclean wrote: > > On 2016/11/03 14:55:50, bokan wrote: > > > > > > https://codereview.chromium.org/2472133002/diff/1/third_party/WebKit/Source/w... > > > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): > > > > > > > > > https://codereview.chromium.org/2472133002/diff/1/third_party/WebKit/Source/w... > > > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:265: 1, > > > view()->minimumPageScaleFactor(), view()->maximumPageScaleFactor()); > > > Is it ok that we'll be clobbering the page scale factor with 1 here? > > > > Yes. Subframes don't set a PageScaleFactor ever (it's only on the main frame). > > So this value always was 1. > > In that case, why do they need the min/max values? This CL is the first in a list of CLs to make subframes re-raster at the correct scale. Since subframes only need page scale for rastering, we will be sending the value via a different route, but the min/max already have a well-defined route, and we will need them.
On 2016/11/03 15:12:44, wjmaclean wrote: > On 2016/11/03 15:08:40, bokan wrote: > > On 2016/11/03 15:02:05, wjmaclean wrote: > > > On 2016/11/03 14:55:50, bokan wrote: > > > > > > > > > > https://codereview.chromium.org/2472133002/diff/1/third_party/WebKit/Source/w... > > > > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2472133002/diff/1/third_party/WebKit/Source/w... > > > > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:265: 1, > > > > view()->minimumPageScaleFactor(), view()->maximumPageScaleFactor()); > > > > Is it ok that we'll be clobbering the page scale factor with 1 here? > > > > > > Yes. Subframes don't set a PageScaleFactor ever (it's only on the main > frame). > > > So this value always was 1. > > > > In that case, why do they need the min/max values? > > This CL is the first in a list of CLs to make subframes re-raster at the correct > scale. Since subframes only need page scale for rastering, we will be sending > the value via a different route, but the min/max already have a well-defined > route, and we will need them. Ok, please add a comment to that effect. lgtm.
Description was changed from ========== Pass min/max page scale to compositor from WebFrameWidget. Remove a FIXME by plumbing the min/max page scale factor from the WebView into the WebFrameWidget's layerTreeView. BUG=645917 ========== to ========== Pass min/max page scale to compositor from WebFrameWidget. Remove a FIXME by plumbing the min/max page scale factor from the WebView into the WebFrameWidget's layerTreeView. This CL is in preparation for plumbing the page scale into the layer tree for subframes, in order that the raster scale can be set properly. The page scale for subframes should remain a 1.f. BUG=645917 ==========
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2472133002/#ps20001 (title: "Add comment.")
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.
Description was changed from ========== Pass min/max page scale to compositor from WebFrameWidget. Remove a FIXME by plumbing the min/max page scale factor from the WebView into the WebFrameWidget's layerTreeView. This CL is in preparation for plumbing the page scale into the layer tree for subframes, in order that the raster scale can be set properly. The page scale for subframes should remain a 1.f. BUG=645917 ========== to ========== Pass min/max page scale to compositor from WebFrameWidget. Remove a FIXME by plumbing the min/max page scale factor from the WebView into the WebFrameWidget's layerTreeView. This CL is in preparation for plumbing the page scale into the layer tree for subframes, in order that the raster scale can be set properly. The page scale for subframes should remain a 1.f. BUG=645917 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Pass min/max page scale to compositor from WebFrameWidget. Remove a FIXME by plumbing the min/max page scale factor from the WebView into the WebFrameWidget's layerTreeView. This CL is in preparation for plumbing the page scale into the layer tree for subframes, in order that the raster scale can be set properly. The page scale for subframes should remain a 1.f. BUG=645917 ========== to ========== Pass min/max page scale to compositor from WebFrameWidget. Remove a FIXME by plumbing the min/max page scale factor from the WebView into the WebFrameWidget's layerTreeView. This CL is in preparation for plumbing the page scale into the layer tree for subframes, in order that the raster scale can be set properly. The page scale for subframes should remain a 1.f. BUG=645917 Committed: https://crrev.com/4d4ff5f9774061cd48d7941596e2c58cd1b5dbe1 Cr-Commit-Position: refs/heads/master@{#429624} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4d4ff5f9774061cd48d7941596e2c58cd1b5dbe1 Cr-Commit-Position: refs/heads/master@{#429624}
Message was sent while issue was closed.
On 2016/11/03 17:13:21, commit-bot: I haz the power wrote: > Patchset 2 (id:??) landed as > https://crrev.com/4d4ff5f9774061cd48d7941596e2c58cd1b5dbe1 > Cr-Commit-Position: refs/heads/master@{#429624} For completeness: the bug you link from the CL is not the right bug :) Would you mind updating it in the description?
Message was sent while issue was closed.
Description was changed from ========== Pass min/max page scale to compositor from WebFrameWidget. Remove a FIXME by plumbing the min/max page scale factor from the WebView into the WebFrameWidget's layerTreeView. This CL is in preparation for plumbing the page scale into the layer tree for subframes, in order that the raster scale can be set properly. The page scale for subframes should remain a 1.f. BUG=645917 Committed: https://crrev.com/4d4ff5f9774061cd48d7941596e2c58cd1b5dbe1 Cr-Commit-Position: refs/heads/master@{#429624} ========== to ========== Pass min/max page scale to compositor from WebFrameWidget. Remove a FIXME by plumbing the min/max page scale factor from the WebView into the WebFrameWidget's layerTreeView. This CL is in preparation for plumbing the page scale into the layer tree for subframes, in order that the raster scale can be set properly. The page scale for subframes should remain a 1.f. BUG=645917 Committed: https://crrev.com/4d4ff5f9774061cd48d7941596e2c58cd1b5dbe1 Cr-Commit-Position: refs/heads/master@{#429624} ==========
Message was sent while issue was closed.
On 2016/11/03 17:33:11, jkrcal wrote: > On 2016/11/03 17:13:21, commit-bot: I haz the power wrote: > > Patchset 2 (id:??) landed as > > https://crrev.com/4d4ff5f9774061cd48d7941596e2c58cd1b5dbe1 > > Cr-Commit-Position: refs/heads/master@{#429624} > > For completeness: the bug you link from the CL is not the right bug :) Would you > mind updating it in the description? Sorry, I transposed two of the digits. It's corrected in the codereview .... I'm not sure if there's any way to amend the commit message in the shared repo ... is there? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
