Last Comment Bug 748384 - RTL pages which scroll horizontally have broken rendering
: RTL pages which scroll horizontally have broken rendering
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 15
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
Depends on: 754233 756555 758768 759762 759775 760458
Blocks: 752959
  Show dependency treegraph
 
Reported: 2012-04-24 08:35 PDT by :Ehsan Akhgari (busy, don't ask for review please)
Modified: 2012-06-01 08:17 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
+


Attachments
WIP part 1 - Update java code to keep page left/top/right/bottom instead of width/height (54.72 KB, patch)
2012-05-14 07:49 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
WIP part 2 (Cleanup) Kill the DrawMetadataProvider (5.23 KB, patch)
2012-05-14 07:50 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
WIP part 3 - Propagate the page values a little further in browser.js and the compositor (22.72 KB, patch)
2012-05-14 07:51 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
WIP part 4 - Get the correct page values going into browser.js (6.95 KB, patch)
2012-05-14 07:51 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
WIP part 5 - Get the page origin coming into the compositor (14.43 KB, patch)
2012-05-14 07:52 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
WIP part 6 - Fix up some java axis calculations (4.36 KB, patch)
2012-05-14 07:53 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
Patch to disable overscroll-bounce (for debugging) (1.55 KB, patch)
2012-05-14 07:53 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
(1/4) Cleanup - remove unused DrawMetadataProvider stuff (5.28 KB, patch)
2012-05-17 12:36 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review
(2/5) Update all Java code to keep rects for page bounds instead of just size (57.83 KB, patch)
2012-05-17 12:37 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
Details | Diff | Review
(3/5) Propagate rect values into java from browser.js and compositor (22.81 KB, patch)
2012-05-17 12:38 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
Details | Diff | Review
(4/5) Calculate correct page bounds for browser.js (7.00 KB, patch)
2012-05-17 12:39 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
Details | Diff | Review
(5/5) Calculate correct page bounds for compositor (14.45 KB, patch)
2012-05-17 12:39 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review-
Details | Diff | Review
(2/4) Update all Java code to keep rects for page bounds instead of just size (59.32 KB, patch)
2012-05-18 10:48 PDT, Kartikaya Gupta (email:kats@mozilla.com)
bugmail.mozilla: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review
(3/4) Propagate rect values into java from browser.js (10.88 KB, patch)
2012-05-18 10:50 PDT, Kartikaya Gupta (email:kats@mozilla.com)
mats: review-
Details | Diff | Review
(4/4) Propagate rect values into java from compositor (29.21 KB, patch)
2012-05-18 10:52 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
ajuma.bugzilla: review+
Details | Diff | Review
(3/4) Propagate rect values into java from browser.js (v3) (11.01 KB, patch)
2012-05-21 08:00 PDT, Kartikaya Gupta (email:kats@mozilla.com)
mats: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review
(4/4) Propagate rect values into java from compositor (v3) (31.08 KB, patch)
2012-05-21 08:02 PDT, Kartikaya Gupta (email:kats@mozilla.com)
mats: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review

Description :Ehsan Akhgari (busy, don't ask for review please) 2012-04-24 08:35:31 PDT
See <http://www.bbc.co.uk/persian/mobile/index.shtml> for example.  This page has an image with class "web-bug" set to { position: absolute; left: -1000px; top: -1000px; }.  This causes the page to be scrolled horizontally.  There are two problems here:

1. The page is scrollable to the right, but it should be scrollable to the left.
2. When you scroll to the right, you'll see broken rendering with us flipping between black and white, presumably because we have no idea what to render.

The stock browser does not let that page to be scrolled horizontally at all.
Comment 1 Aaron Train [:aaronmt] 2012-05-07 09:53:03 PDT
XUL regression? Blocking?
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-14 07:49:47 PDT
Created attachment 623670 [details] [diff] [review]
WIP part 1 - Update java code to keep page left/top/right/bottom instead of width/height
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-14 07:50:16 PDT
Created attachment 623671 [details] [diff] [review]
WIP part 2 (Cleanup) Kill the DrawMetadataProvider
Comment 4 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-14 07:51:06 PDT
Created attachment 623672 [details] [diff] [review]
WIP part 3 - Propagate the page values a little further in browser.js and the compositor
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-14 07:51:44 PDT
Created attachment 623673 [details] [diff] [review]
WIP part 4 - Get the correct page values going into browser.js
Comment 6 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-14 07:52:37 PDT
Created attachment 623674 [details] [diff] [review]
WIP part 5 - Get the page origin coming into the compositor
Comment 7 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-14 07:53:02 PDT
Created attachment 623675 [details] [diff] [review]
WIP part 6 - Fix up some java axis calculations
Comment 8 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-14 07:53:39 PDT
Created attachment 623676 [details] [diff] [review]
Patch to disable overscroll-bounce (for debugging)
Comment 9 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-14 07:55:08 PDT
Reassigning to :ajuma for now. With the WIPs I have posted the necessary values are getting propagated everywhere, but RTL pages still end up being clipped or not drawn correctly. I believe the problem lies in the compositor somewhere, Ali said he would take a look at it to see if he could figure out what's going wrong.
Comment 10 Ali Juma [:ajuma] 2012-05-16 12:29:00 PDT
With tiling disabled, these patches seem to work fine.

With tiling, the page is rendered intermittently while scrolling, and not rendered at all otherwise; this is likely related to a problem with negative tile coordinates, which is being looked at in Bug 754233.
Comment 11 Ali Juma [:ajuma] 2012-05-17 07:06:32 PDT
(In reply to Ali Juma [:ajuma] from comment #10)
> With tiling, the page is rendered intermittently while scrolling, and not
> rendered at all otherwise; this is likely related to a problem with negative
> tile coordinates, which is being looked at in Bug 754233.

The patch for Bug 754233 does indeed fix the intermittent rendering issue.
Comment 12 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-17 12:36:55 PDT
Created attachment 624838 [details] [diff] [review]
(1/4) Cleanup - remove unused DrawMetadataProvider stuff
Comment 13 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-17 12:37:41 PDT
Created attachment 624840 [details] [diff] [review]
(2/5) Update all Java code to keep rects for page bounds instead of just size
Comment 14 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-17 12:38:28 PDT
Created attachment 624841 [details] [diff] [review]
(3/5) Propagate rect values into java from browser.js and compositor
Comment 15 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-17 12:39:01 PDT
Created attachment 624842 [details] [diff] [review]
(4/5) Calculate correct page bounds for browser.js
Comment 16 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-17 12:39:31 PDT
Created attachment 624843 [details] [diff] [review]
(5/5) Calculate correct page bounds for compositor
Comment 17 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-17 12:48:52 PDT
:ehsan, :Cwiiis - I put a test build up at https://people.mozilla.com/~kgupta/tmp/rtl.apk which has these patches if you want to play with it. It also has the patch from bug 754233 which is required for this stuff to work.
Comment 18 Chris Lord [:cwiiis] 2012-05-18 05:08:01 PDT
Comment on attachment 624838 [details] [diff] [review]
(1/4) Cleanup - remove unused DrawMetadataProvider stuff

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

Looks fine - I kind of liked this ability, but I guess, performance-wise, we just never want to do this(?)
Comment 19 Chris Lord [:cwiiis] 2012-05-18 05:48:38 PDT
Comment on attachment 624840 [details] [diff] [review]
(2/5) Update all Java code to keep rects for page bounds instead of just size

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

Very comprehensive, looks good. r+ with comments addressed.

::: mobile/android/base/GeckoAppShell.java
@@ +2199,5 @@
>                  mIsRepaintRunnablePosted = false;
>              }
>  
>              Tab tab = Tabs.getInstance().getSelectedTab();
> +            GeckoAppShell.screenshotWholePage(tab);

Good call.

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +169,5 @@
>          float bottom = metrics.viewportRectBottom + margins.bottom;
> +        left = Math.max(metrics.pageRectLeft, TILE_SIZE * FloatMath.floor(left / TILE_SIZE));
> +        top = Math.max(metrics.pageRectTop, TILE_SIZE * FloatMath.floor(top / TILE_SIZE));
> +        right = Math.min(metrics.pageRectRight, TILE_SIZE * FloatMath.ceil(right / TILE_SIZE));
> +        bottom = Math.min(metrics.pageRectBottom, TILE_SIZE * FloatMath.ceil(bottom / TILE_SIZE));

Note to self, tile boundaries are considered relative to 0,0 and not the top-left of the page rect. (maybe this should be noted in the gfx tiles code somewhere?)

@@ +177,5 @@
>      /**
>       * Adjust the given margins so if they are applied on the viewport in the metrics, the resulting rect
>       * does not exceed the page bounds. This code will maintain the total margin amount for a given axis;
>       * it assumes that margins.left + metrics.getWidth() + margins.right is less than or equal to
> +     * metrics.pageRectRight - metrics.pageRectLeft; and the same for the y axis.

This comment should probably reference metrics.getPageWidth(), in case this encourages anyone to do this when the convenience function exists.

::: mobile/android/base/gfx/ImmutableViewportMetrics.java
@@ +99,5 @@
>      }
>  
>      @Override
>      public String toString() {
>          return "ImmutableViewportMetrics v=(" + viewportRectLeft + "," + viewportRectTop + ","

just a thought, seeing as the values are immutable, we should probably change this at some point to just generate the string on first call and store it (or maybe this just doesn't get called more than once that often?)

::: mobile/android/base/gfx/LayerController.java
@@ +195,5 @@
>          notifyLayerClientOfGeometryChange();
>          mView.requestRender();
>      }
>  
> +    /** Sets the current page rect. You must hold the monitor while calling this. */

Just noticed how amusing 'You must hold the monitor while calling this.' reads :)

@@ +197,5 @@
>      }
>  
> +    /** Sets the current page rect. You must hold the monitor while calling this. */
> +    public void setPageRect(RectF rect, RectF cssRect) {
> +        if (mViewportMetrics.getCssPageRect().equals(cssRect))

Might be nice while you're changing this code to add a comment here to explain why it's only necessary to check for equality on the CSS rect.

::: mobile/android/base/gfx/NinePatchTileLayer.java
@@ +105,2 @@
>          drawPatch(context, PATCH_SIZE * 2, PATCH_SIZE,                                     /* 7 */
> +                  page.right, page.bottom, PATCH_SIZE, PATCH_SIZE);

I can never remember enough to read these values, I assume this has been tested.

@@ +110,5 @@
>                             float tileX, float tileY, float tileWidth, float tileHeight) {
>          RectF viewport = context.viewport;
>          float viewportHeight = viewport.height();
> +        float drawX = tileX - viewport.left;
> +        float drawY = viewportHeight - (tileY + tileHeight - viewport.top);

ditto here.

::: mobile/android/base/gfx/ViewportMetrics.java
@@ +68,5 @@
>          DisplayMetrics metrics = new DisplayMetrics();
>          GeckoApp.mAppContext.getWindowManager().getDefaultDisplay().getMetrics(metrics);
>  
> +        mPageRect = new RectF(0, 0, metrics.widthPixels, metrics.heightPixels);
> +        mCssPageRect = new RectF(0, 0, metrics.widthPixels, metrics.heightPixels);

Is this correct? (i.e. if the page-rect was this size, would the CSS page rect be the same?)

@@ +138,5 @@
>      /** Returns the viewport rectangle, clamped within the page-size. */
>      public RectF getClampedViewport() {
>          RectF clampedViewport = new RectF(mViewportRect);
>  
> +        // The viewport size ought to never exceed the page size.

s/size/bounds/g

@@ +236,5 @@
>          // makes no sense to send non-integer coordinates to Gecko.
>          int height = Math.round(mViewportRect.height());
>          int width = Math.round(mViewportRect.width());
>  
>          StringBuffer sb = new StringBuffer(256);

I'm not sure the maximum length of a Float.toString, but if it's 10 characters, this string will require 322 characters to store. I'd bump this up to 512 to be sure.

@@ +256,5 @@
>      }
>  
>      @Override
>      public String toString() {
>          StringBuffer buff = new StringBuffer(128);

Again here, this likely isn't enough now - two more rects, so 256 ought to be safe.
Comment 20 Chris Lord [:cwiiis] 2012-05-18 05:59:10 PDT
Comment on attachment 624841 [details] [diff] [review]
(3/5) Propagate rect values into java from browser.js and compositor

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

r+ with comments addressed.

::: mobile/android/chrome/content/browser.js
@@ +2690,4 @@
>  
>        let viewport = BrowserApp.selectedTab.getViewport();
>        let vRect = new Rect(viewport.cssX, viewport.cssY, viewport.cssWidth, viewport.cssHeight);
>        let bRect = new Rect(Math.max(0,rect.x - margin),

I guess this should actually be Math.max(viewport.cssPageLeft, rect.x - margin) ?

@@ +2694,5 @@
>                             rect.y,
>                             rect.w + 2*margin,
>                             rect.h);
>  
> +      // constrict the rect to the screen

then this comment ought to read "to the screen's right edge"

::: widget/android/AndroidBridge.h
@@ +365,5 @@
> +    void SetFirstPaintViewport(float aOffsetX, float aOffsetY, float aZoom,
> +                               float aPageLeft, float aPageTop, float aPageRight, float aPageBottom,
> +                               float aCssPageLeft, float aCssPageTop, float aCssPageRight, float aCssPageBottom);
> +    void SetPageRect(float aZoom, float aPageLeft, float aPageTop, float aPageRight, float aPageBottom,
> +                     float aCssPageLeft, float aCssPageTop, float aCssPageRight, float aCssPageBottom);

We have a lot of parameters here now, might it be nicer to just take nsRects as parameters?

::: widget/android/AndroidJavaWrappers.h
@@ +206,5 @@
> +    void SetFirstPaintViewport(float aOffsetX, float aOffsetY, float aZoom,
> +                               float aPageLeft, float aPageTop, float aPageRight, float aPageBottom,
> +                               float aCssPageLeft, float aCssPageTop, float aCssPageRight, float aCssPageBottom);
> +    void SetPageRect(float aZoom, float aPageLeft, float aPageTop, float aPageRight, float aPageBottom,
> +                     float aCssPageLeft, float aCssPageTop, float aCssPageRight, float aCssPageBottom);

And same here, especially seeing SyncViewportInfo below take an nsIntPoint and an nsIntRect.
Comment 21 Chris Lord [:cwiiis] 2012-05-18 06:07:50 PDT
Comment on attachment 624842 [details] [diff] [review]
(4/5) Calculate correct page bounds for browser.js

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

The browser.js changes look good to me, but I'm not the right person to review the nsDOMWindowUtils changes.
Comment 22 Chris Lord [:cwiiis] 2012-05-18 06:16:40 PDT
Comment on attachment 624843 [details] [diff] [review]
(5/5) Calculate correct page bounds for compositor

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

I know it's a larger change, but I think it would be much nicer to use rects than have separate origin and size parameters about the place (unless you had some better reason to do it this way?). An r- because of this, and when re-reviewing, I think someone else should review the nsDisplayList change, despite it being trivial.

::: gfx/layers/Layers.h
@@ +153,1 @@
>    nsIntSize mContentSize;

I think it would be nicer to just have a single nsIntRect mContentRect

@@ +156,5 @@
>    ViewID mScrollId;
>  
> +  // Consumers often want to know the origin/size before scaling to pixels
> +  // so we record this as well.
> +  gfx::Point mCSSContentOrigin;

Similarly, I think it would be nicer to just have a gfx::Rect mCssContentRect

::: gfx/layers/ipc/CompositorParent.h
@@ +158,1 @@
>    nsIntSize mContentSize;

And ditto here.

::: ipc/glue/IPCMessageUtils.h
@@ +774,5 @@
>    }
>  };
>  
>  template<>
> +struct ParamTraits<mozilla::gfx::Point>

