1117796 - Avoid zoomed view inflation - followup of 663803

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dominique Vincent [:domivinc]]

This bug is a followup of bug 663803, see comment 113 from :mcomella (https://bugzilla.mozilla.org/show_bug.cgi?id=663803#c113):


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.

Comment 1

[Dominique Vincent [:domivinc]]

Attachment: Zoomed view in a ViewStub, inflate zoomed view only with nightly builds

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8544576 [details] [diff] [review]
Zoomed view in a ViewStub, inflate zoomed view only with nightly builds

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

Unclear if this should be in GeckoApp (i.e. in more than just the Fennec browser but anything using GeckoView, which is the platform on Android) but let's do that in a followup.

::: mobile/android/base/resources/layout/shared_ui_components.xml
@@ +29,5 @@
> +    <ViewStub android:id="@+id/zoomed_view_stub"
> +               android:inflatedId="@+id/zoomed_view"
> +               android:layout="@layout/zoomed_view"
> +               android:layout_width="wrap_content"
> +               android:layout_height="wrap_content" />

nit: Align all of the "android" lines vertically.

I assume layout_width/height are necessary here? It is a little annoying to duplicate the parameters that are already in @layout/zoomed_view.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

try (+ bug 663803): https://treeherder.mozilla.org/#/jobs?repo=try&revision=de6c45ae2c5a

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Updated push in bug 663803 comment 143.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Please see bug 663803 comment 146.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Landed in bug 663803 comment 147.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #2)
> nit: Align all of the "android" lines vertically.

Ah, was a bit hasty - maybe just take care of this in a followup?

> I assume layout_width/height are necessary here? It is a little annoying to
> duplicate the parameters that are already in @layout/zoomed_view.

NI to answer this question.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/134bcefd5f7b

Comment 9

[Dominique Vincent [:domivinc]]

(In reply to Michael Comella (:mcomella) from comment #7)
> (In reply to Michael Comella (:mcomella) from comment #2)
> > nit: Align all of the "android" lines vertically.
> 
> Ah, was a bit hasty - maybe just take care of this in a followup?
> 
> > I assume layout_width/height are necessary here? It is a little annoying to
> > duplicate the parameters that are already in @layout/zoomed_view.
> 
> NI to answer this question.

Correct, I tried without the 2 parameters and I get an exception:
java.lang.RuntimeException: Binary XML file line #8: You must supply a layout_width attribute.