Paul [pwd] Reporter
	Description • 13 years ago

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:7.0a1) Gecko/20110612 Firefox/7.0a1
Build Identifier: 

Provide an option so as that in areas of clustered links users should be presented with a magnifying glass like that seen in a textarea to ensure they're able to access the content they desire.

Reproducible: Always

	Daniel Buchner [:dbuc]
	Comment 2 • 12 years ago

This is probably the coolest, most useful feature I have seen in Chrome for Android, let's "do it naooow"!

	Ian Barlow (:ibarlow)
	Comment 3 • 12 years ago

Interesting idea. Daniel, can you point me to a place where I can reproduce this using Chrome? Can't seem to activate this mode on my own so far.

	Daniel Buchner [:dbuc]
	Comment 4 • 12 years ago

In the Android Chrome Beta, load up this page - http://sfbay.craigslist.org/ - and try to touch a link, you can even zoom in a bit. If your touch is close to two different links, the browser pops out a zoomed view of the page and lets you make an accurate touch on the one you want. Here is an image of what the zooming link UI looks like when it is shown: http://cdn.androidcentral.com/sites/androidcentral.com/files/postimages/9274/chrome-for-android_016.jpg

	Brad Lassey [:blassey] (use needinfo?)
	Comment 5 • 12 years ago

prototyped something like this a while back using a fisheye zoom http://people.mozilla.com/~blassey/Fisheye.swf

	Ian Barlow (:ibarlow)
	Comment 6 • 12 years ago

This is pretty cool, and I agree we should do something like this. I'll add it to my to-do list, and work on some UI for it after we get some of the upcoming tablet work underway.

	:Margaret Leibovic
	Comment 12 • 10 years ago

ibarlow, have you thought about this recently? Should we target this for a certain release?

	Ian Barlow (:ibarlow)
	Comment 13 • 10 years ago

I believe Arun is going to have a look at this one too. I say pick a release and let's make it happen :)

	Brad Lassey [:blassey] (use needinfo?)
	Comment 14 • 10 years ago

Picking a release would fall in product's domain

	donrhummy
	Comment 15 • 10 years ago

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #5)
> prototyped something like this a while back using a fisheye zoom
> http://people.mozilla.com/~blassey/Fisheye.swf

That's not a good idea. While it looks cool, it makes the text less readable and forces the user to move their finger around a lot more to read the link (because only a small part of that cirle/sphere is in focus and uncurved). Better is to do a flatter magnifying glass effect, similar to what iOS does.

	Deb Richardson [:dria] (plz NEEDINFO)
	Comment 16 • 10 years ago

investigating -- hoping to find some other accessibility quick-wins we can bundle together

	donrhummy
	Comment 17 • 10 years ago

A few extra notes/suggestions:

1. Don't use a long press to bring up the zoom. Some websites or web apps will use the long press to simulate a right-click action. If Firefox steals this, the long press will be non-existent for those pages

2. The best idea is, if Firefox can't tell where you clicked (i.e. there's multiple clickable items underneath the press area, perhaps 40x40 area), show a zoomed in square of that area as large as possible (probably the width of the device vertically, or height if held in landscape) with a message/toast saying something "The area was too small to detect where you pressed, please try again in the zoom box".

3. Doing something where they can slide their finger around the page and it'll zoom in as iOS does on text is a very bad idea as it (like in point #1) removes some event actions from the website. They may wish to detect a long press and drag, but if FF is stealing that, those sites won't work properly. The better solution is #4:

4. In addition to #2, once the zoomed box is shown, you should allow them to drag the area inside that zoomed box around to find the right thing to click on. This is OK since you're not really stealing those long press and drag actions from the site since this is a special situation.

	Ian Barlow (:ibarlow)
	Comment 18 • 10 years ago

Thanks Don we'll take that into account in our design exploration. 

As an aside, it's generally frowned upon to use terms like "that's not a good idea" in a design discussion (like in comment 15), as that can often shut down a creative conversation far too prematurely. Your points are appreciated and well taken, but please try to take a somewhat more positive tone in the future whenever possible. If you'd like to read more about doing design critiques, this is a useful link for reference: http://www.slideshare.net/adamconnor/discussing-design-the-art-of-critique

Thanks :)

	Daniel Buchner [:dbuc]
	Comment 19 • 10 years ago

(In reply to donrhummy from comment #17)
> A few extra notes/suggestions:
> 
> 1. Don't use a long press to bring up the zoom. Some websites or web apps
> will use the long press to simulate a right-click action. If Firefox steals
> this, the long press will be non-existent for those pages

I must note that this is not technically or practically correct:

- Sites do use long-press for right-click context menus occasionally, but that's long-press. This UI is not intended for long-press - I get this UI on other browsers it when the tap occurs in a click-like time-frame, explicitly *not* a long-press. The answer is to *not* magnify links if the release doesn't occur within a given time from the initiation of the tap gesture.

- Even if we showed this UI on long-press instead of click-intended touches (which we shouldn't, IMO), developers would still be able to block this and take their own action using event.preventDefault(). If a developer calls preventDefault(), we'd simply abort the UI flow.

	donrhummy
	Comment 20 • 10 years ago

(In reply to Ian Barlow (:ibarlow) from comment #18)
> Thanks Don we'll take that into account in our design exploration. 
> 
> As an aside, it's generally frowned upon to use terms like "that's not a
> good idea" in a design discussion (like in comment 15), as that can often
> shut down a creative conversation far too prematurely. Your points are
> appreciated and well taken, but please try to take a somewhat more positive
> tone in the future whenever possible. If you'd like to read more about doing
> design critiques, this is a useful link for reference:
> http://www.slideshare.net/adamconnor/discussing-design-the-art-of-critique
> 
> Thanks :)

good point! sorry, hadn't realized how it came across. :)

	donrhummy
	Comment 21 • 10 years ago

(In reply to Daniel Buchner [:dbuc] from comment #19)
> (In reply to donrhummy from comment #17)
> > A few extra notes/suggestions:
> > 
> > 1. Don't use a long press to bring up the zoom. Some websites or web apps
> > will use the long press to simulate a right-click action. If Firefox steals
> > this, the long press will be non-existent for those pages
> 
> I must note that this is not technically or practically correct:
> 
> - Sites do use long-press for right-click context menus occasionally, but
> that's long-press. This UI is not intended for long-press - I get this UI on
> other browsers it when the tap occurs in a click-like time-frame, explicitly
> *not* a long-press. The answer is to *not* magnify links if the release
> doesn't occur within a given time from the initiation of the tap gesture.

My point about the long press was in reference to this: http://people.mozilla.com/~blassey/Fisheye.swf

> - Even if we showed this UI on long-press instead of click-intended touches
> (which we shouldn't, IMO), developers would still be able to block this and
> take their own action using event.preventDefault(). If a developer calls
> preventDefault(), we'd simply abort the UI flow.

I'm not sure that's correct. I believe the special zoom occurs before events are sent to the web page's code. It would be more a case of the browser not being sure where to fire the event, so it "pauses" the event process and asks the user to clarify their input.

	Deb Richardson [:dria] (plz NEEDINFO)
	Comment 22 • 10 years ago

I've checked into this a bit further and UX is in the process of doing design explorations for this.  I'd like to bundle this with a few other things if possible, so will dig into the possibilities a bit further a propose a release when we have a better idea around that.  

That said, I won't clear the needinfo on this for now since it's still on my plate - just wanted to let you know that we're working on it.

	Daniel Buchner [:dbuc]
	Comment 23 • 10 years ago

(In reply to donrhummy from comment #21)
> > - Even if we showed this UI on long-press instead of click-intended touches
> > (which we shouldn't, IMO), developers would still be able to block this and
> > take their own action using event.preventDefault(). If a developer calls
> > preventDefault(), we'd simply abort the UI flow.
> 
> I'm not sure that's correct. I believe the special zoom occurs before events
> are sent to the web page's code. It would be more a case of the browser not
> being sure where to fire the event, so it "pauses" the event process and
> asks the user to clarify their input.

The action of preventDefault() would be no different than a developer choosing to block right-click context menu UI from appearing via click event capture/blocking. The preventDefault() method was provided explicitly for these situations, and should absolutely work if invoked by the developer.

	donrhummy
	Comment 24 • 10 years ago

(In reply to Daniel Buchner [:dbuc] from comment #23)
> (In reply to donrhummy from comment #21)
> > > - Even if we showed this UI on long-press instead of click-intended touches
> > > (which we shouldn't, IMO), developers would still be able to block this and
> > > take their own action using event.preventDefault(). If a developer calls
> > > preventDefault(), we'd simply abort the UI flow.
> > 
> > I'm not sure that's correct. I believe the special zoom occurs before events
> > are sent to the web page's code. It would be more a case of the browser not
> > being sure where to fire the event, so it "pauses" the event process and
> > asks the user to clarify their input.
> 
> The action of preventDefault() would be no different than a developer
> choosing to block right-click context menu UI from appearing via click event
> capture/blocking. The preventDefault() method was provided explicitly for
> these situations, and should absolutely work if invoked by the developer.

Can you please provide some clarification? I'm not sure how zooming in to determine where the user is even clicking is something that can or should be stopped by preventDefault(). Once a tap's location is determined, the javascript can choose to capture it, but the zooming occurs prior to any events being thrown - it's to determine what event (and where) should even be thrown by the browser.

	Arun Balachandran Ganesan [:abc]
	Comment 26 • 10 years ago

I have attached the designs. Visual design has not made a pass yet. We can try this out and see how it works.

1. As shown in the specs, we can have this applied not just for links, but for check boxes, text fields, option buttons and other controls – whenever they are too tiny to tap effortlessly/accurately.

2. My recent trip to India says that people in many countries outside the US would find this a particularly useful feature – most local websites do not yet have mobile versions. And I found myself repeatedly giving up using the mobile browser for the same reason. [the railway ticket booking website on the mock up has half a million bookings in this year so far :) ]

	donrhummy
	Comment 28 • 10 years ago

(In reply to Arun Balachandran Ganesan [:abc] from comment #27)
> Created attachment 8394971 [details]
> clustered-links-specs.png

This looks really good but I'd suggest making it slightly bigger/more zoomed in. The minimum touch size is typically around 44 pixels, and these zoomed in links appear to be about half that size. (I'm guaging this based on an assumed screen size of 1024x768, and in the first zoom, the links are about 5 pixels tall with the whole screen being 236 pixels. That would work out to 21 pixels tall)

	Brad Lassey [:blassey] (use needinfo?)
	Comment 29 • 10 years ago

this is a tracking plus until we have a design

	Ian Barlow (:ibarlow)
	Comment 30 • 10 years ago

There is a design in comment 27 that can be started on.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 31 • 10 years ago

Unless this gets on the roadmap, we don't have the throughput to do it sooner than fx34

Deb - Let us know if this is a roadmap item.

	Deb Richardson [:dria] (plz NEEDINFO)
	Comment 32 • 10 years ago

I thought it was tracking 31, but you're right that isn't reflected in the roadmap.

Mfinkle: what's the earliest release we could get this in, realistically?  Fx32 would be nice, but let me know if that's feasible.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 33 • 10 years ago

At this point "earliest release" is most heavily dependent on having developers available to do the work. We are losing to interns, who could do this, so we are down to a skeleton crew.

	Deb Richardson [:dria] (plz NEEDINFO)
	Comment 36 • 10 years ago

clearing needinfo - I've put this into the project funnel as a P1 and we'll be sorting out where those might fall in the roadmap in the near future

	Dominique Vincent [:domivinc] Assignee
	Comment 37 • 10 years ago

Chenxia Liu [:liuche] suggested me to work on this bug. I started to look at the design requirements and I should have a first demo soon.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 38 • 10 years ago

Thanks for your help, Dominique! There is a design in comment 27 for you to get started, however, I just want to double-check with our UX designers that this is still the direction we want to take.

Yuan, does the design in comment 27 still make sense? Are there any adjustments to be made?

	Dominique Vincent [:domivinc] Assignee
	Comment 39 • 10 years ago

A draft  version of a zoomed view based on the initial UX requirements is available here:

http://rusez.com/apk/fennec-35.0a1.en-US.android-arm.apk  

Feel free to play with this apk to improve the UX design. In this version, I’m using the touch behaviour that search for the closest link and simulate the click on the closest link. I just added the followings: when there are more than one closest link, I send a new message “touchcluster” and stop the click on the closest link. 

I also added the long-press functionality: when a long press occurs and nothing is triggered (no selection, no context menu), the zoomed view is displayed. If you zoom out a page, the selection behavior is not always triggered with a long press.

Due to the recent change (bug 788073), the new message “touchcluster” is implemented in the platform touch handling code.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 40 • 10 years ago

A reminder for myself to take a look at this APK.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 41 • 10 years ago

Looking pretty cool so far Dominique! Some thoughts:
  * The zoomed window does not seem to center on my tap, or the links I'm between, so it's hard to find what I actually tapped on - there could be some refinement here. The mocks seem to indicate that only the links that are subject to being pressed should be shown
  * The aliasing in the zoom screen is a bit annoying - I wonder if there's some way to re-render the zoomed window at a higher resolution? If you want to post your patch (and approach), we can ask some platform/graphics engineers what they think.
  * The zoomed window when long-pressing and no action is taken may be too easily activated (causing accidental activation) - we can iron out the UX later if we decide to go that route but note that its functionality is duplicated by double-tapping or pinch-to-zooming. I'd say focus on this last.

	Dominique Vincent [:domivinc] Assignee
	Comment 42 • 10 years ago

A second version of the apk is available here:
http://rusez.com/apk/fennec-35.0a1.en-US.android-arm.apk 

1) Yes the position was not correctly set. I spent a lot of time to figure out that the closest link is not always correctly detected due to the changes done in bug 788073. I created bug 1078029 to report 2 errors (see my comment here [1]). The closest procedure implemented in 788073 has probably some other hidden bugs that are not easily visible with the closest link feature. But with the zoomed view it’s easy to see if it works or not. I have already found another case where it doesn’t work: non clickable frame with child frames that are clickable. the current procedure implemented in 788073 seems to not detect the links in the children.

2) This issue (aliasing) is visible when you zoom out to a very very small font size. My understanding of the user case was the following: 
a- the user read a page 
b- he tries to click on a link, but several links are available near the touched position. 
c- the zoomed view helps the user to select the correct link
When the aliasing is visible, we are not in this user case: the user cannot read the page in step a, the font size is too small. 
But I agree with you, it could be nice to get a solution. Currently I make a copy of the opengl surface bitmap inside an Android bitmap. Then I create the zoomed bitmap using a matrix 
(Matrix matrix = new Matrix(); …).
So if gecko can provide a better definition for the surface bitmap when the zoomed view is open, it could help. I’m not sure that it’s easy and there will be an impact on the performances.
Regarding the code I’m still playing with it. I will post a first patch in 2 or 3 days.

3) I implemented the long press feature before I understand the issue of bug 1078029. The closest link was randomly detected so it was really hard to test my code. I decided to make my test using the long press. 

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1078029#c8

	Dominique Vincent [:domivinc] Assignee
	Comment 43 • 10 years ago

A part of the code changes is linked to the issues coming from Bug 788073 and Bug 1078029.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 44 • 10 years ago

Hey, Dominique! Sorry for the delay!

I don't understand the extent to which the closest link detection is broken, so I'll try to avoid mentioning refinements behaviors that rely on this functionality.

(In reply to Dominique Vincent [:domivinc] from comment #42)
> 2) This issue (aliasing) is visible when you zoom out to a very very small
> font size.

I apologize - I misused the term aliasing! I meant how you can see the pixels of the zoomed in view because we're scaling it from a smaller render. As I think you mentioned, I wonder if we can't re-render at a higher resolution to fix this.

Kats, what do you think of the previous paragraph? Is there any way to do it (or can you point us to someone who might know better)?

I noticed that clicking links in the zoomed in view sometimes scrolls when I mean to click - is there anything you can do about that?

I also think the zoomed in view can be a little more zoomed (but maybe I'm too heavily influenced by Chrome's implementation).

Clicking on small items that are vertical with one another doesn't seem to bring up the zoomed view, but rather select a link - is this a side effect of the closest link detection issues?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 45 • 10 years ago

Yikes that's a huge patch! :) Thanks Dominique for taking this on, and for making such rapid progress. I haven't gone through the patch in much detail yet but I have a couple of comments:

1) It looks like you're doing a GL readback to rendered the zoomed view. With that approach you can't re-render at a higher resolution. Gecko has the ability to render stuff at a higher resolution but to implement that you'd have to go about it quite differently - probably similar to how the thumbnail stuff works. Doing that approach efficiently is possible, but will likely be tricky.

2) It would be a good idea to get feedback from some other platform folks on the approach you're taking here. I'm not sure the touchcluster event you're adding will pass review, although to be honest I only did a very cursory look over the patch and haven't understood the design properly. roc or smaug might be good people to request feedback from on this.

	Dominique Vincent [:domivinc] Assignee
	Comment 47 • 10 years ago

(In reply to Michael Comella (:mcomella) from comment #44)
> I noticed that clicking links in the zoomed in view sometimes scrolls when I
> mean to click - is there anything you can do about that?
> 
If you really click on a link, the zoomed view should not scroll but the "normal" click behavior should be done. If you click near a link, the zoomed view will be centered around this new point. 

> I also think the zoomed in view can be a little more zoomed (but maybe I'm
> too heavily influenced by Chrome's implementation).
> 
The current zoom factor used in my apk is x2.
I made a comparison with the default internet application on my HTC Desire Z (there is no chrome version available on 2.3.3). You can see the differences on the attached picture (zoom_differences.png).
The default zoom factor is lower than 2 on the HTC Internet application.
It could be nice if you can post a copy screen of the chrome zoom behavior. I could try to match their zoom factor.
You should take care of another point: the size of the zoomed view. The window is large on my implementation of the zoom, smaller on the default HTC internet application. It could influence your perception. 

> Clicking on small items that are vertical with one another doesn't seem to
> bring up the zoomed view, but rather select a link - is this a side effect
> of the closest link detection issues?
I'm not sure to understand the html structure here, could you post a link where I can reproduce the issue?

	Dominique Vincent [:domivinc] Assignee
	Comment 48 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #45)


Thanks for your feedback, just one question and some details/remarks regarding the patch:

> 
> 1) It looks like you're doing a GL readback to rendered the zoomed view.
> With that approach you can't re-render at a higher resolution. Gecko has the
> ability to render stuff at a higher resolution but to implement that you'd
> have to go about it quite differently - probably similar to how the
> thumbnail stuff works. Doing that approach efficiently is possible, but will
> likely be tricky.

Do you have a link to the Gecko code implementing the higher resolution stuff? 

The thumbnail code I found didn’t help me for the following reasons:
a) the resolution of the thumbnail is lower than the Gecko resolution (zoomed view needs a higher resolution), 
b) the thumbnail is a fixed copy screen of the Gecko view. The zoomed view is a “live” copy of the Gecko view, for instance animation are visible in the zoomed view,
c) the thumbnail view is a full screen copy, the zoomed view is limited to a small part of the Gecko view.

> 
> 2) It would be a good idea to get feedback from some other platform folks on
> the approach you're taking here. I'm not sure the touchcluster event you're
> adding will pass review, although to be honest I only did a very cursory
> look over the patch and haven't understood the design properly. roc or smaug
> might be good people to request feedback from on this.

Before bug 788073, I implemented the new event “touchcluster” in the “browser.js” script. Only the Android code was impacted by this change. 

With bug 788073 implementation, all the closest links detection has been moved inside the platform code. The zoomed view code needs to get the information when a cluster is detected. A new event seems to me the best way to communicate such information between Gecko and the device code. But this event part of the code is really not the main part of the patch, I just need the information to start the zoomed view. If there is a better way to get the cluster position, I will make the change.

The fix of bug 1078029 will have an impact on this part of the code. My current patch didn’t fix the issues of 1078029. The closest links detection is not correctly done in the current platform code from an Android point of view. The main issue is that the closest links detection should be done when a “single tap” event is received and not when a “touch start” event is received.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 49 • 10 years ago

Anthony, do you mind looking over the specs from comment 26 and ensure that nothing needs to be changed?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 50 • 10 years ago

(In reply to Dominique Vincent [:domivinc] from comment #48)
> > 1) It looks like you're doing a GL readback to rendered the zoomed view.
> > With that approach you can't re-render at a higher resolution. Gecko has the
> > ability to render stuff at a higher resolution but to implement that you'd
> > have to go about it quite differently - probably similar to how the
> > thumbnail stuff works. Doing that approach efficiently is possible, but will
> > likely be tricky.
> 
> Do you have a link to the Gecko code implementing the higher resolution
> stuff? 

The interface we use to request that Gecko draws stuff at a higher resolution is at [1], and we invoked it in Fennec from [2] so that stuff doesn't show up as blurry when the user does a pinch-zoom. I don't know if you want a link to the implementation itself (which is kinda spread out all over the place) but basically what it boils down to is when the actual textures are painted by Gecko, the textures are made larger by a factor of the resolution, even though the same CSS-pixel content area is drawn. The textures are then scaled down in the GPU at composite time so it takes up the same screen area. In the case of pinch-zoom we also cancel that "scaled down in the GPU" step so that it appears larger on the screen.

> The thumbnail code I found didn’t help me for the following reasons:
> a) the resolution of the thumbnail is lower than the Gecko resolution
> (zoomed view needs a higher resolution), 
> b) the thumbnail is a fixed copy screen of the Gecko view. The zoomed view
> is a “live” copy of the Gecko view, for instance animation are visible in
> the zoomed view,
> c) the thumbnail view is a full screen copy, the zoomed view is limited to a
> small part of the Gecko view.

