Closed Bug 378780 Opened 13 years ago Closed 13 years ago

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


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






(Reporter: bzbarsky, Assigned: bzbarsky)




(1 file, 7 obsolete files)

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.
Blocks: 378776
Attached patch non-automatic-change part (obsolete) — Splinter Review
Assignee: general → bzbarsky
Attachment #262792 - Flags: superreview?(jst)
Attachment #262792 - Flags: review?(jst)
Priority: -- → P1
Summary: Replace GetShellAt(0) with GetPrimaryShell() → [FIX]Replace GetShellAt(0) with GetPrimaryShell()
Target Milestone: --- → mozilla1.9alpha4
Comment on attachment 262792 [details] [diff] [review]
non-automatic-change part

Attachment #262792 - Flags: superreview?(jst)
Attachment #262792 - Flags: superreview+
Attachment #262792 - Flags: review?(jst)
Attachment #262792 - Flags: review+
Attached patch Exploratory patch (obsolete) — Splinter Review
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.
That seems to be missing a lot of the callers (e.g. nsXULElement, etc), no?  See
Attached patch Used nsIDocument this time (obsolete) — Splinter Review
Attachment #262821 - Attachment is obsolete: true
Hmm.  This still doesn't seem to have picked up all the callsites.  e.g. the one in nsHTMLButtonElement::Click...
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)/'

Attached patch This one has more stuff (obsolete) — Splinter Review
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.
Attachment #262847 - Attachment is obsolete: true
Attachment #263274 - Attachment is patch: true
Attachment #263274 - Attachment mime type: application/octet-stream → text/plain
Hmm.  Yeah, that seems to get most of them.  The things I think it should have caught but didn't seem to:

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.
Attached patch more stuff yet (obsolete) — Splinter Review
Sorry about those. I had stale .i files from last week which caused some confusion for the frontend.
Attachment #263274 - Attachment is obsolete: true
Attached patch Correct file (obsolete) — Splinter Review
Uploaded the wrong file.
Attachment #263285 - Attachment is obsolete: true
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.
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.
r+sr=me on Taras' stuff, by the way.
Attachment #262792 - Attachment is obsolete: true
Attachment #263286 - Attachment is obsolete: true
Attachment #263340 - Attachment is obsolete: true
Checked in.
Closed: 13 years ago
Resolution: --- → FIXED
Hmm.  This apparently missed one callsite in nsEventStateManager.cpp (while getting all the others in that file).  Any idea why?
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 :)
> I blame CPP. #if defined(XP_WIN) || defined(XP_OS2)

Oh, I missed that.  Makes sense, then.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.