Don't inflate zoomed view until its needed

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcomella, Assigned: domivinc)

Tracking

unspecified
Firefox 45
All
Android
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We're currently inflating it in BrowserApp.onCreate and will be taking a start-up hit for it.
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.
Dominique, can you take a look at this?
Flags: needinfo?(domivinc)
(Assignee)

Updated

3 years ago
Assignee: nobody → domivinc
Flags: needinfo?(domivinc)
(Assignee)

Comment 3

3 years ago
Created attachment 8687654 [details] [diff] [review]
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.
Attachment #8687654 - Flags: review?(michael.l.comella)
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.
Attachment #8687654 - Flags: review?(michael.l.comella) → review+
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.
(Assignee)

Comment 6

3 years ago
(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.
(Assignee)

Comment 8

3 years ago
(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.
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
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!
Flags: needinfo?(domivinc)
Keywords: checkin-needed
(Assignee)

Comment 10

3 years ago
Created attachment 8693250 [details] [diff] [review]
patch-28112015 1-Bug_1222638___Don_t_inflate_zoomed_view_until_its_needed___r_mcomella.patch

Patch re-based, no code change.
Attachment #8687654 - Attachment is obsolete: true
Flags: needinfo?(domivinc)
Keywords: checkin-needed

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cd278a1e4865
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in before you can comment on or make changes to this bug.