1366672 - (photon) Page loading indicator

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Julian Chu [:walkingice]]

Attachment: Bug 1366672 - part1: add ThemedProgressBar

We are going to involve new implementation for page loading indicator by
ProgressBar. Add ThemedProgressBar for Private Mode.

Review commit: https://reviewboard.mozilla.org/r/167882/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167882/

Comment 2

[Julian Chu [:walkingice]]

Attachment: Bug 1366672 - part2: Add new custom widgets

* DrawableWrapper - a class similiar to DrawableWrapper in Android v23.
* ShiftDrawable - provide shif-animation for its child drawable
* AnimatedProgressBar - ProgressBar.setProgress(int, boolean) involved
    since API v24. This is an implementation for animation.

Review commit: https://reviewboard.mozilla.org/r/167884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167884/

Comment 3

[Julian Chu [:walkingice]]

Attachment: Bug 1366672 - Part3: use AnimatedProgressBar to replace ProgressView

Review commit: https://reviewboard.mozilla.org/r/167886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167886/

Comment 4

[Julian Chu [:walkingice]]

Attachment: Bug 1366672 - part4: use Photon design for ProgressBar

Review commit: https://reviewboard.mozilla.org/r/167888/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167888/

Comment 5

[Julian Chu [:walkingice]]

Attachment: Bug 1366672 - part5: CustomTabs use AnimatedProgressBar

Review commit: https://reviewboard.mozilla.org/r/167890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167890/

Comment 6

[Julian Chu [:walkingice]]

Comment on attachment 8896606 [details]
Bug 1366672 - part2: Add new custom widgets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167884/diff/1-2/

Comment 7

[Julian Chu [:walkingice]]

Comment on attachment 8896607 [details]
Bug 1366672 - Part3: use AnimatedProgressBar to replace ProgressView

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167886/diff/1-2/

Comment 8

[Julian Chu [:walkingice]]

Comment on attachment 8896608 [details]
Bug 1366672 - part4: use Photon design for ProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167888/diff/1-2/

Comment 9

[Julian Chu [:walkingice]]

Comment on attachment 8896609 [details]
Bug 1366672 - part5: CustomTabs use AnimatedProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167890/diff/1-2/

Comment 10

[Jing-wei Wu [:jwu]]

Comment on attachment 8896606 [details]
Bug 1366672 - part2: Add new custom widgets

https://reviewboard.mozilla.org/r/167884/#review173122

::: mobile/android/base/moz.build:616
(Diff revision 2)
> +    'drawable/DrawableWrapper',
> +    'drawable/ShiftDrawable',

No ".java" extenstion in both lines.

Comment 11

[Julian Chu [:walkingice]]

Comment on attachment 8896606 [details]
Bug 1366672 - part2: Add new custom widgets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167884/diff/2-3/

Comment 12

[Julian Chu [:walkingice]]

Comment on attachment 8896607 [details]
Bug 1366672 - Part3: use AnimatedProgressBar to replace ProgressView

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167886/diff/2-3/

Comment 13

[Julian Chu [:walkingice]]

Comment on attachment 8896608 [details]
Bug 1366672 - part4: use Photon design for ProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167888/diff/2-3/

Comment 14

[Julian Chu [:walkingice]]

Comment on attachment 8896609 [details]
Bug 1366672 - part5: CustomTabs use AnimatedProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167890/diff/2-3/

Comment 15

[Jing-wei Wu [:jwu]]

Comment on attachment 8896605 [details]
Bug 1366672 - part1: add ThemedProgressBar

https://reviewboard.mozilla.org/r/167882/#review173212

Comment 16

[Jing-wei Wu [:jwu]]

Comment on attachment 8896608 [details]
Bug 1366672 - part4: use Photon design for ProgressBar

https://reviewboard.mozilla.org/r/167888/#review173258

::: mobile/android/app/src/photon/res/values/colors.xml:82
(Diff revision 3)
> -    <color name="progress_start">#00DCFC</color>
> -    <color name="progress_end">#00A2FE</color>
> +    <color name="normal_progress_start">#00DCFC</color>
> +    <color name="normal_progress_end">#00A2FE</color>
> +    <color name="private_progress_start">#9400FF</color>
> +    <color name="private_progress_end">#FF1AD9</color>

