Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
317633006:
Move SnapToPhysicalPixelBoundary call to RWHVA::InternalSetBounds (Closed)
|
Created:
6 years, 6 months ago by jamesr Modified:
6 years, 6 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, jam, penghuang+watch_chromium.org, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su, miu+watch_chromium.org, scottmg Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMove SnapToPhysicalPixelBoundary call to RWHVA::InternalSetBounds
RWHVAura::SetBounds isn't actually called in most situations, so the snapping
wasn't occuring when it should. ::SetSize is actually called when the bounds
of the RWHVA changes and both call ::InternalSetBounds(), so this calls the
snapping logic there.
BUG=370074
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275994
Patch Set 1 #Patch Set 2 : store subpixel offset in ui::Layer #Patch Set 3 : rebase #
Messages
Total messages: 29 (0 generated)
I broke the previous patch locally at some point by moving the caller to ::SetBounds() since that looked like a better place to put it (since I only care about changes in the views' offset in global space, not resizes), but it's not actually called when the view moves around :\. This works much better. If there's a notification for "your view or some ancestor of your view moved" then that'd be even better.
On 2014/06/04 22:31:48, jamesr wrote: > I broke the previous patch locally at some point by moving the caller to > ::SetBounds() since that looked like a better place to put it (since I only care > about changes in the views' offset in global space, not resizes), but it's not > actually called when the view moves around :\. This works much better. If > there's a notification for "your view or some ancestor of your view moved" then > that'd be even better. There is an observer notification for your window moving (WindowObserver::OnBoundsChanged). There's not a notification for an ancestor moving, but I'm not sure you need one - the browser view layout will always cause your window to be resized.
On 2014/06/05 15:52:07, Ben Goodger (Google) wrote: > On 2014/06/04 22:31:48, jamesr wrote: > > I broke the previous patch locally at some point by moving the caller to > > ::SetBounds() since that looked like a better place to put it (since I only > care > > about changes in the views' offset in global space, not resizes), but it's not > > actually called when the view moves around :\. This works much better. If > > there's a notification for "your view or some ancestor of your view moved" > then > > that'd be even better. > > There is an observer notification for your window moving > (WindowObserver::OnBoundsChanged). There's not a notification for an ancestor > moving, but I'm not sure you need one - the browser view layout will always > cause your window to be resized. The "missing" calls to OnBoundsChanged are coming from cases where (for whatever reason) the aura::Window associated with the WebContentsViewAura gets a bounds change and informs the RWHVA of the change via a ::SetSize() call, but the OnBoundsChanged() notification either doesn't happen at all for the RWHVA or happens before everything has stopped moving. PS#2 moves the snap() call into RWHVA::InternalSetBounds(), which is always called, and also changes how the subpixel offset is stored to be robust to swapping out the cc::Layer type underlying the ui::Layer.
I don't think this helps us with switching out the cc layer (we'll reuse the old cc layer's position in that case), but it will make us preserve the offset when the DSF changes and when the layer's transform animates. Can you make sure we would recompute the offset correctly at the end of a transform animation? Can we add some tests for this to render_widget_host_view_aura_unittest?
On 2014/06/06 15:03:57, danakj wrote: > I don't think this helps us with switching out the cc layer (we'll reuse the old > cc layer's position in that case) right, that is the desired behavior. The position (including subpixel offset) from the previous cc_layer will still apply to the new cc_layer. > > Can you make sure we would recompute the offset correctly at the end of a > transform animation? The offset is recomputed whenever aura redoes layout, which happens at the end of animations. > > Can we add some tests for this to render_widget_host_view_aura_unittest? I can look into this. We need to get a build that isn't blurry to QA so they can start hammering on it for m37, though.
On 2014/06/06 20:41:49, jamesr wrote: > On 2014/06/06 15:03:57, danakj wrote: > > I don't think this helps us with switching out the cc layer (we'll reuse the > old > > cc layer's position in that case) > > right, that is the desired behavior. The position (including subpixel offset) > from the previous cc_layer will still apply to the new cc_layer. > > > > > Can you make sure we would recompute the offset correctly at the end of a > > transform animation? > > The offset is recomputed whenever aura redoes layout, which happens at the end > of animations. > > > > > Can we add some tests for this to render_widget_host_view_aura_unittest? > > I can look into this. We need to get a build that isn't blurry to QA so they > can start hammering on it for m37, though. OK LGTM w/ hope for some unit tests :)
lgtm
The CQ bit was checked by jamesr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamesr@chromium.org/317633006/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamesr@chromium.org/317633006/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was checked by jamesr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamesr@chromium.org/317633006/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamesr@chromium.org/317633006/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was checked by jamesr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamesr@chromium.org/317633006/40001
Message was sent while issue was closed.
Change committed as 275994 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
