:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Description • 11 years ago

Anyone?

	Andrea Marchesini [:baku] Assignee
	Comment 3 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=4c13ff92ea4e

	Andrea Marchesini [:baku] Assignee
	Comment 4 • 11 years ago

In the first patch there is an indentation issue but I don't want to submit a new patch just for this.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 5 • 11 years ago

Comment on attachment 754927 [details] [diff] [review]
part 2 - webidl

Review of attachment 754927 [details] [diff] [review]:
-----------------------------------------------------------------

Just a brief look; I'll try finishing tonight or tomorrow.

::: dom/base/BarProps.cpp
@@ +265,5 @@
>          scroller->GetDefaultScrollbarPreferences(
>                      nsIScrollable::ScrollOrientation_X, &prefValue);
>  
>        if (prefValue == nsIScrollable::Scrollbar_Never)
> +        return false;

With a few early returns, you could deindent this quite a bit, it seems.

::: dom/base/BarProps.h
@@ +38,4 @@
>  
> +  nsPIDOMWindow* GetParentObject() const;
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;

MOZ_FINAL too, I think

::: xpfe/appshell/src/nsXULWindow.cpp
@@ +1987,3 @@
>      contentWin->GetScrollbars(getter_AddRefs(scrollbars));
> +    nsRefPtr<mozilla::dom::BarProp> barProp =
> +      static_cast<mozilla::dom::BarProp*>(scrollbars.get());

I don't like this. If you want to get rid of the interface, can you please add getters on nsPIDOMWindow that return a dom::BarProp* directly? We'll need them for Window anyway.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 7 • 11 years ago

Comment on attachment 754925 [details] [diff] [review]
part 1 - renaming

Review of attachment 754925 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/BarProps.cpp
@@ +119,1 @@
>                                       nsIWebBrowserChrome::CHROME_MENUBAR);

Indentation

::: dom/base/BarProps.h
@@ +9,5 @@
>     the appropriate property (window.menubar.visible)
>  */
>  
> +#ifndef mozilla_dom_BarProps_h___
> +#define mozilla_dom_BarProps_h___

Please drop the trailing underscores while you're here

::: dom/base/moz.build
@@ +69,5 @@
>      'DOMError.cpp',
>      'DOMRequest.cpp',
>      'Navigator.cpp',
>      'URL.cpp',
> +    'BarProps.cpp',

Sort

::: dom/base/nsGlobalWindow.h
@@ +1178,5 @@
> +  nsRefPtr<nsDOMWindowUtils>      mWindowUtils;
> +  nsString                        mStatus;
> +  nsString                        mDefaultStatus;
> +  nsGlobalWindowObserver*         mObserver;
> +  nsCOMPtr<nsIDOMCrypto>          mCrypto;

Leave the others alone, please.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 8 • 11 years ago

Comment on attachment 755394 [details] [diff] [review]
part 2 - webidl

Review of attachment 755394 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/BarProps.cpp
@@ +40,5 @@
> +{
> +  return BarPropBinding::Wrap(aCx, aScope, this);
> +}
> +
> +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(BarProp)

Why don't you need to CC the window?

@@ +255,2 @@
>    nsCOMPtr<nsIDOMWindow> domwin(do_QueryReferent(mDOMWindowWeakref));
> +  if (!domwin) { // dom window not deleted

Drop the comment or at least reverse its meaning

@@ +265,5 @@
> +  }
> +
> +  int32_t prefValue;
> +  scroller->GetDefaultScrollbarPreferences(
> +              nsIScrollable::ScrollOrientation_Y, &prefValue);

if (prefValue != nsIScrollable::Scrollbar_Never) {
  return true;
}

@@ +271,5 @@
> +    scroller->GetDefaultScrollbarPreferences(
> +                nsIScrollable::ScrollOrientation_X, &prefValue);
> +  }
> +
> +  return !(prefValue == nsIScrollable::Scrollbar_Never);

prefValue != nsIScrollable::Scrollbar_Never

::: dom/base/BarProps.h
@@ +38,4 @@
>  
> +  nsPIDOMWindow* GetParentObject() const;
> +
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;

That goes over 80 columns

::: dom/base/nsGlobalWindow.cpp
@@ +3697,5 @@
> +
> +  *aScrollbars = Scrollbars();
> +  if (!*aScrollbars) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }

Drop this

