Closed Bug 856779 Opened 7 years ago Closed 7 years ago

clean up PresShell init some

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Details

(Whiteboard: [leave open])

Attachments

(5 files, 4 obsolete files)

No description provided.
Attached patch outparamdel NS_NewPresShell (obsolete) — Splinter Review
I'd happily just kill it and inline the one user if you don't think its useful to binary extensions instead.
Attachment #732017 - Flags: review?(bzbarsky)
saddly merging it into the constructor is non-trivial because nsDocumentViewer creates and fiddles with it a bit before Init() gets called.
Attachment #732019 - Flags: review?(dholbert)
Attachment #732021 - Flags: review?(dholbert)
can't go away because NS_NewPresShell() doesn't call it though I guess we could change that...
Attachment #732022 - Flags: review?(bzbarsky)
Attachment #732023 - Flags: review?(dholbert)
Comment on attachment 732017 [details] [diff] [review]
outparamdel NS_NewPresShell

r=me, but I don't see how anyone can use a presshell sanely from a binary addon on its own....
Attachment #732017 - Flags: review?(bzbarsky) → review+
Comment on attachment 732022 [details] [diff] [review]
make PresShell::Init() return void

r=me
Attachment #732022 - Flags: review?(bzbarsky) → review+
Comment on attachment 732019 [details] [diff] [review]
make nsStyleSet::Init() return void

>diff --git a/layout/style/nsStyleSet.h b/layout/style/nsStyleSet.h
[...]
>   // Initialize the object.  You must check the return code and not use
>   // the nsStyleSet if Init() fails.
> 
>-  nsresult Init(nsPresContext *aPresContext);
>+  void Init(nsPresContext *aPresContext);

Delete the second sentence of the Init() header-comment, since it's no longer true, and delete the blank line between the comment and the Init() declaration while you're at it.

(Alternately, if you prefer, feel free to just delete the entire comment; "Init" is pretty self-explanatory.)

Thanks! r=me with that.
Attachment #732019 - Flags: review?(dholbert) → review+
Comment on attachment 732021 [details] [diff] [review]
remove nsFrameManager::Init()

