1222638 - Don't inflate zoomed view until its needed

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

We're currently inflating it in BrowserApp.onCreate and will be taking a start-up hit for it.

Comment 1

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

It appears we inflate the zoomed view immediately because it registers GeckoThread listeners when it's constructed. It'd be good to separate the UI from the data processing, allowing us to ViewStub this View.

Comment 2

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

Dominique, can you take a look at this?

Comment 3

[Dominique Vincent [:domivinc]]

Attachment: patch-15112015 1-Bug_1222638___Don_t_inflate_zoomed_view_until_its_needed___r_mcomella.patch

Michael, I removed the call to inflate the zoomed view from the method "BrowserApp.onCreate". The call is done now when the event "Gecko:DelayedStartup" is received.

We can move it later: when the first event "Gesture:clusteredLinksClicked" is received. But in this case, the display of the zoomed view is longer for the first click inside a cluster. So from a user point of view, I would prefer to avoid that.

Comment 4

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

Comment on attachment 8687654 [details] [diff] [review]
patch-15112015 1-Bug_1222638___Don_t_inflate_zoomed_view_until_its_needed___r_mcomella.patch

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

Looks good! Thanks Dominique!

(In reply to Dominique Vincent [:domivinc] from comment #3)
> Michael, I removed the call to inflate the zoomed view from the method
> "BrowserApp.onCreate". The call is done now when the event
> "Gecko:DelayedStartup" is received.

This sounds like a better time – this way we can load Gecko as fast as possible so the user will be able to get search suggestions, start loading pages, etc.

> We can move it later: when the first event "Gesture:clusteredLinksClicked"
> is received. But in this case, the display of the zoomed view is longer for
> the first click inside a cluster. So from a user point of view, I would
> prefer to avoid that.

That makes sense. How noticeable was the delay? e.g. if you weren't expecting it, would you think it provided a bad experience? If not, perhaps it's worth doing.

Comment 5

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

It looks like this is the last of the major zoomed view issues – thanks so much Dominique! You've done an awesome job and we'd love for you to continue helping out! Do you think you're going to focus on refining the zoomed view or perhaps you'd like for me to find some more projects for you to tackle? I'm not sure we have additional UX direction for the zoomed view until it's been in the wild for a bit, however.

Comment 6

[Dominique Vincent [:domivinc]]

(In reply to Michael Comella (:mcomella) from comment #4)
> That makes sense. How noticeable was the delay? e.g. if you weren't
> expecting it, would you think it provided a bad experience? If not, perhaps
> it's worth doing.
I didn't evaluate the delay, it's probably linked to the device speed, the amount of free memory available (the content of the page), ... 
With the current implementation, we are sure that this delay is not visible when the user clicks on a cluster for the first time.

Comment 7

[Dominique Vincent [:domivinc]]

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

Comment 8

[Dominique Vincent [:domivinc]]

(In reply to Michael Comella (:mcomella) from comment #5)
> It looks like this is the last of the major zoomed view issues – thanks so
> much Dominique! You've done an awesome job and we'd love for you to continue
> helping out! Do you think you're going to focus on refining the zoomed view
> or perhaps you'd like for me to find some more projects for you to tackle?
> I'm not sure we have additional UX direction for the zoomed view until it's
> been in the wild for a bit, however.

First, I will update bugzilla regarding the different pending bugs linked to the cluster detection process: bug 1190541, bug 1208836, bug 1096042, ... 
Then, feel free to NI me on other bugs/projects, it's a pleasure to help you Michael! The only constraint I have on my side, is the irregular schedules of my part time activity: some periods with a lot of free time (I can invest on open source projects like Mozilla ...) and some periods totally over-employed.

Comment 9

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

renamed 1222638 -> patch-15112015_1-Bug_1222638___Don_t_inflate_zoomed_view_until_its_needed___r_mcomella.patch
applying patch-15112015_1-Bug_1222638___Don_t_inflate_zoomed_view_until_its_needed___r_mcomella.patch
patching file mobile/android/base/BrowserApp.java
Hunk #1 FAILED at 737
1 out of 2 hunks FAILED -- saving rejects to file mobile/android/base/BrowserApp.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh patch-15112015_1-Bug_1222638___Don_t_inflate_zoomed_view_until_its_needed___r_mcomella.patch

could you take a look, thanks!

Comment 10

[Dominique Vincent [:domivinc]]

Attachment: patch-28112015 1-Bug_1222638___Don_t_inflate_zoomed_view_until_its_needed___r_mcomella.patch

Patch re-based, no code change.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/cd278a1e4865

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cd278a1e4865