Last Comment Bug 378780 - [FIX]Replace GetShellAt(0) with GetPrimaryShell()
: [FIX]Replace GetShellAt(0) with GetPrimaryShell()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: P1 normal (vote)
: mozilla1.9alpha4
Assigned To: Boris Zbarsky [:bz]
: Hixie (not reading bugmail)
Mentors:
Depends on: 408238
Blocks: 378776
  Show dependency treegraph
 
Reported: 2007-04-25 13:23 PDT by Boris Zbarsky [:bz]
Modified: 2007-12-13 12:33 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
non-automatic-change part (3.93 KB, patch)
2007-04-25 13:35 PDT, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
Details | Diff | Review
Exploratory patch (4.09 KB, patch)
2007-04-25 17:09 PDT, (dormant account)
no flags Details | Diff | Review
Used nsIDocument this time (15.38 KB, patch)
2007-04-25 21:00 PDT, (dormant account)
no flags Details | Diff | Review
This one has more stuff (23.64 KB, patch)
2007-04-30 11:32 PDT, (dormant account)
no flags Details | Diff | Review
more stuff yet (23.64 KB, patch)
2007-04-30 14:07 PDT, (dormant account)
no flags Details | Diff | Review
Correct file (24.51 KB, patch)
2007-04-30 14:09 PDT, (dormant account)
no flags Details | Diff | Review
Diff based on my original patch and Taras' changes (92.67 KB, patch)
2007-04-30 22:27 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
Now with all files (102.57 KB, patch)
2007-05-01 10:11 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review

Description Boris Zbarsky [:bz] 2007-04-25 13:23:26 PDT
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 [:bz] 2007-04-25 13:35:24 PDT
Created attachment 262792 [details] [diff] [review]
non-automatic-change part
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2007-04-25 15:31:31 PDT
Comment on attachment 262792 [details] [diff] [review]
non-automatic-change part

r+sr=jst
Comment 3 (dormant account) 2007-04-25 17:09:00 PDT
Created attachment 262821 [details] [diff] [review]
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 [:bz] 2007-04-25 17:25:25 PDT
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) 2007-04-25 21:00:37 PDT
Created attachment 262847 [details] [diff] [review]
Used nsIDocument this time
Comment 6 Boris Zbarsky [:bz] 2007-04-30 09:48:38 PDT
Hmm.  This still doesn't seem to have picked up all the callsites.  e.g. the one in nsHTMLButtonElement::Click...
Comment 7 Boris Zbarsky [:bz] 2007-04-30 09:49:46 PDT
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) 2007-04-30 11:32:21 PDT
Created attachment 263274 [details] [diff] [review]
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 [:bz] 2007-04-30 12:21:01 PDT
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) 2007-04-30 14:07:38 PDT
Created attachment 263285 [details] [diff] [review]
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) 2007-04-30 14:09:35 PDT
Created attachment 263286 [details] [diff] [review]
Correct file

Uploaded the wrong file.
Comment 12 Boris Zbarsky [:bz] 2007-04-30 20:25:25 PDT
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 [:bz] 2007-04-30 22:14:31 PDT
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 [:bz] 2007-04-30 22:27:53 PDT
Created attachment 263340 [details] [diff] [review]
Diff based on my original patch and Taras' changes

r+sr=me on Taras' stuff, by the way.
Comment 15 Boris Zbarsky [:bz] 2007-05-01 10:11:49 PDT
Created attachment 263371 [details] [diff] [review]
Now with all files
Comment 16 Boris Zbarsky [:bz] 2007-05-01 16:55:33 PDT
Checked in.
Comment 17 Boris Zbarsky [:bz] 2007-05-03 13:13:31 PDT
Hmm.  This apparently missed one callsite in nsEventStateManager.cpp (while getting all the others in that file).  Any idea why?
Comment 18 (dormant account) 2007-05-03 13:33:24 PDT
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 [:bz] 2007-05-03 13:54:19 PDT
> I blame CPP. #if defined(XP_WIN) || defined(XP_OS2)

Oh, I missed that.  Makes sense, then.

Note You need to log in before you can comment on or make changes to this bug.