Closed Bug 877728 Opened 7 years ago Closed 6 years ago

Add pixel units to android/widget JNI code uses of gfx::Point, gfx::Rect

Categories

(Core :: Graphics, defect)

23 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(6 files, 1 obsolete file)

Attached patch Part 1 (obsolete) — Splinter Review
Follow-up from bug 865735 - we is an incremental propagation of pixel-units information to some uses of gfx::Point, gfx::Size, and gfx::Rect.
Attachment #756041 - Flags: review?(bgirard)
Assignee: nobody → bugmail.mozilla
Comment on attachment 756041 [details] [diff] [review]
Part 1

Review of attachment 756041 [details] [diff] [review]:
-----------------------------------------------------------------

Big improvement, just a few minor questions.

::: dom/ipc/TabChild.cpp
@@ +349,5 @@
>          mLastMetrics.mViewport =
>              gfx::Rect(0, 0,
>                        kDefaultViewportSize.width, kDefaultViewportSize.height);
> +        mLastMetrics.mCompositionBounds = LayerIntRect::FromUnknownRect(
> +          gfx::IntRect(0, 0, mInnerSize.width, mInnerSize.height));

I don't understand why you're created an IntRect and then doing an 'unsafe' conversion to LayerIntRect. Why not construct a LayerIntRect directly?