Of these things (a) and (c) can be easily fixed. The thumbnailing code requests Gecko to paint at [3] and it's easy to request a different scale or rect to be painted. (b) is indeed much harder to fix, and I don't know of any good way to handle that. Requesting that Gecko repaint this new zoomed view on a separate timer from Java seems error-prone and inefficient.

Another option to deal with the "liveness" requirement (assuming it is a requirement) is to implement the zoomed view using a canvas element in browser.xul and the drawWindow API [4], and request a paint in it in a requestAnimationFrame callback or something similar. The drawWindow API doesn't look like it currently support a resolution but I imagine that would be relatively straightforward and clean to add.

> Before bug 788073, I implemented the new event “touchcluster” in the
> “browser.js” script. Only the Android code was impacted by this change. 
> 
> With bug 788073 implementation, all the closest links detection has been
> moved inside the platform code. The zoomed view code needs to get the
> information when a cluster is detected. A new event seems to me the best way
> to communicate such information between Gecko and the device code. But this
> event part of the code is really not the main part of the patch, I just need
> the information to start the zoomed view. If there is a better way to get
> the cluster position, I will make the change.

Ok, sounds good. If it's just a communication thing it shouldn't be hard to change to use some other API if that needs to be done.

> 
> The fix of bug 1078029 will have an impact on this part of the code. My
> current patch didn’t fix the issues of 1078029. The closest links detection
> is not correctly done in the current platform code from an Android point of
> view. The main issue is that the closest links detection should be done when
> a “single tap” event is received and not when a “touch start” event is
> received.

Yup, understood. I'll try to find some time in the coming week to fix bug 1078029, and hopefully restore the old Android behaviour (i.e. leave touch events as-is and only move the click).

[1] http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl?rev=8892214038df&mark=197-220#197
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=bb58a5fc381b#3696
[3] http://mxr.mozilla.org/mozilla-central/source/widget/android/AndroidBridge.cpp?rev=01cd9a72cf48#1768
[4] https://developer.mozilla.org/en-US/docs/Web/API/Canvas_API/Drawing_graphics_with_canvas#Rendering_Web_Content_Into_A_Canvas

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 51 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #50)
> Another option to deal with the "liveness" requirement (assuming it is a
> requirement) is to implement the zoomed view using a canvas element in
> browser.xul and the drawWindow API [4], and request a paint in it in a
> requestAnimationFrame callback or something similar. The drawWindow API
> doesn't look like it currently support a resolution but I imagine that would
> be relatively straightforward and clean to add.

Oh, I forgot to mention that if you do it this way, then dragging the zoomed view around will be slower, because you'll have to dispatch events to browser.js and the Gecko main thread. Currently I believe you can drag it around entirely on the android UI thread which is fast and responsive. So there's downsides to this approach too, and it's not obvious to me what the best solution is.

	Brad Lassey [:blassey] (use needinfo?)
	Comment 52 • 10 years ago

filter on [mass-p5]

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 53 • 10 years ago

