Closed Bug 942762 Opened 6 years ago Closed 4 years ago

[Fugu][Buri] unable to update shortcut name

Categories

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

Other
Other
defect
Not set

Tracking

(blocking-b2g:-)

RESOLVED WONTFIX
blocking-b2g -

People

(Reporter: angelc04, Unassigned)

References

Details

Attachments

(4 files, 1 obsolete file)

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?
Can someone check if this reproduces on 1.1?
Component: Gaia → Gaia::Homescreen
Keywords: qawanted
QA Contact: sparsons
This issue reproduces on Leo 1.1 Build ID: 20131122041201

Gaia   b7610870ec71495685557744bfbcbce357032251
SourceStamp c699a8e7bde9
BuildID 20131122041201
Version 18.0
Keywords: qawanted
Flags: needinfo?(tzhuang)
Assignee: nobody → tzhuang
Flags: needinfo?(tzhuang)
Attached patch bookmark_name_not_updated.patch (obsolete) — Splinter Review
Tzu-Lin,

   Could you help to review the patch?
Please feel free to take it
Please Tzu-Lin, take my patches and check them on master and v1.2. They are the correct way to fix the issue
Status: NEW → ASSIGNED
Flags: needinfo?(tzhuang)
blocking-b2g: --- → fugu+
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.
Attachment #8342141 - Flags: feedback?(crdlc)
Flags: needinfo?(tzhuang)
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
Attachment #8342141 - Flags: feedback?(crdlc) → feedback+
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.
After discussed with our partner, it will not block fugu.
blocking-b2g: fugu+ → ---
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
Attachment #8342141 - Flags: review?(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
Attachment #8342141 - Flags: review?(zcampbell)
Attachment #8342141 - Flags: review?(crdlc)
Attachment #8342141 - Flags: review+
Comment on attachment 8342141 [details] [review]
pull request for master

I can't speak for the js tests, sorry.
Attachment #8342141 - Flags: review?(zcampbell)
And do you know who is familiar with UI tests developed with JS? 10x
Flags: needinfo?(zcampbell)
The JS tests are shared ownership by all the devs so I'd speak to the module owner for the app you're testing.
Flags: needinfo?(zcampbell)
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
Attachment #8342141 - Flags: review?(21)
Comment on attachment 8342141 [details] [review]
pull request for master

I'm 100% confident in you, so your r+ is enough for me.
Attachment #8342141 - Flags: review?(21)
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
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 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
Attachment #8342141 - Flags: review?(evanxd)
Hi Tzu-Lin,

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

Thanks. :)
Attachment #8342141 - Flags: review?(evanxd)
Look good, just some nits.
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
Attachment #8342141 - Flags: review?(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.
Attachment #8342141 - Flags: review?(evanxd) → review+
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
blocking-b2g: --- → 1.3?
not a regression, so this is not a blocker
blocking-b2g: 1.3? → -
Assignee: tzhuang → nobody
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.
Blocks: 1231115
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.