Closed Bug 483634 Opened 14 years ago Closed 14 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: Gavin, Assigned: Gavin)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 4 obsolete files)

Session store would like a way to get the current scroll state without flushing layout. See bug 483330 for more details.
Assignee: nobody → gavin.sharp
Attached patch patch (obsolete) — Splinter Review
Trying to share code between this method and the window scrollX/Y getters didn't seem like a worthwhile endeavor.
Attachment #368117 - Flags: superreview?
Attachment #368117 - Flags: review?(roc)
Attachment #368117 - Flags: superreview? → superreview?(bzbarsky)
Comment on attachment 368117 [details] [diff] [review]
patch

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

{} around this statement
Attachment #368117 - Flags: superreview?(bzbarsky)
Attachment #368117 - Flags: superreview+
Attachment #368117 - Flags: review?(roc)
Attachment #368117 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/699fc684188c
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
roc, bz: any ideas for why the test would be failing?
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.
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...
The test passes if I remove the scrolling="no" from the testrunner iframe.
Could run the test on a subframe of your own, as an expedient measure.
Attached patch with test in its own iframe (obsolete) — Splinter Review
Yeah, that allows it to pass in both cases (directly and in the test harness).
Attachment #368117 - Attachment is obsolete: true
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.
Blocks: 484648
Yes, returning (0,0) would be the right thing.
Attached patch with that change (obsolete) — Splinter Review
Attachment #368355 - Attachment is obsolete: true
Attachment #368819 - Flags: review?(roc)
+  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.
Attachment #368819 - Attachment is obsolete: true
Attachment #368830 - Flags: review?(roc)
Attachment #368819 - Flags: review?(roc)
Attached patch don't throw+testSplinter Review
Added a test that covers that case.
Attachment #368830 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/54c37f642c5c
Status: REOPENED → RESOLVED
tracking-fennec: --- → ?
Closed: 14 years ago14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #368833 - Flags: approval1.9.1?
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.
I think that should be fine, if we put it on a new interface there.
Is that necessary? I'm adding it to the end, and haven't bumped the IID.
Er... you do need to bump the iid (on trunk).  And yes, it's necessary.
Why is it necessary? Don't mean to be a pain, but I'd like to be able to answer that question.
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.
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.
I don't think it's likely either, but we promised API compat on branch for scriptable interfaces, so...
This change _is_ compatible for scripted callers, no?
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 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.
Attachment #368833 - Flags: approval1.9.1? → approval1.9.1+
tracking-fennec: ? → ---
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.