1272806 - Remove notifyTabState pref in the tabs engine

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P3)

Description

[Edouard Oger [:eoger]]

It seems that this pref is set in tabs.js but never used anywhere, should we delete it?

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Richard, the code in question is at https://dxr.mozilla.org/mozilla-central/rev/4a8ed77f6bb573f20980056bf8c1dadd125c1a85/services/sync/modules/engines/tabs.js#265 - not only do we think it's unused, we think it is broken - ie:

>    // Lose some precision, but that's good enough (seconds).
>    let roundModify = Math.floor(record.modified / 1000);

doesn't make sense if record.modified is in seconds - which we are convinced is the case. For this reason I think we should nuke the code.

Comment 2

[Richard Newman [:rnewman]]

This was introduced a long, long, long time ago.

commit a47da3f213dff4db21a352cca2f50393d450b4a6
Author: Edward Lee <[e-mail]>
Date:   Thu Dec 17 12:20:07 2009 -0800

    Bug 535479 - Be smarter in getting rid of the "remote tabs" notification
    Add a tri-state pref that is unset, 0, or a number (modified time in seconds) and don't show the notification if it's in the "0" state. Unset pref means no modified time has been saved, and a non-0 state is the modified time of the last synced tab. So when tabs arrive with a different modified time, switch to the 0 state. Additionally, still remember if the user ever dismissed the notification and never show it again.


For this:

ui/fennec/content/tabs.js
22:    if (prefs.get("notifyTabState") == 0)
43:      prefs.set("notifyTabState", 0);


This code can die.

Comment 3

[Sagar Manchanda]

I am new to the community. I would like to take this up and try to solve the same. Please help me to get started on the same. I've already build desktop Firefox on my system. Thank you.

Comment 4

[Edouard Oger [:eoger]]

Hello Sagar, glad that you've built Firefox and want to get involved!

Your first step is to remove the code relative to notifyTabState in tabs.js#create: https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/tabs.js#266

Your second step is to remove all references to notifyTabState in the tests: https://dxr.mozilla.org/mozilla-central/source/services/sync/tests/unit/test_tab_store.js

After this you should check if the tests pass using ./mach xpcshell-test services/sync/tests/unit.

Good luck!

Comment 5

[Sagar Manchanda]

Hello, I removed the code relative to notifyTabState in tabs.js and also removed references to the same in the tests.

Also, I ran unit tests and all of them passed except for one file i.e. tests/unit/test_errorhandler.js which has got nothing to do with notifyTabState changes I made. Not sure why  this happened.

What should be my next step? Thank you.

Comment 6

[Sagar Manchanda]

Attachment: 1272806.patch

I've also created a patch of the same. Thank you.

Comment 7

[Edouard Oger [:eoger]]

Comment on attachment 8757661 [details] [diff] [review]
1272806.patch

Review of attachment 8757661 [details] [diff] [review]:
-----------------------------------------------------------------

Great patch Sagar!
I ran the tests myself, it seems like you were just unlucky with test_errorhandler.js.
Sync is a hard code to test because there's a lot of moving parts, and sometimes a test will not pass for a number of reasons :)

I gave your patch an r+, just add the checkin-needed keyword to this bug to get your patch landed in the mozilla codebase by a sheriff.

Thanks again for getting involved!

Comment 8

[Mark Hammond [:markh] [:mhammond]]

We should probably get a try run

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Edouard, do you mind pushing a try run first?

Comment 10

[Edouard Oger [:eoger]]

Try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=e17e883dc1c9
Please also re-submit your patch and add r=eoger at the end of your commit message in your patch Sagar.

Comment 11

[Sagar Manchanda]

Attachment: 1272806.patch

Please find attached the new patch with the required changes.

Comment 12

[Sagar Manchanda]

Hello!

Also, I've never really tried "try server". Should I just follow the steps on the following page.

https://wiki.mozilla.org/ReleaseEngineering/TryServer

It says "To use try server, I'll need Level 1 commit access". Should I just follow the instructions on the above wiki page. Please guide me. Thank you.

Comment 13

[Edouard Oger [:eoger]]

Don't worry Safar I tried push for you in comment 10, the tests are green we are good to go.

Comment 14

[Sagar Manchanda]

Thanks! Should I add checkin-needed keyword to this bug?

Comment 15

[Edouard Oger [:eoger]]

Yes go on!

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8757661 [details] [diff] [review]
1272806.patch

Please mark old patches as obsolete to avoid the wrong patch getting checked in and add the r=eoger (after r+) to make it easier for landing.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3db9a42c96af

Comment 18

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #16)
>  and add the r=eoger (after r+) to make it easier for landing.

Oops, you already did this. I checked this in now. The bug will get resolved when it merges to mozilla-central.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3db9a42c96af