All users were logged out of Bugzilla on October 13th, 2018

[FIX]Eliminate the mPresShell member of nsBoxObject

RESOLVED FIXED in mozilla1.9alpha1

Status

()

P2
normal
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla1.9alpha1
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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?
(Assignee)

Comment 1

13 years ago
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

13 years ago
(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.
(Assignee)

Comment 3

13 years ago
Created attachment 214148 [details] [diff] [review]
Fix
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #214148 - Flags: superreview?(jst)
Attachment #214148 - Flags: review?(neil)
(Assignee)

Comment 4

13 years ago
Created attachment 214149 [details] [diff] [review]
Same as diff -w for ease of review
Attachment #214149 - Flags: superreview?(jst)
Attachment #214149 - Flags: review?(neil)
(Assignee)

Updated

13 years ago
Attachment #214148 - Flags: superreview?(jst)
Attachment #214148 - Flags: review?(neil)
(Assignee)

Updated

13 years ago
Priority: -- → P2
Summary: Eliminate the mPresShell member of nsBoxObject → [FIX]Eliminate the mPresShell member of nsBoxObject
Target Milestone: --- → mozilla1.9alpha

Comment 5

13 years ago
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 :-)
Attachment #214149 - Flags: review?(neil) → review+
Comment on attachment 214149 [details] [diff] [review]
Same as diff -w for ease of review

sr=jst
Attachment #214149 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 7

13 years ago
Created attachment 214270 [details] [diff] [review]
Updated to comments
Attachment #214148 - Attachment is obsolete: true
Attachment #214149 - Attachment is obsolete: true
(Assignee)

Comment 8

13 years ago
Fixed.  And yes, better hashtables would be nice.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
It looks like this increased pageload times by about 0.5% on btek.
(Assignee)

Comment 10

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

Updated

10 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.