618249 - <browser remote> doesn't properly handle scrolling on its own

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

STR
 (1) Load
firefox -no-remote -chrome chrome://global/content/test-ipcbrowser.xul
 (2) Enter http://people.mozilla.com/~cjones/testanchor.html#anchor into the URL bar

Only a gray screen shows up, not the text as expected.  The platform has all the right rendered pixels, they're just not being drawn at the right place.

mbrubeck was kind enough to point me at bug 598391, which AFAICT implemented this for fennec's setup.

The fundamental problem is, our shadow-layer code was designed with fennec in mind, which uses asynchronous scrolling and re-rendering.  This ends up meaning that the "real" scroll offset drawn to screen in the chrome process is usually different than the scroll offset web content sees.  This is never the case in desktop firefox, which synchronously scrolls content.  (Well, depending on how one defines "the scroll offset web content sees".  Let's pretend that this is true.)

So, long story short, for <browser remote> the frontend must manually tell the platform what the frontend wants the scroll offset to be (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#1507).  It doesn't have to be that way; the platform knows what the scroll offset is.  This test would actually be working if it weren't for the code to support fennec.

This has two implications
 (1) (minor) What should the default be?  Async or sync scrolling?  I lean towards default-sync, and having fennec set some flag that tells the platform it wants async scrolling.
 (2) (major) What scrolling behavior do we want in FF5?  Stay with the current model, or adopt a model like fennec's?  (Probably sans checkerboarding; that is, don't allow panning into garbage space.)

Fixing (1) is what this bug will cover.  It's actually somewhat nontrivial, with the interface freeze and impact on fennec.

I'm bringing up (2) so that we firefox and fennec folk can start finding a solution that makes everyone happy.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Proof-of-concept patch

This is the effect we want for a real fix for this bug.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(This bug also affects scrollTo/scrollBy.)

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Is this related/same problem as bug 615395?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yes!

Mind if I dup that to this, since we've got more discussion+patch here?

Comment 6

[:Felipe Gomes (needinfo for replies!)]

not at all, already dup'ed

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Default remote-browser to synchronous scrolling, and let users change that

See comment 0.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Enable async scrolling for fennec (BROKEN!)

None of the approaches here seem to be able to get at the browser's nsIFrameLoader.  Pointers very much appreciated.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 497038 [details] [diff] [review]
Enable async scrolling for fennec (BROKEN!)

I would expect this one to work:

let fl = browser.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader;

but I think you need to do that _after_ we add it to the document, a few lines below. How about trying this after the | browser.stop() | call?

This looks like the right general area to set the flag though

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Enable async scrolling for fennec

Works!

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 497035 [details] [diff] [review]
Default remote-browser to synchronous scrolling, and let users change that

Would like to land this so that we can turn on reftest-ipc.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 497047 [details] [diff] [review]
Enable async scrolling for fennec

Would like to land this so that we can turn on reftest-ipc.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Approval ping.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/bddac4e2bf74
http://hg.mozilla.org/mobile-browser/rev/e390bb319350