483634 - add scrollX/scrollY getter on nsIDOMWindowUtils that doesn't flush layout

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Session store would like a way to get the current scroll state without flushing layout. See bug 483330 for more details.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Trying to share code between this method and the window scrollX/Y getters didn't seem like a worthwhile endeavor.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 368117 [details] [diff] [review]
patch

+  if (aFlushLayout)
+    doc->FlushPendingNotifications(Flush_Layout);

{} around this statement

Comment 3

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/699fc684188c

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Had to back this out, the unit test was failing:

*** 40448 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_domWindowUtils_scrollXY.html | getScrollXY x for test: flushed layout for getScrollXY - got 900, expected 0
*** 40449 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_domWindowUtils_scrollXY.html | getScrollXY y for test: flushed layout for getScrollXY - got 1000, expected 0

Not sure why that would be happening... does the flush not cause the scroll position to change synchronously?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

roc, bz: any ideas for why the test would be failing?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No idea really, but have you tried running the entire directory full of tests, so your test runs in a small-ish IFRAME? That may affect this test.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Gah! I should have thought of that.

It does fail when run in an iframe. I can't figure out why, though. The iframe has scrolling=no specified, which seems likely to be related. The scrollTo calls cause the document position to change, but resizing the body element (and forcing a flush of layout) does not. I thought scrolling=no only implied overflow: hidden, but styling the body that way in the non-frame case doesn't cause a failure. I wonder if this is actually an existing bug with scrolling in subframes...

Comment 8

[:Gavin Sharp [email: [e-mail]]]

The test passes if I remove the scrolling="no" from the testrunner iframe.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Could run the test on a subframe of your own, as an expedient measure.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Attachment: with test in its own iframe

Yeah, that allows it to pass in both cases (directly and in the test harness).

Comment 11

[:Gavin Sharp [email: [e-mail]]]

I've noticed something else while testing this further... GetRootScrollableView sometimes returns null while the document is loading.

I wonder if it would be best to just return 0, 0 in that case and avoid throwing? The alternative is to have the session store patch catch and silently discard exceptions.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, returning (0,0) would be the right thing.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Attachment: with that change

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  nsIPresShell *presShell = doc->GetPrimaryShell();
+  NS_ENSURE_TRUE(presShell, NS_ERROR_UNEXPECTED);
+
+  nsIViewManager *viewManager = presShell->GetViewManager();
+  NS_ENSURE_TRUE(viewManager, NS_ERROR_UNEXPECTED);

Actually we should return 0,0 in these cases too. The document might be in adisplay:none IFRAME and then there would be no pres-shell.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Attachment: don't throw for display:none documents

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Attachment: don't throw+test

Added a test that covers that case.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/54c37f642c5c

Comment 18

[:Gavin Sharp [email: [e-mail]]]

I'd like this on the branch because Fennec would like to use this, and the plan is to ship Fennec off something 1.9.1-based.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I think that should be fine, if we put it on a new interface there.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Is that necessary? I'm adding it to the end, and haven't bumped the IID.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Er... you do need to bump the iid (on trunk).  And yes, it's necessary.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Why is it necessary? Don't mean to be a pain, but I'd like to be able to answer that question.

Comment 23

[Jeff Walden [:Waldo]]

It's necessary because external code which had an interface which inherited from the old version would have that interface's members at N, N+4, N+8, etc. offsets in the virtual table, but under the new version (with one extra method) those members should actually be at N+4, N+8, N+12, etc. offsets.  A QI in that external code to that external code's interface (which implicitly assumes the old version, with one fewer method) and then an attempt to call any of the methods on that interface would use the wrong offset and call the wrong method, with the wrong arguments, and chaos ensues.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Ah, sure. Assuming someone inherits from this interface in third party code... seems quite unlikely, especially in this case. I suppose I'm not strongly opposed to adding a new interface for the branch, but I also don't think it's necessary.

I guess I'll bump the IID on trunk.

Comment 25

[Boris Zbarsky [:bzbarsky]]

I don't think it's likely either, but we promised API compat on branch for scriptable interfaces, so...

Comment 26

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This change _is_ compatible for scripted callers, no?

Comment 27

[Boris Zbarsky [:bzbarsky]]

For scripted callers, yes.  But I thought we were avoiding changes to non-internal (and this is _definitely_ non-internal; it's whole reason for existence is to expose platform functionality) interfaces unless a gun is being held to our heads.  Did that plan get changed at some point?

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 368833 [details] [diff] [review]
don't throw+test

a191=beltzner

We are avoiding changes to binary compatibility issues, definitely, unless there's good cause. Between this change and some other gesture-support related changes in bug 471883 and such, there seems to be good cause.

We should definitely get out in front of this, though, and make a list of all the interfaces that have changed for non-script callers so we can publish it with the beta 4 release.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/810f5fb0b1c7