1395841 - implement change for separating “add to home screen” & “add page shortcut”

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), enhancement, P1)

Description

[Joe Cheng [:jcheng] (please needinfo)]

Change the existing menu -> page -> "add to home screen" to "add page shortcut"
Have the Add PWA to homescreen badge to use the name "Add to home screen"

Comment 1

[Jing-wei Wu [:jwu]]

Attachment: Bug 1395841 - Part 1: Change page option strings fom 'add to home screen' to 'add page shortcut'.

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

Comment 2

[Jing-wei Wu [:jwu]]

Attachment: Bug 1395841 - Part 2: Support creating a shortcut from page option in menu.

When page option 'add page shortcut' is clicked, creating a shortcut(not PWA) on launcher.
Also make sure that heavy tasks are executed in background thread.

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

Comment 3

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8912620 [details]
Bug 1395841 - Part 2: Support creating a shortcut from page option in menu.

https://reviewboard.mozilla.org/r/183944/#review189220

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:583
(Diff revision 1)
> -        urlAnnotations.insertHomeScreenShortcut(context.getContentResolver(), aURI, true);
> +                urlAnnotations.insertHomeScreenShortcut(context.getContentResolver(), aURI, true);
> +            }
> +        });
>  
>          // After shortcut is created, show the mobile desktop.
>          ActivityUtils.goToHomeScreen(context);

Should we wait for insertHomeScreenShortcut() to complete and go back to ui thread to do this?

Comment 4

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8912619 [details]
Bug 1395841 - Part 1: Change page option strings fom 'add to home screen' to 'add page shortcut'.

https://reviewboard.mozilla.org/r/183942/#review189222

Comment 5

[Jing-wei Wu [:jwu]]

Comment on attachment 8912619 [details]
Bug 1395841 - Part 1: Change page option strings fom 'add to home screen' to 'add page shortcut'.

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

Comment 6

[Jing-wei Wu [:jwu]]

Comment on attachment 8912620 [details]
Bug 1395841 - Part 2: Support creating a shortcut from page option in menu.

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

Comment 7

[Jing-wei Wu [:jwu]]

(In reply to Nevin Chen [:nechen] from comment #3)
> Comment on attachment 8912620 [details]
> Bug 1395841 - Part 2: Support creating a shortcut from page option in menu.
> 
> https://reviewboard.mozilla.org/r/183944/#review189220
> 
> ::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:583
> (Diff revision 1)
> > -        urlAnnotations.insertHomeScreenShortcut(context.getContentResolver(), aURI, true);
> > +                urlAnnotations.insertHomeScreenShortcut(context.getContentResolver(), aURI, true);
> > +            }
> > +        });
> >  
> >          // After shortcut is created, show the mobile desktop.
> >          ActivityUtils.goToHomeScreen(context);
> 
> Should we wait for insertHomeScreenShortcut() to complete and go back to ui
> thread to do this?

Yes you're right, I should use `UIAsyncTask` to wait `insertHomeScreenShortcut()` finished.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13e001aa0253
Part 1: Change page option strings fom 'add to home screen' to 'add page shortcut'. r=nechen
https://hg.mozilla.org/integration/autoland/rev/38571a57eb11
Part 2: Support creating a shortcut from page option in menu. r=nechen

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/13e001aa0253
https://hg.mozilla.org/mozilla-central/rev/38571a57eb11

Comment 10

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

These patches conflicted with the ones from bug 1372926 when they got merged together. I think I resolved the conflicts, but it wouldn't hurt to make sure I did it right.

Comment 11

[Francesco Lodolo [:flod]]

Sorry but I'm going to ask for a backout at this point. Sadly, I think that's the only way to make sure reviewers get the importance of checking for this kind of issues
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for altering strings without using new string ids:

https://hg.mozilla.org/mozilla-central/rev/02eed89f56e3006140d475855d0d22e742f79ab2

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

The parts of this bug that did land have removed the ability to add a PWA shortcut. Can we add that back until bug 1379115 lands? It'll be impossible to test standalone PWA mode otherwise.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

I backed this out for now, it looks like it got a little busted in the merge per comment #10 anyway.

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d4e89492b76
Back out - Part 2 for breaking PWA shortcut installation r=me

Comment 16

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

Merged backout: https://hg.mozilla.org/mozilla-central/rev/4d4e89492b76

Comment 17

[Nevin Chen(Not active on Bugzilla)]

we can land this now

Comment 18

[Nevin Chen(Not active on Bugzilla)]

I can't reopen this bug. I'll try to assign to myself and see if it works.

