User-Agent: Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040202 Firebird/0.8.0+ When implementing horizontal scrollbar support for xul tree, I've hit a situation where the nsIScrollbarMediator interface needs to be able to support more than one scrollbar. See comments in bug 212789 for details: http://bugzilla.mozilla.org/show_bug.cgi?id=212789#c22 Reproducible: Always Steps to Reproduce:

The attached patch fixes the issue in both the layout component and some some (mac specific) code in the widget component. I don't have a mac, so I haven't been able to test these changes, although I expect them to work as they're not major.

all these changes look good to me maybe we could only hold mFrame in the native scrollbar and get content from it when needed

Here's the updated patch. As I noted before I haven't tested or compiled the native mac stuff.

Comment on attachment 140517 [details] [diff] [review] Updated patch (see 'only hold mFrame' suggestion) nice, thanks for doing that however, I'd like to hear from Mike Pinkerton, since he wrote the native scrollbar stuff Could you email him directly?

you also need to update the scollbar code in widget/src/cocoa/nsNativeScrollbar.mm

oh, you did. my bad. sorry.

this doesn't build on mac. nsIScrollbarFrame.h needs to be exported from layout/xul/base/src, from which nothing is currently exported. Seems odd that we're making widget depend on a private layout frame header, no?

Yes, I agree. nsIScrollMediator is also a private interface of the layout component, as noted here: http://bugzilla.mozilla.org/show_bug.cgi?id=212789#c23 Suggestions, anyone? One idea is to leave the current state of affairs (with nsIScrollMediator being exported even though it's a private interface) and instead of using nsIScrollFrame arguments in the nsIScrollMediator methods, we use nsISupports instead. The nsIScrollMediator can then QI these to the appropriate interface. However a more correct solution should probably be found, but I'm not familiar enough with the entire mozilla code set.

See layout/xul/base/public, there are 4 interfaces which are exported and nsIScrollbarMediator is one of them. nsIScrollbarFrame has 2 methods, GetScrollbarMediator(nsIScrollbarMediator** aResult) and SetScrollbarMediator(nsIScrollbarMediator* aMediator) So I don't think we're adding a new "bad" dependency. And according to bug 194385, these dlls might be merged anyway.

Agreed. Roc, this ok with you?

Exporting nsIScrollbarFrame would be OK, but why not just replace the nsIScrollbarFrame parameters in nsIScrollbarMediator with a PRBool aHorizontal (or aVertical) to indicate which scrollbar is affected?

I think it would be more correct to use an interface (whether nsISupports or nsIScrollFrame) to identify which scrollbar is being affected or changed. It's conceivable in theory that there would be more than two scrollbars on a widget (think a 3D rendering widget for example). In addition most of the scrollbar code doesn't know whether it's vertical or horizontal right off hand. Figuring this out whould mean parsing content attributes at many different points. Which is fine if everyone thinks that's the way to go, just adds a bit more complexity.

If we want to go with aHorizontal parameter, we can use nsIBox:GetOrientation for that purpose.

> It's conceivable in theory that there would be more than two scrollbars on a > widget (think a 3D rendering widget for example). Alright then, use a PRInt32 and constants to identify which scrollbar is of interest.

(In reply to comment #13) > If we want to go with aHorizontal parameter, we can use nsIBox:GetOrientation > for that purpose. I'm trying to implement this ... could be mistaken but nsIBox appears to be a private interface of layout. Any other way to get the box other than parsing attributes? And is parsing attributes reliable? I guess it should be for scrollbars? Kludgy though.

Here's a patch using nsISupports instead of nsIScrollbarFrame parameters in the callback methods.

Yeah, it seems you would need nsIBox in the widget module :( Anyway, parsing of attributes is not reliable, because the orientation can be specified in CSS.

Either that, or what seems more apparent is that the layout and widget modules need to be merged (bug 194385) So if we want to get this done any time soon we're left with two solutions: 1. Use nsIScrollFrame arguments and make nsIScrollFrame a public layout header (at least until such a time as the two modules merge). 2. Use nsISupports arguments (3rd patch) #1 seems "more correct" in the programming sense, but personally I just want to get this completed and into the tree.

I incline to the solution #1 (nsIScrollbarFrame public).

Nate, how come you're passing mContent as the scrollframe parameter in the nsNativeScrollbar Mac code?

(In reply to comment #20) > Nate, how come you're passing mContent as the scrollframe parameter in the > nsNativeScrollbar Mac code? My mistake. A dumb assumption that nsIContent was implemented on the same object as nsIFrame/nsIScrollFrame. I'm attaching updated patches for both solutions.

Implementation using nsIScrollbarFrame in the mediator notification methods. When applying this patch, you also need to move nsIScrollbarFrame from layout/xul/base/src to layout/xul/base/public Q: Is there anything else that needs to be done to make the nsIScrollbarFrame.h header public? I updated Makefile.in and Manifest in layout/xul/base/public

Implementation using nsISupports in the mediator methods.

widget really shouldn't know about anything in layout or content. How about adding nsINativeScrollbarObserver to widget, or something like that, and implementing it in the frame code?

(In reply to comment #24) > widget really shouldn't know about anything in layout or content. How about > adding nsINativeScrollbarObserver to widget, or something like that, and > implementing it in the frame code? I won't be able to get into this. Doing anything more than small changes for the Mac without a Mac sucks. Besides, if the two components are going to be merged, then this would just end up as another unnecessary layer of indirection.

Merging libraries doesn't mean we should stop maintaining the logical separation between layers.

Alright, then this bug is up for grabs... Mike, do you have time to get into this?

(In reply to comment #24) > widget really shouldn't know about anything in layout or content. How about > adding nsINativeScrollbarObserver to widget, or something like that, and > implementing it in the frame code? That would cost an extra word on scrollbar frames. Personally I think the nsISupports approach is OK. The nsISupports parameter is opaque as far as the native widget code is concerned. We're not exposing layout guts to widget.

Comment on attachment 145206 [details] [diff] [review] Patch using nsISupports (bug fix, sync with trunk) hmm, it looks simple r=varga

Comment on attachment 145206 [details] [diff] [review] Patch using nsISupports (bug fix, sync with trunk) roc, could you sr?

Comment on attachment 145206 [details] [diff] [review] Patch using nsISupports (bug fix, sync with trunk) #define NS_ISCROLLBARMEDIATOR_IID \ { 0x351c003a, 0xe8f7, 0x4d10, { 0x8b, 0xfa, 0x63, 0x5c, 0x98, 0x60, 0xd6, 0x50 } } As per the new policy, because you're breaking interface compatibility, you should change this to a new IID. Just generate one randomly, there are tools around. [uuid(dd1cb116-1dd1-11b2-9e67-abac4e09d533)] ditto.

Nate, Jan, Roc, is this change something we want to take for 1.7, or would it be better to wait for 1.8a?

Changed the UUIDs as per roc's comment. I don't think it's a priority to get this in till after 1.7. I can continue work on the horizontal scrolling bug as long as I know this is going to get checked in as is.

OK, that works. Mind pinging in this bug once 1.8a opens? Roc or Jan or I can check it in for you then.

Sure. I have several other patches on hold till after 1.7. I'm also going to ask Mike Pinkerton to review, as I haven't built this on the mac, and some code is mac specific.

Comment on attachment 145219 [details] [diff] [review] Identical patch changing UUIDs I don't think we need to do that, the Mac changes are quite trivial.

Nate, are you going to land this?

Sorry, been involved in other things.... I don't have commit access. So, Roc, do you have time to check this in for me?

yes. Taking for checkin.

checked in. Sorry about the delay.

Thanks.