Merge nsView into nsIView and clean it up a bit

RESOLVED FIXED in mozilla20

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

18 Branch
mozilla20
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments)

81.79 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.39 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.47 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.44 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.08 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.22 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
17.35 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
10.59 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
227.55 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.18 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
There's a lot of cruft here. This bug is about removing some of it. In particular we can remove nsIView's QueryInterface and IID, which will avoid having to rev the IID in future cleanup/removal of nsIView.
Created attachment 696985 [details] [diff] [review]
Part 1: Merge nsView into nsIView, making all references to nsView refer to nsIView instead
Attachment #696985 - Flags: review?(tnikkel)
Created attachment 696986 [details] [diff] [review]
Part 2: Remove nsIView::ExternalIsRoot
Attachment #696986 - Flags: review?(tnikkel)
Created attachment 696987 [details] [diff] [review]
Part 3: Remove some unnecessary 'virtual's
Attachment #696987 - Flags: review?(tnikkel)
Created attachment 696988 [details] [diff] [review]
Part 4: Remove nsIView's IID as it's not needed anymore
Attachment #696988 - Flags: review?(tnikkel)
Created attachment 696989 [details] [diff] [review]
Part 5: Remove NS_IMETHOD declarations that aren't needed anymore
Attachment #696989 - Flags: review?(tnikkel)
Created attachment 696990 [details] [diff] [review]
Part 6: Annotate nsIWidgetListener method implementations to indicate that they're virtual and must override
Attachment #696990 - Flags: review?(tnikkel)
Attachment #696990 - Attachment is patch: true
Comment on attachment 696985 [details] [diff] [review]
Part 1: Merge nsView into nsIView, making all references to nsView refer to nsIView instead

Can we make as many functions as possible private in nsIView? Most of the ones added from nsView could be private, no?

It would be nice if we didn't have both nsView and nsIView floating around, even if it's just a filename (nsView.cpp).

>diff --git a/view/public/nsIView.h b/view/public/nsIView.h
>+  // These are defined exactly the same in nsIView, but for now they have to be redeclared
>+  // here because of stupid C++ method hiding rules

I think this comment needs to be changed, are we re-declaring anything?

