Closed Bug 462425 Opened 12 years ago Closed 12 years ago

Convert Fennec to use WidgetStack framework for panning

Categories

(Firefox for Android Graveyard :: Panning/Zooming, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached file diff from Vlad's original WidgetStack (obsolete) —
Part of the unified panning work will depend on switching to a helper class for doing layout management. We are building on a class called WidgetStack that Vlad put together.
Attached patch wip 1: first patch (obsolete) — Splinter Review
This patch is my first attempt to integrate WidgetStack into Fennec.
* added a redrawHandler to deckbrowser
* commented out all mouse handlers in deckbrowser
* commented out all mouse handlers in browser-ui
* added constraints to parts of the XUL chrome UI in browser.xul

This patch pans the welcome screen, but does not handle pages wider than 800px correctly.
Assignee: nobody → mark.finkle
Note that the patch has some cruft in it I forgot to remove. The cruft shouldn't hurt.
Attached image panning problem for wide pages (obsolete) —
Shows the content not being painted for wide pages
where can I learn more about this widgetstack element?  I haven't heard a thing about it until now and querying bugzilla is turning up nothing except this bug.
(In reply to comment #4)
> where can I learn more about this widgetstack element?  I haven't heard a thing
> about it until now and querying bugzilla is turning up nothing except this bug.

WidgetStack is a JS helper class that manages panning of child XUL elements inside a <stack>. 

attachment 346258 [details] has the code in it.
This patch is based on the previous patch. It makes some broader changes. WidgetStack is currently designed to assume all elements are their true size, no faking like we currently do with the content area. Therefore, I decided to make the canvas the exact same size as the web content:
* This makes widget stack work much better
* This is bad for performance since we don't need to draw the entire canvas, just what's visible.
* Adding and switching tabs seems to work too

Problems:
* For some reason, when a page is wider than 800px, the content area never becomes smaller than that size.
  * deckbrowser._contentAreaDimensions checks the body and html widths. the html width is always the size of the largest content to get loaded.

Anyway, it's getting better
Attachment #346258 - Attachment is obsolete: true
Attached patch wip 3: canvas is now 800x480 (obsolete) — Splinter Review
This patch tweaks the previous code so that the <canvas> is no longer as large as the content. The <canvas> is now fixed at 800x480, while the parent container is the full webcontent size. As we pan, the canvas is moved around so it always sits in the right spot.

You can now see the checkerboard as you pan around.

code notes:
* The only method we touch in deckbrowser (for rendering) is redrawHandler. It supports "move" and "paint" stages.
  * "move" should only move the canvas around (mousemove)
  * "paint" should move and render (mouseup)
Blocks: 462446
OS: Windows XP → All
Hardware: PC → All
This patch merges mb-cleanup into mobile-browser. I added the 2 patches from m-b that weren't already in the mb-cleanup repo. I also fixed some tabs, commented out some dumps and removed some dead code.
Attachment #345562 - Attachment is obsolete: true
Attachment #346164 - Attachment is obsolete: true
Attachment #346514 - Attachment is obsolete: true
Attachment #346756 - Attachment is obsolete: true
Comment on attachment 349485 [details] [diff] [review]
patch to merge from mb-cleanup to mobile-browser


>   _setContentPosition : function (aProp, aValue) {
>-    let value = Math.round(aValue);
>-    if (aProp == "left") {
>-      gContentBox.style.marginLeft = value + "px";
>-      gContentBox.style.marginRight = -value + "px";
>-    } else if (aProp == "top") {
>-      gContentBox.style.marginTop = value + "px";
>-      gContentBox.style.marginBottom = -value + "px";
>-    }
>   },

this function should be removed rather than no-op'd


>+// XXX
>+/*
>       tabbar.left = newLeft;
> 
>       let newToolbarLeft = newLeft;
>       if (newToolbarLeft < 0 && aMode != UIMODE_PANEL)
>         newToolbarLeft = 0;
>       else if (newToolbarLeft < 0 && aMode == UIMODE_PANEL)
>         newToolbarLeft += tabbarW + sidebarW;
>       toolbar.left = newToolbarLeft;
> 
>       this._setContentPosition("left", newLeft + tabbarW);
>       sidebar.left = newLeft + tabbarW + contentW;
>       panelUI.left = newLeft + tabbarW + contentW + sidebarW;
>       panelUI.width = contentW;
>+*/
>   },

delete this instead of commenting it out

>   _sizeControls : function(aEvent) {
>+    // XXX
>+/*
>     if (window != aEvent.target) {
>       return
>     }
>     var rect = document.getElementById("browser-container").getBoundingClientRect();
>     var containerW = rect.right - rect.left;
>     var containerH = rect.bottom - rect.top;
>     var toolbar = document.getElementById("toolbar-main");
>     var toolbarH = toolbar.boxObject.height;
> 
>     var sidebar = document.getElementById("browser-controls");
>     var panelUI = document.getElementById("panel-container");
>-    var tabbar = document.getElementById("tab-list-container");
>+    var tabbar = document.getElementById("tabs-container");
>     tabbar.left = -tabbar.boxObject.width;
>     panelUI.left = containerW + sidebar.boxObject.width;
>     sidebar.left = containerW;
>     sidebar.height = tabbar.height = (panelUI.height = containerH) - toolbarH;
>     panelUI.width = containerW - sidebar.boxObject.width - tabbar.boxObject.width;
> 
>     var popup = document.getElementById("popup_autocomplete");
>     toolbar.width = containerW;
>     popup.height = containerH - toolbarH;
>+*/
>   },

same here



>+    // XXX these really want to listen whatever is the current browser, not the current browser

nit, this comment doesn't really make sense


>     // XXXndeakin remove the try-catch once the search service is properly built
>     try {
>       var searchService = Cc["@mozilla.org/browser/search-service;1"].
>                           getService(Ci.nsIBrowserSearchService);
>-    } catch (ex) {
>+    }
>+    catch (ex) {
>       this.engines = [ ];
>       return;
>     }

is there a reason for the new line?
(In reply to comment #9)
> (From update of attachment 349485 [details] [diff] [review])
> 
> >   _setContentPosition : function (aProp, aValue) {
> >   },
> 
> this function should be removed rather than no-op'd

Done, & removed _contentTop as well
> 
> 
> >+// XXX
> >+/*
> >       tabbar.left = newLeft;
> > 
> >       let newToolbarLeft = newLeft;
> >       if (newToolbarLeft < 0 && aMode != UIMODE_PANEL)
> >         newToolbarLeft = 0;
> >       else if (newToolbarLeft < 0 && aMode == UIMODE_PANEL)
> >         newToolbarLeft += tabbarW + sidebarW;
> >       toolbar.left = newToolbarLeft;
> > 
> >       this._setContentPosition("left", newLeft + tabbarW);
> >       sidebar.left = newLeft + tabbarW + contentW;
> >       panelUI.left = newLeft + tabbarW + contentW + sidebarW;
> >       panelUI.width = contentW;
> >+*/
> >   },
> 
> delete this instead of commenting it out

I left this in because we may need to move some of the logic

> 
> >   _sizeControls : function(aEvent) {
> >   },
> 
> same here

I actually put the popup resizing back in place and left the rest commented out. We will need to handle resizing the toolbar width and the sidebar heights.

> >     // XXXndeakin remove the try-catch once the search service is properly built
> >     try {
> >       var searchService = Cc["@mozilla.org/browser/search-service;1"].
> >                           getService(Ci.nsIBrowserSearchService);
> >-    } catch (ex) {
> >+    }
> >+    catch (ex) {
> >       this.engines = [ ];
> >       return;
> >     }
> 
> is there a reason for the new line?

I was able to just remove the entire try/catch since search service is now properly built.
Comment on attachment 349485 [details] [diff] [review]
patch to merge from mb-cleanup to mobile-browser

r+ from stuart on IRC
Attachment #349485 - Flags: review+
http://hg.mozilla.org/mobile-browser/rev/cdff44cd6045

We still have some stuff to fix in order to get back to alpha1 level of functionality, but the overall responsiveness of the UI panning and painting is much better than alpha1.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 466280
Depends on: 466281
we are using tile caching, yet another fun rewrite.
Status: RESOLVED → VERIFIED
Component: General → Panning/Zooming
You need to log in before you can comment on or make changes to this bug.