There are already four colors defined above which aren't been uesd yet:

photon_loading_indicator_light,
photon_loading_indicator_light_private,
photon_loading_indicator_dark,
photon_loading_indicator_dark_private

Should these new color names just reference to old ones instead of using hex code?

Comment 17

[Jing-wei Wu [:jwu]]

Comment on attachment 8896606 [details]
Bug 1366672 - part2: Add new custom widgets

https://reviewboard.mozilla.org/r/167884/#review173344

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8896605 [details]
Bug 1366672 - part1: add ThemedProgressBar

https://reviewboard.mozilla.org/r/167882/#review173352

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8896606 [details]
Bug 1366672 - part2: Add new custom widgets

https://reviewboard.mozilla.org/r/167884/#review173354

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8896607 [details]
Bug 1366672 - Part3: use AnimatedProgressBar to replace ProgressView

https://reviewboard.mozilla.org/r/167886/#review173356

::: mobile/android/app/src/main/res/layout/gecko_app.xml:144
(Diff revision 3)
>  
>              </ViewFlipper>
>  
>          </LinearLayout>
>  
> -        <org.mozilla.gecko.toolbar.ToolbarProgressView android:id="@id/page_progress"
> +        <org.mozilla.gecko.widget.AnimatedProgressBar

Can we remove ToolbarProgressView now?

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8896608 [details]
Bug 1366672 - part4: use Photon design for ProgressBar

https://reviewboard.mozilla.org/r/167888/#review173358

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8896609 [details]
Bug 1366672 - part5: CustomTabs use AnimatedProgressBar

https://reviewboard.mozilla.org/r/167890/#review173360

Comment 23

[Julian Chu [:walkingice]]

Comment on attachment 8896608 [details]
Bug 1366672 - part4: use Photon design for ProgressBar

https://reviewboard.mozilla.org/r/167888/#review173258

> There are already four colors defined above which aren't been uesd yet:
> 
> photon_loading_indicator_light,
> photon_loading_indicator_light_private,
> photon_loading_indicator_dark,
> photon_loading_indicator_dark_private
> 
> Should these new color names just reference to old ones instead of using hex code?

OK I will use exsiting ones.

Comment 24

[Julian Chu [:walkingice]]

Comment on attachment 8896607 [details]
Bug 1366672 - Part3: use AnimatedProgressBar to replace ProgressView

https://reviewboard.mozilla.org/r/167886/#review173356

> Can we remove ToolbarProgressView now?

I think so, there is no other class is using this.

Comment 25

[Julian Chu [:walkingice]]

Attachment: Bug 1366672 - part6: Clean up ToolbarProgressView

We invovled another implementation. We can get it back from repository
easily for any reason, so remove it for now.

Review commit: https://reviewboard.mozilla.org/r/168588/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168588/

Comment 26

[Julian Chu [:walkingice]]

Comment on attachment 8896605 [details]
Bug 1366672 - part1: add ThemedProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167882/diff/1-2/

Comment 27

[Julian Chu [:walkingice]]

Comment on attachment 8896606 [details]
Bug 1366672 - part2: Add new custom widgets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167884/diff/3-4/

Comment 28

[Julian Chu [:walkingice]]

Comment on attachment 8896607 [details]
Bug 1366672 - Part3: use AnimatedProgressBar to replace ProgressView

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167886/diff/3-4/

Comment 29

[Julian Chu [:walkingice]]

Comment on attachment 8896608 [details]
Bug 1366672 - part4: use Photon design for ProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167888/diff/3-4/

Comment 30

[Julian Chu [:walkingice]]

Comment on attachment 8896609 [details]
Bug 1366672 - part5: CustomTabs use AnimatedProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167890/diff/3-4/

Comment 31

[Jing-wei Wu [:jwu]]

Comment on attachment 8896607 [details]
Bug 1366672 - Part3: use AnimatedProgressBar to replace ProgressView

https://reviewboard.mozilla.org/r/167886/#review173818

Comment 32

[Jing-wei Wu [:jwu]]

Comment on attachment 8896608 [details]
Bug 1366672 - part4: use Photon design for ProgressBar

https://reviewboard.mozilla.org/r/167888/#review173820

Comment 33

[Jing-wei Wu [:jwu]]

