942762 - [Fugu][Buri] unable to update shortcut name

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Description

[Peipei Cheng (needinfo if you need my action)]

Test build Buri V1.2
Gaia:     5a0802f44565ac9a17f5a720259ac2689ab67ff7 
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/64e1acc29811
BuildID   20131124004001                     
Version   26.0    


Steps:
 1. Open Browser => Browse a website
 2. After webpage is loaded, click on the star icon=> select "Add to Home screen" => Change website name to A.
 3. go to homescreen and check the shortcut created. Shortcut's name is A.
 4. Go back to browser
 5. Click on click on the star icon=> select "Add to Home screen" => Change website name to B.
 6. Go to Homescreen and check the shortcut
    => [issue] Short cut name remains as A
    => [Expected] shortcut name should be updated to B.
 7. Long tap on the shortcut and click on the delete icon
    The msg showing as : Remove B from the homescreen?

Comment 1

[Jason Smith [:jsmith]]

Can someone check if this reproduces on 1.1?

Comment 2

[Sarah Parsons]

This issue reproduces on Leo 1.1 Build ID: 20131122041201

Gaia   b7610870ec71495685557744bfbcbce357032251
SourceStamp c699a8e7bde9
BuildID 20131122041201
Version 18.0

Comment 3

[Jesse]

Attachment: bookmark_name_not_updated.patch

Tzu-Lin,

   Could you help to review the patch?

Comment 4

[Cristian Rodriguez (:crdlc)]

Attachment: This is the correct patch for master

Please feel free to take it

Comment 5

[Cristian Rodriguez (:crdlc)]

Attachment: This is the correct way to do this in v1.2

Comment 6

[Cristian Rodriguez (:crdlc)]

Please Tzu-Lin, take my patches and check them on master and v1.2. They are the correct way to fix the issue

Comment 7

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Attachment: pull request for master

Hi Cristian,
Thanks for the patch. I've made a PR for master.
Since Vivien is under review time off until Dec. 9, I am wondering whom should I ask for review? Wiki page said you are module owner too.

Comment 8

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Attachment: pull request for v1.2f

Comment 9

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8342141 [details] [review]
pull request for master

Simple solution and works fine, great work! I would like you add some unit test for this new feature, thanks

1) Install a bookmark http://xxxx.yyy called xxx
2) Install another bookmark with the same URL called yyy

Check if the label changes

When you have this done , please ask the review to me or :julien

10x

Comment 10

[Cristian Rodriguez (:crdlc)]

Yes, Vivien and me are the homescreen's owners although Julien could do it as well thought

(In reply to Tzu-Lin Huang [:dwi2] from comment #7)
> Created attachment 8342141 [details] [review]
> pull request for master
> 
> Hi Cristian,
> Thanks for the patch. I've made a PR for master.
> Since Vivien is under review time off until Dec. 9, I am wondering whom
> should I ask for review? Wiki page said you are module owner too.

Comment 11

[Steven Yang [:styang]]

After discussed with our partner, it will not block fugu.

Comment 12

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 8342141 [details] [review]
pull request for master

Hi Cristian,

I found it would be complicated to write unit tests for the feature (compare to writing marionette tests). So I wrote two marionette tests for it.
  1. Install bookmark with title "Sample page", and check the title of bookmark on homescreen matches "Sample page" or not.
  2. Back to browser, install bookmark of the same page as 1 but with different title "New Title", and check the title of bookmark on homescreen matches "New Title" or not.

to run these marionette tests, simply type the below commands in gaia folder
    bin/gaia-marionette apps/homescreen/test/marionette/install_bookmark_test.js

Please kindly help to review the patch, thanks

Comment 13

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8342141 [details] [review]
pull request for master

Nice work my friend, congratulations for your great patch. The homescreen code looks good for me r=me. I will redirect the review of the tests to Zac because I don't have a strong knowledge about that. Thanks a lot mate

Comment 14

[Zac C (:zac)]

Comment on attachment 8342141 [details] [review]
pull request for master

I can't speak for the js tests, sorry.

Comment 15

[Cristian Rodriguez (:crdlc)]

And do you know who is familiar with UI tests developed with JS? 10x

Comment 16

[Zac C (:zac)]

The JS tests are shared ownership by all the devs so I'd speak to the module owner for the app you're testing.

Comment 17

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8342141 [details] [review]
pull request for master

Hi Vivien, do you want to take a look to this set of UI tests

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8342141 [details] [review]
pull request for master

I'm 100% confident in you, so your r+ is enough for me.

Comment 19

[Cristian Rodriguez (:crdlc)]

I have never written, checked or run these kind of tests and I am not comfortable reviewing this. Please find someone to review these tests. Where are they run? maybe CI? Thanks

Comment 20

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Hi Cristian,

Yes, marionette JS tests run on travis. For example, result of marionette JS tests for the pull request is at https://travis-ci.org/mozilla-b2g/gaia/jobs/15268788. 
New tests I wrote is at line 1213. 

I'll find Evan Tseng (who works on marionette JS client development and taught me how to write marionette tests) to review my tests. Thanks

Comment 21

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 8342141 [details] [review]
pull request for master

Hi Evan,

Could you help to review the marionette JS tests in this patch? 
The test step is describe at Comment 12. Thanks

Comment 22

[Evan Tseng [:evanxd]]

Hi Tzu-Lin,

I left comments for your patch.
Please request review again once you update the patch.

Thanks. :)

Comment 23

[Evan Tseng [:evanxd]]

Look good, just some nits.

Comment 24

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 8342141 [details] [review]
pull request for master

Hi Evan,

Please help to review the revised JS marionette tests. During the revising, I also found a bug of JSMarionette - client.helper.waitForElementToDisappear() has no effect on #notification-toaster in system app.

I filed a bug  on bugzilla to track it - https://bugzilla.mozilla.org/show_bug.cgi?id=952377

Thanks

Comment 25

[Evan Tseng [:evanxd]]

Comment on attachment 8342141 [details] [review]
pull request for master

Hi Tzu-Lin,

r+.

And one more thing.
We already has same code for http server things in the code base of Browser app. 

In the future, we could make server.js and server_child.js in the patch as a marionette plugin. It is good for reuse.

We should do this once we have time. If you have time, would you like to help this in that time.

Comment 26

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Hi Evan,

Thanks, I'll write a plugin for server.js and server_child.js once I have time.

Travis is green, landed on master
https://github.com/mozilla-b2g/gaia/commit/af74217ac2ca0204420b2a33432f3c2b28919c47

I am still working on the patch for v1.2f since JS Marionette tests are a little different on v1.2/v1.2f

Comment 27

[Jason Smith [:jsmith]]

not a regression, so this is not a blocker

Comment 28

[Kevin Grandon :kgrandon]

Mass update: Resolve wontfix all issues with legacy homescreens.

As of 2.6 we have a new homescreen and having these issues open is confusing. All issues will block bug 1231115 so we can use that to re-visit any of these if needed.