On HiDPI - Use of layer masks blurs extension browser-action popups - also Omnibox popup blurred |
||||||||||||||||||||||
Issue descriptionThis is essentially a copy of crbug.com/824963 . I'm opening a new one to not confuse the current RBS labels etc. When layer masks are turned on for inkdrops on Windows the browser-action popups get blurred. The hand-wavy hypothesis is that turning layer masks on triggers a rendering path where the original resolution is used as a render target instead of using the HDPI one. We need inkdrop layer masks in Refresh, so this blocks Refresh or any newer material UI on Windows. I'll turn the masks on for IsNewerMaterialUi, but we need this resolved before getting it out to a wider audience.
,
May 17 2018
Tested the issue on latest chrome version 68.0.3433.0 w.r.to the issue:824963 mentioned in comment#0, launched chrome version and installed 'dark reader' extension, clicked on extension icon, Observed "Text, images, borders have sharp edges" and it is working as intended. Please find the attached screenshot for your reference and let us know if we need to follow any alternate steps in confirming the fix. Thanks!
,
May 17 2018
Hi, this isn't fixed so we haven't turned on the switch that previously would've made the extension blurry. Nothing to verify (and no easy way to invoke broken behavior).
,
May 31 2018
Triage: Assigning to pbos@ for layer mask work. pbos: Please update the EstimatedDays to give a feel on how expensive this will be to fix. Thanks!
,
Jun 5 2018
I can't reasonably guess as I barely know where to start. Is there anyone we can loop in who can ballpark why this would happen?
,
Jun 6 2018
This looks suspicious:
void PictureLayerImpl::AppendQuads(..) {
...
if (raster_source_->IsSolidColor()) {
// TODO(sunxd): Solid color non-mask layers are forced to have contents
// scale = 1. This is a workaround to temperarily fix
// https://crbug.com/796558.
// We need to investigate into the ca layers logic and remove this
// workaround after fixing the bug.
float max_contents_scale =
!(mask_type_ == Layer::LayerMaskType::MULTI_TEXTURE_MASK)
? 1
: CanHaveTilings() ? ideal_contents_scale_
: std::min(kMaxIdealContentsScale,
std::max(GetIdealContentsScale(),
MinimumContentsScale()));
,
Jun 6 2018
Note there's MULTI_TEXTURE_MASK and SINGLE_TEXTURE_MASK . But I *think* LayerTreeSettings::enable_mask_tiling is always true (so all masks are "MULTI") Note, I'm not saying I think there's a bug in that code - I don't really know this cc code well enough to say - but it might be around this place where "stuff" needs to be tweaked. And hopefully sunxd has the expertise to help :)
,
Jun 6 2018
The workaround in comment #6 is meant to fix blurry masked contents on high-dpi mac. While I don't understand how this could possibly lead to blurry result, we can probably check if the bug still persists when turning off enable_mask_tiling (mentioned in #7). If the flag is off, all mask layer will be "SINGLE_TEXTURE_MASK" while if the flag is on, all mask layer will be "MULTI_TEXTURE_MASK" unless the masked element has a filter.
,
Jun 6 2018
The bug seems orthogonal to enable_mask_tiling. To repro now (at r564967): Remove the Windows-specific disable code in NativeViewHostAura *and* remove a border->set_md_shadow_elevation() call in BubbleDialogDelegateView::CreateNonClientFrameView(). Then use >100% DPI and check your extensions' browser action popups. For some reason, setting md_shadow_elevation makes the problem go away. The content inside is rendered at the correct resolution and no upscale happens. The BubbleBorder created likely makes the content inside use a different rendering path. If that rings a bell or even remotely makes sense to you so far, or you have further tips on what to look out for, please speak up. :)
,
Jun 6 2018
I'm not very familiar with bubble dialog, asking for help from paint folks. Hi trchen@, enne@ and chrishtr@, do you have any opinions on this windows mask layer blurry bug?
,
Jun 6 2018
So, really hoping this will ring some sort of bell. This boils down to enabling dialog corner radius on Windows and using low values (<31 on my system) for BubbleBorder ShadowValue blur(). Attaching screenshot with 30 and 31 as blur values. This seems to trigger a different render path (blur-related optimization?) than >=31 blur at which the HDPI scale is not correctly taken into account when rendering the bubble content. Any other flags to try than enable_mask_tiling or brute force hammers to disable other optimizations? The ShadowValues parameter being used as the DrawLooper here afaict: https://cs.chromium.org/chromium/src/ui/views/bubble/bubble_border.cc?l=463&rcl=1cecab463353bdbb4ad45bef32d5f7b0f6c3663c Note that this also requires enabling chrome://flags -> top-chrome-md = Refresh for these shadows and rounded corners to be in use. Patch that hopefully helps explain what I changed to get blur / non-blur. Correct rendering is >=31 for the ambient shadow blur instead of 30: diff --git a/ui/gfx/shadow_value.cc b/ui/gfx/shadow_value.cc index b2487651ec9a..4c5b666fca5a 100644 --- a/ui/gfx/shadow_value.cc +++ b/ui/gfx/shadow_value.cc @@ -81,7 +81,7 @@ ShadowValues ShadowValue::MakeRefreshShadowValues(int elevation) { case 3: { ShadowValue key = {gfx::Vector2d(0, 1), 12, SkColorSetA(shadow_base_color, 0x66)}; - ShadowValue ambient = {gfx::Vector2d(0, 4), 64, + ShadowValue ambient = {gfx::Vector2d(0, 4), 30, SkColorSetA(shadow_base_color, 0x40)}; return {key, ambient}; } diff --git a/ui/views/controls/native/native_view_host_aura.cc b/ui/views/controls/native/native_view_host_aura.cc index 097bcd4879ba..420b005a4335 100644 --- a/ui/views/controls/native/native_view_host_aura.cc +++ b/ui/views/controls/native/native_view_host_aura.cc @@ -149,18 +149,12 @@ void NativeViewHostAura::RemovedFromWidget() { } bool NativeViewHostAura::SetCornerRadius(int corner_radius) { -#if defined(OS_WIN) - // TODO(crbug/843250): On Aura, layer masks don't play with HiDPI. Fix this - // and enable this on Windows. - return false; -#else mask_ = views::Painter::CreatePaintedLayer( views::Painter::CreateSolidRoundRectPainter(SK_ColorBLACK, corner_radius)); mask_->layer()->SetFillsBoundsOpaquely(false); InstallMask(); return true; -#endif }
,
Jun 7 2018
In case I just threw someone into a wild goose chase. This latest <31 and >=31 distinction is probably a red herring. I tried using 57 as blur and that has content-blur issues as well. This might not be a layer-mask problem at all. bsep@ pointed out that this may be caused by content alignment on pixel boundaries and not necessarily be bilinear upscale at all. Lightly edited chat for whoever's interested or notes for me next week. pbos: Why would blur + corner radii be related to pixel alignment though? Isn't the area positioned the same regardless? bsep: not necessarily the corner radius affected the bubble size (perhaps unintentionally), and the shadow affects the Widget size (if not the actual bubble size) if what I said above is correct, changing the size may have made it go from odd to even, or even to odd, and thus affect how rounding works so if before it was, for example, 300 pixels it would scale up to 450 pixels, no problem, but if it was 301 pixels it will scale up to 451.5 pixels, and then not be aligned to the pixel grid pbos: do we need to LHS pad the blur in size to align on pixel grid? bsep: I don't know. it depends on the specifics of the problem you might just have to find where the widget is being positioned, and round the origin so its origin is always at a whole pixel value, basically pbos: I don't know the rendering stack enough. Windows can't do subpixel window placements for sure. bsep: I would start at Widget::Init and see what the initial bounds are and how they change between repro and non-repro cases pbos: Are widget bounds with or without shadows? bsep: then use spyxx to see if they have different bounds according to Windows with shadows a Widget can't draw outside its own bounds if that digging isn't illuminating, you may have to debug into Skia and see what actual rectangles it's drawing after DPI scale is applied
,
Jun 11 2018
Triage: update estimated days please. How much work is left?
,
Jun 11 2018
Shot in the dark, we still haven't root caused it so it's really just a guess.
,
Jun 12 2018
bsep@'s hunch was right, this is related to bubble bounds. [bubble border][bubble content][bubble border] If the LHS border width (or top-side height) * HDPI is not an integer value, the bubble content will not be aligned to pixels after HDPI is applied. Changing BubbleBorder::GetBorderAndShadowInsets() to return top/left insets as multiples of 4 fixes this for Windows' HDPI modes (125%, 150%...). This makes sense as the widget content will only be displaced by an integer number of pixels. sky@: Is changing BubbleBorder insets here reasonable, or could you suggest another place where we can make sure the bubble content is aligned pixel boundaries post-HDPI? Snippet of related patch that fixes content alignment, but it looks super hacky: insets.Set((insets.top() + 3) / 4 * 4, (insets.left() + 3) / 4 * 4, insets.bottom(), insets.right());
,
Jun 12 2018
Sorry, I can't quite tell from the comments, what is this the specific bug you're trying to fix? Is the content in its own view? You could try forcing it to have a layer, which should trigger snapping to a pixel boundary.
,
Jun 12 2018
Hope this makes more sense: Content inside a bubble (extension browser actions, new omnibox popup, for instance) isn't explicitly aligning to pixel boundaries. I believe the bubble itself (NonClientFrameView?) is pixel aligned. When the {bubble border * hdpi scale} is not an integer width the content is rendered out of alignment (screenshot attached). I assume this means that the NonClientFrameView is pixel aligned on its left-hand side and the client view is implicitly placed based on border insets.
The repro for this on Windows is:
1: Enable NativeViewHostAura::SetCornerRadius (remove OS_WIN ifdef block) so a layer mask is created.
2: Make sure that the created layer mask is not a multiple of 4 wide (running with a default top-chrome-md seems enough).
Is there anything we can do on an Aura level to make sure the client view is positioned on pixel boundaries instead of the NonClientFrameView? Padding insets in BubbleBorder to multiples of 4 seems more like an off-by-one fix (which assumes x * 25% HDPI). Pixel aligning the content seems more important than pixel aligning the left / top sides of the border.
,
Jun 12 2018
I'm not entirely sure what the issue is. No doubt that views/aura is dips makes this painful to figure out where things are going wrong. My suspicion is, this bug is not windows specific, but rather specific to hi-dpi. Maybe the mask layer and/or the NativeViewHost need to use SnapLayerToPhysicalPixelBoundary.
,
Jun 12 2018
I think that's correct, this is likely a HDPI bug on all Aura platforms (and busted on Linux). I got this bug down to the call of: cc_layer_->SetMaskLayer(layer_mask ? layer_mask->cc_layer_ : nullptr); inside Layer::SetMaskLayer. From there it's really hard for me to trace down what actually happens to the layer mask. I assume GPU compositing takes over from there somewhere. I couldn't figure out good candidate layers to SnapLayerToPhysicalPixelBoundary on ((this, layer_mask_) or vice versa doesn't work as they're not in the same parent/child hierarchy, which makes this DCHECK and error out). I also tried combinations of SetSubpixelPositionOffset(layer_mask->subpixel_position_offset()); and layer_mask_->SetSub... in compositor land as a shot in the dark. Neither helped. I'm inclined to go ahead with padding the BubbleBorder by 0-3dp as a workaround to be able to turn on NativeViewHostAura::SetCornerRadius without regressions on Windows (where HDPI multiples of 25% are the norm). Finding a proper fix for this right now would require a significant amount of additional investment or help from someone who knows these things better.
,
Jun 13 2018
+piman We can also reproduce this on Omnibox popup with the following steps. 0. Turn your Windows machine to 125% scaling in Display settings. 1. On Windows Canary (it's the most obvious on Windows because the fonts are not anti-aliased). 2. Turn on chrome://flags/#upcoming-ui-features 3. Maximize the window and use the Omnibox. Notice the popup contents are NOT blurred, but look fine. See Maximized.PNG. 4. Now make the window "windowed" and move it somewhere arbitrary on the screen. Use the Omnibox now again. The text is somewhat blurred now. See WindowedBlurred.PNG. pbos@'s theory is that somewhere within the cc or gfx layer, the 125% scaling is causing the text contents to be no longer pixel aligned and thus blurry. It seems to be related to the MaskLayer, but at this point I think it would be most productive for someone from gfx or cc to take a look. Would you be able to bounce this bug to the resident gfx or cc HiDPI person. Thanks!
,
Jun 13 2018
Note: If you view the above two screenshots in HiDPI mode, they will both appear blurred. View them at 100% scaling for best results.
,
Jun 13 2018
Another note is that this affects other Aura platforms too, but it's most noticeable on Windows, since Windows fonts are not anti-aliased.
,
Jun 13 2018
Note that the Omnibox popup blurs based on Chrome browser window location, while the extension browser-action popups blurs regardless of window positioning. The extension blur requires enabling NativeViewHostAura::SetCornerRadius() on Windows (and using default chrome://flags, not top-chrome-md=Refresh or upcoming-ui-features).
,
Jun 13 2018
I believe Bret has a patch out to revert the use of layer mask. If that's the case, should the priority be lowered? Here's the patch I'm referring to: https://chromium-review.googlesource.com/c/chromium/src/+/1098376 .
,
Jun 13 2018
sky: The Omnibox popup will continue to use layer masks despite that patch, so it would still be good to fix it. I agree that it's a 2 though.
,
Jun 13 2018
Issue 850991 has been merged into this issue.
,
Jun 13 2018
Misaligned layers will cause blurriness indeed. I could believe that if the mask isn't aligned things like this could happen? Dana (or Wei), could you help providing guidance about how to snap the right layers?
,
Jun 14 2018
MaskLayers hang off on the side and don't really participate in usual draw property calculation code path. Will need to check the code to see where is best place to try snap them.
,
Jun 14 2018
PictureLayers snap to pixels when doing raster, thanks to trchen@'s work. Can take a look at his commit history in cc/ to see that. So maybe the painting is being done off the pixel grid, not the compositing?
,
Jun 15 2018
I was reading the code for using a raster offset, and I think it won't work in the single threaded compositor.
It looks like PictureLayerImpl's use_transformed_rasterization_ is set by the pending tree layer's PushPropertiesTo():
layer_impl->SetUseTransformedRasterization(
ShouldUseTransformedRasterization());
That's the only call to ShouldUseTransformedRasterization(). But in single thread there is no pending layer so this function never happens.
Instead the offset_to_transform_parent() should be pushed through to the active layer, and it should make that decision independently. Then it works without a pending layer.
This should be verifiable if its related to this bug by setting use_transformed_rasterization_ on the blurry layer.
,
Jun 15 2018
Ah, also noting that PropertyTreeBuilder doesn't set offset_to_transform_parent(), only blink sets that. Maybe it would do the right thing if it's always 0, based on the screen space transform alone, I'm not certain.
,
Jun 15 2018
Ignore #32, code search indexing is broken.
,
Jun 15 2018
To keep a paper trail and remember what I looked at this Friday (today): We hard coded in use_transformed_rasterization_ everywhere to no avail (still fuzzy) and set breakpoints in PictureLayerImpl::CalculateRasterTranslation, which was definitely called after this. All observed return values are either (0,0) or very close to integers (0, 0.999962 or so). No {0.25, 0.50, 0.75} translations observed here.
,
Jun 26 2018
Issue 779401 has been merged into this issue.
,
Jun 26 2018
More info from offline discussion with @trchen and the duplicate bug thread: Adding a mask results in the layer having its own render surface. which means "all layers in the subtree must first draw into that render surface, then that render surface will have mask applied while drawing into its target render surface. And the fuzziness are from the resampling errors during the draw, which is due to a sampling space mismatch. The render surface uses the local space of its primary layer as its sample space, i.e. so its primary layer's raster space always match the surface. But then that means the surface won't match its target surface's space. For example, if a layer is located at (50.5, 50.5), then the draw transform of the surface will be TransformationMatrix.Translate(50.5, 50.5), while the layer's draw transform will be identity. To have everything lined up perfectly, you want only the layer to RASTER in a translated space, while everything DRAWN perfectly aligned. i.e. the surface should have a draw transform of Translate(50, 50), and the layer's draw transform being identity, and the raster transform begin Translate(0.5, 0.5).
,
Jun 26 2018
I tried rounding the draw transforms translation component and got positive results. However the result is still not 100% sharp. I am guessing this is due to the missing subpixel offset correction in the raster transform. (@trchen). Attaching screenshots.
,
Jun 27 2018
As said before, picture layers snap to pixels and then raster thanks to trchen's work. However, I am skeptical about applying this technique to render surfaces. They can have filters on them (like drop shadows), and these are expected to be applied around the rectangle of the render surface. I think this won't work properly with filters. For example, if you have a render surface with a drop shadow that's (0.5, 0.5, 10, 10), with a (0, 0, 10, 10) layer inside, it's expected that the drop shadow is flush with the rectangle. If you followed the technique to align the render surface, you would need to make the render surface (0, 0, 11, 11) here, and the drop shadow would be off by half a pixel. You'd expect the drop shadow to be blended with the last pixel of content from the render surface in the final frame buffer, but instead there will be a larger gap and it'll look not quite right in a different way, in my opinion. Ignoring that, I don't doubt that we could implement such an offset, but I'm skeptical about the engineering effort / bugs / etc to do so. Correct me if I'm wrong, but it seems to me that the UI here isn't intended to be at a half pixel offset, and that it would be acceptable to snap everything to device pixels if we could. If so, I would propose a simpler solution here, which is to modify and expose Layer::IsSnappedToPixelGridInTarget. Make this a boolean getter/setter on layer instead of a virtual function that's only true for certain kinds of layers. We already support code to snap layers to device pixels, and it seems like this would be a way for UI to request that certain layers are always sharp. One way to prototype this would be to just return true from IsSnappedToPixelGridInTarget always and see if that fixes the issue.
,
Jun 27 2018
,
Jun 28 2018
Do we need this feature for MdRefresh?
,
Jun 28 2018
#39 I have been working on a patch and its almost done except for a couple of corner cases that are unlikely to happen soon. One of them being, if we set IsSnappedToPixelGridInTarget for a layer A, that means we would have to set that for all children of A that have some offset from A. Am I correct in that? For the given bug there is not such child of A with offset. So its working fine.
,
Jun 28 2018
Please file bugs on corner cases so we can track!
,
Jun 28 2018
I think it is sort of up to the creator of the layers what they want. If you have a parent at a non-integer offset and children at an integer offset from the parent, if the parent is snapped then the children don't need to be offset either. I could see an argument that everything in UI should be snapped, though.
,
Jun 28 2018
robliao@: I think so, without it the omnibox popup layer gets blurred in high-DPI as text in the omnibox effectively seems to end up not aligned to pixel bounds. Other surfaces I'd say it's less urgent but the omnibox would be a very prominent regression.
,
Jun 28 2018
I found the bug on the client side that is causing this. (Thanks @trchen for taking the time to explain things in detail!
It took me a while to understand what was going on.)
On the ui compositor side we align subpixel offsets for layers (windows) to prevent the blurriness.
In the case of the popup extension, we have a layer (window) hierarchy of:
R (Root Render Surface)
A (Snapped to root render surface)
B (Not snapped because layer is empty)
C (Not snapped because layer is empty)
D (Snapped to A)
When there is no mask, the window A is snapped to R and the layer D is snapped to A. Layers B and C are skipped
and not snapped because they are not drawn.
When we introduce a mask at layer C (to give the popup a rounded corner), the following happens:
R (Root Render Surface)
A (Snapped to root render surface)
B (Not snapped because layer is empty)
C [Mask Filter](Becomes a render surface but still not snapped because its still empty)
D (Snapped to A)
Layer C is forced to have a render surface of its own due to a mask layer associated with it. On the CC side
we rasterize each render surfaces separately and then composit them with at draw. However, in this case, when
we are rasterizing D and drawing it onto render surface C, we dont have it aligned to the pixels in C. This is
because D is still snapped to A, so it has the subpixel correction that will align it with pixels in A but not
in C. Also when we composit and draw C onto R, C is not aligned with R because we skipped computing subpixel
correction for C since it was an empty layer.
The blurriness happens due to these two misalignments.
D -> C (Causes some blurriness)
C -> R (Causes more blurriness)
The right way to solve this would be to not ignore layers that have render surfaces of their own when computing
subpixel correction. In this case, If we snap C to A and D to C, the blurriness would go away. (I tried this by hardcoding
subpixel correction values, and it works).
,
Jun 28 2018
@enne @trchen In the above scenario, does the cc layer for C(Render Surface) need to be snapped to R or A? In my tests snapping to A worked because A has no offset from R. But I dont know how render surfaces are composited. Will C have to be snapped to R? Or will snapping it to any CC layer in the target render surface work?
,
Jun 28 2018
Ultimately C needs to snap to R, but from engineering perspective, it is easier to snap A to R and C to A, so C will also snap to R by induction. Also if you want to snap C to R directly, you will also need to determine whether there are any intermediate render surface, but cc may or may not generate render surfaces for A and B for internal reasons, and try to speculate that from the client would be a layering violation. Is there a reason why the client can't just snap every layer, not just drawable layers and layers with effects?
,
Jul 12
,
Jul 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ef0d6ad085a1403792d83a62294bb70a210e24b3 commit ef0d6ad085a1403792d83a62294bb70a210e24b3 Author: Malay Keshav <malaykeshav@chromium.org> Date: Fri Jul 13 22:33:14 2018 Fixes the extension popup blurriness by snapping the appropriate layers This patch achieves two goals. 1. Snaps the NativeHostView of the extension popup to the physical pixels. This is because NativeHostView's layer will have a render surface of its own which needs to be snapped to the physical pixels for all its content to be snapped as well. 2. Snaps the RenderWidgetHostView to the nearest ancestor instead of of the toplevel or root window. This ensures that it snaps to the render surface it belonds too, instead of snapping to some other render surface in the hierarchy. To achieve the two goals, this patch also moves the utility method that snaps windows to its nearest snapped ancestor window from src/ash to ui/wm/core. Bug: 843250 Change-Id: I9d9f0bdd2391b1d9904537ec8580de72d225fdf6 Component: Windows, Snap to physical pixel, HighDPI, extensions Reviewed-on: https://chromium-review.googlesource.com/1121330 Commit-Queue: Malay Keshav <malaykeshav@chromium.org> Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Reviewed-by: Mitsuru Oshima <oshima@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#575095} [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ash/accelerators/debug_commands.cc [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ash/system/message_center/arc/arc_notification_content_view.cc [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ash/wm/panels/panel_layout_manager.cc [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ash/wm/window_properties.cc [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ash/wm/window_properties.h [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ash/wm/window_state.cc [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ash/wm/window_util.cc [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ash/wm/window_util.h [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ash/wm/wm_snap_to_pixel_layout_manager.cc [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ash/wm/workspace/workspace_layout_manager.cc [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/content/browser/renderer_host/render_widget_host_view_aura.cc [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ui/views/controls/native/native_view_host_aura.cc [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ui/wm/core/window_properties.cc [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ui/wm/core/window_properties.h [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ui/wm/core/window_util.cc [modify] https://crrev.com/ef0d6ad085a1403792d83a62294bb70a210e24b3/ui/wm/core/window_util.h
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cb16c9cb3731269dbf6e2435ebae79b696136df5 commit cb16c9cb3731269dbf6e2435ebae79b696136df5 Author: Malay Keshav <malaykeshav@chromium.org> Date: Wed Jul 25 16:36:40 2018 Snap the omnibox popup widget window to the pixel boundary This patch manually snaps the omnibox dropdown popup widget window to the physical pixel grid. On CrOS the popup is treated as a top level window which is by default snapped to the root window. This patch also lazily marks the root windows as snapped. The root windows are snapped by default so marking them snapped makes sense. Bug: 843250 Change-Id: I1dd20ea57522e4987d23ba7dcaa3ae891bbc7182 Component: Omnibox popup, desktop window tree host Reviewed-on: https://chromium-review.googlesource.com/1139078 Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Commit-Queue: Malay Keshav <malaykeshav@chromium.org> Cr-Commit-Position: refs/heads/master@{#577929} [modify] https://crrev.com/cb16c9cb3731269dbf6e2435ebae79b696136df5/chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc [modify] https://crrev.com/cb16c9cb3731269dbf6e2435ebae79b696136df5/chrome/browser/ui/views/omnibox/omnibox_popup_contents_view_browsertest.cc [modify] https://crrev.com/cb16c9cb3731269dbf6e2435ebae79b696136df5/ui/wm/core/window_util.cc [modify] https://crrev.com/cb16c9cb3731269dbf6e2435ebae79b696136df5/ui/wm/core/window_util_unittest.cc
,
Jul 25
,
Sep 5
The fix for Bug 880513 is #51. However it needs to be merged back to M69.
,
Sep 5
How safe is the merge for cl listed at #51 to M69? Cl landed on July 25th, may I please know why M69 merge was not requested then?
,
Sep 5
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 5
I am not sure why the patch was not merged to M69 back then. I must have missed it. It should be safe to merge back since there are no bugs on ToT due to this CL.
,
Sep 5
Approving merge to M69 branch 3497 based on comment #53, #56 and per offline group chat "M69 bug 880513 - Font rendering". Pls merge ASAP. Thank you. Also requesting a postmortem for this, pls see go/chrome-postmortems for more details.
,
Sep 5
,
Sep 5
The fix has been merged to M69 branch https://chromium-review.googlesource.com/c/chromium/src/+/1208256
,
Sep 5
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/87f67d8d0d1727217d00dedf08cd4f456108d800 commit 87f67d8d0d1727217d00dedf08cd4f456108d800 Author: Malay Keshav <malaykeshav@chromium.org> Date: Wed Sep 05 20:04:06 2018 Snap the omnibox popup widget window to the pixel boundary merge to M69 This patch manually snaps the omnibox dropdown popup widget window to the physical pixel grid. On CrOS the popup is treated as a top level window which is by default snapped to the root window. This patch also lazily marks the root windows as snapped. The root windows are snapped by default so marking them snapped makes sense. Bug: 843250 Change-Id: I1dd20ea57522e4987d23ba7dcaa3ae891bbc7182 Component: Omnibox popup, desktop window tree host Reviewed-on: https://chromium-review.googlesource.com/1139078 Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Commit-Queue: Malay Keshav <malaykeshav@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#577929}(cherry picked from commit cb16c9cb3731269dbf6e2435ebae79b696136df5) Reviewed-on: https://chromium-review.googlesource.com/1208256 Reviewed-by: Malay Keshav <malaykeshav@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#884} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/87f67d8d0d1727217d00dedf08cd4f456108d800/chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.cc [modify] https://crrev.com/87f67d8d0d1727217d00dedf08cd4f456108d800/chrome/browser/ui/views/omnibox/omnibox_popup_contents_view_browsertest.cc [modify] https://crrev.com/87f67d8d0d1727217d00dedf08cd4f456108d800/ui/wm/core/window_util.cc [modify] https://crrev.com/87f67d8d0d1727217d00dedf08cd4f456108d800/ui/wm/core/window_util_unittest.cc
,
Sep 5
,
Sep 6
Issue 880914 has been merged into this issue. |
||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||
Comment 1 by bugdroid1@chromium.org
, May 16 2018