> nsViewManager::CreateView(const nsRect& aBounds,
>-    v->SetParent(static_cast<nsView*>(const_cast<nsIView*>(aParent)));
>+    v->SetParent(static_cast<nsIView*>(const_cast<nsIView*>(aParent)));

Extra unneeded cast here.

> NS_IMETHODIMP nsViewManager::SetRootView(nsIView *aView)
> {
>-  nsView* view = static_cast<nsView*>(aView);
>+  nsIView* view = static_cast<nsIView*>(aView);

You don't need this cast, it's already the right type.

> NS_IMETHODIMP nsViewManager::InvalidateViewNoSuppression(nsIView *aView,
>-  nsView* view = static_cast<nsView*>(aView);
>+  nsIView* view = static_cast<nsIView*>(aView);

And here too.

>-  nsView* displayRoot = static_cast<nsView*>(GetDisplayRootFor(view));
>-  nsViewManager* displayRootVM = displayRoot->GetViewManager();
>+  nsIView* displayRoot = static_cast<nsIView*>(GetDisplayRootFor(view));
>+  nsViewManager* displayRootVM = displayRoot->GetViewManagerInternal();

And here.

>@@ -807,18 +807,18 @@ void nsViewManager::ReparentChildWidgets
>-  nsView* view = static_cast<nsView*>(aView);
>-  for (nsView *kid = view->GetFirstChild(); kid; kid = kid->GetNextSibling()) {
>+  nsIView* view = static_cast<nsIView*>(aView);
>+  for (nsIView *kid = view->GetFirstChild(); kid; kid = kid->GetNextSibling()) {

And here.

>@@ -827,33 +827,33 @@ void nsViewManager::ReparentWidgets(nsIV
>-  nsView* view = static_cast<nsView*>(aView);
>+  nsIView* view = static_cast<nsIView*>(aView);

And here.

> NS_IMETHODIMP nsViewManager::InsertChild(nsIView *aParent, nsIView *aChild, nsIView *aSibling,
>-  nsView* parent = static_cast<nsView*>(aParent);
>-  nsView* child = static_cast<nsView*>(aChild);
>-  nsView* sibling = static_cast<nsView*>(aSibling);
>+  nsIView* parent = static_cast<nsIView*>(aParent);
>+  nsIView* child = static_cast<nsIView*>(aChild);
>+  nsIView* sibling = static_cast<nsIView*>(aSibling);

Here.

> NS_IMETHODIMP nsViewManager::RemoveChild(nsIView *aChild)
>-  nsView* child = static_cast<nsView*>(aChild);
>+  nsIView* child = static_cast<nsIView*>(aChild);

Here.

> NS_IMETHODIMP nsViewManager::MoveViewTo(nsIView *aView, nscoord aX, nscoord aY)
>-  nsView* view = static_cast<nsView*>(aView);
>+  nsIView* view = static_cast<nsIView*>(aView);

Here.

> NS_IMETHODIMP nsViewManager::ResizeView(nsIView *aView, const nsRect &aRect, bool aRepaintExposedAreaOnly)
>-  nsView* view = static_cast<nsView*>(aView);
>+  nsIView* view = static_cast<nsIView*>(aView);

Here.

> NS_IMETHODIMP nsViewManager::SetViewFloating(nsIView *aView, bool aFloating)
>-  nsView* view = static_cast<nsView*>(aView);
>+  nsIView* view = static_cast<nsIView*>(aView);

Here.

> NS_IMETHODIMP nsViewManager::SetViewVisibility(nsIView *aView, nsViewVisibility aVisible)
>-  nsView* view = static_cast<nsView*>(aView);
>+  nsIView* view = static_cast<nsIView*>(aView);

Here.

> NS_IMETHODIMP nsViewManager::SetViewZIndex(nsIView *aView, bool aAutoZIndex, int32_t aZIndex, bool aTopMost)
>-  nsView* view = static_cast<nsView*>(aView);
>+  nsIView* view = static_cast<nsIView*>(aView);

Here.
Attachment #696986 - Flags: review?(tnikkel) → review+
Attachment #696987 - Flags: review?(tnikkel) → review+
Attachment #696988 - Flags: review?(tnikkel) → review+
Attachment #696989 - Flags: review?(tnikkel) → review+
Attachment #696990 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tn) from comment #7)
> Can we make as many functions as possible private in nsIView? Most of the
> ones added from nsView could be private, no?

I'll do that in a followup patch.

> It would be nice if we didn't have both nsView and nsIView floating around,
> even if it's just a filename (nsView.cpp).

I'll do a mass rename of nsIView to nsView in a followup patch.

> >diff --git a/view/public/nsIView.h b/view/public/nsIView.h
> >+  // These are defined exactly the same in nsIView, but for now they have to be redeclared
> >+  // here because of stupid C++ method hiding rules
> 
> I think this comment needs to be changed, are we re-declaring anything?

Fixed.

I'll attach another followup patch to remove the unnecessary casts.
Comment on attachment 696985 [details] [diff] [review]
Part 1: Merge nsView into nsIView, making all references to nsView refer to nsIView instead

Sounds good.
Attachment #696985 - Flags: review?(tnikkel) → review+
Created attachment 697417 [details] [diff] [review]
Part 1.5: Remove unnecessary static casts
Attachment #697417 - Flags: review?(tnikkel)
Created attachment 697419 [details] [diff] [review]
Part 7: Make a number of nsIView methods private
Attachment #697419 - Flags: review?(tnikkel)
Created attachment 697420 [details] [diff] [review]
Part 8:  Mass-rename of nsIView to nsView
Attachment #697420 - Flags: review?(tnikkel)
Created attachment 697421 [details] [diff] [review]
Part 9: Remove Set/GetViewFlags since they're unused
Attachment #697421 - Flags: review?(tnikkel)
Attachment #697417 - Attachment is patch: true
Attachment #697417 - Flags: review?(tnikkel) → review+
Attachment #697419 - Flags: review?(tnikkel) → review+
Attachment #697421 - Flags: review?(tnikkel) → review+
Comment on attachment 697420 [details] [diff] [review]
Part 8:  Mass-rename of nsIView to nsView

> diff --git a/docshell/base/nsIContentViewer.idl b/docshell/base/nsIContentViewer.idl
> -[ptr] native nsIViewPtr(nsIView);
> +[ptr] native nsIViewPtr(nsView);

nsViewPtr.
Attachment #697420 - Flags: review?(tnikkel) → review+
You need to log in before you can comment on or make changes to this bug.