Closed Bug 692357 Opened 8 years ago Closed 8 years ago

Code cleanups in sidebar dragging code

Categories

(Firefox for Android Graveyard :: General, defect, minor)

Firefox 10
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 10

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

(Whiteboard: [pushed][QA-])

Attachments

(2 files)

I don't like the way ViewableAreaObserver accesses a "private" member of TabletSidebar.  Especially now that TabletSidebar is in its own lazy-loaded file, it would be good not to access it at all on non-tablet systems.  This patch removes that dependency and instead adds a more proper way for TabletSidebar to control ViewableAreaObserver's behavior.
Attachment #565122 - Flags: review?(lucasr.at.mozilla)
This takes the code from bug 689494, cleans it up a little, and moves it into TabletSidebar where it can be used for drags that start inside *or* outside the sidebar.  (Currently bug 689494 is still unfixed for drags that start in the 30px "gutter" next the sidebar.)

It also makes sure that this code is called only on the first dragMove so it can't impact panning performance.
Attachment #565124 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 565124 [details] [diff] [review]
part 2: Move grabbing logic out of dragger code and into TabletSidebar

Drive by 

>diff --git a/mobile/chrome/content/TabletSidebar.js b/mobile/chrome/content/TabletSidebar.js

>+  tryGrab: function tryGrab(aDx) {

>+    if (willShow != this.visible) {
>+      TabletSidebar.grab();
>+      return true;
>+    }

TabletSidebar.grab() -> this.grab()
Comment on attachment 565122 [details] [diff] [review]
part 1: ViewableAreaObserver should not depend on TabletSidebar

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

Looks good with the nits fixed.

::: mobile/chrome/content/TabletSidebar.js
@@ +69,5 @@
>    grab: function grab() {
>      this._grabbed = true;
> +    // While sliding the sidebar, tell ViewableAreaObserver to size the browser
> +    // as if the sidebar weren't there.  This avoids resizing the browser while dragging.
> +    ViewableAreaObserver.update({ignoreSidebar: true});

Maybe it should be ignoreTabletSidebar for clarity. There are other "sidebars" in the code.

There should be spaces around the json content for consistency. i.e. { ignoreSidebar: true }

@@ +103,5 @@
>      if (finalOffset > (ViewableAreaObserver.sidebarWidth / 2) ^ rtl)
>        this.hide();
>      else
>        // we already called show() in grab; just need to update the width again.
> +      ViewableAreaObserver.update({ignoreSidebar: false});

Same here.

::: mobile/chrome/content/browser.js
@@ +3253,5 @@
>  
> +  update: function va_update(aParams) {
> +    aParams = aParams || {};
> +    if ("ignoreSidebar" in aParams)
> +      this._ignoreSidebar = aParams.ignoreSidebar;

I'd find it slightly surprising that the params you're passing to the update() call 'stick'. Usually params like this only apply to the given method call. No strong feelings about it though. Just wondering.
Attachment #565122 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 565124 [details] [diff] [review]
part 2: Move grabbing logic out of dragger code and into TabletSidebar

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

Cool.

::: mobile/chrome/content/browser.js
@@ +1339,5 @@
>  
>    dragMove: function dragMove(dx, dy, scroller, aIsKinetic) {
> +    if (this._canGrabSidebar) {
> +      this._grabSidebar = TabletSidebar.tryGrab(dx);
> +      this._canGrabSidebar = false; // After trying once, don't keep checking every move.

Coding style nit: I usually prefer comments on top of the line when it's inside a code block. For constant and object property declarations, comments on the side a fine. Feel free to ignore if comment on the side is the common practice.

::: mobile/chrome/content/tabs.xml
@@ +217,5 @@
>  
>              dragMove: function dragMove(dx, dy) {
> +              if (this._canGrabSidebar) {
> +                this._grabSidebar = TabletSidebar.tryGrab(dx);
> +                this._canGrabSidebar = false; // After trying once, don't keep checking every move.

Same coding style nit here.
Attachment #565124 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #3)
> I'd find it slightly surprising that the params you're passing to the
> update() call 'stick'. Usually params like this only apply to the given
> method call. No strong feelings about it though. Just wondering.

I changed the name of the param to setIgnoreTabletSidebar to try to make this clearer.

Pushed with that and other review comments fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c2ce4e7ff2
https://hg.mozilla.org/integration/mozilla-inbound/rev/87f58aa55ce8
Whiteboard: [pushed]
Target Milestone: --- → Firefox 10
https://hg.mozilla.org/mozilla-central/rev/d8c2ce4e7ff2
https://hg.mozilla.org/mozilla-central/rev/87f58aa55ce8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [pushed] → [pushed][QA-]
Depends on: 709485
You need to log in before you can comment on or make changes to this bug.