329181 - [FIX]Eliminate the mPresShell member of nsBoxObject

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

We should just reget the presshell from mContent's current doc as needed.  Except boxobjects are tied to a given doc, so what if anything should we do if the node moves to a different doc?  Not sure how this is supposed to work.  Thoughts?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that if we do this, we can also nix nsXULDocument::OnHide, InvalidatePresentationStuff, etc....  Also not have to pass a presshell to the init method of boxobjects.

Comment 2

[[e-mail]]

(In reply to comment #0)
>We should just reget the presshell from mContent's current doc as needed. 
>Except boxobjects are tied to a given doc, so what if anything should we do if
>the node moves to a different doc?  Not sure how this is supposed to work. 
Removing the element from the document should uninit the box object.
A new box object can be created when the element is back in a document.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w for ease of review

Comment 5

[[e-mail]]

Comment on attachment 214149 [details] [diff] [review]
Same as diff -w for ease of review

> class nsPIBoxObject : public nsIBoxObject
> {
> public:
>-  NS_DEFINE_STATIC_CID_ACCESSOR(NS_PIBOXOBJECT_IID)
>+  NS_DECLARE_STATIC_IID_ACCESSOR(NS_PIBOXOBJECT_IID)
> 
>-  NS_IMETHOD Init(nsIContent* aContent, nsIPresShell* aShell) = 0;
>-  NS_IMETHOD SetDocument(nsIDocument* aDocument) = 0;
>+  NS_IMETHOD Init(nsIContent* aContent) = 0;
> 
>-  NS_IMETHOD InvalidatePresentationStuff() = 0;
>+  // Drop the week ref to the content node as needed
>+  NS_IMETHOD Clear() = 0;
> };
s/week/weak/
s/NS_IMETHOD/virtual void/ perhaps?

>+  virtual nsIFrame* GetFrame(PRBool aFlushLayout);
s/virtual//? I only see one implementation.

>+  if (!aFlushLayout) {
>+  if (aFlushLayout) {
Assuming that this is correct then perhaps one variable name needs changing, or some other form of documentation?

>     nsCOMPtr<nsISupports> supp;
>     mBoxObjectTable->Remove(&key, getter_AddRefs(supp));
>     nsCOMPtr<nsPIBoxObject> boxObject(do_QueryInterface(supp));
Allow me to file a bug on using a decent nsIContent->nsPIBoxObject hashtable :-)

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 214149 [details] [diff] [review]
Same as diff -w for ease of review

sr=jst

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed.  And yes, better hashtables would be nice.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It looks like this increased pageload times by about 0.5% on btek.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yeah... I'm not quite sure why (it really shouldn't take that much more time to get the presshell off the document), and it didn't affect the other tinderboxen...

I suppose we could try to restore the mPresShell cache, but it'd need to be updated when the document's presentation is reconstructed or something...