Closed Bug 254039 Opened 20 years ago Closed 19 years ago

deCOMtaminate nsIScrollableFrame

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: wlevine, Assigned: wlevine)

Details

Attachments

(1 file, 2 obsolete files)

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
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.
Attached patch New interface methods (obsolete) — Splinter Review
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.
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?
Attached patch patch (obsolete) — Splinter Review
+#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.
Attached patch patch 2Splinter Review
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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: