327868 - Get rid of nsIFrameSelection implementation in nsTextControlFrame

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Eli Friedman]

Patch coming up; patch unifies getting an nsIFrameSelection pointer from a frame and removes implemetation from nsTextControlFrame.  Also a few bits of misc. nsTextControlFrame cleanup.

I actually hadn't figured out what the whole QI+PresShell business that was copied all over the place was supposed to do until I started thinking about what removing the nsTextControlFrame implementation would break.  You'd think somebody would've come up with a helper after about 10 copies of the same nonsense pattern got into the tree.

Comment 1

[Eli Friedman]

Attachment: Patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 212432 [details] [diff] [review]
Patch

+  mSelCon = 0;
+  mFrameSel = 0;

make these nsnull

+  mSelCon = (nsISelectionController*)

NS_STATIC_CAST

+  GetFrameSelection()->GetDisplaySelection(&displayresult);
....
+    frameselection = GetFrameSelection();

You could just call GetFrameSelection() once here.

-  if (aPresContext->Medium() == nsLayoutAtoms::print) {
-    if (aPresContext->Type() == nsPresContext::eContext_PrintPreview) {
-      // for print preview we want to create the view and widget but
-      // we do not want to load the document, it is already loaded.
-      rv = CreateViewAndWidget(eContentTypeContent);

Why are you removing this?

Why is GetFrameSelection virtual? Seems like you could make it a non-virtual method of nsIFrame. If it's because of the call from embedding/components/find/src/nsWebBrowserFind.cpp, then I think you should define a virtual helper function in nsIPresShell, GetFrameSelectionFor, so we don't have to bloat the vtable of every frame class.

Comment 3

[Eli Friedman]

(In reply to comment #2)
> +  mSelCon = 0;
> +  mFrameSel = 0;
> 
> make these nsnull
Done.

> +  mSelCon = (nsISelectionController*)
> 
> NS_STATIC_CAST
Done.

> +  GetFrameSelection()->GetDisplaySelection(&displayresult);
> ....
> +    frameselection = GetFrameSelection();
> 
> You could just call GetFrameSelection() once here.
Done.

> -  if (aPresContext->Medium() == nsLayoutAtoms::print) {
> -    if (aPresContext->Type() == nsPresContext::eContext_PrintPreview) {
> -      // for print preview we want to create the view and widget but
> -      // we do not want to load the document, it is already loaded.
> -      rv = CreateViewAndWidget(eContentTypeContent);
> 
> Why are you removing this?
I didn't mean to put that into this patch.  I'll take it out of the next patch.

> Why is GetFrameSelection virtual? Seems like you could make it a non-virtual
> method of nsIFrame. If it's because of the call from
> embedding/components/find/src/nsWebBrowserFind.cpp, then I think you should
> define a virtual helper function in nsIPresShell, GetFrameSelectionFor, so we
> don't have to bloat the vtable of every frame class.
Actually, no good reason; made non-virtual.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

These are all simple changes. Just go ahead and make them and check in with my r+sr.

> I didn't mean to put that into this patch.

Ooh, does this mean you're fixing printing? I can't wait :-)

Comment 5

[Eli Friedman]

Attachment: What was checked in

For reference.