Using nsRect/nsIntRect would then make this unnecessary, not that this is a bad thing.
Comment 23 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-18 07:54:45 PDT
(In reply to Chris Lord [:cwiiis] from comment #18)
> (1/5) Cleanup - remove unused DrawMetadataProvider stuff
> 
> Looks fine - I kind of liked this ability, but I guess, performance-wise, we
> just never want to do this(?)

Performance-wise, and also code-cleanliness wise. Generally if this information is needed it can be gotten more directly.

(In reply to Chris Lord [:cwiiis] from comment #19)
> (2/5) Update all Java code to keep rects for page bounds instead of just size
> 
> Note to self, tile boundaries are considered relative to 0,0 and not the
> top-left of the page rect. (maybe this should be noted in the gfx tiles code
> somewhere?)

I added a BenWa-approved comment to TiledLayerBuffer.h.

> This comment should probably reference metrics.getPageWidth(), in case this
> encourages anyone to do this when the convenience function exists.

Updated the comment. I think I wrote that comment before I added getPageWidth() and then forgot to update it.

> just a thought, seeing as the values are immutable, we should probably
> change this at some point to just generate the string on first call and
> store it (or maybe this just doesn't get called more than once that often?)

I think in general we only send it once for a given ImmutableViewportMetrics instance. We create a lot of ImmutableViewportMetrics instances though :(

> Might be nice while you're changing this code to add a comment here to
> explain why it's only necessary to check for equality on the CSS rect.

Added.

> I can never remember enough to read these values, I assume this has been
> tested.

Yup, seems to work fine.

> > +        mPageRect = new RectF(0, 0, metrics.widthPixels, metrics.heightPixels);
> > +        mCssPageRect = new RectF(0, 0, metrics.widthPixels, metrics.heightPixels);
> 
> Is this correct? (i.e. if the page-rect was this size, would the CSS page
> rect be the same?)
> 

Yeah, because we initialize the zoom factor to 1.0.

> s/size/bounds/g

Fixed

> I'm not sure the maximum length of a Float.toString, but if it's 10
> characters, this string will require 322 characters to store. I'd bump this
> up to 512 to be sure.
> 
> Again here, this likely isn't enough now - two more rects, so 256 ought to
> be safe.

Fixed.

(In reply to Chris Lord [:cwiiis] from comment #20)
> (3/5) Propagate rect values into java from browser.js and compositor
> 
> >        let bRect = new Rect(Math.max(0,rect.x - margin),
> 
> I guess this should actually be Math.max(viewport.cssPageLeft, rect.x -
> margin) ?
> 

Good catch! Yup, fixed.

> then this comment ought to read "to the screen's right edge"

Fixed.

> We have a lot of parameters here now, might it be nicer to just take nsRects
> as parameters?

Yeah, I was debating whether to do that here or in a follow-up bug. I decided to do it the lazy way (also easier to bisect way) since I already have bug 753444 which will remove some of these values. But you're right, it's nicer to just change it now. I'll do that for parts 4 and 5.
Comment 24 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-18 07:55:34 PDT
> parts 4 and 5.

err, that should be parts 3 and 5.
Comment 25 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-18 10:48:29 PDT
Created attachment 625166 [details] [diff] [review]
(2/4) Update all Java code to keep rects for page bounds instead of just size

Updated patch with review comments addressed, carrying r+
Comment 26 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-18 10:50:36 PDT
Created attachment 625167 [details] [diff] [review]
(3/4) Propagate rect values into java from browser.js

Updated patch which contains some of part 3 and all of part 4 in the previous series (I decided to just make separate patches for the browser.js and compositor paths).

r? to matspal for the nsIDOMWindowUtils changes, the rest has already been r+'d by Cwiiis
Comment 27 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-18 10:52:52 PDT
Created attachment 625168 [details] [diff] [review]
(4/4) Propagate rect values into java from compositor

For the compositor half. r? to :Cwiiis for the java and JNI stuff, to :ajuma for the compositor changes and :cjones for the nsDisplayList part (since he reviewed jrmuizel's earlier patch that introduced mCSSContentSize).
Comment 28 Chris Lord [:cwiiis] 2012-05-18 11:35:51 PDT
Comment on attachment 625168 [details] [diff] [review]
(4/4) Propagate rect values into java from compositor

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

This all looks good to me, r+ with the comment addressed.

::: gfx/layers/opengl/ReusableTileStoreOGL.cpp
@@ +223,2 @@
>            const nsIntPoint& contentOrigin = metrics.mViewportScrollOffset;
>            gfxRect contentRect = gfxRect(-contentOrigin.x, -contentOrigin.y,

This should take into account the metrics.mContentRect origin now.
Comment 29 Ali Juma [:ajuma] 2012-05-18 11:41:24 PDT
Comment on attachment 625168 [details] [diff] [review]
(4/4) Propagate rect values into java from compositor

The compositor changes look good to me.
Comment 30 Mats Palmgren (:mats) 2012-05-18 17:12:04 PDT
CC bz, roc, in case they want to comment on the new nsDOMWindowUtils API.
Comment 31 Mats Palmgren (:mats) 2012-05-18 17:12:47 PDT
Comment on attachment 625167 [details] [diff] [review]
(3/4) Propagate rect values into java from browser.js

>+nsDOMWindowUtils::GetScrollPortBounds(bool aFlushLayout,
>+                                      PRInt32* aX, PRInt32* aY,
>+                                      PRInt32* aWidth, PRInt32* aHeight)

nsDOMWindowUtils::GetScrollPortBounds returns 0,0,0,0 if the root frame
isn't scrollable (e.g. <frameset>).  It seems better from an API point
of view if the method returned the root frame bounds in that case.
If so, maybe you need a more generic name, GetRootBounds?

The four out params seems clunky to me - returning a nsIDOMClientRect,
like getBoundingClientRect(), would be easier to use I think.

Also, rounding the (CSS pixel) results to integer values seems wrong to me.

The aFlushLayout param doesn't really seem motivated (you always pass false
right?).  If needed, we can add a separate flush method rather than have
every method carry that option.
Comment 32 Mats Palmgren (:mats) 2012-05-18 17:22:12 PDT
Comment on attachment 625168 [details] [diff] [review]
(4/4) Propagate rect values into java from compositor

-  gfx::Size mCSSContentSize;
> +  nsIntRect mCSSContentRect;

Note that you're changing from float to integer here.
Did you mean gfx::Rect?

It seems really odd to me to use nsIntRect for CSS pixels...
Comment 33 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-18 18:34:39 PDT
(In reply to Mats Palmgren [:mats] from comment #31)
> (3/4) Propagate rect values into java from browser.js
> 
> nsDOMWindowUtils::GetScrollPortBounds returns 0,0,0,0 if the root frame
> isn't scrollable (e.g. <frameset>).  It seems better from an API point
> of view if the method returned the root frame bounds in that case.
> If so, maybe you need a more generic name, GetRootBounds?

Ah yes, I hadn't considered that case properly. I'll test that and upload a new patch. Thanks for catching that.

> The four out params seems clunky to me - returning a nsIDOMClientRect,
> like getBoundingClientRect(), would be easier to use I think.

Will also do this.

> Also, rounding the (CSS pixel) results to integer values seems wrong to me.

To be honest, a lot of the rounding in Gecko seems arbitrary to me. I don't fully understand what is rounded when. However, in this case I basically need the valid range that window.scrollX/Y can take on (plus the size of scroll port), and since nsDOMWindowUtils::GetScrollXY rounds values using AppUnitsToIntCSSPixels it made sense to do that here too.

> 
> The aFlushLayout param doesn't really seem motivated (you always pass false
> right?).  If needed, we can add a separate flush method rather than have
> every method carry that option.

I can take it out. Again I mostly just copied that from GetScrollXY and it didn't seem like an altogether bad idea to leave it in.

(In reply to Mats Palmgren [:mats] from comment #32)
> (4/4) Propagate rect values into java from compositor
> 
> -  gfx::Size mCSSContentSize;
> > +  nsIntRect mCSSContentRect;
> 
> Note that you're changing from float to integer here.
> Did you mean gfx::Rect?

No, that was intentional. When jrmuizel added that code the intention was to pass to Java the integer CSS pixel dimensions of the page. It turns out the code he added was returning fractional values and he agreed that I should change it.

> 
> It seems really odd to me to use nsIntRect for CSS pixels...

Any suggestion as to what I should use instead?
Comment 34 Mats Palmgren (:mats) 2012-05-19 01:22:21 PDT
(In reply to Kartikaya Gupta (:kats) from comment #33)
> However, in this case I basically need the valid range that
> window.scrollX/Y can take on (plus the size of scroll port)

The size of the scroll port is fractional though.  I think using
non-rounded float values are better in general for layout metrics
in CSS pixels (when you can't use app units).

> No, that was intentional. When jrmuizel added that code the intention was to
> pass to Java the integer CSS pixel dimensions of the page.

But the CSS pixel dimensions of the page can be fractional, right?

The name 'mCSSContentRect' suggested to me that it was used for more than
scrolling purposes.  Glancing at the Java code in the patch and I get the
impression float is used for width/heights there, so it seems more accurate
to carry the fraction all the way, but I'm not familiar with this code so
I don't know.

> Any suggestion as to what I should use instead?

gfx::Rect
Comment 35 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-19 10:58:40 PDT
> To be honest, a lot of the rounding in Gecko seems arbitrary to me

At this point, the only coordinate rounding that should be happening is whatever is needed for backwards compat or required for grid-snapping.  New APIs should use floating-point coordinates.
Comment 36 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-21 08:00:22 PDT
Created attachment 625650 [details] [diff] [review]
(3/4) Propagate rect values into java from browser.js (v3)

Updated patch to take into account review comments, re-requesting review.

The GetScrollPortBounds function is now GetRootBounds, and returns a nsIDOMClientRect instead of having 4 out-parameters. It also handles the case where there is no scrollable frame, and keeps everything in unrounded floats. browser.js code was also update to compensate for the change in API.
Comment 37 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-21 08:02:23 PDT
Created attachment 625652 [details] [diff] [review]
(4/4) Propagate rect values into java from compositor (v3)

Updated patch with review comments. This basically uses gfx::Rect instead of nsIntRect for the CSS rect everywhere, and keeps it in unrounded floats rather than ints.
Comment 38 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-22 20:36:33 PDT
API seems ok.  Doesn't obviously expose any security holes...
Comment 39 Mats Palmgren (:mats) 2012-05-23 04:23:46 PDT
Comment on attachment 625650 [details] [diff] [review]
(3/4) Propagate rect values into java from browser.js (v3)

> dom/base/nsDOMWindowUtils.cpp
>+  nsIPresShell *presShell = doc->GetShell();

s/nsIPresShell */nsIPresShell* /

> mobile/android/chrome/content/browser.js
>+       * Also, we need to compare the page size in CSS pixels to the floored screen size in CSS
>+       * pixels because the page size may also be floored.
> ...
>+      let pageLargerThanScreen = (cssPageRect.width >= Math.floor(viewport.cssWidth))
>+                              && (cssPageRect.height >= Math.floor(viewport.cssHeight));

Do you still need Math.floor here?
I don't understand the "the page size may also be floored" -
getRootBounds() never returns floored values, afaict.

r=mats
Comment 40 Mats Palmgren (:mats) 2012-05-23 04:24:31 PDT
Comment on attachment 625652 [details] [diff] [review]
(4/4) Propagate rect values into java from compositor (v3)

> gfx/layers/ipc/CompositorParent.cpp
>@@ -350,31 +350,25 @@ CompositorParent::TransformShadowTree()
> ...
>   const FrameMetrics* metrics = &container->GetFrameMetrics();

Checking 'metrics' for NULL in this method doesn't make any sense.
Please file a follow-up bug to change it to 'const FrameMetrics& metrics'
unless you want to fix it here.

> gfx/layers/ipc/CompositorParent.h
>+CompositorParent::SetFirstPaintViewport(nsIntPoint aOffset, float aZoom,
>+                                        nsIntRect aPageRect, gfx::Rect aCssPageRect)

If possible, I'd prefer const nsIntPoint&, const nsIntRect&, const gfx::Rect&.

>+  virtual void SetPageRect(float aZoom, nsIntRect aPageRect, gfx::Rect aCssPageRect);

ditto

r=mats
Comment 41 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-23 07:49:10 PDT
(In reply to Mats Palmgren [:mats] from comment #39)
> (3/4) Propagate rect values into java from browser.js (v3)
> 
> s/nsIPresShell */nsIPresShell* /

Fixed.

> Do you still need Math.floor here?
> I don't understand the "the page size may also be floored" -
> getRootBounds() never returns floored values, afaict.

Technically it shouldn't be needed any more, no, since getRootBounds doesn't floor the values. However, there will still be some slight rounding error introduced by the page size getting converted to and from app units, so there is still some fuzz needed on that comparison. I decided to leave the floor in and updated the comment. The floor over-corrects for the rounding error but extra page size updates will be harmless.

(In reply to Mats Palmgren [:mats] from comment #40)
> (4/4) Propagate rect values into java from compositor (v3)
> Checking 'metrics' for NULL in this method doesn't make any sense.
> Please file a follow-up bug to change it to 'const FrameMetrics& metrics'
> unless you want to fix it here.

Filed bug 757847 for it.

> If possible, I'd prefer const nsIntPoint&, const nsIntRect&, const
> gfx::Rect&.
> 
> ditto
> 

Fixed, thanks.
Comment 43 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-23 08:12:59 PDT
Backed out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d36479e22b4a

Build bustage caused by uncaught merge error with bug 607417 which landed earlier today. I've fixed up the merge and pushed to try to verify.
Comment 46 Joe Drew (not getting mail) 2012-05-28 11:48:08 PDT
Kats, if you are confident in this, can you nominate it for aurora?
Comment 47 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-28 12:02:31 PDT
Comment on attachment 624838 [details] [diff] [review]
(1/4) Cleanup - remove unused DrawMetadataProvider stuff

Rolling up request for all four patches

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: RTL pages don't render properly, and may cause crashing/visual corruption
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): most of the patches are mobile-only, there are some parts that are non-mobile but those are either in the OMTC code, or part of the new API added to layout.
String or UUID changes made by this patch: YES! The UUID for nsIDOMWindowUtils was changed, and the nsIAndroidDrawMetadataProvider interface was removed entirely. No string changes.

Also just to note, this bug interacts with bug 607417 a little; see https://bugzilla.mozilla.org/show_bug.cgi?id=607417#c215 for details. If both of these bugs are uplifted, then whoever uplifts the second one should ensure that the final code matches that on m-c because there was some accidental clobbering and cleanup.
Comment 48 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-29 06:43:38 PDT
Note to self: this may have caused bug 758768; do not uplift to aurora until that has been determined.
Comment 49 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-30 08:28:27 PDT
Note to self: bug 759762 should be uplifted with this as well.
Comment 50 Johnathan Nightingale [:johnath] 2012-05-30 12:40:00 PDT
Kats - does comment 49 mean that we don't worry about comment 48 any more?
Comment 51 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-30 12:49:38 PDT
No, I think we still need to worry about comment 48 (bug 758768). If that is in fact a regression caused by this patch, it may or may not be fixed bug 759762. Either way we should wait until it is resolved before uplifting this.
Comment 52 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-31 08:26:23 PDT
comment 48 has been resolved; we're good to go on this bug.
Comment 53 Brad Lassey [:blassey] (use needinfo?) 2012-05-31 12:05:47 PDT
Comment on attachment 624838 [details] [diff] [review]
(1/4) Cleanup - remove unused DrawMetadataProvider stuff

be sure to land this with the fix for bug 759775
Comment 54 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-31 13:58:12 PDT
Landed along with the patch from bug 759762. There was some minor rebasing involved because this landed on top of bug 607417 on m-c but that isn't being uplifted to aurora.

https://hg.mozilla.org/releases/mozilla-aurora/rev/9904f5a0a14b
https://hg.mozilla.org/releases/mozilla-aurora/rev/0b048cb8efdb
https://hg.mozilla.org/releases/mozilla-aurora/rev/864882b43f5e
https://hg.mozilla.org/releases/mozilla-aurora/rev/984b96b2341d

Note You need to log in before you can comment on or make changes to this bug.