Comment on attachment 8896609 [details]
Bug 1366672 - part5: CustomTabs use AnimatedProgressBar

https://reviewboard.mozilla.org/r/167890/#review173822

Comment 34

[Jing-wei Wu [:jwu]]

Comment on attachment 8897292 [details]
Bug 1366672 - part6: Clean up ToolbarProgressView

https://reviewboard.mozilla.org/r/168588/#review173824

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13affa6a9586
part1: add ThemedProgressBar r=jwu,sebastian
https://hg.mozilla.org/integration/autoland/rev/64ff8cc86fec
part2: Add new custom widgets r=jwu,sebastian
https://hg.mozilla.org/integration/autoland/rev/80a2c096cfa9
Part3: use AnimatedProgressBar to replace ProgressView r=jwu,sebastian
https://hg.mozilla.org/integration/autoland/rev/dd2474c2eaf9
part4: use Phton design for ProgressBar r=jwu,sebastian
https://hg.mozilla.org/integration/autoland/rev/e655bcd49af8
part5: CustomTabs use AnimatedProgressBar r=jwu,sebastian
https://hg.mozilla.org/integration/autoland/rev/df6eadeb55a9
part6 Clean up ToolbarProgressView r=jwu

Comment 36

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

I had to back these out for android linting failures like https://treeherder.mozilla.org/logviewer.html#?job_id=123237743&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/cfef5aec8f82a572acaafc56e50d7125de7076e1

Comment 37

[Robert Wood [:rwood]]

FYI, the original patch also caused at least one talos performance regression:

== Change summary for alert #8830 (as of August 15 2017 08:59 UTC) ==

Regressions:

  8%  remote-nytimes summary android-4-2-armv7-api15 opt      3,244.59 -> 3,505.62

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8830

However since it was backed out, talos shows the improvement/fix after the back out:

== Change summary for alert #8827 (as of August 15 2017 18:38 UTC) ==

Improvements:

  7%  remote-nytimes summary android-4-2-armv7-api15 opt      3,482.93 -> 3,240.92

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8827

So just FYI if this patch is re-introduced, it may cause talos performance regressions which would need to be resolved. Thanks!

Comment 38

[Julian Chu [:walkingice]]

Attachment: Bug 1366672 - part7: Remove unused resources to make lint happy

Review commit: https://reviewboard.mozilla.org/r/170010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170010/

Comment 39

[Julian Chu [:walkingice]]

Comment on attachment 8896605 [details]
Bug 1366672 - part1: add ThemedProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167882/diff/2-3/

Comment 40

[Julian Chu [:walkingice]]

Comment on attachment 8896606 [details]
Bug 1366672 - part2: Add new custom widgets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167884/diff/4-5/

Comment 41

[Julian Chu [:walkingice]]

Comment on attachment 8896607 [details]
Bug 1366672 - Part3: use AnimatedProgressBar to replace ProgressView

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167886/diff/4-5/

Comment 42

[Julian Chu [:walkingice]]

Comment on attachment 8896608 [details]
Bug 1366672 - part4: use Photon design for ProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167888/diff/4-5/

Comment 43

[Julian Chu [:walkingice]]

Comment on attachment 8896609 [details]
Bug 1366672 - part5: CustomTabs use AnimatedProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167890/diff/4-5/

Comment 44

[Julian Chu [:walkingice]]

Comment on attachment 8897292 [details]
Bug 1366672 - part6: Clean up ToolbarProgressView

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168588/diff/1-2/

Comment 45

[Julian Chu [:walkingice]]

Hi Robert,

Thanks for your information. In current design we are going to add animation to progress-bar, so I can see there will be some performance drop on battery or CPU.

According to our design spec, we are applying a ending-animation to progress-bar. Before hiding progress-bar, there will be 300ms ending animation. Will this effect the performance test?

Comment 46

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8898623 [details]
Bug 1366672 - part7: Remove unused resources to make lint happy

https://reviewboard.mozilla.org/r/170010/#review175188

Comment 47

[Robert Wood [:rwood]]

