Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Assignee
	Description • 12 years ago

For B2G it would be useful to be able to hide the overlaying scrollbars if they are unactive.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Assignee
	Comment 1 • 12 years ago

This patch is shamelessly stolen from bug 636564 but I have removed all the unneeded parts. It adds a new ScrollbarActivity class that toggle an active flag for scrollbars.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 3 • 12 years ago

Comment on attachment 647233 [details] [diff] [review]
Add a ScrollbarActivity class

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

::: layout/generic/ScrollbarActivity.h
@@ +16,5 @@
> +
> +namespace mozilla {
> +namespace layout {
> +
> +class ScrollbarActivity : public nsISupports {

This needs a big comment explaining what it's for and how it works.

Also, it can just live in the mozilla namespace.

@@ +33,5 @@
> +  void ActivityEnded();
> +  void Destroy();
> +
> +protected:
> +  PRInt32 mNestedActivityCounter;

This needs to be documented. Also, put the member variables together in the file.

@@ +38,5 @@
> +  bool IsActivityOngoing() {
> +    return mNestedActivityCounter > 0;
> +  }
> +
> +  bool mIsActive;

Same here.

@@ +41,5 @@
> +
> +  bool mIsActive;
> +  void SetIsActive(bool aNewActive);
> +
> +  static const PRUint32 kScrollbarActivityEndedDelay = 450; // milliseconds

This won't compile on some compilers. Use an enum instead.

::: layout/generic/nsGfxScrollFrame.h
@@ +117,5 @@
>    private:
>      nsGfxScrollFrameInner *mInner;
>    };
>  
> +  void FinishReflowForScrollbar(nsIContent* aContent, nscoord aMinX,

Why did this change to aMinX?

	[:fabrice] Fabrice Desré
	Comment 4 • 12 years ago

Comment on attachment 647234 [details] [diff] [review]
b2g/ part. Show/Hide the scrollbar based on the |active| attribute

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

::: b2g/chrome/content/content.css
@@ +46,3 @@
>  xul|scrollbar[disabled] {
> +  opacity: 0;
> +  -moz-transition: opacity 1s ease;

Nit: maybe 1s is a bit long?

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Assignee
	Comment 5 • 12 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
 
> ::: layout/generic/nsGfxScrollFrame.h
> @@ +117,5 @@
> >    private:
> >      nsGfxScrollFrameInner *mInner;
> >    };
> >  
> > +  void FinishReflowForScrollbar(nsIContent* aContent, nscoord aMinX,
> 
> Why did this change to aMinX?

That's a typo, sorry.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 6 • 12 years ago

Comment on attachment 648198 [details] [diff] [review]
Patch v0.2

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

::: layout/generic/ScrollbarActivity.cpp
@@ +13,5 @@
> +NS_IMPL_ISUPPORTS0(ScrollbarActivity)
> +
> +void
> +ScrollbarActivity::Destroy() {
> +  CancelActivityFinishedTimer();

{ on new line

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Assignee
	Comment 7 • 12 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Comment on attachment 648198 [details] [diff] [review]
> Patch v0.2
> 
> Review of attachment 648198 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/ScrollbarActivity.cpp
> @@ +13,5 @@
> > +NS_IMPL_ISUPPORTS0(ScrollbarActivity)
> > +
> > +void
> > +ScrollbarActivity::Destroy() {
> > +  CancelActivityFinishedTimer();
> 
> { on new line

Thanks a lot for the quick review.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 12 years ago

Comment on attachment 648198 [details] [diff] [review]
Patch v0.2

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

::: layout/generic/ScrollbarActivity.h
@@ +36,5 @@
> + * ActivityOccured() has been made. When the timeout expires ActivityFinished()
> + * is call and reset the active state.
> + */
> +
> +class ScrollbarActivity : public nsISupports {

Actually, why does this have to be refcounted? I think you can and should just make this explicitly managed and give nsGfxScrollFrameInner an nsAutoPtr<ScrollbarActivity>. Instead of Destroy() you can just use the destructor.

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) Assignee
	Comment 10 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f687cf5c1bb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/c449b548784e

	Ed Morley [:emorley]
	Comment 11 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/f687cf5c1bb5
https://hg.mozilla.org/mozilla-central/rev/c449b548784e

	Leonard Camacho [:lcamacho]
	Comment 12 • 12 years ago

With XFCE with Ubuntu I'm seeing a regression in Firefox nightly where bugzilla, gmail and other pages shows a horizontal scrollbar even when the content fit on window.

After use mozregression I get this pushlog
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=20fc34efd733&tochange=9453cf029b72

It is possible that this bug or another one related is causing this regression?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 13 • 12 years ago

That's bug 781086.