Closed Bug 1366672 Opened 4 years ago Closed 4 years ago

(photon) Page loading indicator

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox57 verified)

VERIFIED FIXED
Firefox 57
Tracking Status
firefox57 --- verified

People

(Reporter: wesley_huang, Assigned: walkingice)

References

Details

(Whiteboard: [FNC][SPT57.2][MVP])

User Story

Motion spec: https://drive.google.com/drive/folders/0B1V1eo-I8g3dRnpjY2VDOFg3YjA?usp=sharing

Attachments

(7 files)

No description provided.
User Story: (updated)
Assignee: nobody → walkingice0204
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 on attachment 8896605 [details]
Bug 1366672 - part1: add ThemedProgressBar

https://reviewboard.mozilla.org/r/167882/#review173212
Attachment #8896605 - Flags: review?(topwu.tw) → review+
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 on attachment 8896606 [details]
Bug 1366672 - part2: Add new custom widgets

https://reviewboard.mozilla.org/r/167884/#review173344
Attachment #8896606 - Flags: review?(topwu.tw) → review+
Comment on attachment 8896605 [details]
Bug 1366672 - part1: add ThemedProgressBar

https://reviewboard.mozilla.org/r/167882/#review173352
Attachment #8896605 - Flags: review?(s.kaspari) → review+
Comment on attachment 8896606 [details]
Bug 1366672 - part2: Add new custom widgets

https://reviewboard.mozilla.org/r/167884/#review173354
Attachment #8896606 - Flags: review?(s.kaspari) → review+
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?
Attachment #8896607 - Flags: review?(s.kaspari) → review+
Comment on attachment 8896608 [details]
Bug 1366672 - part4: use Photon design for ProgressBar

https://reviewboard.mozilla.org/r/167888/#review173358
Attachment #8896608 - Flags: review?(s.kaspari) → review+
Comment on attachment 8896609 [details]
Bug 1366672 - part5: CustomTabs use AnimatedProgressBar

https://reviewboard.mozilla.org/r/167890/#review173360
Attachment #8896609 - Flags: review?(s.kaspari) → review+
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 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.
Attachment #8896608 - Flags: review?(topwu.tw)
Attachment #8897292 - Flags: review?(topwu.tw)
Comment on attachment 8896607 [details]
Bug 1366672 - Part3: use AnimatedProgressBar to replace ProgressView

https://reviewboard.mozilla.org/r/167886/#review173818
Attachment #8896607 - Flags: review?(topwu.tw) → review+
Comment on attachment 8896608 [details]
Bug 1366672 - part4: use Photon design for ProgressBar

https://reviewboard.mozilla.org/r/167888/#review173820
Attachment #8896608 - Flags: review+
Comment on attachment 8896609 [details]
Bug 1366672 - part5: CustomTabs use AnimatedProgressBar

https://reviewboard.mozilla.org/r/167890/#review173822
Attachment #8896609 - Flags: review?(topwu.tw) → review+
Comment on attachment 8897292 [details]
Bug 1366672 - part6: Clean up ToolbarProgressView

https://reviewboard.mozilla.org/r/168588/#review173824
Attachment #8897292 - Flags: review?(topwu.tw) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
Whiteboard: [FNC][SPT57.2][MVP]
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!
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?
Flags: needinfo?(walkingice0204) → needinfo?(rwood)
Comment on attachment 8898623 [details]
Bug 1366672 - part7: Remove unused resources to make lint happy

https://reviewboard.mozilla.org/r/170010/#review175188
Attachment #8898623 - Flags: review?(cnevinchen) → review+
(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!
Flags: needinfo?(rwood) → needinfo?(bob)
If it affects the timing of the application start, page load start or page load stop, then it will appear in Autophone.
Flags: needinfo?(bob)
The reported errors from treeherder are tier-2, and both of them have corresponding bug to track. This should be ok to check in.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Depends on: 1394404
Depends on: 1394720
Depends on: 1282262
Verified as fixed on Nightly 57. The loading indicator has been changed following the design in the browser, search widget, and custom tabs.
Status: RESOLVED → VERIFIED
Depends on: 1414838
Blocks: 1414928
Depends on: 1484528
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.