::: dom/interfaces/base/nsIDOMWindow.idl
@@ +52,5 @@
>    readonly attribute nsIDOMHistory                      history;
>  
>  
>    /* [replaceable] locationbar */
> +  readonly attribute nsISupports                        locationbar;

Add /* BarProp */ for all those

::: dom/webidl/BarProp.webidl
@@ +6,5 @@
> +
> +interface BarProp
> +{
> +           [Throws]
> +           attribute boolean          visible;

Use

  [Throws]
           attribute boolean visible;

::: xpfe/appshell/src/nsXULWindow.cpp
@@ +1981,5 @@
>  }
>  
>  void nsXULWindow::SetContentScrollbarVisibility(bool aVisible)
>  {
>    nsCOMPtr<nsIDOMWindow> contentWin(do_GetInterface(mPrimaryContentShell));

Make this nsPIDOMWindow; the cast isn't safe otherwise.

	Andrea Marchesini [:baku] Assignee
	Comment 11 • 11 years ago

> @@ +40,5 @@
> > +{
> > +  return BarPropBinding::Wrap(aCx, aScope, this);
> > +}
> > +
> > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(BarProp)
> 
> Why don't you need to CC the window?

because mDOMWindow is a raw pointer. There is a comment too. Can you give a glance to this and tell me if we should change it to nsRefPtr<>.

	Andrea Marchesini [:baku] Assignee
	Comment 12 • 11 years ago

Comment on attachment 756488 [details] [diff] [review]
part 2 - webidl

Smaug, can you tell me if the use of mDOMWindow without refcount, is right here?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 11 years ago

nsRefPtr<nsGlobalWindow> mDOMWindow would be good and then drop also the weakref.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 14 • 11 years ago

Comment on attachment 756488 [details] [diff] [review]
part 2 - webidl

I think I'd like to even see such change before adding new use for mDOMWindow

	Andrea Marchesini [:baku] Assignee
	Comment 16 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=1e80a02e0b66

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 11 years ago

Comment on attachment 756519 [details] [diff] [review]
part 2 - webidl


> already_AddRefed<nsIWebBrowserChrome>
> BarProp::GetBrowserChrome()
> {
>-  // Check that the window is still alive.
>-  nsCOMPtr<nsIDOMWindow> domwin(do_QueryReferent(mDOMWindowWeakref));
>-  if (!domwin)
>-    return nullptr;
>-
>   return mDOMWindow->GetWebBrowserChrome();
Null-check mDOMWindow

>+bool
>+ScrollbarsProp::GetVisible(ErrorResult& aRv)
> {
>-  *aVisible = true; // one assumes
>+  nsCOMPtr<nsIScrollable> scroller =
>+    do_QueryInterface(mDOMWindow->GetDocShell());
Null-check mDOMWindow

>+  nsCOMPtr<nsIScrollable> scroller =
>+    do_QueryInterface(mDOMWindow->GetDocShell());
ditto
>+'BarProp': {
>+    'headerFile': 'mozilla/dom/BarProps.h',
>+},
Hmm, why do we need this. Shouldn't mozilla/dom/BarProps.h be the default value


>+interface BarProp
>+{
>+  [Throws]
>+           attribute boolean          visible;
odd indentation


nsGlobalWindow should traverse/unlink various *Bars

	Andrea Marchesini [:baku] Assignee
	Comment 18 • 11 years ago

> >+'BarProp': {
> >+    'headerFile': 'mozilla/dom/BarProps.h',
> >+},
> Hmm, why do we need this. Shouldn't mozilla/dom/BarProps.h be the default
> value

Because the object is called 'BarProp' but the header 'BarProps.h' (plural).

> >+interface BarProp
> >+{
> >+  [Throws]
> >+           attribute boolean          visible;
> odd indentation

This is the standard indentation for webidl files. Check the comment written by Ms2ger.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 20 • 11 years ago

Comment on attachment 756893 [details] [diff] [review]
part 2 - webidl

Review of attachment 756893 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/BarProp.webidl
@@ +6,5 @@
> +
> +interface BarProp
> +{
> +  [Throws]
> +           attribute boolean          visible;

Just one space between 'boolean' and 'visible'.

	Phil Ringnalda (:philor)
	Comment 22 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/5eb5520a8ff4
https://hg.mozilla.org/integration/mozilla-inbound/rev/a92e9a47f2c2

	Phil Ringnalda (:philor)
	Comment 23 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/5eb5520a8ff4
https://hg.mozilla.org/mozilla-central/rev/a92e9a47f2c2