The inIFlasher API hasn't worked on OS X for years (bug 368608), has probably broken for some hardware accelerated drawing paths on other platforms with the switch to Moz2D/layers, and will work on ever fewer going forward. We should remove it, especially since supporting it is blocking completing the migration from Thebes to Moz2D. I've taken a look at the add-ons that use inIFlasher. There are only four in current use and the only significant one is DOM Inspector. I guess bug 368608 covers DOM Inspector, and I'll contact the authors of the other three individually.

Hmm, regarding the removal of inIFlasher::scrollElementIntoView, no other API that we expose has exactly the same semantics. It calls: presShell->ScrollContentIntoView(element, nsIPresShell::ScrollAxis(), nsIPresShell::ScrollAxis(), nsIPresShell::SCROLL_OVERFLOW_HIDDEN); HTMLElement::scrollIntoView is similar, but calls: presShell->ScrollContentIntoView(element, nsIPresShell::ScrollAxis( vpercent, nsIPresShell::SCROLL_ALWAYS), nsIPresShell::ScrollAxis(), nsIPresShell::SCROLL_OVERFLOW_HIDDEN); The default arguments to nsIPresShell::ScrollAxis are SCROLL_MINIMUM and SCROLL_IF_NOT_FULLY_VISIBLE, so this will scroll to the top/bottom rather than the minimum amount necessary to get it fully on screen. nsIScrollBoxObject::ensureElementIsVisible is also similar but calls: presShell->ScrollContentIntoView(element, nsIPresShell::ScrollAxis(), nsIPresShell::ScrollAxis(), nsIPresShell::SCROLL_FIRST_ANCESTOR_ONLY | nsIPresShell::SCROLL_OVERFLOW_HIDDEN); So this won't scroll ancestor scroll boxes of the nearest scroll box, so isn't guaranteed to get the element on screen.

If this can't be implemented in script, then we should still move inIFlasher::scrollElementIntoView to some other more appropriate interface. I'm not sure where that would be though...

> Attachment #8431709 [details] [diff] - Flags: review?(philip.chee@gmail.com) Hi! I'm not a layout peer or a peer of anything in mozilla-central. I think you want Neil @ parkway

Thanks, Philip. Neil, I'll get a layout peer to review this. Do you want me to hold off on landing though, or does it not matter to you since you don't merge to seamonkey very often? I don't want to hold off for long, but if it helps let me know.

