Bug 72919 – [chromium] updateRect is probably incorrect when contentBounds != bounds

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Shawn Singh]

We should do the following
  (1) fix the bug, of course
  (2) re-name the transform conversion functions in the m_tiler - contentRectToLayerRect and layerRectToContentRect - so that they are clearer
  (3) add testing that the updateRect is computed correctly for the case where contentBounds != bounds.

[Shawn Singh]

Created attachment 123054 [details]
Patch

[Shawn Singh]

Note that (2) on the list above was already done by Enne in a previous patch.  This patch addresses (1) and (3).

[Adrienne Walker]

Comment on attachment 123054 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123054&action=review

In general, looks good.  Can you also leave a comment on LayerChromium::m_updateRect about what space it's supposed to be in?

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:367
> +    // The updateRect (that indicates what was actually painted) should be in
> +    // layer space, not the "content space".

This is quite minor, but I'm kind of curious why content space gets scare quotes everywhere in this patch. Do you want to be calling it something else?

[Shawn Singh]

Created attachment 123228 [details]
addressed reviewer comments, removed unnecessary code in FakeTiledLayerEtc

Yeah, the reason I put quotes there is obsolete now anyway.  and I tend to overuse quotes, too.

[Shawn Singh]

Enne and James, could you please review?  Should be a relatively quick and easy review... thanks!

[Adrienne Walker]

Comment on attachment 123228 [details]
addressed reviewer comments, removed unnecessary code in FakeTiledLayerEtc

Thanks for the extra comment.  LGTM.

[James Robinson]

Comment on attachment 123228 [details]
addressed reviewer comments, removed unnecessary code in FakeTiledLayerEtc

View in context: https://bugs.webkit.org/attachment.cgi?id=123228&action=review

R=me, some minor cleanups suggested

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:245
> +    float widthScale = bounds().width() / static_cast<double>(contentBounds().width());
> +    float heightScale = bounds().height() / static_cast<double>(contentBounds().height());

nit: if we're assigning to a float, why static_cast<> to double? why not float?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:246
> +    m_updateRect = FloatRect(m_paintRect.x() * widthScale, m_paintRect.y() * heightScale, m_paintRect.width() * widthScale, m_paintRect.height() * heightScale);

nit: i think i'd weakly prefer doing something like FloatRect(m_paintRect).scale(widthScale, heightScale); just to make the line a bit shorter and have less risk of silly copy/paste errors

[Shawn Singh]

Created attachment 123414 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 123414 [details]
Patch for landing

Rejecting attachment 123414 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Shawn Singh]

Created attachment 123501 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 123501 [details]
Patch for landing

Clearing flags on attachment: 123501

Committed r105680: <http://trac.webkit.org/changeset/105680>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.