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

RESOLVED FIXED in mozilla1.9alpha4



11 years ago
10 years ago


(Reporter: bz, Assigned: bz)


Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment, 7 obsolete attachments)

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.


11 years ago
Blocks: 378776
Created attachment 262792 [details] [diff] [review]
non-automatic-change part
Assignee: general → bzbarsky
Attachment #262792 - Flags: superreview?(jst)
Attachment #262792 - Flags: review?(jst)


11 years ago
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+

Comment 3

11 years ago
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.
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

11 years ago
Created attachment 262847 [details] [diff] [review]
Used nsIDocument this time
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)/'

Comment 8

10 years ago
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.
Attachment #262847 - Attachment is obsolete: true


10 years ago
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.

Comment 10

10 years ago
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.
Attachment #263274 - Attachment is obsolete: true

Comment 11

10 years ago
Created attachment 263286 [details] [diff] [review]
Correct file

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.
Created attachment 263340 [details] [diff] [review]
Diff based on my original patch and Taras' changes

r+sr=me on Taras' stuff, by the way.
Attachment #262792 - Attachment is obsolete: true
Attachment #263286 - Attachment is obsolete: true
Created attachment 263371 [details] [diff] [review]
Now with all files
Attachment #263340 - Attachment is obsolete: true
Checked in.
Last Resolved: 10 years ago
Resolution: --- → FIXED
Hmm.  This apparently missed one callsite in nsEventStateManager.cpp (while getting all the others in that file).  Any idea why?

Comment 18

10 years ago
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.
Depends on: 408238
You need to log in before you can comment on or make changes to this bug.