(In reply to Jonathan Watt from comment #5) > Neil, I'll get a layout peer to review this. Do you want me to hold off on > landing though, or does it not matter to you since you don't merge to > seamonkey very often? I don't want to hold off for long, but if it helps let > me know. It turns out that there are (since Firefox 26) new APIs that will have a similar effect to these APIs you are removing, so I will look into rewriting DOM Inspector to use them where possible, however I'd prefer if I didn't have to rush to fix it before the uplift if possible. (After the uplift, feel free to remove inIFlasher, since we'll then have a few weeks in which to get a fix in to trunk.)

That should be okay. Please let me know if you get your fix in before then though.

I was wondering whether you'd noticed comment 40 on bug 368608. Is there no ability to do out-of-band drawing at all, which is why devtools has resorted to fiddling around with CSS psuedoclasses? (In reply to Jonathan Watt from comment #2) > Hmm, regarding the removal of inIFlasher::scrollElementIntoView, no other > API that we expose has exactly the same semantics. It calls: Unfortunately the page on which I tested my patch for bug 368608 didn't have a scrollbar so I didn't notice that difference. Element.scrollIntoView is better than nothing, but it would be nice if it only did it where necessary (bug 403510). nsFocusManager calls ScrollContentIntoView but we're not changing the focus here. nsDocumentViewer calls ScrollContentIntoView but it's effectively the same as Element's ScrollIntoView in that it treats it like an anchor. nsScrollBoxObject only makes sense in the context of a XUL <scrollbox> element. nsISelectionController has a ScrollSelectionIntoView method, but we don't have a "spare" selection we can use.

(In reply to neil@parkwaycc.co.uk from comment #9) > I was wondering whether you'd noticed comment 40 on bug 368608. Is there no > ability to do out-of-band drawing at all, which is why devtools has resorted > to fiddling around with CSS psuedoclasses? roc: would you be able to comment on this? I see mention of your considering this in bug 368608 comment 39 and you understand what the new graphics/layers architecture can and can't do better than I.

(In reply to neil@parkwaycc.co.uk from comment #9) > Unfortunately the page on which I tested my patch for bug 368608 didn't have > a scrollbar so I didn't notice that difference. Element.scrollIntoView is > better than nothing, but it would be nice if it only did it where necessary > (bug 403510). Maybe we could add the existing inIFlasher::scrollElementIntoView behavior to nsIDOMWindowUtils, nsIFocusManager or nsISelectionController, or something. None of those seem quite ideal, but I'm not aware of anything else.

(In reply to Jonathan Watt [:jwatt] from comment #10) > (In reply to neil@parkwaycc.co.uk from comment #9) > > I was wondering whether you'd noticed comment 40 on bug 368608. Is there no > > ability to do out-of-band drawing at all, which is why devtools has resorted > > to fiddling around with CSS psuedoclasses? > > roc: would you be able to comment on this? I see mention of your considering > this in bug 368608 comment 39 and you understand what the new > graphics/layers architecture can and can't do better than I. I will comment in that bug.

(In reply to Jonathan Watt from comment #11) > (In reply to comment #9) > > Unfortunately the page on which I tested my patch for bug 368608 didn't have > > a scrollbar so I didn't notice that difference. Element.scrollIntoView is > > better than nothing, but it would be nice if it only did it where necessary > > (bug 403510). > > Maybe we could add the existing inIFlasher::scrollElementIntoView behavior > to nsIDOMWindowUtils, nsIFocusManager or nsISelectionController, or > something. None of those seem quite ideal, but I'm not aware of anything > else. Ah, it just occurs to me that inIDOMUtils might be suitable.

This is in addition to jwatt's patch that removes it from inIFlasher.

the bug this supposedly blocked has been resolved-- should this now be marked as blocking what that bug blocked? bug #627699

(In reply to neil@parkwaycc.co.uk from comment #7) > (After the uplift, > feel free to remove inIFlasher, since we'll then have a few weeks in which > to get a fix in to trunk.) Now done: https://hg.mozilla.org/integration/mozilla-inbound/rev/4140ef233349

> (After the uplift, feel free to remove inIFlasher, since we'll then have a few weeks in > which to get a fix in to trunk.) Except people use DOM inspector with nightlies for day to day work. This broke several front-end folks' workflows, as well as mine...

Ugh. I looked through the source of the various add-ons using inIFlasher when figuring out who to contact about this removal, and I had it in my head that only the flasher of DOMi would break. Sorry about that. Regarding backout, I have to head AFK right now. Note that there have been other patches that subsequently landed that depend on this so they'd need to be backed out too. Probably it's just better to disable the DOMi flasher code if bug 368608 isn't going to get reviewed today. If someone wants to try backing out feel free though. I'll be back online in a few hours.

The real question is how long it'll take for a fixed DOMi to be deployed to AMO, no? Which is also the problem with the disable bit: how is someone who's using DOMi with nightlies supposed to do that?

For those of you who want a working version right now: http://neil.rashbrook.org/inspector-2.0.15pre.xpi

(In reply to Boris Zbarsky [:bz] from comment #22) > The real question is how long it'll take for a fixed DOMi to be deployed to > AMO, no? I thought DOMi was build and bundled with nightly builds.

That's just a straight revert, with inFlasher::RepaintElement and inFlasher::DrawElementOutline gutted to return NS_OK.

> I thought DOMi was build and bundled with nightly builds. It's not.

Comment on attachment 8438651 [details] [diff] [review] restore inIFlasher, but with drawElementOutline and repaintElement as no-ops r=me, if you document in the IDL that they're no-ops.

(In reply to On vacation Aug 5-18. Please do not request review. from comment #27) > > I thought DOMi was build and bundled with nightly builds. > > It's not. It isn't in Firefox or Thunderbird, but IIUC, _SeaMonkey_ trunk nightly (and hourly) builds are shipped together with a ./distribution/extensions/inspector@mozilla.org.xpi built from the tip of the default branch of the DOMi hg repo. So breaking the latest DOMi changeset automatically breaks DOMi-in-SeaMonkey for any user installing SeaMonkey-trunk on that day, or for any trunk user installing the newly shipped version on that day.

...the newly shipped *DOMi* version on that day.

Is this safe to land now?

It looks like Neil's fixes to DOM Inspector from bug 368608 shipped in the new DOM Inspector release in December: https://addons.mozilla.org/en-US/firefox/addon/dom-inspector-6622/versions/?page=1#version-2.0.15 I'll try landing this again next time I land some patches.

https://hg.mozilla.org/mozilla-central/rev/4cd85bbe71b4 https://hg.mozilla.org/mozilla-central/rev/44fc5d2b4dfe