1018324 - Remove inIFlasher

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Jonathan Watt [:jwatt]]

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.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

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.

Comment 3

[Jonathan Watt [:jwatt]]

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...

Comment 4

[Philip Chee]

> Attachment #8431709 [details] [diff] - Flags: review?([e-mail])

Hi! I'm not a layout peer or a peer of anything in mozilla-central. I think you want Neil @ parkway

Comment 5

[Jonathan Watt [:jwatt]]

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.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 7

[[e-mail]]

(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.)

Comment 8

[Jonathan Watt [:jwatt]]

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

Comment 9

[[e-mail]]

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.

Comment 10

[Jonathan Watt [:jwatt]]

(In reply to [e-mail] 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.

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to [e-mail] 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.

Comment 12

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

(In reply to Jonathan Watt [:jwatt] from comment #10)
> (In reply to [e-mail] 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.

Comment 13

[[e-mail]]

(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.

Comment 14

[[e-mail]]

Attachment: Add scrollElementIntoView to inIDOMUtils

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

Comment 15

[[e-mail]]

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

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c3696dc11743

Comment 17

[Kyra]

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

Comment 18

[Jonathan Watt [:jwatt]]

(In reply to [e-mail] 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

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4140ef233349

Comment 20

[Boris Zbarsky [:bzbarsky]]

> (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...

Comment 21

[Jonathan Watt [:jwatt]]

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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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?

Comment 23

[[e-mail]]

For those of you who want a working version right now:

http://neil.rashbrook.org/inspector-2.0.15pre.xpi

Comment 24

[Jonathan Watt [:jwatt]]

(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.

Comment 25

[Jonathan Watt [:jwatt]]

Attachment: restore inIFlasher, but with drawElementOutline and repaintElement as no-ops

Comment 26

[Jonathan Watt [:jwatt]]

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

Comment 27

[Boris Zbarsky [:bzbarsky]]

> I thought DOMi was build and bundled with nightly builds.

It's not.

Comment 28

[Boris Zbarsky [:bzbarsky]]

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.

Comment 29

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/199108b7f25d

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/199108b7f25d

Comment 31

[Tony Mechelynck [:tonymec]]

(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/[e-mail] 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.

Comment 32

[Tony Mechelynck [:tonymec]]

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

Comment 33

[Jonathan Watt [:jwatt]]

Is this safe to land now?

Comment 34

[Jonathan Watt [:jwatt]]

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.

Comment 35

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd85bbe71b4

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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