Get rid of nsIFrameSelection implementation in nsTextControlFrame

RESOLVED FIXED

Status

()

Core
Selection
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Eli Friedman, Assigned: Eli Friedman)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

12 years ago
Created attachment 212432 [details] [diff] [review]
Patch
Attachment #212432 - Flags: review?(roc)
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.
(Assignee)

Comment 3

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

Comment 5

12 years ago
Created attachment 212542 [details] [diff] [review]
What was checked in

For reference.
Attachment #212432 - Attachment is obsolete: true
Attachment #212432 - Flags: review?(roc)
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.