(In reply to Julian Chu [:walkingice] from comment #45)
> Hi Robert,
> 
> Thanks for your information. In current design we are going to add animation
> to progress-bar, so I can see there will be some performance drop on battery
> or CPU.
> 
> According to our design spec, we are applying a ending-animation to
> progress-bar. Before hiding progress-bar, there will be 300ms ending
> animation. Will this effect the performance test?

Hi Julian, thank you for your reply. I am forwrarding this on to :bc, he is familiar with this area, thanks :bc!

Comment 48

[Bob Clary [:bc] (inactive)]

If it affects the timing of the application start, page load start or page load stop, then it will appear in Autophone.

Comment 49

[Julian Chu [:walkingice]]

Comment on attachment 8896608 [details]
Bug 1366672 - part4: use Photon design for ProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167888/diff/5-6/

Comment 50

[Julian Chu [:walkingice]]

Comment on attachment 8896609 [details]
Bug 1366672 - part5: CustomTabs use AnimatedProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167890/diff/5-6/

Comment 51

[Julian Chu [:walkingice]]

Comment on attachment 8897292 [details]
Bug 1366672 - part6: Clean up ToolbarProgressView

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168588/diff/2-3/

Comment 52

[Julian Chu [:walkingice]]

Comment on attachment 8898623 [details]
Bug 1366672 - part7: Remove unused resources to make lint happy

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170010/diff/1-2/

Comment 53

[Julian Chu [:walkingice]]

Comment on attachment 8896605 [details]
Bug 1366672 - part1: add ThemedProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167882/diff/3-4/

Comment 54

[Julian Chu [:walkingice]]

Comment on attachment 8896606 [details]
Bug 1366672 - part2: Add new custom widgets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167884/diff/5-6/

Comment 55

[Julian Chu [:walkingice]]

Comment on attachment 8896607 [details]
Bug 1366672 - Part3: use AnimatedProgressBar to replace ProgressView

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167886/diff/5-6/

Comment 56

[Julian Chu [:walkingice]]

Comment on attachment 8896608 [details]
Bug 1366672 - part4: use Photon design for ProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167888/diff/6-7/

Comment 57

[Julian Chu [:walkingice]]

Comment on attachment 8896609 [details]
Bug 1366672 - part5: CustomTabs use AnimatedProgressBar

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167890/diff/6-7/

Comment 58

[Julian Chu [:walkingice]]

Comment on attachment 8897292 [details]
Bug 1366672 - part6: Clean up ToolbarProgressView

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168588/diff/3-4/

Comment 59

[Julian Chu [:walkingice]]

Comment on attachment 8898623 [details]
Bug 1366672 - part7: Remove unused resources to make lint happy

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170010/diff/2-3/

Comment 60

[Julian Chu [:walkingice]]

The reported errors from treeherder are tier-2, and both of them have corresponding bug to track. This should be ok to check in.

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3403e2cea224
part1: add ThemedProgressBar r=jwu,sebastian
https://hg.mozilla.org/integration/autoland/rev/7dfd7586ce70
part2: Add new custom widgets r=jwu,sebastian
https://hg.mozilla.org/integration/autoland/rev/f701c0a83fba
Part3: use AnimatedProgressBar to replace ProgressView r=jwu,sebastian
https://hg.mozilla.org/integration/autoland/rev/82f13eb2ff6e
part4: use Photon design for ProgressBar r=jwu,sebastian
https://hg.mozilla.org/integration/autoland/rev/92d0fb788d05
part5: CustomTabs use AnimatedProgressBar r=jwu,sebastian
https://hg.mozilla.org/integration/autoland/rev/664fa0772e37
part6: Clean up ToolbarProgressView r=jwu
https://hg.mozilla.org/integration/autoland/rev/4da1c8e2fc43
part7: Remove unused resources to make lint happy r=nechen

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3403e2cea224
https://hg.mozilla.org/mozilla-central/rev/7dfd7586ce70
https://hg.mozilla.org/mozilla-central/rev/f701c0a83fba
https://hg.mozilla.org/mozilla-central/rev/82f13eb2ff6e
https://hg.mozilla.org/mozilla-central/rev/92d0fb788d05
https://hg.mozilla.org/mozilla-central/rev/664fa0772e37
https://hg.mozilla.org/mozilla-central/rev/4da1c8e2fc43

Comment 63

[Oana Horvath [:ohorvath - out of office]]

Verified as fixed on Nightly 57. The loading indicator has been changed following the design in the browser, search widget, and custom tabs.