Clicker code should happen on content side

RESOLVED FIXED

Status

Firefox for Android Graveyard
General
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 452275 [details] [diff] [review]
Patch

This patch add the clicks event to compute data on the content side and keeps the highlighting functionality.

Right now form helper is disabled in the patch but I need this one to port the form helper code.

For form helper we could potentially add a message temporarily from content to chrome to keep the functionality.
Assignee: nobody → 21
Attachment #452275 - Flags: review?(mark.finkle)
Attachment #452275 - Flags: feedback?(mbrubeck)
Blocks: 566288
OS: Linux → All
Hardware: x86 → All
Comment on attachment 452275 [details] [diff] [review]
Patch

>+    let overlay = document.getElementById("content-overlay");

This looks like it might be slower than the old version, which could be a problem according to the following comment (which you deleted for some reason).  Are we accepting a performance hit in this code in order to get it working cross-process?  Do we expect it to go away soon?

>-      // This code is sensitive to performance. Please profile changes you make
>-      // to keep this running fast.
>-      let bv = this._browserView;
>-      let overlay = this._overlay;


>+function getContentClientRects(aElement) {

You forgot to remove the original copy from browser.js.

>   addMessageListener("Browser:SaveAs", this);
> 
>+
>   this._coalescer = new Coalescer();

extra newline here?
Attachment #452275 - Flags: feedback?(mbrubeck) → feedback-
(In reply to comment #2)
> (From update of attachment 452275 [details] [diff] [review])
> >+    let overlay = document.getElementById("content-overlay");
> 
> This looks like it might be slower than the old version, which could be a
> problem according to the following comment (which you deleted for some reason).
>  Are we accepting a performance hit in this code in order to get it working
> cross-process?  Do we expect it to go away soon?
> 
> >-      // This code is sensitive to performance. Please profile changes you make
> >-      // to keep this running fast.
> >-      let bv = this._browserView;
> >-      let overlay = this._overlay;

We do expect this code to go away when layers lands. Tap highlighting will occur in the platform, not in the front-end.
Created attachment 452665 [details] [diff] [review]
patch v0.2

(In reply to comment #2)
> (From update of attachment 452275 [details] [diff] [review])
> >+    let overlay = document.getElementById("content-overlay");
> 
> This looks like it might be slower than the old version, which could be a
> problem according to the following comment (which you deleted for some reason).
>  Are we accepting a performance hit in this code in order to get it working
> cross-process?  Do we expect it to go away soon?

To be honest I didn't know that Layers will do the work for us (wooot!) but I think this comment had been made with the fear of regressing panning in mind (by stechz if I remember well) to be sure to not regress panning but has been made obsolete when bug 547722 has landed mostly because the highlighting is now not instantaneous.

> >+function getContentClientRects(aElement) {
> 
> You forgot to remove the original copy from browser.js.

Oups. Thanks for noticing it.

> 
> >   addMessageListener("Browser:SaveAs", this);
> > 
> >+
> >   this._coalescer = new Coalescer();
> 
> extra newline here?

Obviously.
Attachment #452275 - Attachment is obsolete: true
Attachment #452665 - Flags: review?(mark.finkle)
Attachment #452275 - Flags: review?(mark.finkle)
Created attachment 452704 [details] [diff] [review]
Patch v0.3

This version add a call to hideCanvas on dblClick otherwise the canvas was staying on the screen but misplaced.
Attachment #452665 - Attachment is obsolete: true
Attachment #452704 - Flags: review?(mark.finkle)
Attachment #452665 - Flags: review?(mark.finkle)
Comment on attachment 452704 [details] [diff] [review]
Patch v0.3

>diff -r e801891d8abb chrome/content/browser-ui.js

>     // listen returns messages from content
>     messageManager.addMessageListener("Browser:SaveAs:Return", this);
>+    messageManager.addMessageListener("Browser:Focusable", this);

I don't like "Browser:Focusable" since it means so many different things. In this case, the message is sent to highlight the rects on the canvas. Let's use "Browser:Highlight" ?
 
>   receiveMessage: function receiveMessage(aMessage) {

>+        let rects = [];
>+        let bv = Browser._browserView;

"bv" is unused

>+        for (let i = 0; i < json.rects.length; i++) {
>+          let rect = json.rects[i];
>+          rects.push(new Rect(rect.left, rect.top, rect.width, rect.height));
>+        }
>+        this._showCanvas(rects);

Can we move _showCanvas and _hideCanvas into a new helper class? TapHighight.show and TapHightlight.hide ?
That way the code for highlighting is nicely separated and can be removed later.

>diff -r e801891d8abb chrome/content/browser.js


>diff -r e801891d8abb chrome/content/browser.xul
>-                <html:div id="tile-container" style="overflow: hidden;">
>-                  <html:canvas id="content-overlay" style="display: none; position: absolute; z-index: 1000;">
>+                <html:div id="tile-container" style="overflow: hidden; position: relative">
>+                  <html:canvas id="content-overlay" style="disaply: none; position: absolute; z-index: 1000; left: 0; top: 0;">

Why the "position: relative" ? Were we broken before?

>diff -r e801891d8abb chrome/content/content.js

>   _getRectForCaret: function _getRectForCaret() {
>     let currentElement = this.getCurrent();
>     let rect = currentElement.getCaretRect();
>     return null;
>   },
>-  
> };

Remove the trailing comma too

> function Content() {

>-
>-  this._mousedownTimeout = new Util.Timeout();

Can we remove the Timeout class from Util.js now too? Followup bug?

r+ with nits
Attachment #452704 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mobile-browser/rev/0d5e3d434811
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.