Closed
Bug 254039
Opened 20 years ago
Closed 19 years ago
deCOMtaminate nsIScrollableFrame
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
People
(Reporter: wlevine, Assigned: wlevine)
Details
Attachments
(1 file, 2 obsolete files)
58.88 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040726 Firefox/0.9.1+ Build Identifier: . Reproducible: Always Steps to Reproduce:
Assignee: nobody → wlevine
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 1•20 years ago
|
||
Okay, here are some thoughts. nsIScrollableFrame is only implemented by nsXULScrollFrame and nsHTMLScrollFrame which have a number of similarities outside of their common inheritance. If some of these got put into nsIScrollableFrame, it could perhaps reduce code size and virtual function calls. For example, if mInner were in nsIScrollableFrame, then certain functions that are implemented identically in ns{XUL|HTML}ScrollFrame that need to access mInner can now be devirtualized. I assume I am allowed to add member variable for this purpose. I guess identical member functions of ns*ScrollFrame might also be moved, but that would only get rid of a duplicated function and not get rid of any virtual-ness. Maybe I am just confused. question: If a function does not need to be virtual, when should it be inlined and when not? If it's not inlined, in what file should the implementation be?
Let's leave nsIScrollableFrame as all pure virtual methods for now. Putting actual code and data in there would violate our general rule that we try to stick to single inheritance. The most important and hardest thing is to clean up the method signatures, because that requires changes to all users of the interface. After we've done that we can think about moving mInner into nsIScrollableFrame and making some stuff inline.
Assignee | ||
Comment 3•20 years ago
|
||
Please take a look at this. I'm not really sure if i should consolidate x and y args into nsPoint, because I don't really see how it's helping.
+ virtual *nsIFrame GetScrolledFrame() const = 0;
that'd be virtual nsIFrame* :-)
+ //not sure if i want to do this
Yes you do! :-)
+ //bleh. not like
+ virtual nsresult ScrollTo(nsPoint aPoint, PRUint32 aFlags =
NS_VMREFRESH_NO_SYNC)=0;
This is the right thing to do. So is there code that checks if ScrollTo failed?
If the answer is "no", make it void.
+ //changing this will require changing nsIScrollableViewProvider and the stuff
that inherits it
Good catch. Let's do it :-)
+ virtual *nsIScrollableView GetScrollableView()=0;
you'll want that to be virtual nsIScrollableView*
+ virtual *nsIBox GetScrollbarBox(PRBool aVertical) = 0;
Similar here
+ virtual nsIContent CurPosAttributeChanged(PRInt32 aModType) = 0;
No, this is a notification. It returns void and takes the nsIContent* as a
parameter.
> I'm not really sure if i should consolidate x and y args into nsPoint, because
> I don't really see how it's helping.
The nsPoint is better because you can do things like "point1 += point2", "rect
+= point", etc. You can also name the parameter something better like
"aScrollPosition". You can also do something like
"ScrollTo(frame->GetPosition(), ...)" which you can't do as easily otherwise.
Assignee | ||
Comment 5•20 years ago
|
||
I saw your comment for GetScrollPreference * XXX roc only 'Auto' is really tested for. This API should be simplified or * eliminated. This is still the case. Can I get rid of GetScrollPreference and add something like PRBool GetAutoPref() (this could use a better name) and then also change nsGfxScrollFrameInner to match?
Assignee | ||
Comment 6•20 years ago
|
||
+#define NS_ISCROLLABLEVIEWPROVIDER_IID_STR "dafcbf5f-701f-4697-a513-81d80e01412c" This isn't used anywhere, let's get rid of it. I'm not completely sure what to do with GetAutoPref yet. It's not right yet (and has never been) since auto-ness can be different for horizontal and vertical scrollbars and the callers probably only care about one of the scrollbars... + return (!(styles.mHorizontal == NS_STYLE_OVERFLOW_SCROLL || styles.mVertical == NS_STYLE_OVERFLOW_SCROLL) && (styles.mVertical == NS_STYLE_OVERFLOW_AUTO || styles.mHorizontal == NS_STYLE_OVERFLOW_AUTO)); This line's too long, break it - scrollFrame->ScrollTo(mPresContext, x, aPosition, NS_SCROLL_PROPERTY_ALWAYS_BLIT); + scrollFrame->ScrollTo(nsPoint(scrollPosition.x, aPosition)); Did you intend to discard NS_SCROLL_PROPERTY_ALWAYS_BLIT?
Also, using -p on your diffs makes them a little easier to read.
+ if (sv = svp->GetScrollableView()) Just do the assignment outside the 'if', this causes warnings on many compilers.
+ if (*aScrollableView = scrollProvider->GetScrollableView()) { Same here
I think what you should do with GetAutoPref is just remove it; make the callers call GetScrollbarStyles() and then write out the definition of GetAutoPref at each call site, with an XXX comment saying that this probably isn't exactly what the caller wants to do there. Then if you want to do a bit more work you can look hard at the callers and see what they're trying to do, and change the code to match.
Assignee | ||
Comment 12•20 years ago
|
||
Update patch addresses comments. Looks like dbaron's recent patches for bug 72747 have dealt with the GetScrollPreference/GetAutoPref mess, which is good because I didn't want to have to do that.
Attachment #155100 -
Attachment is obsolete: true
Attachment #155362 -
Attachment is obsolete: true
Comment on attachment 157905 [details] [diff] [review] patch 2 looks good. I'll check it in. THanks!!!
Attachment #157905 -
Flags: superreview+
Attachment #157905 -
Flags: review+
Checked in, thanks!!!! If you want to do another one, choose from http://wiki.mozilla.org/GeckoDev?DeCOMtamination or find one yourself. I recommend GetNext/PrevInFlow.
This saved about 300 bytes of code in Linux builds.
Comment 16•19 years ago
|
||
This bug could be resolved:fixed, right?
Yeah, OK
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•