@@ +589,5 @@
>  
>    FrameMetrics metrics(mLastMetrics);
>    metrics.mViewport = gfx::Rect(0.0f, 0.0f, viewportW, viewportH);
> +  metrics.mScrollableRect = CSSRect(0.0f, 0.0f, pageWidth, pageHeight);
> +  metrics.mCompositionBounds = LayerIntRect::FromUnknownRect(

same?

::: gfx/layers/composite/ThebesLayerComposite.cpp
@@ +236,5 @@
>      if (parentMetrics.IsScrollable())
>        scrollableLayer = parent;
>      if (!parentMetrics.mDisplayPort.IsEmpty() && scrollableLayer) {
>        // Get the composition bounds, so as not to waste rendering time.
> +      compositionBounds = gfxRect(parentMetrics.mCompositionBounds.x,

mCompositionBounds is a LayerIntRect. Why not propagate it to compositionBounds?

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +917,5 @@
>    double estimatedPaintDuration =
>      aEstimatedPaintDuration > EPSILON ? aEstimatedPaintDuration : 1.0;
>  
> +  gfxSize resolution = CalculateResolution(aFrameMetrics);
> +  CSSIntRect compositionBounds = LayerIntRect::ToCSSIntRectRoundIn(

Nice!

@@ +919,5 @@
>  
> +  gfxSize resolution = CalculateResolution(aFrameMetrics);
> +  CSSIntRect compositionBounds = LayerIntRect::ToCSSIntRectRoundIn(
> +    aFrameMetrics.mCompositionBounds, resolution);
> +  gfx::Rect scrollableRect = aFrameMetrics.mScrollableRect.ToUnknownRect();

convert later?

::: layout/base/nsDisplayList.cpp
@@ +683,5 @@
>  
>    if (nsIWidget* widget = aForFrame->GetNearestWidget()) {
> +    nsIntRect bounds;
> +    widget->GetBounds(bounds);
> +    metrics.mCompositionBounds = LayerIntRect::FromUnknownRect(

The widget is returning bounds in layer coordinate? This seems wrong to me.

::: layout/ipc/RenderFrameParent.cpp
@@ +792,5 @@
>  RenderFrameParent::NotifyDimensionsChanged(int width, int height)
>  {
>    if (mPanZoomController) {
>      mPanZoomController->UpdateCompositionBounds(
> +      LayerIntRect::FromUnknownRect(gfx::IntRect(0, 0, width, height)));

again, why not create LayerIntRect directly?
Attachment #756041 - Flags: review?(bgirard)
Comment on attachment 756041 [details] [diff] [review]
Part 1

Review of attachment 756041 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comment addressed verbally.

::: dom/ipc/TabChild.cpp
@@ +349,5 @@
>          mLastMetrics.mViewport =
>              gfx::Rect(0, 0,
>                        kDefaultViewportSize.width, kDefaultViewportSize.height);
> +        mLastMetrics.mCompositionBounds = LayerIntRect::FromUnknownRect(
> +          gfx::IntRect(0, 0, mInnerSize.width, mInnerSize.height));

A note to future reader. The reason we do this is because later when we change the type of mInnerSize if we applied my suggestion then this will bypass the type checking and still compile. So this pattern is preferred.
Attachment #756041 - Flags: review+
Updated with some comments about the FromUnknownRect calls. I also changed LayerPixel::FromCSSRectRoundOut to return LayerIntRect rather than a LayerRect because it is rounded and so should be ints. Carrying r+
Attachment #756041 - Attachment is obsolete: true
Attachment #756292 - Flags: review+
Attachment #756292 - Attachment description: Part 1 → Part 1 - Update CSS rects passed to Java
Try run with these patches is at https://tbpl.mozilla.org/?tree=Try&rev=fc5d423a935f and looking pretty green so far. Sorry for splitting it up into so many patches but I thought they would be easier to review this way. Let me know if you prefer them folded together. In each patch I start with one or two variables and convert them and propagate that as far as I feel comfortable doing. These 5 patches complete the conversion of all the JNI methods currently used in Fennec. There's still one gfx::Margin parameter which I could convert; we didn't templat-ize the Margin class but I'm thinking it would be a good idea to do that as well.
Comment on attachment 756293 [details] [diff] [review]
Part 2 - Convert the scroll offset out-param to SyncViewportInfo from nsIntPoint to ScreenPoint

Review of attachment 756293 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ -26,5 @@
>  namespace mozilla {
>  namespace layers {
>  
> -void
> -AsyncCompositionManager::SetTransformation(float aScale,

I really hope all of this is dead code and not just accidental dead code for the layers refactoring. Otherwise great removal of this + the members :)

@@ +445,5 @@
>  
>    gfx::Margin fixedLayerMargins(0, 0, 0, 0);
>    gfx::Point offset(0, 0);
> +  ScreenPoint scrollOffset(0, 0);
> +  float scaleX = 1.0,

Perhaps now is a good time to only have a single scale factor? Follow up is fine to not rot this patch queue.
Attachment #756293 - Flags: review?(bgirard) → review+
Attachment #756294 - Flags: review?(bgirard) → review+
Attachment #756295 - Flags: review?(bgirard) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> Try run with these patches is at
> https://tbpl.mozilla.org/?tree=Try&rev=fc5d423a935f and looking pretty green
> so far. Sorry for splitting it up into so many patches but I thought they
> would be easier to review this way. 

Yes it certainly made the reviewing very easy. Thanks.

> Let me know if you prefer them folded
> together. In each patch I start with one or two variables and convert them
> and propagate that as far as I feel comfortable doing. These 5 patches
> complete the conversion of all the JNI methods currently used in Fennec.
> There's still one gfx::Margin parameter which I could convert; we didn't
> templat-ize the Margin class but I'm thinking it would be a good idea to do
> that as well.

IMO I haven't seen the margin code used much. I don't think it's worth it but feel free to do it.
Attachment #756296 - Flags: review?(bgirard) → review+
(In reply to Benoit Girard (:BenWa) from comment #9)
> I really hope all of this is dead code and not just accidental dead code for
> the layers refactoring. Otherwise great removal of this + the members :)

I looked quickly through the code history and it looks this code was used for some testing functions that were removed a while ago. So yeah I don't think it was introduced by the layers refactoring.

> Perhaps now is a good time to only have a single scale factor? Follow up is
> fine to not rot this patch queue.

Perhaps. I actually prefer having x and y scale factors everywhere and would like to propagate it through java as well. It might be useful to be able to stretch the page differently in the different axes. I feel like if layout has the ability to do this wired in everywhere then we should maintain that ability in new code too.

(In reply to Benoit Girard (:BenWa) from comment #10)
> IMO I haven't seen the margin code used much. I don't think it's worth it
> but feel free to do it.

I'll leave it for now but if I run into places where we get margins from Rect objects or vice-versa maybe I'll do it.

Landed these patches on inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1fbdc5adccbe
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f0fb17e872ea
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f3591c9f16fd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a8cd81a2186b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/65f9092f767f
It would be really helpful to carefully document each new type that gets added. Otherwise the profusion of new types will just be bewildering.
Thanks for the reminder. I added some documentation in the "part 4" patch on bug 879004.
Comment on attachment 756295 [details] [diff] [review]
Part 4 - Convert the page rect passed to SetFirstPaintViewport to LayerIntRect

Review of attachment 756295 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/ThebesLayerComposite.cpp
@@ +255,4 @@
>        gfx::Point scrollOffset =
>          gfx::Point((metrics.mScrollOffset.x * metrics.LayersPixelsPerCSSPixel().width) / scaleX,
>                     (metrics.mScrollOffset.y * metrics.LayersPixelsPerCSSPixel().height) / scaleY);
> +      const nsIntPoint& contentOrigin = nsIntPoint(

I'm not good with C++, but this seems like the nsIntPoint might die after this statement.
(In reply to :Ms2ger from comment #15)
> I'm not good with C++, but this seems like the nsIntPoint might die after
> this statement.

You might be right. I tried writing a test case to demonstrate this but failed. I'm still not convinced either way though.
The original code looks like it had the same issue (if it -is- an issue), as it was using a reference to an anonymous local variable (the result of the subtraction of two nsIntPoint values). But AIUI, this is in fact safe according to C++ rules: normally, the lifetime of the temporary variable would be just the statement in which it occurs, but binding the temporary to a const ref will extend its lifetime until the reference goes out of scope.

But I don't see any reason to declare this as a reference in the first place. Surely the more natural thing to write would be simply

  const nsIntPoint contentOrigin(metrics.mContentRect.x - NS_lround(scrollOffset.x),
                                 metrics.mContentRect.y - NS_lround(scrollOffset.y));

so that the lifetime of contentOrigin is simple and clear.
Thanks for the info! And yeah, I'll put up a patch to use the simpler version.
Comment on attachment 760408 [details] [diff] [review]
Part 6 - Minor variable fixup

Review of attachment 760408 [details] [diff] [review]:
-----------------------------------------------------------------

Looks sensible enough.
Attachment #760408 - Flags: review?(Ms2ger) → review+
https://hg.mozilla.org/mozilla-central/rev/b735641402d7
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.