Comment 19

[Nevin Chen(Not active on Bugzilla)]

No luck.. :( 
Hi Jing Wei.
Could you please help re-land this patch again?

Comment 20

[Francesco Lodolo [:flod]]

(In reply to Nevin Chen [:nechen] from comment #19)
> Could you please help re-land this patch again?

Please don't… It was backed out for a reason (comment 11 and comment 12), and that was never addressed.

Comment 21

[Nevin Chen(Not active on Bugzilla)]

Ok! Can repon this bug and submit a new patch?

Comment 22

[Nevin Chen(Not active on Bugzilla)]

Can I reopen this bug and submit a new patch?

Comment 23

[Francesco Lodolo [:flod]]

(In reply to Nevin Chen [:nechen] from comment #22)
> Can I reopen this bug and submit a new patch?

The bug is already "reopened", no blocker on submitting a new patch. I assumed you were asking to re-land the same patch.

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

Nevin, Andreas would really like this done this week. Are you able to do this? Would you like help?

Comment 25

[Nevin Chen(Not active on Bugzilla)]

doing it.

Comment 26

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1395841 - Part 1: Change page option strings fom 'add to home screen' to 'add page shortcut'.

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

Comment 27

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1395841 - Part 2: Support creating a shortcut from page option in menu.

When page option 'add page shortcut' is clicked, creating a shortcut(not PWA) on launcher.
Also make sure that heavy tasks are executed in background thread.

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

Comment 28

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8919964 [details]
Bug 1395841 - Part 1: Change page option strings fom 'add to home screen' to 'add page shortcut'.

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

Comment 29

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8919965 [details]
Bug 1395841 - Part 2: Support creating a shortcut from page option in menu.

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

Comment 30

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8919964 [details]
Bug 1395841 - Part 1: Change page option strings fom 'add to home screen' to 'add page shortcut'.

https://reviewboard.mozilla.org/r/190910/#review196428

Comment 31

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8919965 [details]
Bug 1395841 - Part 2: Support creating a shortcut from page option in menu.

https://reviewboard.mozilla.org/r/190912/#review196434

::: mobile/android/base/java/org/mozilla/gecko/promotion/HomeScreenPrompt.java:123
(Diff revision 2)
>  
>      private void addToHomeScreen() {
>          ThreadUtils.postToBackgroundThread(new Runnable() {
>              @Override
>              public void run() {
> -                GeckoApplication.createShortcut(title, url);
> +                GeckoApplication.createBrowserShortcut(title, url);

This will break manifest-enabled shortcuts, no?

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8919965 [details]
Bug 1395841 - Part 2: Support creating a shortcut from page option in menu.

https://reviewboard.mozilla.org/r/190912/#review196518

I guess this is ok to land if you fix the one problem.

Comment 33

[Nevin Chen(Not active on Bugzilla)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #31)
> Comment on attachment 8919965 [details]
> Bug 1395841 - Part 2: Support creating a shortcut from page option in menu.
> 
> https://reviewboard.mozilla.org/r/190912/#review196434
> 
> :::
> mobile/android/base/java/org/mozilla/gecko/promotion/HomeScreenPrompt.java:
> 123
> (Diff revision 2)
> >  
> >      private void addToHomeScreen() {
> >          ThreadUtils.postToBackgroundThread(new Runnable() {
> >              @Override
> >              public void run() {
> > -                GeckoApplication.createShortcut(title, url);
> > +                GeckoApplication.createBrowserShortcut(title, url);
> 
> This will break manifest-enabled shortcuts, no?

We don't create manifest-enabled shortcuts in HomeScreenPrompt.
This is because the only place to add PWA shortcut is through the badge. 

Hi Joe, Mark
Please help confirm if my understanding is correct.

Comment 34

[Joe Cheng [:jcheng] (please needinfo)]

I have the same understanding as you Nevin.

Comment 35

[Nevin Chen(Not active on Bugzilla)]

Since Mark is on PTO, and this is uregent. I'll land this as is. I'll submit another patch if the requirement changes.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e12e4ab70862
Part 1: Change page option strings fom 'add to home screen' to 'add page shortcut'. r=snorp
https://hg.mozilla.org/integration/autoland/rev/8bfc3d92c6ef
Part 2: Support creating a shortcut from page option in menu. r=snorp

Comment 37

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e12e4ab70862
https://hg.mozilla.org/mozilla-central/rev/8bfc3d92c6ef

Comment 38

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

Verified that the distinction between add to home screen and add page shortcut is working as described.