(In reply to Dominique Vincent [:domivinc] from comment #47)
> > I also think the zoomed in view can be a little more zoomed (but maybe I'm
> > too heavily influenced by Chrome's implementation).
> > 
> The default zoom factor is lower than 2 on the HTC Internet application.
> It could be nice if you can post a copy screen of the chrome zoom behavior.
> I could try to match their zoom factor.

Looking at size difference comparison (comment 46), it seems our zoom level is similar, if not larger than Chrome. See [1] from comment 4.

> You should take care of another point: the size of the zoomed view.

I imagine this, perhaps in addition to the border shape, changes my perception.

> > Clicking on small items that are vertical with one another doesn't seem to
> > bring up the zoomed view, but rather select a link - is this a side effect
> > of the closest link detection issues?
> I'm not sure to understand the html structure here, could you post a link
> where I can reproduce the issue?

I've been using http://news.ycombinator.com

[1]: http://cdn.androidcentral.com/sites/androidcentral.com/files/postimages/9274/chrome-for-android_016.jpg

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 54 • 10 years ago

Already in the funnel as a P2

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 55 • 10 years ago

Comment on attachment 8504792 [details] [diff] [review]
patchZoomedView141014.diff

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

f+ for the interactive portions of the patch (as opposed to the code).

	Anthony Lam (:antlam) Find me, @antlam on Slack!
	Comment 56 • 10 years ago

(In reply to Michael Comella (:mcomella) from comment #49)
> Anthony, do you mind looking over the specs from comment 26 and ensure that
> nothing needs to be changed?

Looks like a good start. Let's see where we can get with these and then we can refine after if need be.

Is there a build somewhere I can test this?

	Dominique Vincent [:domivinc] Assignee
	Comment 57 • 10 years ago

(In reply to Anthony Lam (:antlam) from comment #56)

> Is there a build somewhere I can test this?

There is a link to an APK in comment 42: https://bugzilla.mozilla.org/show_bug.cgi?id=663803#c42

	Dominique Vincent [:domivinc] Assignee
	Comment 58 • 10 years ago

A new version of the apk is available here:
http://rusez.com/apk/fennec-36.0a1.en-US.android-arm.apk 

In this version, I'm still using a GL readback to rendered the zoomed view when frames are changing in the main page.
When there is no more new frame to display, a message is sent to Gecko. The message makes a render of the zoomed part in order to re-render to a higher resolution (it should fix the aliasing  issue).
This part is using the logic used by the thumbnailing code (suggested by Kats).

One remaining issue of this approach: when the page content is "live" (animation, video, ...), the higher resolution display of the zoomed view is never done because the flow of new frames is never stopped. The zoomed view is visible but with the readback resolution (not the higher resolution). Any idea to improve this user case ? Do we need to improve that?

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 59 • 10 years ago

(NI :kats for the question in comment 58)

(NI :antlam to check out the build in comment 58)

Looking pretty awesome so far - I'm really impressed! Below are some UX comments, in no particular order, where I've marked "(followup)" for things that can be done in a followup bug, to avoid making this bug too large in scope:

  * (followup) I think Chrome makes the zoomed-in-upon text always the same size, no matter how big or small it is when tapped. This seems useful - I wonder if there's anything we can do here. For example, the top text on reddit is still awfully small (i.e. "front", "all", "random", etc.)
  * I find I still often click a link when I think the zoomed in view will pop up to disambiguate - the "hard to determine" tap metric can be a bit more sensitive (e.g. happens often with the "hot", "new", "rising", etc. tabs on reddit)
  * (followup) Reddit seems to periodically refresh at which point the zoomed in view gets momentarily blurry before it re-renders - I wonder if there's anyway to avoid this skip
  * (followup) There's a slight delay when first clicking in an ambiguous space where the zoomed in view is blurry - I wonder if we can fill this pause with either an animation, or something. Similarly when scrolling (but I think there's less to do here)
  * (followup) The "X" button can be hard to read sometimes due to the underlying content - maybe :antlam can come up with something there (and other UI nits)

	Anthony Lam (:antlam) Find me, @antlam on Slack!
	Comment 60 • 10 years ago

Awesome! this is looking good Dominique.

How did we arrive at the size of the zoom area?
As Michael pointed out ^ the 'x' seems like it would be better served not being there in the "zoomed in area". That being said, I feel like it should just dismiss when I tap outside the magnifying glass area (in an area without links).

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 61 • 10 years ago

Thought: we can flag the feature off by default so that we can work on all of the issues raised in my and :antlam's previous comments in followups - this bug is already a lot of code, no need to make it more stressful than it has to be!

Let's try to find a final reviewer for this patch - I can try to do the Java parts.

kats, do you know who might be appropriate to review the C++ components?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 62 • 10 years ago

For a first pass I would request f? from smaug to see what he thinks of the touchcluster event. There might be high-level improvements he can suggest to the flow here. For actually reviewing the code I think roc and smaug should both review. Unfortunately they both have review queues a mile long.

	Dominique Vincent [:domivinc] Assignee
	Comment 63 • 10 years ago

(In reply to Anthony Lam (:antlam) from comment #60)
> Awesome! this is looking good Dominique.
> 
> How did we arrive at the size of the zoom area?

The size of the zoomed area used in the first UX design (comment 27) is not fixed. It looks like the size is based on the assumption that we are able to understand at a high level the page structure and guess what are the elements the user wants to zoom, their positions relative to the touch point, and their sizes.
Without this “super power”, I made the first implementations using the following rules:
a) the cluster touch position is the center of the zoomed area (except on the screen borders)
b) the size of the zoomed area is fixed: width = 80% of the smallest screen dimension, height = 50% of the smallest screen dimension 
c) the size is the same in both orientations

Some ideas and comments about the current size:
1- We can easily change in the code the 80% and 50% values. 
2- The current code is designed to accept a dynamic change of the zoomed size.
3- We can also move those values in the user preferences. 
4- The default values should probably be different based on the size of the screen (small screens, tablets, …)
5- We can also imagine that the user could change directly the size moving the border of the zoomed area (with a larger border and some designed elements in this border)…
6- The size should leave enough space outside the zoomed area to still have a clear view of the unzoomed parts of the page.

> As Michael pointed out ^ the 'x' seems like it would be better served not
> being there in the "zoomed in area". That being said, I feel like it should
> just dismiss when I tap outside the magnifying glass area (in an area
> without links).

One issue with this approach is here: “in an area without links”. On some pages, the body has click/touch events attached. It will be impossible to close the zoomed view without trigger the body event. I’m going to investigate on this approach but I’m not sure that we won’t have undesirable side effects.

	Dominique Vincent [:domivinc] Assignee
	Comment 64 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #62)
> For a first pass I would request f? from smaug to see what he thinks of the
> touchcluster event. There might be high-level improvements he can suggest to
> the flow here. For actually reviewing the code I think roc and smaug should
> both review. Unfortunately they both have review queues a mile long.

Before a first code review, I need to get a fix for bug 1078029. The implementation of the “touchcluster” event is directly linked to the final decision regarding bug 1078029. If bug 788073 is backed out (suggestion from :bnicholson in [1]), I will re-implement the “touchcluster” event directly in the file “browser.js”. If the final decision is to fix the issue in Gecko platform code, I will re-implement the event based on this fix.

Kats, did you make some progress since your last comment in 1078029 [2] ? 
From my point of view, the changes required in Gecko code to correctly fix 1078029 are not small. My different designs to fix it, always use a new Gecko event to implement the same behavior used in the Android code : onSingleTapConfirmed and onSingleTapUp [3]


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1078029#c19 
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1078029#c20
[3] http://developer.android.com/reference/android/view/GestureDetector.SimpleOnGestureListener.html

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 65 • 10 years ago

(In reply to Dominique Vincent [:domivinc] from comment #64)
> Kats, did you make some progress since your last comment in 1078029 [2] ? 

Sorry, no I've been working on other things. I'll start looking at it now.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 66 • 10 years ago

Pretty much done with bug 1078029 modulo reviews and maybe tests and so on. The patches might change a little but you should at least be able to work on top of what's there now.

@mcomella: Also for the record I brought up this feature (and specifically that it's using GL readback) in the gfx daily meeting, and everybody there was very much against it, as doing GL readback will significantly impact general performance. I'm assuming that right now you don't care so much about this, particularly since the magnifying glass is only temporarily visible. However, from the point of view of the platform graphics team this is a "front end" feature and if anybody complains to us about Fennec performance resulting from this we'll just bounce it to you.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 67 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #66)
> @mcomella: Also for the record I brought up this feature (and specifically
> that it's using GL readback) in the gfx daily meeting, and everybody there
> was very much against it, as doing GL readback will significantly impact
> general performance.

Are there any alternative methods we can use to make this feature that won't make such a negative performance impact?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 68 • 10 years ago

The drawWindow approach mentioned earlier would probably be better in terms of performance. If you want a more detailed discussion on the performance implications of different approaches with the gfx team you should set up a meeting through :milan. Not trying to pass the buck here but we have a lot of different things to deal with and it's important to prioritize things accordingly.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 69 • 10 years ago

Dominique, are there any straight-forward ways that you can see to improve the performance of your current implementation? If not, since I can already see some performance issues (comment 59) and the gfx team doesn't seem to think it's a good idea (comment 66), I'd say we can take some of the alternative approaches suggested by kats in comment 68 - I leave it up to you whether you want to set up a meeting with :milan to discuss the pros and cons of possible alternative approaches, or try out the drawWindow API (note: kats said "The drawWindow approach mentioned earlier would *probably* be better in terms of performance.", emphasis is mine) so I'd probably encourage the meeting, or at least a ping on IRC. :)

	Dominique Vincent [:domivinc] Assignee
	Comment 70 • 10 years ago

(In reply to Michael Comella (:mcomella) from comment #69)
> Dominique, are there any straight-forward ways that you can see to improve
> the performance of your current implementation? If not, since I can already
> see some performance issues (comment 59) and the gfx team doesn't seem to
> think it's a good idea (comment 66), I'd say we can take some of the
> alternative approaches suggested by kats in comment 68 - I leave it up to
> you whether you want to set up a meeting with :milan to discuss the pros and
> cons of possible alternative approaches, or try out the drawWindow API
> (note: kats said "The drawWindow approach mentioned earlier would *probably*
> be better in terms of performance.", emphasis is mine) so I'd probably
> encourage the meeting, or at least a ping on IRC. :)

First I'm going to update the code in order to use the fix of bug 1078029. I have to implement the suggestion made by Kats in [1]. I'm not sure that it's a perfect design but it should work (see my answer here [2]).

I will also work on the 2 following points: the first time load of the zoomed view could probably be better. The allocation of the read back buffer could be done only once.  

Regarding the alternative approach using the "drawWindow" API, the DrawWindow method uses a call to "shell->RenderDocument" to get the copy of the screen here [3]. In my last apk with the higher resolution fix, I'm already using the same "shell->RenderDocument" call to get the higher resolution screen copy (see comment 58). I will try to measure the performances in both cases (display using a read back and display using the RenderDocument). Based on the data, we could get a feedback of the gfx team in order to implement the final design.

From the end-user point of view, 2 points need to be discussed/approuved with the UX team:
1- When the page is "live" (animation, video, ...), is it acceptable to get a zoomed view without a higher resolution display (last APK)?
2- If the previous answer is "no", the zoomed view is always displayed to the higher resolution, do we want to display the zoomed view with a higher priority than the background view (partially hidden by the zoomed view)?

If the answer to the second question is "yes", the zoomed view has to be displayed with a higher priority, we have to slow down the background refresh in order to speed up the zoomed view refresh (in the front). In this case, additional works need to be done in order to achieve this UX goal.


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1078029#c37
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1078029#c39
[3] http://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#4356

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 71 • 10 years ago

(In reply to Dominique Vincent [:domivinc] from comment #70)
> From the end-user point of view, 2 points need to be discussed/approuved
> with the UX team:
> 1- When the page is "live" (animation, video, ...), is it acceptable to get
> a zoomed view without a higher resolution display (last APK)?
> 2- If the previous answer is "no", the zoomed view is always displayed to
> the higher resolution, do we want to display the zoomed view with a higher
> priority than the background view (partially hidden by the zoomed view)?

Anthony, any thoughts here?

	Anthony Lam (:antlam) Find me, @antlam on Slack!
	Comment 72 • 10 years ago

Can we just freeze what's in the magnifying glass frame but let everything outside keep running? (in terms of animation/motion)

	Dominique Vincent [:domivinc] Assignee
	Comment 73 • 10 years ago

(In reply to Anthony Lam (:antlam) from comment #72)
> Can we just freeze what's in the magnifying glass frame but let everything
> outside keep running? (in terms of animation/motion)

Not really because we don't know why the frames are changing and when to take the screen copy.
It could be due to different reasons and not only animation/motion in the page: scroll of the page, zoom in/out, events asking for a re-display ... 

Another main issue with the freeze of the zoomed view: the links in the cluster can be animated. In this case the user will click on the zoomed view link but the position of the link won't be correct in the main page. The link could also be hidden after a moment or the text of the link can be updated ...

For the video case, I'm not sure that a freeze of the zoomed view is really user friendly. I prefer to get a zoomed view of a video with a non perfect resolution than a zoomed copy screen (high res) of the video that is no more valid after a very short period of time. It's perhaps a user preference: static zoomed view with higher resolution or live zoom with a non perfect resolution. The user feedback's could probably give us the correct UX direction in this case. 

We can also add a button in the zoomed view (top left) to pause/start the zoomed view refresh while the flow of frames is running. When the zoomed view is paused we can re-render to a higher resolution. The pause/start buttons won't be visible for a static page. But we have still the following issue: the links inside the zoomed view are perhaps invalid (updated in the main screen) in the paused state.

	Dominique Vincent [:domivinc] Assignee
	Comment 74 • 10 years ago

I updated the APK here: http://rusez.com/apk/fennec-36.0a1.en-US.android-arm.apk

Two config flags can be used with this apk:
ui.zoomedview.enabled (default = true) 
ui.zoomedview.readback (default=true)

With readback flag set to “true”, the zoomed view display is done using GL read back when there is a flow of frames. At the end of the flow, a re-render display (method RenderDocument) is done in order to get the higher resolution display.

With readback flag set to “false”, the zoomed view display is always done using RenderDocument to get the best resolution. For each frame if the previous RenderDocument call is completed, a new call is done.

I made some tests. On my old device, the results depend of the structure of the page. The Rerender version works well with a simple html page (text), but not with the page with an image. Some RenderDocument calls are very very long (10 s) with the second URL. It’s visible when the zoomed view is moved over the picture.
The read back version is more “stable” in term of maximum duration of the display.
You can get the duration of the zoomed view display in the LogCat using the following filter: “text:updateView”

URL1: Text only
http://office.microsoft.com/fr-fr/training/cours-de-formation-sur-office-2013-HA104096598.aspx

Rerender duration, Average 64 ms, Max 900 ms, Min 9 ms
Read Back duration, Average 241 ms, Max 579 ms, Min 24  ms

URL2: Text and one big jpeg 1400x440 (94,9Kb)
http://rusez.com/bb.html

Rerender, Average 653 ms, Max 9957 ms, Min 9 ms
Read Back, Average 98 ms, Max 223 ms, Min 29  ms

I divided the code changes in 3 different parts:
Part 1: the cluster detection based on the changes done in bug 1078029
Part 2: the zoomed view using the GL read back
Part 3: the zoomed view using the RenderDocument in order to get the higher resolution

	Dominique Vincent [:domivinc] Assignee
	Comment 78 • 10 years ago

:milan or :kats, in my patch,the call to the method RenderDocument can take several seconds to display the zoomed view. Based on the figures in my log, I can see for instance 50 ms for a first call, and the next RenderDocument call can take 5 seconds (x 100). The part of the page used in this case is not really different between the 2 calls (move of the zoomed view of 2 or 3 pixels). So the content is probably not the only reason explaining the difference of duration. But the issue is more visible with an image (see my comment 74 [1]).

You can have a look to my code in the third patch here [2]. The method is AndroidBridge::CaptureZoomedView, it works like the existing method AndroidBridge::CaptureThumbnail.

Is there a way to control the duration of the RenderDocument call? 
Could we set a timeout? or set a higher priority (to get a lower max duration)?
Using the GL read back option, the zoomed view works better because the duration of the read back seems to have a maximum duration around 500-600 ms. We could have a perfect solution if there is way to set a timeout/max duration when the call to the method RenderDocument is done.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=663803#c74
[2] https://bug663803.bugzilla.mozilla.org/attachment.cgi?id=8520687

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 79 • 10 years ago

It shouldn't be taking that long. Do you know if the CaptureThumbnail call also takes a similar amount of time? Things I would check: the area you're requesting be rendered and the resolution of the render. If those seem sane then I would suggest getting a profile using the instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Profiling_with_the_Built-in_Profiler to see if anything obvious jumps out.

	Dominique Vincent [:domivinc] Assignee
	Comment 80 • 10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #79)
> It shouldn't be taking that long. Do you know if the CaptureThumbnail call
> also takes a similar amount of time? Things I would check: the area you're
Correct, measuring the duration directly in C++, it's really better. 
The issue is probably somewhere else in my code.
Thanks Kats

	Dominique Vincent [:domivinc] Assignee
	Comment 81 • 10 years ago

Mickael, I found where is the issue using the RenderDocument method. When I started to investigate the Thumbnail code, I duplicated too quickly the CaptureThumbnail design and there was an idle post ... 
I made a quick fix to remove it, and the render option works very well now!!!

So if you don't care, I'm going to clean up my patch and totally remove the read back option.

	Dominique Vincent [:domivinc] Assignee
	Comment 82 • 10 years ago

This patch replaces the previous P2/P3 patches.
The display of the zoomed view is done using the render document method only (high resolution).
No change in patch P1.

New version of the APK available here: http://rusez.com/apk/fennec-36.0a1.en-US.android-arm.apk

	Dominique Vincent [:domivinc] Assignee
	Comment 83 • 10 years ago

Testing the last version (call to method RenderDocument only), I found an issue with the resolution change test in method progressiveUpdateCallback (GeckoLayerClient.java).
With the zoomed view open, after several zoom in/out, it looks like the display is no more done in high resolution on the background. The draw is aborted because a resolution change is detected. 
The resolution change reported is not correct: it's the resolution change used inside the zoomed view and not in the background view.
I'm going to try to fix this issue.

Another pending issue, is linked to the getClosest method. If some clusters links are not detected, it could help to log the url of the pages in Bug 1096042 (spinoff from bug 1078029).

	Dominique Vincent [:domivinc] Assignee
	Comment 84 • 10 years ago

With a fresh version of the code, I cannot reproduce the resolution issue (see comment 83). My previous build was probably impacted by bug 1090168. 

But now the build fails due to Bug 1096627 (GeckoView / Fennec dependencies) .
I made the changes in order to remove the ZoomedView dependency: a new interface in LayerView: 
public interface OnZoomedViewListener

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 85 • 10 years ago

(I'll take a look at the build tomorrow)

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 86 • 10 years ago

Anthony, what do you think needs to be done here in terms of UX polish? The latest build is in comment 82.

We can fix these in followups.

---

Dominique, this is looking great!

Feedback:
  * I deliberately press between the header links on Reddit and I still manage to click a link ~80% of the time - I think the "ambiguous click" metric can be more sensitive
  * The close button on the zoomed view is difficult to press - (I think :antlam maybe have suggested this before but) can we just have the zoom view cleared when clicking outside of the zoomed view?

That being said, this are all nitty changes - if you think your code is ready, you should flag some reviewers as per comment 62. Feel free to flag me for the Java parts.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 87 • 10 years ago

We don't expose new stuff to the web using moz prefix. Either the feature is good and stable enough and doesn't need to be prefixed, or the behavior isn't stable enough and shouldn't be exposed.
One option is to expose the new properties only conditionally until the API is stable enough.
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Pref


Is there a specification for this new API? W3C or WhatWG or something else?

Usually there shouldn't be need to add new features to .idl interfaces. Those are considered
mostly deprecated, and new properties could go to .webidl only.

But it is not clear to me why we need to expose the API to web content. Would
[ChromeOnly] in .webidl be enough?
If only mobile/android/chrome/content/browser.js needs the hitCluster flag, it certainly should be
marked as [ChromeOnly]. And it the property is [ChromeOnly], it doesn't need [Pref], nor does it need to be moz-prefixed.


However, I don't know what hitCluster means in this context. .webidl files certainly need some explanation. When is the attribute
true?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 88 • 10 years ago

Comment on attachment 8520684 [details] [diff] [review]
P1-Bug_663803___Add_hitCluster_flag.patch

So this needs updates and some clarifications.


And random comment.
>  static nsIFrame*
> GetClosest(nsIFrame* aRoot, const nsPoint& aPointRelativeToRootFrame,
>            const nsRect& aTargetRect, const EventRadiusPrefs* aPrefs,
>-           nsIFrame* aRestrictToDescendants, nsTArray<nsIFrame*>& aCandidates)
>+           nsIFrame* aRestrictToDescendants, nsTArray<nsIFrame*>& aCandidates,
>+           int* elementsInCluster)

should be int32_t* aElementsInCluser


In general if adding a new [ChromeOnly] attribute or method makes implementing some nice UI features easier, that is fine.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 89 • 10 years ago

Comment on attachment 8523541 [details] [diff] [review]
Part1611 2-Bug_663803___Zoomed_view_implementation_using_render_document.patch

I should not review our Android specific .java code, 
nor Android specific c++

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 90 • 10 years ago

(In reply to Olli Pettay [:smaug] from comment #89)
> I should not review our Android specific .java code, 
> nor Android specific c++

Kats, do you have a suggestion for an alternative reviewer?

I can do the java stuff that doesn't intermingle too closely with the platform code.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 91 • 10 years ago

snorp or jchen could review the android C++ code.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 92 • 10 years ago

Comment on attachment 8523541 [details] [diff] [review]
Part1611 2-Bug_663803___Zoomed_view_implementation_using_render_document.patch

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

We have a volunteer. :D

	Dominique Vincent [:domivinc] Assignee
	Comment 93 • 10 years ago

Thanks Olli for the first pass review.
hitCluster flag is now marked as [ChromeOnly]. I should have removed all the unnecessary changes from the first patch.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 94 • 10 years ago

Comment on attachment 8527222 [details] [diff] [review]
hitCluster flag - patch663803Part1.diff

>+  [ChromeOnly]
>+  readonly attribute float hitCluster; // True when touch occurs in a cluster of links
How can 'float' be 'true'? The name 'hitCluster' sounds like a boolean property, but it is 
float. So, if float is actually the right type, what does it mean?

> GetClosest(nsIFrame* aRoot, const nsPoint& aPointRelativeToRootFrame,
>            const nsRect& aTargetRect, const EventRadiusPrefs* aPrefs,
>-           nsIFrame* aRestrictToDescendants, nsTArray<nsIFrame*>& aCandidates)
>+           nsIFrame* aRestrictToDescendants, nsTArray<nsIFrame*>& aCandidates,
>+           int32_t* elementsInCluster)
aElementsInCluster

>+++ b/mobile/android/chrome/content/browser.js
Changes to this file should be reviewed by some FF Mobile peer



r- because 'readonly attribute float hitCluster;' is unclear

	Dominique Vincent [:domivinc] Assignee
	Comment 95 • 10 years ago

Correct, hitCluster is a boolean and not a float! Change done. 
Rename of “elementsInCluster” in “aElementsInCluster” done.
Added :mcomella for the browser.js code review.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 96 • 10 years ago

Hey, Dominique. I wanted to apologize for the delay of my review. There is a Mozilla employee event this week so, unfortunately, I don't think I'll have time to get to it this week but I'll definitely get to it early next week (looking like Tuesday).

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 97 • 10 years ago

Comment on attachment 8527719 [details] [diff] [review]
Code changes after first review

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

kats, do you mind double-checking the changes to browser.js? There is some additional context in Part 2.

Note: kats is (likely) more knowledgeable about this code than I am and can override anything I am unsure about.

---

I'm unfamiliar with the MozMouseHittest event and thus when _handleRetargetedTouchStart is called - when is that event used?

::: mobile/android/chrome/content/browser.js
@@ +4897,5 @@
>      let target = aEvent.target;
>      if (!target) {
>        return;
>      }
> +    this._inCluster = aEvent.hitCluster;

nit: newline above and below this line

nit: I feel like hitCluster could be more descriptive - "tappedInLinkCluster"?

@@ +5025,2 @@
>          // scrollToFocusedInput does its own checks to find out if an element should be zoomed into
>          BrowserApp.scrollToFocusedInput(BrowserApp.selectedBrowser);

How does scrolling to a focused input interact with the zoomed view?

@@ +5046,5 @@
>  
> +  _clusterClicked: function sh_clusterClicked(aX, aY) {
> +      Messaging.sendRequest({
> +        type: "Gesture:clusteredLinksClicked",
> +        clicPosition: {

nit: Why not "clickPosition"?

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 98 • 10 years ago

Comment on attachment 8523541 [details] [diff] [review]
Part1611 2-Bug_663803___Zoomed_view_implementation_using_render_document.patch

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

Here's some initial thoughts - I'll get to the rest tomorrow.

I apologize again - it really is unacceptable for me to have taken so long on this review so I'll be sure to finish up tomorrow.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +85,5 @@
>       *    case 1 above) you should always first grab a local copy of the reference, and then use
>       *    that because mViewportMetrics might get reassigned in between reading the different
>       *    fields. */
>      private volatile ImmutableViewportMetrics mViewportMetrics;
> +    private List<LayerView.OnMetricsChangedListener> mViewportChangeListener;

nit: -> mViewportChangeListeners / mViewportChangeListenerList

@@ +910,5 @@
>  
>      /** Implementation of PanZoomTarget */
>      @Override
>      public void panZoomStopped() {
> +        for (LayerView.OnMetricsChangedListener listener : mViewportChangeListener) {

I see two issues here:
  1) Garbage collection: as far as I know, under the hood, this statement allocates an Iterator and iterates through it, garbage collecting the iterator when it is finished. It'd probably be wasteful to do this every time panZoomStopped is called (which I believe is every time the page stops panning) so, since we know this is an ArrayList under the hood, we use a `for (int i = 0; i < mViewportChangeListener.size(); ...)` loop over this. However...
  2) ConcurrentModification: add and remove can be called while we're in this method, iterating over this list. A better solution might be to use a CopyOnWriteArrayList (or other concurrent data) structure but then we'd need to use the iterator and risk the performance hit. Since it's not during panning, it might not be so bad.

Perhaps there's a better solution if we look at this problem specifically (e.g. forbidding additional/removal during the iteration, or queueing addition/removal until after the iteration completes)).

Similarly, for the loop in viewportMetricsChanged (garbage collection being an issue only the method is called often).

@@ +983,5 @@
>          return layerPoint;
>      }
>  
> +    void addOnMetricsChangedListener(LayerView.OnMetricsChangedListener listener) {
> +        synchronized(mViewportChangeListener) {

I think we can reach a ConcurrentModificationException here - 

nit: synchronized (mViewportChangeListener {

@@ +984,5 @@
>      }
>  
> +    void addOnMetricsChangedListener(LayerView.OnMetricsChangedListener listener) {
> +        synchronized(mViewportChangeListener) {
> +            mViewportChangeListener.add(listener);

I'm concerned that this approach allows us to insert duplicate elements into the array (which can hide some nasty bugs!) - can you ensure the listener does not already exist in the array before adding it? Since BrowserApp.setDynamicToolbar enabled can be called with the same argument multiple times, perhaps just silently log if you find the element already exists.

Alternatively, perhaps there is a Map implementation that is more efficient or intuitive.

@@ +989,5 @@
> +        }
> +    }
> +
> +    public void removeOnMetricsChangedListener(LayerView.OnMetricsChangedListener listener) {
> +        synchronized(mViewportChangeListener) {

Same `synchronized (` nit.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 99 • 10 years ago

Comment on attachment 8527222 [details] [diff] [review]
hitCluster flag - patch663803Part1.diff

Obsoleting this old version since it confused me :)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 100 • 10 years ago

Comment on attachment 8527719 [details] [diff] [review]
Code changes after first review

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

This look great to me! Just a couple of comments below.

::: mobile/android/chrome/content/browser.js
@@ +4897,5 @@
>      let target = aEvent.target;
>      if (!target) {
>        return;
>      }
> +    this._inCluster = aEvent.hitCluster;

I think hitCluster is fine as a name. However I think here if this._inCluster is true, then you don't want to execute the rest of this function. Instead you should just return. Opening a tcp connection to the URI and highlighting it only makes sense if that is the link we're going to navigate to. If we are in a cluster then the user might pick a different link from the zoomed view and so returning early here will prevent highlighting the wrong link.

@@ +5044,5 @@
>      }
>    },
>  
> +  _clusterClicked: function sh_clusterClicked(aX, aY) {
> +      Messaging.sendRequest({

You can remove "sh_clusterClicked" from the name, as it's not needed. If you want to keep it for debugging purposes rename it to just "clusterClicked". If I remember correctly the "sh_" prefix originally referred to things inside the selection helper (hence "sh") but that doesn't apply here. Also please use 2-space indent inside this function.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 101 • 10 years ago

Comment on attachment 8527719 [details] [diff] [review]
Code changes after first review

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

If kats says it's okay, you don't need me.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 102 • 10 years ago

Finkle, is this a feature we'd like to have in web apps? I noticed that this is added to .../layout/shared_ui_components.xml in the patch.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 103 • 10 years ago

Comment on attachment 8523541 [details] [diff] [review]
Part1611 2-Bug_663803___Zoomed_view_implementation_using_render_document.patch

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

Still digging into ZoomedView - here are some comments in the mean time:

For my comments on zoomed_view.xml, feel free to file a follow-up.

::: mobile/android/base/GeckoEvent.java
@@ +765,5 @@
> +        event.mPoints = new Point[2];
> +        event.mPoints[0] = new Point(x, y);
> +        event.mPoints[1] = new Point(bufw, bufh);
> +        // TODO move scaleFactor in another event field?
> +        event.mX = (double) scaleFactor;

What is mX supposed to be used for?

I'm not sure mMetaState is supposed to be used for the tabId (I'd imagine meta key state).

Perhaps ping kats about the proper uses of these fields and whether using them for an unintended purpose is reasoable?

::: mobile/android/base/ZoomedView.java
@@ +43,5 @@
> +    private int wCapturedView;
> +    private int hCapturedView;
> +    private int xLastPosition;
> +    private int yLastPosition;
> +    private int zoomFactor = 2;

nit: Because this is a constant, define at the top, below LOGTAG, as `private static final int ZOOM_FACTOR = 2;`

@@ +62,5 @@
> +        this(context,null,0);
> +    }
> +
> +    public ZoomedView(Context context, AttributeSet attrs) {
> +        this(context,attrs,0);

nit: Spaces after commas. Same above.

@@ +68,5 @@
> +
> +    public ZoomedView(Context context, AttributeSet attrs, int defStyle) {
> +        super(context, attrs, defStyle);
> +        handler = new Handler();
> +        runnable = new Runnable() {

nit: -> requestRenderRunnable

@@ +91,5 @@
> +            "Content:LocationChange");
> +    }
> +
> +    @Override
> +    protected void onFinishInflate (){

lucsar has told me in the past onFinishInflate is not guaranteed to be called across all devices, API levels, etc. Instead, you should create a separate internal layout file (with a <merge> tag at the root), inflate that in this constructor, and then make your calls awaiting inflation after that inflation.

nit: `() {`

There seem to be a few other stylistic issues in this file - do you mind giving it a go to fix them up (rather than me typing them all out)?

@@ +94,5 @@
> +    @Override
> +    protected void onFinishInflate (){
> +        super.onFinishInflate();
> +        final ZoomedView mZoomedView = this;
> +        ImageView closebutton = (ImageView) findViewById(R.id.dialog_close);

nit: closeButton

@@ +104,5 @@
> +        });
> +
> +        mZoomedImageView = (ImageView ) findViewById(R.id.zoomed_image_view);
> +
> +        if (mZoomedImageView != null){

Why should this be null? If it's null, something absolutely terrible happened and we can NullPointerException out anyway, right?

@@ +105,5 @@
> +
> +        mZoomedImageView = (ImageView ) findViewById(R.id.zoomed_image_view);
> +
> +        if (mZoomedImageView != null){
> +            mZoomedImageView.setOnTouchListener(new OnTouchListener()

nit: I'd move this definition to the top-level and have ZoomedView (or an inner class) implement OnTouchListener, so this method isn't huge.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +12,5 @@
>  import org.mozilla.gecko.gfx.Layer.RenderContext;
>  import org.mozilla.gecko.gfx.RenderTask;
>  import org.mozilla.gecko.mozglue.DirectBufferAllocator;
>  
> +import android.annotation.SuppressLint;

nit: Unused.

@@ +31,2 @@
>  import org.mozilla.gecko.mozglue.JNITarget;
> +import org.mozilla.gecko.util.FloatUtils;

nit: Unused.

@@ +95,5 @@
>      private int mTextureHandle;
>      private int mSampleHandle;
>      private int mTMatrixHandle;
>  
> +    private List<LayerView.OnZoomedViewListener> mZoomedViewListener;

nit: -> mZoomedViewListeners / mZoomedViewListenerList

@@ +597,5 @@
>              runRenderTasks(mTasks, true, mFrameStartTime);
>  
>          }
>  
> +        public void updateZoomedView(RenderContext context){

nit: You have two updateZoomedView methods in this class - perhaps this one should be, "maybeRequestZoomedViewRender" ?

@@ +600,5 @@
>  
> +        public void updateZoomedView(RenderContext context){
> +            final float viewLeft = context.viewport.left - context.offset.x;
> +            final float viewTop = context.viewport.top - context.offset.y;
> +            boolean bWait = false;

nit: Is the "b" in "bWait" for "boolean"? We don't use hungarian notation anymore so perhaps, "shouldWaitToRender" or similar is a better name for the variable.

@@ +602,5 @@
> +            final float viewLeft = context.viewport.left - context.offset.x;
> +            final float viewTop = context.viewport.top - context.offset.y;
> +            boolean bWait = false;
> +            if (Math.abs(mViewLeft - viewLeft) > 5 ||
> +                Math.abs(mViewTop - viewTop) > 5) {

How did this logic come about? Why 5?

@@ +610,5 @@
> +            mViewTop = viewTop;
> +            if (bWait) {
> +                return;
> +            }
> +            ThreadUtils.postToUiThread(new Runnable() {

nit: Whitespace above this line - you can be more liberal with vertical whitespace in this method as a whole.

@@ +629,5 @@
>                  mView.requestRender();
>  
>              PanningPerfAPI.recordFrameTime();
>  
> +            if (mZoomedViewListener.size() > 0){

Might as well put this size check inside `updateZoomedView`.

@@ +692,5 @@
> +            }
> +        });
> +    }
> +
> +    void addOnZoomedViewListener(LayerView.OnZoomedViewListener listener) {

Why not public (like remove)?

Also, I assume there aren't any concurrency issues here?

::: mobile/android/base/gfx/LayerView.java
@@ +5,5 @@
>  
>  package org.mozilla.gecko.gfx;
>  
>  import java.nio.IntBuffer;
> +import java.nio.ByteBuffer;

nit: Alphabetize.

@@ +536,5 @@
> +                if (layerRenderer != null){
> +                    layerRenderer.updateZoomedView(data);
> +                }
> +            }
> +        } catch (Exception e) {

Is there a specific Exception you're trying to catch here? It's generally better not to catch everything so we don't catch Exceptions we didn't mean to.

::: mobile/android/base/resources/layout/zoomed_view.xml
@@ +12,5 @@
> +    android:layout_height="wrap_content"
> +    android:layout_alignParentLeft="true"
> +    android:layout_alignParentTop="true"
> +    android:background="@android:color/white"
> +    android:visibility="invisible" >

This should probably be "gone" until we need it - a layout traversal will run on it if it's just invisible.

@@ +14,5 @@
> +    android:layout_alignParentTop="true"
> +    android:background="@android:color/white"
> +    android:visibility="invisible" >
> +
> +    <FrameLayout

Why not have ZoomedView extend FrameLayout (instead of RelativeLayout) and remove this extra View?

@@ +26,5 @@
> +            android:layout_width="wrap_content"
> +            android:layout_height="wrap_content"
> +            android:background="#000000"
> +            android:padding="1dip"
> +            android:src="@drawable/ab_search" />

Is src and background necessary here? If not, perhaps we just shouldn't set it because (and correct me if I'm wrong) I don't think those elements will every be visible to the user.

@@ +34,5 @@
> +            android:background="@drawable/close"
> +            android:layout_height="20dp"
> +            android:layout_width="20dp"
> +            android:layout_gravity ="top|right" >
> +        </ImageView>

nit: Put the close tag in the original tag, like the other ImageView.

	Dominique Vincent [:domivinc] Assignee
	Comment 104 • 10 years ago

Hi Kats, thanks for the code review of browser.js. 

Could you have a look to the following code from my patch: method createZoomedViewEvent in mobile/android/base/GeckoEvent.java (attachment 8523541 [details] [diff] [review]).

For the ZoomedView event, I used the same implementation used in the "Thumbnail" event. I re-used the same fields:  mPoints[0], mMetaState, mBuffer (see method createThumbnailEvent just before my new method) . And I also used 2 other fields to pass additional data required for the zoomed view: mPoints[1] and mX.

The question we have (Michael and me): is it reasonable to re-used these fields for the zoomed view event? Should I move the new data (in particular scaleFactor -> mX) in another field?

> Review of attachment 8523541 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoEvent.java
> @@ +765,5 @@
> > +        event.mPoints = new Point[2];
> > +        event.mPoints[0] = new Point(x, y);
> > +        event.mPoints[1] = new Point(bufw, bufh);
> > +        // TODO move scaleFactor in another event field?
> > +        event.mX = (double) scaleFactor;
> 
> What is mX supposed to be used for?
> 
> I'm not sure mMetaState is supposed to be used for the tabId (I'd imagine
> meta key state).
> 
> Perhaps ping kats about the proper uses of these fields and whether using
> them for an unintended purpose is reasoable?
>

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 105 • 10 years ago

(In reply to Dominique Vincent [:domivinc] from comment #104)
> The question we have (Michael and me): is it reasonable to re-used these
> fields for the zoomed view event? Should I move the new data (in particular
> scaleFactor -> mX) in another field?

Re-using the fields is fine. In general there isn't really a strong relationship between the names of the fields and what they are used for. They might as well just be called mVar0, mVar1, ... mVarN. The names come from what they were used for when they were first added, but then other events reuse them for other things. Pretty much the only time new fields are added are if there aren't enough existing fields of a particular type (e.g. float) for some new event that somebody is trying to add.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 106 • 10 years ago

Comment on attachment 8523541 [details] [diff] [review]
Part1611 2-Bug_663803___Zoomed_view_implementation_using_render_document.patch

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

Here are some more comments - sorry for the delay, it's a lot of code. :)

::: mobile/android/base/ZoomedView.java
@@ +34,5 @@
> +                                    GeckoEventListener {
> +    private static final String LOGTAG = "GeckoZoomedView";
> +
> +    private ImageView  mZoomedImageView;
> +    private LayerView mLayerView;

nit: Be consistent with the notation - "m" represents a member variable so either use it for all member variables, or none of them.

Currently, new front-end code does not use it (I'm not sure about platform related code), so you can do whichever.

@@ +38,5 @@
> +    private LayerView mLayerView;
> +    private int currentState;
> +    private MotionEvent actionDownEvent;
> +    private static final int wCapturedViewInPercent = 80;
> +    private static final int hCapturedViewInPercent = 50;

nit: W_CAPUTRED_VIEW_IN_PERCENT, like zoomFactor.

@@ +141,5 @@
> +                        case MotionEvent.ACTION_DOWN:
> +                            currentState = -1;
> +                            originRawX = event.getRawX();
> +                            originRawY = event.getRawY();
> +                            PointF convertedPosition = getUnzoomedPositionFromPointInZoomedView(event.getX(), event.getY());

The MotionEvent.getX() docs say: "getX(int) for the first pointer index (may be an arbitrary pointer identifier)."

Why is getX() okay here? If it isn't, feel free to do this in a followup.

@@ +153,5 @@
> +                }
> +
> +                private void moveZoomedView(MotionEvent event, RelativeLayout.LayoutParams params) {
> +                    if ((currentState != MotionEvent.ACTION_MOVE) &&
> +                            (Math.abs((int) (event.getRawX()  - originRawX)) < 1) &&

Why raw coordinates here?

@@ +162,5 @@
> +                            originRawX;
> +                    float newTopMargin = params.topMargin + event.getRawY() -
> +                            originRawY;
> +                    ImmutableViewportMetrics metrics = mLayerView.getViewportMetrics();
> +                    mZoomedView.moveZoomedView(metrics, newLeftMargin, newTopMargin);

ZoomedView.this.moveZoomedView(..., rather than keeping an extra reference to `this`.

@@ +183,5 @@
> +        final int mZoomedViewHeight =  getZoomedHeight();
> +        RelativeLayout.LayoutParams params = (RelativeLayout.LayoutParams) getLayoutParams();
> +        float calX = ((x / zoomFactor) + offset.x + (((float) params.leftMargin) - offset.x) *
> +                ((parentWidth - offset.x - (mZoomedViewWidth / zoomFactor)) / (parentWidth -
> +                        offset.x - mZoomedViewWidth)));

Can you break apart this calculation into temporary values or explain it in comments? I don't really know what's going on here. Same with getZoomedViewTopLeftPositionFromTouchPosition.

@@ +213,5 @@
> +        if (mLayerView == null){
> +            return;
> +        }
> +        final int mZoomedViewWidth = getZoomedWidth();
> +        final int mZoomedViewHeight = getZoomedHeight();

nit: These aren't member variables, don't prefix them with "m".

@@ +218,5 @@
> +        parentWidth = metrics.getWidth();
> +        parentHeight = metrics.getHeight();
> +        RelativeLayout.LayoutParams newLayoutParams = new RelativeLayout.LayoutParams(
> +                RelativeLayout.LayoutParams.WRAP_CONTENT,
> +                RelativeLayout.LayoutParams.WRAP_CONTENT);

Rather than constructing new LayoutParams each time, call getLayoutParams() and update that reference to avoid allocation and garbage collection.

@@ +228,5 @@
> +
> +        if (newTopMargin < topMarginMin){
> +            newLayoutParams.topMargin = topMarginMin;
> +        }
> +        if (newLeftMargin < 0){

Why not leftMarginMin?

@@ +278,5 @@
> +        }
> +    }
> +
> +    public void setCapturedSize(ImmutableViewportMetrics metrics) {
> +        if (mLayerView == null /*|| mListener == null*/){

nit: Remove this comment

@@ +315,5 @@
> +        startTimeReRender = 0;
> +        changeVisibility = false;
> +        this.setVisibility(View.GONE);
> +        handler.removeCallbacks(runnable);
> +        changeVisibility = false;

Duplicate line - remove it.

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 107 • 10 years ago

(In reply to Michael Comella (:mcomella) from comment #102)
> Finkle, is this a feature we'd like to have in web apps? I noticed that this
> is added to .../layout/shared_ui_components.xml in the patch.

Good catch. We probably would not want this in WebApps, but if we put this behind a NIGHTLY_BUILD flag, we could fix the WebApp part later.

I think a build flag might be a good idea anyway, as it might take some time to tweak the feature after the initial code lands.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 108 • 10 years ago

(In reply to Mark Finkle (:mfinkle) from comment #107)
> (In reply to Michael Comella (:mcomella) from comment #102)
> Good catch. We probably would not want this in WebApps, but if we put this
> behind a NIGHTLY_BUILD flag, we could fix the WebApp part later.

Good idea - Dominique, can you add a NIGHTLY_BUILD flag (such as [1])?

This will also allow us to just fix the larger issues, land, and fix the rest after that (where more hands can get on it) - I also know how stressful it gets to manage such a large piece of code in patches so thanks for bearing with us, Dominique!

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=5288b15d22de#84

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 109 • 10 years ago

Comment on attachment 8523541 [details] [diff] [review]
Part1611 2-Bug_663803___Zoomed_view_implementation_using_render_document.patch

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

No other glaring issues at the moment though I skimped on the math and when-to-update logic - I'll have a closer look at those when the next revision comes out.

I know I gave a lot of comments - focus on the larger issues as we can always land it after those and address the smaller issues in followups.

Thanks for being patient, Dominique!

::: mobile/android/base/ZoomedView.java
@@ +45,5 @@
> +    private int xLastPosition;
> +    private int yLastPosition;
> +    private int zoomFactor = 2;
> +    private float parentHeight;
> +    private float parentWidth;

We have a copy of the metrics every time parent* is used - why not use the local varibales instead?

@@ +67,5 @@
> +    }
> +
> +    public ZoomedView(Context context, AttributeSet attrs, int defStyle) {
> +        super(context, attrs, defStyle);
> +        handler = new Handler();

Calling `new Handler()` will allow you to post to the Thread that created the class instance, which is bug-prone - you should use ThreadUtils to be explicit about which Thread you'd like to post your Runnables to.

@@ +253,5 @@
> +    public int getZoomedHeight() {
> +        return hCapturedView * zoomFactor;
> +    }
> +
> +    public PointF getInLayerViewPosition(int xZoomedView, int yZoomedView) {

Unused?

@@ +307,5 @@
> +        }
> +        startTimeReRender = 0;
> +        changeVisibility = true;
> +        moveUsingGeckoPosition(leftFromGecko, topFromGecko);
> +        requestZoomedViewRender();

This is called inside moveUsingGeckoPosition - remove this line and add a comment mentioning this.

@@ +352,5 @@
> +            }
> +        });
> +    }
> +
> +    void moveUsingGeckoPosition(int leftFromGecko, int topFromGecko) {

Any reason not to make this private?

@@ +396,5 @@
> +            if (mZoomedImageView != null) {
> +                mZoomedImageView.setImageDrawable(ob3);
> +            }
> +        }
> +        if (changeVisibility) {

nit: -> shouldSetVisibleOnUpdate, or similar.

@@ +435,5 @@
> +            handler.postDelayed(runnable, 2000);
> +            return;
> +        }
> +        // Wait at least 1s. between 2 calls
> +        if ((System.nanoTime() - lastStartTimeReRender) < 1000000){

1000000ns is 1ms - did you mean 1000000000ns, or 1s? Also, nit: declare this as a constant.

	Dominique Vincent [:domivinc] Assignee
	Comment 110 • 10 years ago

No code change in this patch just a rebase.

	Dominique Vincent [:domivinc] Assignee
	Comment 111 • 10 years ago

Michael, thanks for your detailed code review. I made all the code changes except for the following points:

1) Regarding the potential issue with “onFinishInflate”,  I didn’t find any device specific bugs linked to this method. I found the original comments about the issue here [1] / [2] .
I’m not sure that it’s really a device issue. From my point of view, the problem with  “onFinishInflate”  is due to the 2 different ways used to display a custom view like “ZoomedView”. 
You can put “<org.mozilla.gecko.ZoomedView” in the xml view structure or you can call “new  ZoomedView(context);” in the java code. In the first case, the method  “onFinishInflate” is always called. In the second case, the method is never called. This difference is not linked to a bug, but to the Android design.
Curently, in the case of the “ZoomedView” I think we should always be in the first case, no? Let me know if you have more details about this “onFinishInflate” device bug. 

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1002303#c27
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1024778

2) Regarding the stylistic issues, I tried to fix them. Is there somewhere a formatter editor config file for Eclipse matching the Fennec stylistic rules?

3)  In “mobile/android/base/gfx/LayerRenderer.java” I added a comment regarding the “5”  constant (moved into MAX_SCROLL_SPEED_TO_REQUEST_ZOOM_RENDER in the new patch). With this arbitrary value, I tried to avoid useless zoomed view render calls in case of scroll. 

4) “Why is getX() okay here?”  It should be ok because the event position is only used when there is no move, so probably only one pointer position. When we are in a move gesture, the values are not used. 

5) “Why raw coordinates here? event.getRawX()  - originRawX” I want to get the original touch positions to calculate the move of the zoomed view. Methods getX/getY returns adjusted positions taking into account the containing views. 

6) Regarding the “NIGHTLY_BUILD flag”, there is already a preference flag in mobile.js to activate/deactivate the zoomed view functionality: “pref("ui.zoomedview.enabled", true);”
When this flag is set to false, the zoomed view should totally be deactivated because the touch cluster event is not sent. In the last patch, I set the default value to false. You will have to change the value to test the zoomed view functionality in “about:config”.
Let me know if I still have to add a nightly build flag. In this case, should I keep the preference flag? 
I can also make the following in mobile.js: in nightly build the preference flag is set to true, and in the other case, the preference flag is set to false?

7) I’m not sure that the following files have been reviewed:
widget/android/AndroidBridge.cpp
widget/android/AndroidBridge.h
widget/android/AndroidJavaWrappers.cpp
widget/android/nsAppShell.cpp

	Dominique Vincent [:domivinc] Assignee
	Comment 112 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=43a37ee30df2

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 113 • 9 years ago

@nalexander: Is there a config file for preferably Eclipse, or intellij, for Fennec style? See #2 for more.

---

Hey, Dominique - I apologize about the delay again, this is really irresponsible of me. As a heads up, I am going to be on PTO from 1/1 - 1/13 but I'm going to make responding to this my top priority until then. When I leave, I'll make sure I get somebody else to fill in for me and try to check on it periodically while I'm gone.

(In reply to Dominique Vincent [:domivinc] from comment #111)
> Curently, in the case of the “ZoomedView” I think we should always be in the
> first case, no? Let me know if you have more details about this
> “onFinishInflate” device bug.

This is true - currently, we use the layout inflater for ZoomedView and thus can use onFinishInflate. It is a little more future proof to not make this assumption, however (especially considering how non-obvious it apparently is! :P). If you feel this is the best solution, then I'm fine with using it, just add a comment to onFinishInflate to this effect.

> 2) Regarding the stylistic issues, I tried to fix them. Is there somewhere a
> formatter editor config file for Eclipse matching the Fennec stylistic rules?

I don't believe so - the style changes between files so the best advice is to stay consistent to the given file. NI nalexander to double-check.

> 6) Regarding the “NIGHTLY_BUILD flag”

The NIGHTLY_BUILD flag ensures the feature is not available in non-Nightly builds so we don't end up shipping an unpolished feature. I mentioned we could refine this feature in followup builds so it'd be good to add the flag, in addition to the preference, so we can make sure to knock those out before this lands in release.

Let's also leave the preference disabled by default for now so we can get UX to check it over and make sure it's something that's polished enough to give to all of our Nightly users (and if not, nail the few followup bugs that will make that work).

Looking at the code, it appears that you are correct that the preference does prevent any events from occuring. However, we do inflate the ZoomedView and attach listeners independent of this preference. Currently, these listeners exit early (e.g. `if (layerView == null) return;`) but, for the sake of future proofing, this may not be the case. It'd be cool if we could avoid inflation all-together if the view is disabled (and it's also great to free up that memory) - perhaps we can stub it?

But it sounds like an annoying amount of work - let's file a followup. Note that this patch would have to land in the same version that this bug does.

> 7) I’m not sure that the following files have been reviewed:

Thanks for making sure we're thorough. :) I don't have the expertise to review these files -  snorp should take care of these so I'll be sure to poke him a little harder.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 114 • 9 years ago

Comment on attachment 8536151 [details] [diff] [review]
Zoomed view implementation using render document, after first code review

iirc, my last review didn't see too many glaring issues - snorp, do you want to give this a look? I'm also going to do a review on this new version ASAP.

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 115 • 9 years ago

I'm going to try to give this a look today.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 116 • 9 years ago

Comment on attachment 8536151 [details] [diff] [review]
Zoomed view implementation using render document, after first code review

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

Review for all java except ZoomedView.

::: mobile/android/base/GeckoEvent.java
@@ +764,5 @@
> +        GeckoEvent event = GeckoEvent.get(NativeGeckoEvent.ZOOMEDVIEW);
> +        event.mPoints = new Point[2];
> +        event.mPoints[0] = new Point(x, y);
> +        event.mPoints[1] = new Point(bufw, bufh);
> +        // TODO move scaleFactor in another event field?

nit: We decided mX is good, right? You can probably remove the todo.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +86,5 @@
>       *    case 1 above) you should always first grab a local copy of the reference, and then use
>       *    that because mViewportMetrics might get reassigned in between reading the different
>       *    fields. */
>      private volatile ImmutableViewportMetrics mViewportMetrics;
> +    private CopyOnWriteArrayList<LayerView.OnMetricsChangedListener> mViewportChangeListeners;

Actually, this solution doesn't quite work either - if we remove or add, we may be missing listeners when we calling out to them (unless we synchronize while iterating).

Being generic here isn't being helpful as we only have two values and this list is not likely to grow - let's just have mDynamicToolbarViewportChangeListener and mZoomedViewViewprotChangeListener, or similar.

I spoke with rnewman on IRC too:

rnewman> mcomella: agree with not using a list if you can avoid it and it'll be called very frequently
<@rnewman> but you might find it valuable to read how Android's view classes do it
<@rnewman> because obviously the draw methods they use every 20msec involve exactly this kind of hierarchical dispatch
<@rnewman> I think their solution is predicated on everything happening on the UI thread
<@rnewman> so consider how applicable that is

So there's that option too. :)

@@ +861,1 @@
>          }

nit: Was this empty line removal intentional?

@@ +986,5 @@
>  
> +    void addOnMetricsChangedListener(LayerView.OnMetricsChangedListener listener) {
> +        synchronized (mViewportChangeListeners) {
> +            if (!mViewportChangeListeners.contains(listener)) {
> +                mViewportChangeListeners.add(listener);

For your edification, `CopyOnWriteArrayList.addIfAbsent` would be cleaner.

@@ +993,5 @@
> +    }
> +
> +    public void removeOnMetricsChangedListener(LayerView.OnMetricsChangedListener listener) {
> +        synchronized (mViewportChangeListeners) {
> +            mViewportChangeListeners.remove(listener);

For your edification, remove is synchronized so the synchronized block is not necessary.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +167,5 @@
>          mCoordByteBuffer.order(ByteOrder.nativeOrder());
>          mCoordBuffer = mCoordByteBuffer.asFloatBuffer();
>  
>          Tabs.registerOnTabsChangedListener(this);
> +        mZoomedViewListeners = new ArrayList<LayerView.OnZoomedViewListener>();

Are there any concurrency issues with this list? It seems like addOnZoomedViewListener can be called from any thread and while iteration only occurs on the UI thread.

If not, add some comments explaining why operations on this list on occur on the UI thread, or assertOnUiThread assertions.

Alternatively, it seems like we'd only add a single onZoomedView listener (assuming a single LayerView) so perhaps we can avoid being generic by having a single listener - be sure to add a comment that only one layerview can be active at a time if you decide to go this route.

@@ +598,5 @@
>  
>          }
>  
> +        public void maybeRequestZoomedViewRender(RenderContext context){
> +            if (mZoomedViewListeners.size() == 0){

nit: Spaces before {

@@ +609,5 @@
> +            final float viewLeft = context.viewport.left - context.offset.x;
> +            final float viewTop = context.viewport.top - context.offset.y;
> +            boolean shouldWaitToRender = false;
> +
> +            if (Math.abs(mViewLeft - viewLeft) > MAX_SCROLL_SPEED_TO_REQUEST_ZOOM_RENDER ||

mViewLeft/Top is first initialized after this statement is evaluated - doesn't that mean this expression might be evaluated inconsistently?

::: mobile/android/base/resources/layout/shared_ui_components.xml
@@ +24,5 @@
>                                         android:layout_height="match_parent"
>                                         android:visibility="gone"/>
>  
>      <include layout="@layout/text_selection_handles"/>
> +    <include layout="@layout/zoomed_view"/>

Reminder to file a followup to move this out of shared_ui_components (and thus GeckoView).

::: mobile/android/chrome/content/browser.js
@@ +4952,5 @@
>  
>    _handleRetargetedTouchStart: function(aEvent) {
>      // we should only get this called just after a new touchstart with a single
>      // touch point.
> +

nit: unintentional line add?

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 117 • 9 years ago

Comment on attachment 8536151 [details] [diff] [review]
Zoomed view implementation using render document, after first code review

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

nits for the ZoomedViewTouchListener.

Good work on the style fixes by the way! :D

::: mobile/android/base/ZoomedView.java
@@ +29,5 @@
> +import android.widget.RelativeLayout;
> +
> +public class ZoomedView extends FrameLayout implements LayerView.OnMetricsChangedListener,
> +        LayerView.OnZoomedViewListener, GeckoEventListener {
> +    private static final String LOGTAG = "GeckoZoomedView";

nit: "Gecko" + ZoomedView.class.getSimpleName();

Newline below this.

@@ +33,5 @@
> +    private static final String LOGTAG = "GeckoZoomedView";
> +    private static final int ZOOM_FACTOR = 2;
> +    private static final int W_CAPTURED_VIEW_IN_PERCENT = 80;
> +    private static final int H_CAPTURED_VIEW_IN_PERCENT = 50;
> +    private static final int MINIMUM_DELAY_BETWEEN_TWO_RENDER_CALLS = 1000000; // ns

nit: You can include the units in the names: MINIMUM_DELAY_BETWEEN_TWO_RENDER_CALLS_MS

@@ +60,5 @@
> +        private float originRawY;
> +
> +        @Override
> +        public boolean onTouch(View view, MotionEvent event) {
> +            RelativeLayout.LayoutParams params = (RelativeLayout.LayoutParams) ZoomedView.this.getLayoutParams();

This is only accessed in moveZoomedView so just get the reference in that method.

@@ +64,5 @@
> +            RelativeLayout.LayoutParams params = (RelativeLayout.LayoutParams) ZoomedView.this.getLayoutParams();
> +            if (layerView == null) {
> +                return false;
> +            }
> +            switch (event.getAction()) {

nit: newline above this.

I like to put newlines after early returns

@@ +66,5 @@
> +                return false;
> +            }
> +            switch (event.getAction()) {
> +            case MotionEvent.ACTION_MOVE:
> +                moveZoomedView(event, params);

Would be slightly cleaner if moveZoomedView returned a boolean as to whether or not the view could be moved so currentState is only written to inside onTouch.

if (moveZoomedView(event, params)) {
  currentState = MotionEvent.ACTION_MOVE;
}

nit: currentState is not accessed outside of the listener and should be a member of it.

nit: currentState -> touchState

@@ +77,5 @@
> +                    layerView.dispatchTouchEvent(actionDownEvent);
> +                    actionDownEvent.recycle();
> +                    PointF convertedPosition = getUnzoomedPositionFromPointInZoomedView(event.getX(), event.getY());
> +                    MotionEvent e = MotionEvent.obtain(event.getDownTime(), event.getEventTime(),
> +                            MotionEvent.ACTION_UP, Math.round(convertedPosition.x), Math.round(convertedPosition.y),

Why did you round here? These values take a float.

@@ +90,5 @@
> +                originRawX = event.getRawX();
> +                originRawY = event.getRawY();
> +                PointF convertedPosition = getUnzoomedPositionFromPointInZoomedView(event.getX(), event.getY());
> +                actionDownEvent = MotionEvent.obtain(event.getDownTime(), event.getEventTime(),
> +                        MotionEvent.ACTION_DOWN, Math.round(convertedPosition.x), Math.round(convertedPosition.y),

Same.

@@ +99,5 @@
> +        }
> +
> +        private void moveZoomedView(MotionEvent event, RelativeLayout.LayoutParams params) {
> +            if ((currentState != MotionEvent.ACTION_MOVE) && (Math.abs((int) (event.getRawX() - originRawX)) < 1)
> +                    && (Math.abs((int) (event.getRawY() - originRawY)) < 1)) {

I would assume the android system would send out ACTION_MOVE events only when the touch point has actually moved - is there something special about having to move 1 unit (I assume pixel) first?

@@ +101,5 @@
> +        private void moveZoomedView(MotionEvent event, RelativeLayout.LayoutParams params) {
> +            if ((currentState != MotionEvent.ACTION_MOVE) && (Math.abs((int) (event.getRawX() - originRawX)) < 1)
> +                    && (Math.abs((int) (event.getRawY() - originRawY)) < 1)) {
> +                return;
> +            }

nit: newline after this early return

@@ +142,5 @@
> +    @Override
> +    protected void onFinishInflate() {
> +        super.onFinishInflate();
> +        ImageView closeButton = (ImageView) findViewById(R.id.dialog_close);
> +        closeButton.bringToFront();

Doesn't the order Views are declared in XML affect z-ordering? Perhaps this call is unnecessary. If not...

From the docs [1]:

Prior to KITKAT this method should be followed by calls to requestLayout() and invalidate() on the view's parent to force the parent to redraw with the new child ordering.

Is onFinishInflate called after layout (I would assume not)? If so, seems dumpy to force a layout right after layout - double-check that this is not the case.

nit: bringChildToFront(closeButton) is (negligibly?) more efficient.

[1]: https://developer.android.com/reference/android/view/View.html#bringToFront%28%29

@@ +151,5 @@
> +        });
> +
> +        zoomedImageView = (ImageView) findViewById(R.id.zoomed_image_view);
> +
> +        zoomedImageView.setOnTouchListener(new ZoomedViewTouchListener());

nit: no newline above this

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 118 • 9 years ago

Comment on attachment 8536151 [details] [diff] [review]
Zoomed view implementation using render document, after first code review

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

A lot of nits, but this is starting to look pretty close to me! Thanks for hanging in there, Dominique!

::: mobile/android/base/ZoomedView.java
@@ +34,5 @@
> +    private static final int ZOOM_FACTOR = 2;
> +    private static final int W_CAPTURED_VIEW_IN_PERCENT = 80;
> +    private static final int H_CAPTURED_VIEW_IN_PERCENT = 50;
> +    private static final int MINIMUM_DELAY_BETWEEN_TWO_RENDER_CALLS = 1000000; // ns
> +    private static final int DELAY_BEFORE_NEXT_REQUEST = 2000; // ms

nit: -> DELAY_BEFORE_NEXT_RENDER_REQUEST

@@ +45,5 @@
> +    private int hCapturedView;
> +    private int xLastPosition;
> +    private int yLastPosition;
> +    private boolean shouldSetVisibleOnUpdate;
> +    private int readBackBuffer[];

nit: Unused - remove.

@@ +169,5 @@
> +        RelativeLayout.LayoutParams params = (RelativeLayout.LayoutParams) getLayoutParams();
> +
> +        float calX =    (x / ZOOM_FACTOR) +     // Conversion of the x offset inside the zoomed view (using the scale factor)
> +
> +                        offset.x +               // The offset of the layerView

scroll offset?

@@ +188,5 @@
> +                        (((float) params.topMargin) - offset.y) *
> +                            ((parentHeight - offset.y - (zoomedViewHeight / ZOOM_FACTOR)) /
> +                            (parentHeight - offset.y - zoomedViewHeight));
> +
> +        return new PointF((int) calX, (int) calY);

To avoid unnecessary allocation (and thus GC), create a member PointF (returnValue ?) that you pass into this function and set the values there instead. Perhaps you can use the same PointF in the other method that allocates.

@@ +219,5 @@
> +
> +                + offset.x;     // The offset of the layerView
> +
> +        // Same comments here vertically
> +        float calY = (((y - (zoomedViewHeight / (2 * ZOOM_FACTOR)))) / ((parentHeight - offset.y - (zoomedViewHeight / ZOOM_FACTOR)) / (parentHeight

nit: format this as you did calX

@@ +230,5 @@
> +    private void moveZoomedView(ImmutableViewportMetrics metrics, float newLeftMargin, float newTopMargin) {
> +        if (layerView == null) {
> +            return;
> +        }
> +        final int zoomedViewWidth = getZoomedWidth();

nit: whitespace above this line.

@@ +245,5 @@
> +        leftMarginMin = (int) offset.x;
> +
> +        if (newTopMargin < topMarginMin) {
> +            newLayoutParams.topMargin = topMarginMin;
> +        }

nit: else if (newTopMargin + zoomedViewHeight >= parentHeight) {

Same for leftMargin

@@ +264,5 @@
> +        requestZoomedViewRender();
> +    }
> +
> +    public int getZoomedWidth() {
> +        return wCapturedView * ZOOM_FACTOR;

It seems every time this (and height) is used, you multiply it by 2 - is that not the zoom factor? Perhaps you should just store wCapturedView * ZOOM_FACTOR as a zoomedViewWidth member (or just viewWidth - this is the ZoomedView class after all) and do away with this method.

@@ +274,5 @@
> +
> +    @Override
> +    public void onConfigurationChanged(Configuration newConfig) {
> +        super.onConfigurationChanged(newConfig);
> +        blockZoomedViewUpdate(true);

Will onMetricsChanged always be called after onConfigurationChanged? If so, add a comment here saying that.

If not, when do we start updating the zoomed view again?

@@ +277,5 @@
> +        super.onConfigurationChanged(newConfig);
> +        blockZoomedViewUpdate(true);
> +    }
> +
> +    public void refreshZoomedView(ImmutableViewportMetrics viewport) {

nit: -> refreshZoomedViewSize or similar

@@ +278,5 @@
> +        blockZoomedViewUpdate(true);
> +    }
> +
> +    public void refreshZoomedView(ImmutableViewportMetrics viewport) {
> +        if (layerView != null) {

nit: Your intent is a bit clearer with:

if (layerView == null) {
  return;
}

@@ +294,5 @@
> +        wCapturedView = (int) (parentMinSize * W_CAPTURED_VIEW_IN_PERCENT / (ZOOM_FACTOR * 100.0));
> +        hCapturedView = (int) (parentMinSize * H_CAPTURED_VIEW_IN_PERCENT / (ZOOM_FACTOR * 100.0));
> +    }
> +
> +    public void blockZoomedViewUpdate(boolean bBlock) {

nit: -> shouldBlockUpdate

@@ +316,5 @@
> +        moveUsingGeckoPosition(leftFromGecko, topFromGecko);
> +    }
> +
> +    public void stopZoomDisplay() {
> +        startTimeReRender = 0;

No need to update this, right? We'll set it when we need it again in startZoomDisplay

@@ +338,5 @@
> +                        final JSONObject clickPosition = message.getJSONObject("clickPosition");
> +                        int left = clickPosition.getInt("x");
> +                        int top = clickPosition.getInt("y");
> +                        // Start to display inside the zoomedView
> +                        LayerView aLayerView = GeckoAppShell.getLayerView();

nit: not aLayerView - the "a" prefix (which we don't use) is for arguments. I dislike shadowing (i.e. "layerView"), so maybe geckoAppLayerView ?

@@ +367,5 @@
> +    }
> +
> +    @Override
> +    public void onMetricsChanged(final ImmutableViewportMetrics viewport) {
> +        ThreadUtils.postToUiThread(new Runnable() {

Why do we post to the UI thread here?

@@ +410,5 @@
> +        int pixelSize = (GeckoAppShell.getScreenDepth() == 24) ? 4 : 2;
> +        int capacity = 2 * wCapturedView * 2 * hCapturedView * pixelSize;
> +        if (buffer == null || buffer.capacity() != capacity) {
> +            if (buffer != null) {
> +                buffer = DirectBufferAllocator.free(buffer);

if (buffer == null), DirectBufferAllocator.free just returns null, no need for the check above.

@@ +424,5 @@
> +    }
> +
> +    @Override
> +    public void requestZoomedViewRender() {
> +        if (stopUpdateView == true) {

nit: if (stopUpdateView) {

@@ +431,5 @@
> +        // remove pending runnable
> +        ThreadUtils.removeCallbacksFromUiThread(requestRenderRunnable);
> +
> +        // Waiting for the end of the previous zoomed view display
> +        if (startTimeReRender != 0) {

What is the logic behind your re-render request strategy? The code/comments seem a bit unclear.

From (startTimeReRender != 0), it seems this prevents us from continuing unless updateView has been called (to set startTimeReRender to 0). I'm not sure why we'd post a runnable to call this method again when we could just wait for updateView to be called.

Then if we call updateView, and updateView has been called less than MINIMUM_DELAY_BETWEEN_TWO_RENDER_CALLS ago, we wait 2 seconds before rendering again - shouldn't we wait `MINIMUM_DELAY_BETWEEN_TWO_RENDER_CALLS - (System.nanoTime() - lastStartTimeReRender)`?

I think this code could be a bit clearer.

@@ +436,5 @@
> +            // post a new runnable 2s later
> +            ThreadUtils.postDelayedToUiThread(requestRenderRunnable, DELAY_BEFORE_NEXT_REQUEST);
> +            return;
> +        }
> +        // Wait at least 1ms. between 2 calls

nit: this comment could get out-of-date quickly - it should be "Wait at least MINIMUM_DELAY_..."

@@ +443,5 @@
> +            ThreadUtils.postDelayedToUiThread(requestRenderRunnable, DELAY_BEFORE_NEXT_REQUEST);
> +            return;
> +        }
> +        startTimeReRender = System.nanoTime();
> +        updateBufferSize();

Add a comment saying this does nothing if the buffer is already the right size, or allocates if it's the first call.

@@ +445,5 @@
> +        }
> +        startTimeReRender = System.nanoTime();
> +        updateBufferSize();
> +
> +        if (buffer == null) {

This should never happen unless something terrible happened and we already threw, right? I think this check is unnecessary.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 119 • 9 years ago

Snorp, the coordinate math in ZoomedView.:
  * getUnzoomedPositionFromPointInZoomedView
  * getZoomedViewTopLeftPositionFromTouchPosition
  * setCapturedSize
  * requestZoomedViewRender

is probably more accessible to you, given my unfamiliarity with the different values' meanings - do you mind reviewing those parts?

	Nick Alexander :nalexander [he/him]
	Comment 120 • 9 years ago

(In reply to Michael Comella (:mcomella) [PTO until 1/14] from comment #113)
> @nalexander: Is there a config file for preferably Eclipse, or intellij, for
> Fennec style? See #2 for more.

> > 2) Regarding the stylistic issues, I tried to fix them. Is there somewhere a
> > formatter editor config file for Eclipse matching the Fennec stylistic rules?
> 
> I don't believe so - the style changes between files so the best advice is
> to stay consistent to the given file. NI nalexander to double-check.

Sadly, no.  I would have expected Java formatting to be a solved problem, and it *kinda sorta* is.  The best option appears to be to use Eclipse to format (which in theory can be done from the command line), and then to use an IJ plugin called "Eclipse Code Formatter" [1] to share the formatting options.  Not very clean.  But we also don't have an Eclipse style for Fennec code (we do for background services code, which is gratuitously different) so it's all a ways into the future.

Sorry!

[1] https://plugins.jetbrains.com/plugin/6546

	Dominique Vincent [:domivinc] Assignee
	Comment 121 • 9 years ago

Hi Michael, I made all the changes. Some comments about your review:

1) Regarding your comment about the method moveZoomedView and ACTION_MOVE event, I share your point: “the android system would send out ACTION_MOVE events only when the touch point has actually moved”.
But on my device, when I just touch the screen I get an ACTION_MOVE with a very small delta on the position. To avoid the confusion between a TOUCH and a MOVE, I decided to keep only the MOVE with a value higher than 1 device unit. I added a comment in the code.

2) Regarding your question about the offset value here: 
“offset.x +               // The offset of the layerView”.
It’s not the scroll offset. I didn’t find any case where offset.x is different to 0 but offset.y can be set to different values. It’s the case when the search/url toolbar is hidden. In order to have the same calculation logic, I added the offset.x in the calculation but it’s probably useless. 

3) Very good catch regarding the issue in onConfigurationChanged/onMetricsChanged .
Only the orientation change will be taken into account. All the other configuration changes will be ignored.

4) I created bug 1117796 for the inflation issue. I’m going to work on it now.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 122 • 9 years ago

Comment on attachment 8544044 [details] [diff] [review]
Code changes after the second code review.

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

lgtm w/ nits, thanks so much Dominique!

Flagging snorp for comment 119 and *.h/cpp

::: mobile/android/base/ZoomedView.java
@@ +106,5 @@
> +        private boolean moveZoomedView(MotionEvent event) {
> +            RelativeLayout.LayoutParams params = (RelativeLayout.LayoutParams) ZoomedView.this.getLayoutParams();
> +            if ((touchState != MotionEvent.ACTION_MOVE) && (Math.abs((int) (event.getRawX() - originRawX)) < 1)
> +                    && (Math.abs((int) (event.getRawY() - originRawY)) < 1)) {
> +                // ACTION_MOVE can be detected for a very small delta on position (in fact the user touch the screen)

Thanks for the comment but it's unclear as is - maybe "...(e.g. when the user just touches the screen)"

@@ +203,5 @@
> +     * A touch point (x,y) occurs in LayerView, this point should be displayed
> +     * in the center of the zoomed view. The returned point is the position of
> +     * the Top-Left zoomed view point on the screen device
> +     */
> +    private void getZoomedViewTopLeftPositionFromTouchPosition(float x, float y) {

nit: I prefered when this returned a value. Methods without side effects are easier to test and reason about.

@@ +251,5 @@
> +            newLayoutParams.topMargin = topMarginMin;
> +        } else if (newTopMargin + viewHeight >= parentHeight) {
> +            newLayoutParams.topMargin = (int) (parentHeight - viewHeight);
> +        }
> +        if (newLeftMargin < leftMarginMin) {

nit: newline above this to make the separation between if-else blocks obvious.

@@ +273,5 @@
> +        // zoomed view update is restarted again.
> +        if (lastOrientation != newConfig.orientation) {
> +            shouldBlockUpdate(true);
> +        }
> +        lastOrientation = newConfig.orientation;

nit: Might as well do this in the `if` block - we're making a check in anyway and the assignment is a no-op outside of it.

@@ +295,5 @@
> +        viewWidth = (int) (parentMinSize * W_CAPTURED_VIEW_IN_PERCENT / (ZOOM_FACTOR * 100.0)) * ZOOM_FACTOR;
> +        viewHeight = (int) (parentMinSize * H_CAPTURED_VIEW_IN_PERCENT / (ZOOM_FACTOR * 100.0)) * ZOOM_FACTOR;
> +    }
> +
> +    public void shouldBlockUpdate(boolean bBlock) {

nit: bBlock -> shouldBlockUpdate

We don't use hungarian notation for variables in new files (i.e. the "b" in "bBlock").

@@ +419,5 @@
> +                buffer = DirectBufferAllocator.allocate(capacity);
> +            } catch (IllegalArgumentException iae) {
> +                Log.w(LOGTAG, iae.toString());
> +            } catch (OutOfMemoryError oom) {
> +                Log.w(LOGTAG, "Unable to allocate zoomedview buffer of capacity " + capacity);

Actually, I think we shouldn't catch the IllegalArgumentException - we want to see this obviously if the allocation fails as it's programmer error.

Also, in general, it's bad to catch Errors - they represent unsaveable problems. In particular, if there is not enough memory left to allocate, we're pretty screwed. I don't think there's a reason to catch the OutOfMemoryError, or at the very least if we still want to log it, we should also rethrow it.

@@ +443,5 @@
> +        if (startTimeReRender != 0) {
> +            // post a new runnable DELAY_BEFORE_NEXT_RENDER_REQUEST_MS later
> +            // We need to post with a delay to be sure that the last call to requestZoomedViewRender will be done.
> +            // For a static html page WITHOUT any animation/video, there is a last call to endDrawing and we need to make
> +            // the zoomed render on this last call.

Thanks for the comments - I think this could be simpler. Followup?

The states are:
  * Is something rendering?
    * If yes, has a render request been received while we are rendering?
  * Is there a render request waiting on the UI thread?
  * Has it been DELAY_MS since the last render?

And the flow, calling into requestRender:

If nothing is rendering, there's no ui thread render request, and it's been >= DELAY_MS since our last render:
  Render
If nothing is rendering, there's no ui thread render request, and it's been < DELAY_MS since our last render:
  Queue render for DELAY_MS - (MS since last render)
If nothing is rendering and there's a ui thread render request:
  Do nothing - we'll render shortly (DELAY_MS values have already been calculated for this request)
If something is rendering:
  Note that a render request has been made

And when the update completes (i.e. in updateView):

A render request has not been received:
  Do nothing
A render request has been received:
  Queue a ui thread render request to occur after DELAY_MS and clear the render request received flag

---

I think you can use a few booleans here to make the code more readable. Don't be afraid to use booleans for readability even though it can be solved using an int comparison (e.g. `startTimeReRender != 0` can be `isRendering`).

Don't forget to use concurrent data structures or the `volatile` keyword as necessary.

Let me know if this does not make sense.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +598,5 @@
>  
>          }
>  
> +        public void maybeRequestZoomedViewRender(RenderContext context){
> +            if (mZoomedViewListeners.size() == 0) {

Technically, this (and less likely mZoomedViewListeners.clear) can still be called concurrently. I don't think this is too bad though because it's just a short-circuit - if you agree, add a comment explaining this behavior.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 123 • 9 years ago

Dominique, make sure part 2 is rebased when you fix these nits - I get some merge conflicts that I don't have the expertise to fix locally so I can't throw this at our test servers.

	Dominique Vincent [:domivinc] Assignee
	Comment 124 • 9 years ago

Michael I made the changes.

1) Regarding the methods “getZoomedViewTopLeftPositionFromTouchPosition” and “getUnzoomedPositionFromPointInZoomedView”, I tried here to apply your previous comment regarding those methods :
>>>>
To avoid unnecessary allocation (and thus GC), create a member PointF (returnValue ?) that you pass into this function and set the values there instead. Perhaps you can use the same PointF in the other method that allocates.
<<<<

But I probably didn’t implement it correctly.
In the last patch, I made another attempt to implement your comment. 


2) Regarding the method “requestZoomedViewRender”, I’m not sure that your proposal flow will simplify the 2 current tests to delay the render process.
I kept your idea about the readability of the tests. I added “isRendering” and “renderFrequencyTooHigh” boolean methods. I also updated the 2 main comments about those tests.
If you think that it’s not enough, a fresh pair of eyes could also be a good idea in a follow up bug. 


3) I rebased the code. Different issues have been fixed due to the fresh version of the code in “widget/android/nsAppShell.cpp” and “widget/android/AndroidBridge.cpp”.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 125 • 9 years ago

Dominique, it looks like your latest patch (comment 124) does not contain ZoomedView.java! Can you make sure you rebased properly and `hg add`ed all the related files?

	Dominique Vincent [:domivinc] Assignee
	Comment 127 • 9 years ago

Hi Michael, 

Sorry for the 2 missing files in my last patch. 
I made another rebase/merge and added in the patch "ZoomedView.java" and "zoomed_view.xml" files.

I didn't change the code, so you can apply the same comments from the previous patch with the 2 missing files.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 128 • 9 years ago

Dominique, I should get to this tomorrow - sorry for the delays again, I've been busy with finishing the new tablet work (which is currently in beta).

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 129 • 9 years ago

Comment on attachment 8552237 [details] [diff] [review]
Rebase of Part 2 with last code changes

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

r+, assuming no other changes than the ones one mentioned.

(In reply to Dominique Vincent [:domivinc] from comment #124)
> To avoid unnecessary allocation (and thus GC), create a member PointF
> (returnValue ?) that you pass into this function and set the values there
> instead. Perhaps you can use the same PointF in the other method that
> allocates.
> <<<<
> 
> But I probably didn’t implement it correctly.
> In the last patch, I made another attempt to implement your comment. 

Make convertedPosition a function local variable (i.e. not a member variable) and you got it!

::: mobile/android/base/gfx/LayerRenderer.java
@@ +598,5 @@
>  
>          }
>  
> +        public void maybeRequestZoomedViewRender(RenderContext context){
> +            // Concurrently update of mZoomedViewListeners should not be an issue here

Add the reason why it's not an issue

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 131 • 9 years ago

Comment on attachment 8552237 [details] [diff] [review]
Rebase of Part 2 with last code changes

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

Wow, this is a doosie!

It looks mostly alright to me, assuming kats has gone over all the viewport insanity. If we want to get this in, we should do it sooner rather than later, as I already see the JNI stuff here has bitrotted. Dominique you may want to rebase this on the latest mozilla-central when you get an opportunity.


I wonder if we could get away with letting the compositor zoom us instead of redrawing the page. The performance would be quite good, but the quality would be different. It should be possible to do a pixel-for-pixel zoom, though, where the pixels just look larger. Jeff I don't know the best way to do that, do you have ideas?

Anyway, I'm cautiously ok with this going in as long as we have it on nightly only. Dominique, you'll need to add a "if (AppConstants.NIGHTLY_BUILD)" guard somewhere. Also a preference. We're going to at least need the UX folks to go through this before turning it on for everyone and riding the trains.

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 132 • 9 years ago

Jeff, NI on the question above re: opengl magnification

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 133 • 9 years ago

Huh, I guess 'nearest' does what I was thinking for the most part. So this should be pretty easy.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 134 • 9 years ago

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #131)
> It looks mostly alright to me, assuming kats has gone over all the viewport
> insanity.

Which viewport insanity? I haven't reviewed the patch that you commented on.

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 135 • 9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #134)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #131)
> > It looks mostly alright to me, assuming kats has gone over all the viewport
> > insanity.
> 
> Which viewport insanity? I haven't reviewed the patch that you commented on.

Ah. I saw some comments above indicating you had at least looked at it. You should review the stuff in ZoomedView.java, I think.

	Dominique Vincent [:domivinc] Assignee
	Comment 136 • 9 years ago

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #131)
> It looks mostly alright to me, assuming kats has gone over all the viewport
> insanity. If we want to get this in, we should do it sooner rather than
> later, as I already see the JNI stuff here has bitrotted. Dominique you may
> want to rebase this on the latest mozilla-central when you get an
> opportunity.
> 
I made a rebase but I didn’t notice any issue in the JNI part. The only file merged was “browser.j”. 
I don’t know what is the correct procedure in order to manage the JNI changes. 
Should I remove the 2 generated files ( “widget/android/AndroidJavaWrappers.cpp” and “widget/android/AndroidJavaWrappers.h”) from the patch? Should I change a build options in my .mozconfig file? 
I found the following command :
make -C /obj-arm-linux-androideabi/mobile/android/base update-generated-wrappers
but I don’t know if it will be automatically run by the build.

> Anyway, I'm cautiously ok with this going in as long as we have it on
> nightly only. Dominique, you'll need to add a "if
> (AppConstants.NIGHTLY_BUILD)" guard somewhere. Also a preference. We're
> going to at least need the UX folks to go through this before turning it on
> for everyone and riding the trains.

Regarding the nightly build flag, bug 1117796 will implement it. The review of this patch should be done by Michael. The 2 bugs will land together.

	Kelsey Gilbert [:jgilbert]
	Comment 137 • 9 years ago

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #133)
> Huh, I guess 'nearest' does what I was thinking for the most part. So this
> should be pretty easy.

NEAREST does this when used with a tex lookup in the fragment shader, yeah.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 138 • 9 years ago

Comment on attachment 8554413 [details] [diff] [review]
Part2 updates - Comment change and move of convertedPosition in a local variable

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

Cool beans - I'll look at bug 1117796 ASAP!

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 139 • 9 years ago

Comment on attachment 8552237 [details] [diff] [review]
Rebase of Part 2 with last code changes

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

The code seems pretty sane to me and I don't see any major problems. However there are quite a few nits and small improvements that could be made; see my comments below. Up to you which ones you want to do and which ones you want to ignore - I haven't worked in this code for a while so I'm probably out of date with respect to what the current conventions are. You already have two r+'s so you don't need mine as well, but giving an f+ anyway to indicate I've looked over it and it looks good.

::: mobile/android/base/BrowserApp.java
@@ +959,5 @@
>          ThreadUtils.assertOnUiThread();
>  
>          if (enabled) {
>              if (mLayerView != null) {
> +                mLayerView.setOnMetricsChangedDynamicToolbarViewportListener(this);

Rather than having OnMetricsChangedDynamicToolbarViewportListener and OnMetricsChangedZoomedViewportListener I would prefer you just had "addMetricsChangedListener" and "removeMetricsChangedListener" and store a List<LayerView.OnMetricsChangedListener> in GeckoLayerClient.

::: mobile/android/base/ZoomedView.java
@@ +1,2 @@
> +package org.mozilla.gecko;
> +

This file needs a copyright header thingy. And many of the functions in this file can be private, please reduce the visibility wherever you can. That way you can also remove some of the layerView==null checks which will become redundant. For example all the call sites of setCapturedSize already check for a null layerView or enforce that it is non-null, so that function (once private) doesn't need to do the check again.

@@ +1,5 @@
> +package org.mozilla.gecko;
> +
> +import java.text.DecimalFormat;
> +
> +import java.nio.ByteBuffer;

Organize imports by blocks separated by empty line: org.mozilla.*, com.*, net.*, org.*, android.*, then java.*

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Java_practices

@@ +29,5 @@
> +import android.widget.RelativeLayout;
> +
> +public class ZoomedView extends FrameLayout implements LayerView.OnMetricsChangedListener,
> +        LayerView.OnZoomedViewListener, GeckoEventListener {
> +    private static final String LOGTAG = "Gecko" + ZoomedView.class.getSimpleName();

I assume this getSimpleName business is some new fennec convention since I see it in a few files. Silly, if you ask me :)

@@ +39,5 @@
> +    private static final int DELAY_BEFORE_NEXT_RENDER_REQUEST_MS = 2000;
> +
> +    private ImageView zoomedImageView;
> +    private LayerView layerView;
> +    private MotionEvent actionDownEvent;

Move actionDownEvent to be inside ZoomedViewTouchListener, you don't see to use it outside that anywhere.

@@ +50,5 @@
> +    private PointF returnValue;
> +
> +    private boolean stopUpdateView;
> +
> +    private int lastOrientation = 0;

Class members get initialized to 0 by default, so no need for explicit initialization here.

@@ +55,5 @@
> +
> +    private ByteBuffer buffer;
> +    private Runnable requestRenderRunnable;
> +    private long startTimeReRender = 0;
> +    private long lastStartTimeReRender = 0;

Ditto

@@ +60,5 @@
> +
> +    private class ZoomedViewTouchListener implements View.OnTouchListener {
> +        private float originRawX;
> +        private float originRawY;
> +        private int touchState;

Instead of an int I would just make this a "bool dragged" or something. It only ever has two values anyway.

@@ +106,5 @@
> +
> +        private boolean moveZoomedView(MotionEvent event) {
> +            RelativeLayout.LayoutParams params = (RelativeLayout.LayoutParams) ZoomedView.this.getLayoutParams();
> +            if ((touchState != MotionEvent.ACTION_MOVE) && (Math.abs((int) (event.getRawX() - originRawX)) < 1)
> +                    && (Math.abs((int) (event.getRawY() - originRawY)) < 1)) {

Instead of hard-coding 1 here I would suggest using PanZoomController.CLICK_THRESHOLD, or maybe PanZoomController.PAN_THRESHOLD. You can see how those are used and decide what's appropriate.

@@ +133,5 @@
> +
> +    public ZoomedView(Context context, AttributeSet attrs, int defStyle) {
> +        super(context, attrs, defStyle);
> +        convertedPosition = new PointF();
> +        returnValue = new PointF();

Get rid of convertedPosition and use local variables instead. You can keep returnValue as-is to avoid allocating lots of new PointF instances. The way you have it set up now, convertedPosition is assigned a new PointF here which is never actually used. And then in all the functions where convertedPosition is used, it is modified to point to returnValue anyway.

@@ +254,5 @@
> +        leftMarginMin = (int) offset.x;
> +
> +        if (newTopMargin < topMarginMin) {
> +            newLayoutParams.topMargin = topMarginMin;
> +        } else if (newTopMargin + viewHeight >= parentHeight) {

Use > instead of >= for consistency

@@ +267,5 @@
> +
> +        setLayoutParams(newLayoutParams);
> +        convertedPosition = getUnzoomedPositionFromPointInZoomedView(0, 0);
> +        xLastPosition = Math.round(convertedPosition.x);
> +        yLastPosition = Math.round(convertedPosition.y);

store [xy]LastPosition as a Point instead of separate ints, and use org.mozilla.gecko.gfx.PointUtils.round on convertedPosition to compute it.

@@ +384,5 @@
> +                if (layerView == null) {
> +                    return;
> +                }
> +                shouldBlockUpdate(false);
> +                refreshZoomedViewSize(viewport);

Probably don't need this null check here since there's one inside refreshZoomedViewSize anyway.

@@ +482,5 @@
> +        final int xPos = (int) (origin.x - offset.x) + xLastPosition;
> +        final int yPos = (int) (origin.y - offset.y) + yLastPosition;
> +
> +        GeckoEvent e = GeckoEvent.createZoomedViewEvent(tabId, xPos, yPos, viewWidth,
> +                viewHeight, (float) (2.0 * metrics.zoomFactor), buffer);

Use ZOOM_FACTOR instead of 2.0

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +86,5 @@
>       *    that because mViewportMetrics might get reassigned in between reading the different
>       *    fields. */
>      private volatile ImmutableViewportMetrics mViewportMetrics;
> +    private LayerView.OnMetricsChangedListener mDynamicToolbarViewportChangeListener;
> +    private LayerView.OnMetricsChangedListener mZoomedViewViewportChangeListener;

See comment in BrowserApp, I would rather have a List<LayerView.OnMetricsChangedListener> here.

@@ +917,5 @@
> +        if (mDynamicToolbarViewportChangeListener != null) {
> +        	mDynamicToolbarViewportChangeListener.onPanZoomStopped();
> +        }
> +        if (mZoomedViewViewportChangeListener != null) {
> +        	mZoomedViewViewportChangeListener.onPanZoomStopped();

indentation

::: mobile/android/base/gfx/LayerRenderer.java
@@ +97,5 @@
>      private int mTMatrixHandle;
>  
> +    private List<LayerView.OnZoomedViewListener> mZoomedViewListeners;
> +    private float mViewLeft = 0.0f;
> +    private float mViewTop = 0.0f;

no need to initialize these, they are 0 by default. Also i would prefer if they were called mLastViewLeft and mLastViewTop

@@ +597,5 @@
>              runRenderTasks(mTasks, true, mFrameStartTime);
>  
>          }
>  
> +        public void maybeRequestZoomedViewRender(RenderContext context){

nit: space before {. Also make this private

@@ +600,5 @@
>  
> +        public void maybeRequestZoomedViewRender(RenderContext context){
> +            // Concurrently update of mZoomedViewListeners should not be an issue here
> +            if (mZoomedViewListeners.size() == 0) {
> +                return;

I think concurrent update could be an issue, since this code runs on the compositor thread and all the other accesses/mutations of mZoomedViewListeners are on the UI thread. You could probably get away with adding a volatile bool mHasZoomedViewListeners and then reading that here, setting/clearing it on the UI thread when listeners are added and removed. Because this function gets called a lot we want to avoid adding synchronization here so the volatile bool might be a cheap solution. Be sure to document the purpose of the bool if you add it.

::: mobile/android/base/gfx/LayerView.java
@@ +526,5 @@
>  
> +    @WrapElementForJNI(allowMultithread = true, stubName = "updateZoomedView")
> +    public static void updateZoomedView(ByteBuffer data) {
> +        data.position(0);
> +        LayerView layerView = GeckoAppShell.getLayerView();

I would add a comment here that this function runs on the Gecko main thread. Also I would move the data.position(0) call to be inside the LayerRenderer.updateZoomedView function, so that it happens just before handing the buffer to the listener. That way if one listener moves the position you'll still reset it before handing it to the next listener.

@@ +529,5 @@
> +        data.position(0);
> +        LayerView layerView = GeckoAppShell.getLayerView();
> +        if (layerView != null) {
> +            LayerRenderer layerRenderer = layerView.getRenderer();
> +            if (layerRenderer != null){

nit: space before {

@@ +533,5 @@
> +            if (layerRenderer != null){
> +                layerRenderer.updateZoomedView(data);
> +            }
> +        }
> +        return;

Unnecessary return

@@ +678,5 @@
> +    }
> +
> +    // Public hooks for zoomed view
> +
> +    public interface OnZoomedViewListener {

Would prefer to call this "ZoomedViewListener" (i.e. drop the "On")

::: widget/android/AndroidBridge.cpp
@@ +1705,5 @@
>  }
>  
> +static float
> +GetScaleFactor(nsPresContext* mPresContext) {
> +  nsIPresShell* presShell = mPresContext->PresShell();

The "m" prefix is used for class member variables. In this case it's not a member variable, so just use presContext or aPresContext.

@@ +1707,5 @@
> +static float
> +GetScaleFactor(nsPresContext* mPresContext) {
> +  nsIPresShell* presShell = mPresContext->PresShell();
> +  LayoutDeviceToLayerScale cumulativeResolution(presShell->GetCumulativeResolution().width);
> +  return cumulativeResolution.scale;

This file seems to have 4-space indentation in general, would be good to maintain that for consistency.

@@ +1711,5 @@
> +  return cumulativeResolution.scale;
> +}
> +
> +nsresult
> +AndroidBridge::CaptureZoomedView (nsIDOMWindow *window, nsIntRect zoomedViewRect, Object::Param buffer,

nit: no space before (

@@ +1718,5 @@
> +  struct timeval        timeEnd;
> +  struct timeval        timeEndAfter;
> +  struct timeval        timeStart;
> +  struct timeval        res;
> +  gettimeofday (&timeStart, NULL);

Looks like this stuff was for local testing/timing? Please remove it as it doesn't seem to be used any more.

@@ +1723,5 @@
> +
> +  if (!buffer)
> +    return NS_ERROR_FAILURE;
> +
> +  nsCOMPtr < nsIDOMWindowUtils > utils = do_GetInterface (window);

remove spaces around < and >, here and throughout this function. Also remove the space before (, throughout the function. See the rest of the code in this file for coding style, or refer to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +1750,5 @@
> +  nsCOMPtr < nsIPresShell > presShell = presContext->PresShell ();
> +
> +  float scaleFactor = GetScaleFactor(presContext) ;
> +
> +      nscolor bgColor = NS_RGB (255, 255, 255);

indentation

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 140 • 9 years ago

Dominique, I just want to land this - let's work on Kats' feedback in a followup. I'll review bug 1117796 now so we can land this.

Thanks, Kats!

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 141 • 9 years ago

try run in bug 1117796 comment 3.

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 142 • 9 years ago

(In reply to Dominique Vincent [:domivinc] from comment #136)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #131)
> > It looks mostly alright to me, assuming kats has gone over all the viewport
> > insanity. If we want to get this in, we should do it sooner rather than
> > later, as I already see the JNI stuff here has bitrotted. Dominique you may
> > want to rebase this on the latest mozilla-central when you get an
> > opportunity.
> > 
> I made a rebase but I didn’t notice any issue in the JNI part. The only file
> merged was “browser.j”. 
> I don’t know what is the correct procedure in order to manage the JNI
> changes. 
> Should I remove the 2 generated files (
> “widget/android/AndroidJavaWrappers.cpp” and
> “widget/android/AndroidJavaWrappers.h”) from the patch? Should I change a
> build options in my .mozconfig file? 
> I found the following command :
> make -C /obj-arm-linux-androideabi/mobile/android/base
> update-generated-wrappers
> but I don’t know if it will be automatically run by the build.

Oops, I guess that stuff you touched is not generated (yet), so it's not affected. Sorry for the noise.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 143 • 9 years ago

The build in the try push in bug 1117796 comment 3 failed because of some
unused vars - this fixes that.

new push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=544278f87f3b

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 144 • 9 years ago

Sorry, I'm silly with bzexport sometimes. :)

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 145 • 9 years ago

One more update & a green push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1dc8eb1f0b7

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 146 • 9 years ago

Please land in order:
  * Rebase pt 1
  * Rebase pt 2
  * pt 2 updates
  * pt 4
  * bug 1117796

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 147 • 9 years ago

Nvm, I was way too excited to land this.

Congrats, Dominique!!!

remote:   https://hg.mozilla.org/integration/fx-team/rev/a7736ab45ab4
remote:   https://hg.mozilla.org/integration/fx-team/rev/32ce7927011c
remote:   https://hg.mozilla.org/integration/fx-team/rev/43847e260e1f
remote:   https://hg.mozilla.org/integration/fx-team/rev/913310f4e144
remote:   https://hg.mozilla.org/integration/fx-team/rev/134bcefd5f7b

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 148 • 9 years ago

Dominique, do you mind going through the comment history (find in page FTW!) and filing followups where necessary? Mark them to depend on this bug.

	Dominique Vincent [:domivinc] Assignee
	Comment 149 • 9 years ago

(In reply to Michael Comella (:mcomella) from comment #148)
> Dominique, do you mind going through the comment history (find in page FTW!)
> and filing followups where necessary? Mark them to depend on this bug.

I will do it today!

	Carsten Book [:Tomcat]
	Comment 150 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/a7736ab45ab4
https://hg.mozilla.org/mozilla-central/rev/32ce7927011c
https://hg.mozilla.org/mozilla-central/rev/43847e260e1f
https://hg.mozilla.org/mozilla-central/rev/913310f4e144

	Teodora Vermesan (:TeoVermesan)
	Comment 151 • 9 years ago

Tried with the latest fx-team build and using Alcatel One Touch (Android 4.1.2) this is how magnifying glass in areas of clustered links looks like: http://i.imgur.com/2cuQMwP.png
Sometimes it is very hard to trigger it and you have to try a lot of times to succeed. But once it is triggered, you can move the glass around. Panning the page up/down or zooming it will also change the content from the box. The box is still displayed when you touch the screen and will disappear only when tapping the "X" button.

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 152 • 9 years ago

That's a pretty encouraging start. I think we still need to have it preffed off for now, at least. Is there a followup bug for that?

	Dominique Vincent [:domivinc] Assignee
	Comment 153 • 9 years ago

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #152)
> That's a pretty encouraging start. I think we still need to have it preffed
> off for now, at least. Is there a followup bug for that?

The pref (ui.zoomedview.enabled) is already set to false by default in mobile/android/app/mobile.js. If you have the zoomed view functionality without changing the pref in "about:config" there is probably a bug.

Could you verify in "about:config" the value of ui.zoomedview.enabled ?

	u421692
	Comment 154 • 9 years ago

(In reply to Dominique Vincent [:domivinc] from comment #153)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #152)
> > That's a pretty encouraging start. I think we still need to have it preffed
> > off for now, at least. Is there a followup bug for that?
> 
> The pref (ui.zoomedview.enabled) is already set to false by default in
> mobile/android/app/mobile.js. If you have the zoomed view functionality
> without changing the pref in "about:config" there is probably a bug.
> 
> Could you verify in "about:config" the value of ui.zoomedview.enabled ?

The "ui.zoomedview.enabled" pref is indeed "false" by default, we've tested with the pref set to "true".

	Dominique Vincent [:domivinc] Assignee
	Comment 155 • 9 years ago

Michael, the followups are bug 1126866, bug 1126989, bug 1127901 and bug 1127909.

Regarding the following points:

  * (followup) Reddit seems to periodically refresh at which point the zoomed in view gets momentarily blurry before it re-renders - I wonder if there's anyway to avoid this skip
  * (followup) There's a slight delay when first clicking in an ambiguous space where the zoomed in view is blurry - I wonder if we can fill this pause with either an animation, or something. Similarly when scrolling (but I think there's less to do here)

I cannot reproduce them.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 156 • 9 years ago

NI self to address comment 155.

Sorry for the delay, Dominique - I'll get to this tomorrow!

By the way, if you use the "Need more information from" box at the bottom of the page, you will send me a notification so it'll be much harder for me to miss your questions! :)

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 157 • 9 years ago

Sorry, I glossed over this today - I will get to it tomorrow for sure.

	Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]
	Comment 158 • 9 years ago

(In reply to Dominique Vincent [:domivinc] from comment #155)
> Michael, the followups are...

Thanks!

>   * (followup) Reddit seems to periodically refresh at which point the
> zoomed in view gets momentarily blurry before it re-renders - I wonder if
> there's anyway to avoid this skip

I can no longer repro either - perhaps this was from an old build?

>   * (followup) There's a slight delay when first clicking in an ambiguous
> space where the zoomed in view is blurry - I wonder if we can fill this
> pause with either an animation, or something. Similarly when scrolling (but
> I think there's less to do here)

I cannot reproduce this either - also, old build?

	donrhummy
	Comment 159 • 9 years ago

This bug says the target milestone is FF 38. What is the actual target?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 160 • 9 years ago

It landed in 38 but was disabled by default. It is enabled on current Nightly (43), or you can probably find and flip the pref ui.zoomedview.disabled in earlier versions.

	donrhummy
	Comment 161 • 9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #160)
> It landed in 38 but was disabled by default. It is enabled on current
> Nightly (43), or you can probably find and flip the pref
> ui.zoomedview.disabled in earlier versions.

In FF Beta Android, that preference `ui.zoomedview.disabled` is set to "false". Would setting it true turn this on? That seem backwards.

	Dominique Vincent [:domivinc] Assignee
	Comment 162 • 9 years ago

(In reply to donrhummy from comment #161)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #160)
> > It landed in 38 but was disabled by default. It is enabled on current
> > Nightly (43), or you can probably find and flip the pref
> > ui.zoomedview.disabled in earlier versions.
> 
> In FF Beta Android, that preference `ui.zoomedview.disabled` is set to
> "false". Would setting it true turn this on? That seem backwards.

Currently the zoomed view is available in nightly build only (see bug 1182809 comment 2). The preference `ui.zoomedview.disabled` should have no effect in FF Beta Android. Bug 1196146 will implement it in all Android versions.

	Barbara Bermes [:barbara] - NI please!
	Comment 163 • 8 years ago

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]: Provide magnifying glass in areas of clustered links
[Links (documentation, blog post, etc)]:

	Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland
	Comment 164 • 8 years ago

needinfoing barbara: is this actually in FF45 for android? if yes how do you trigger it? if no, when will we ship?

	:Margaret Leibovic
	Comment 165 • 8 years ago

(In reply to Roland Tanglao :rolandtanglao from comment #164)
> needinfoing barbara: is this actually in FF45 for android? if yes how do you
> trigger it? if no, when will we ship?

No, we disabled this in bug 1245930.

	Julien Cristau [:jcristau] (back April 22)
	Comment 166 • 7 years ago

It sounds like this never shipped and the code is being removed, removing relnote-firefox flag.