378780 - [FIX]Replace GetShellAt(0) with GetPrimaryShell()

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

As a preliminary to bug 378776 (in which I would like to eliminate GetShellAt(0)),  I think we should add the following API on nsIDocument:

  virtual nsIPresShell *GetPrimaryShell() const = 0;

and implement this on nsDocument as follows:

  nsIPresShell *
  nsDocument::GetPrimaryShell() const
  {
    return mShellsAreHidden ? nsnull :
      NS_STATIC_CAST(nsIPresShell*, mPresShells.SafeElementAt(0));
  }

The change all callers of GetShellAt(0) to calling GetPrimaryShell().

I can post a patch for the former part; taras says he can do the mass-change using his tools.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: non-automatic-change part

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 262792 [details] [diff] [review]
non-automatic-change part

r+sr=jst

Comment 3

[(dormant account)]

Attachment: Exploratory patch

Since the number of lines affected by the patch is rather small it will be quicker to do a simple rename from GetShellAt to GetPrimaryShell(as done in attachment) and then modify the resulting patch by hand.
The file above shows the changes that would happen on linux. For the final patch I would combine linux & mac code, adjust the code by hand and check it on windows. 
Alternatively, I can implement a parameter matching replace for squash by the end of the week and save the manual patch editing step.

Comment 4

[Boris Zbarsky [:bzbarsky]]

That seems to be missing a lot of the callers (e.g. nsXULElement, etc), no?  See http://lxr.mozilla.org/seamonkey/search?string=GetShellAt(0)

Comment 5

[(dormant account)]

Attachment: Used nsIDocument this time

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.  This still doesn't seem to have picked up all the callsites.  e.g. the one in nsHTMLButtonElement::Click...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Note to self: to convert Taras's diff into what I want to check in:

cat ~/foo.patch | sed 's/GetPrimaryShell(0)/GetPrimaryShell()/' |
                  perl -pe 's/GetPrimaryShell\(([^)]+)\)/GetShellAt(\1)/'

Comment 8

[(dormant account)]

Attachment: This one has more stuff

Thanks for the bugreport. This was due to my assumption that the class hierarchy can be inferred from looking at class declarations. I forgot that templates can extend the hierarchy as nsDerivedSafe does.

Hope this one works.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm.  Yeah, that seems to get most of them.  The things I think it should have caught but didn't seem to:

nsDocAccessible::GetBoundsRect
nsElementSH::PostCreate
FreezeSubDocument in nsPresShell.cpp

All the other things it missed are in extensions/, mac widget, beos widget, seamonkey-only code, platform-specific accessibility code, and GTK embedding, which you probably just don't build.

I'll probably run with this version, but you might want to check on those three callsites to see why they didn't get caught.

Comment 10

[(dormant account)]

Attachment: more stuff yet

Sorry about those. I had stale .i files from last week which caused some confusion for the frontend.

Comment 11

[(dormant account)]

Attachment: Correct file

Uploaded the wrong file.

Comment 12

[Boris Zbarsky [:bzbarsky]]

So when I try to actually apply that diff (after changing the +++/--- lines to be relative to the srctree root), it fails to apply...  Not sure why, exactly.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Weird.  It only fails to apply if I actually modify the diff's +++/--- lines.  If I use "patch -p5", it works fine.

Oh, and this change in nsDocument.cpp:

@@ -1980,2 +1980,1 @@
-nsIPresShell *
-nsDocument::GetShellAt(PRUint32 aIndex) const
+class nsIPresShell *nsDocument::GetShellAt(unsigned int aIndex) const 

is not at all what we want; I backed it out.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Diff based on my original patch and Taras' changes

r+sr=me on Taras' stuff, by the way.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with all files

Comment 16

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Hmm.  This apparently missed one callsite in nsEventStateManager.cpp (while getting all the others in that file).  Any idea why?

Comment 18

[(dormant account)]

I blame CPP. #if defined(XP_WIN) || defined(XP_OS2)

That's why we need to add some serious hackery to oink to deal with platform specific macros. I have a two alternatives in mind, all are painful so I haven't gotten far.
a) Have a mac/windows/linux cluster + add support for ms compiler's C++ dialect to elsa(will take some work). Then have squash process 3x as much stuff as it does normally.
b) This one is harder but nicer. Add support for partial parsing, and integrate CPP into oink. Then we could process files and ignore includes that are outside of mozilla. Could run squash with any CPP flags needed then. Problem is that elsa wasn't designed to work that way so it's likely to be a big job.

If you have any ideas on how to tackle this, I'm all ears. This is gonna bite us in every single refactoring & analysis we do.
Of course there is also 
c) Eliminate CPP conditionals in the moz source code :)

Comment 19

[Boris Zbarsky [:bzbarsky]]

> I blame CPP. #if defined(XP_WIN) || defined(XP_OS2)

Oh, I missed that.  Makes sense, then.