>-nsresult
>-nsFrameManager::Init(nsStyleSet* aStyleSet)
>-{
>-  if (!mPresShell) {
>-    NS_ERROR("null pres shell");
>-    return NS_ERROR_FAILURE;
>-  }
>-
>-  if (!aStyleSet) {
>-    NS_ERROR("null style set");
>-    return NS_ERROR_FAILURE;
>-  }
>-
>-  mStyleSet = aStyleSet;
>-  return NS_OK;
>-}
[...]
>+++ b/layout/base/nsFrameManager.h
>-  nsFrameManager(nsIPresShell *aPresShell) NS_HIDDEN {
>+  nsFrameManager(nsIPresShell *aPresShell, nsStyleSet* aStyleSet) NS_HIDDEN {
>     mPresShell = aPresShell;
>+    mStyleSet = aStyleSet;
>   }

Switch to using a constructor-init-list rather than assignment, in the nsFrameManager constructor there. Also, instead of just dropping Init()'s null-checks, please replace them with assertions (in the constructor).

So the nsFrameManager constructor should look something like the following:

 : mPresShell(aPresShell), mStyleSet(aStyleSet)
 {
   MOZ_ASSERT(mPresShell, "frame manager needs a pres shell");
   MOZ_ASSERT(mStyleSet, "frame manager needs a style set");
 }

r=me with that
Attachment #732021 - Flags: review?(dholbert) → review+
Comment on attachment 732023 [details] [diff] [review]
outparamdel nsIDocument::CreatePresShell()

>diff --git a/content/base/src/nsDocument.h b/content/base/src/nsDocument.h
>   /**
>    * Create a new presentation shell that will use aContext for
>    * its presentation context (presentation context's <b>must not</b> be
>    * shared among multiple presentation shell's).
>    */
>-  virtual nsresult CreateShell(nsPresContext* aContext,
>-                               nsViewManager* aViewManager,
>-                               nsStyleSet* aStyleSet,
>-                               nsIPresShell** aInstancePtrResult);
>+  virtual already_AddRefed<nsIPresShell> CreateShell(nsPresContext* aContext,
>+                                                     nsViewManager* aViewManager,
>+                                                     nsStyleSet* aStyleSet);

While you're here, please fix the typos in the comment above this method:
 s/context's/contexts/
 s/shell's/shells/

>diff --git a/content/html/document/src/nsHTMLDocument.cpp b/content/html/document/src/nsHTMLDocument.cpp
>+++ b/content/html/document/src/nsHTMLDocument.h
>-  virtual nsresult CreateShell(nsPresContext* aContext,
>-                               nsViewManager* aViewManager,
>-                               nsStyleSet* aStyleSet,
>-                               nsIPresShell** aInstancePtrResult);
>+  virtual already_AddRefed<nsIPresShell> CreateShell(nsPresContext* aContext,
>+                                                     nsViewManager* aViewManager,
>+                                                     nsStyleSet* aStyleSet);

Mark this as MOZ_OVERRIDE to ensure that you're actually modifying the signature correctly such that it still overrides the parent impl.

(Looks like a lot of nsHTMLDocument methods could stand to be marked as MOZ_OVERRIDE; but you should at least mark the method whose signature you're changing, to be sure that you're changing it correctly.)

r=me with the above.
Attachment #732023 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #10)
> (Looks like a lot of nsHTMLDocument methods could stand to be marked as
> MOZ_OVERRIDE; but you should at least mark the method whose signature you're
> changing, to be sure that you're changing it correctly.)


I filed bug 856822 on marking methods in nsDocument subclasses as MOZ_OVERRIDE. No obligation, but if you feel like fixing that bug first and then layering this on top of that (so we'll get the "still overriding the inherited impls correctly" verification for free), I'll be happy to review the patch.
> >+++ b/layout/base/nsFrameManager.h
> >-  nsFrameManager(nsIPresShell *aPresShell) NS_HIDDEN {
> >+  nsFrameManager(nsIPresShell *aPresShell, nsStyleSet* aStyleSet) NS_HIDDEN {
> >     mPresShell = aPresShell;
> >+    mStyleSet = aStyleSet;
> >   }
> 
> Switch to using a constructor-init-list rather than assignment, in the

you can't actually do that because  they're members of nsFrameManagerBase, and passing them up to its constructor doesn't help you because its constructor wants to do memset(this, 0, sizeof(nsFrameManager))

> nsFrameManager constructor there. Also, instead of just dropping Init()'s
> null-checks, please replace them with assertions (in the constructor).

I can add asserts
(In reply to Trevor Saunders (:tbsaunde) from comment #12)
> > Switch to using a constructor-init-list rather than assignment, in the
> you can't actually do that because  they're members of nsFrameManagerBase,
> and passing them up to its constructor doesn't help you because its
> constructor wants to do memset(this, 0, sizeof(nsFrameManager))

Ah, I see -- thanks for the explanation. Never mind about that part, then.

(aside: It'd be nice if we could make operator new() do the zeroing, with NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW, like we do for nsPresContext, as noted in its constructor here: https://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#191  That way we could still be clean and use an init list.  But that's orthogonal to this bug.)
(In reply to Daniel Holbert [:dholbert] from comment #13)
> (In reply to Trevor Saunders (:tbsaunde) from comment #12)
> > > Switch to using a constructor-init-list rather than assignment, in the
> > you can't actually do that because  they're members of nsFrameManagerBase,
> > and passing them up to its constructor doesn't help you because its
> > constructor wants to do memset(this, 0, sizeof(nsFrameManager))
> 
> Ah, I see -- thanks for the explanation. Never mind about that part, then.
> 
> (aside: It'd be nice if we could make operator new() do the zeroing, with
> NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW, like we do for nsPresContext, as
> noted in its constructor here:
> https://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.
> cpp#191  That way we could still be clean and use an init list.  But that's
> orthogonal to this bug.)

if someone wanted to be totally crazy they could just merge nsFrameManager / nsFrameManagerBase / nsCSSFrameConstructor into one giant class I'd guess.
next patch will get rid of Init() but this was easy to avoid bit rott
Attachment #732575 - Flags: review?(bzbarsky)
Attachment #732577 - Flags: review?(bzbarsky)
Comment on attachment 732575 [details] [diff] [review]
bug 856779 - kill NS_NewPresShell()

r=me
Attachment #732575 - Flags: review?(bzbarsky) → review+
Comment on attachment 732577 [details] [diff] [review]
bug 856779 - muke nsIPresShell::Init()

>+  NS_PRECONDITION(nullptr != aDocument, "null ptr");

Just drop the "nullptr != " bit.

But also, why is this patch moving the isupports impl and the dtor around?  Can you please put those back?
> But also, why is this patch moving the isupports impl and the dtor around? 
> Can you please put those back?

WE USED OT HAVE

{
CONSTRUCTOR STUFF;
}

DTOR;

NSisUPPORTS IMPL;

{
init stuff;
}

I moved the init stuff up into the constructor, but diff decided to represent that as moving the dtor and nsISupports stuff down.
Comment on attachment 732577 [details] [diff] [review]
bug 856779 - muke nsIPresShell::Init()

Ah, I see.  And we don't really want to move the ctor header down below the nsisupports bits, I guess.

r=me
Attachment #732577 - Flags: review?(bzbarsky) → review+
Whiteboard: [leave open]
part 4 was bad because nsCaret::INit() happens now in PreshShell::PresShell() and refcounts the pres shell but landed parts 1 - 3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/590e354d2750
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b56af2a80519
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eebe60eebd95
Status: NEW → ASSIGNED
Version: unspecified → Trunk
Attached patch final part 4Splinter Review
more or less brought back the first version of this that made PresShell::Init() return void, but since NS_NewPresShell() went away I devirtualized it too.
Attachment #732575 - Attachment is obsolete: true
Attached patch fianl part 5Splinter Review
Attachment #732017 - Attachment is obsolete: true
Attachment #732023 - Attachment is obsolete: true
Attachment #732577 - Attachment is obsolete: true
Comment on attachment 744609 [details] [diff] [review]
final part 4

Boris not sure if you want to review this again or not, but I figure its changed enough from the first version making PresShell::Init() return void that I figure I might as well ask.
Attachment #744609 - Flags: review?(bzbarsky)
Comment on attachment 744609 [details] [diff] [review]
final part 4

r=me
Attachment #744609 - Flags: review?(bzbarsky) → review+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.