1446719 - Rename getTabValue / setTabValue / deleteTabValue to getCustomTabValue / setCustomTabValue / deleteCustomTabValue

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement, P3)

Description

[Dão Gottwald [:dao]]

getTabValue and setTabValue are for external data rather than accessing the data sessionstore keeps automatically for a tab. We should rename these methods to make this clearer.

https://searchfox.org/mozilla-central/search?q=getTabValue&case=true&path=browser%2F

https://searchfox.org/mozilla-central/search?q=setTabValue&case=true&path=browser%2F

Comment 1

[Rob]

Hiya. I'd like to have a go at a few bugs on here but I was hoping somebody could point me towards a crash course on what to do. I've read the docs and a few articles but I've only ever used github directly (and not a whole lot in terms of PRs) so I'm not sure how to go about the equivalent of cloning the code or submitting it back up here. Probably missing something obvious! Thanks muchly

Comment 2

[Dão Gottwald [:dao]]

(In reply to Rob from comment #1)
> Hiya. I'd like to have a go at a few bugs on here but I was hoping somebody
> could point me towards a crash course on what to do. I've read the docs and
> a few articles but I've only ever used github directly (and not a whole lot
> in terms of PRs) so I'm not sure how to go about the equivalent of cloning
> the code or submitting it back up here. Probably missing something obvious!
> Thanks muchly

Probably best to ask in #introduction on IRC (https://developer.mozilla.org/en-US/docs/Mozilla/QA/Getting_Started_with_IRC).

Comment 3

[Manish [:manishkk]]

Hi. I'd like to be assigned to solve this bug. :)


Do I need to do renaming in all these files right?

https://searchfox.org/mozilla-central/search?q=getTabValue&case=true&path=browser%2F

https://searchfox.org/mozilla-central/search?q=setTabValue&case=true&path=browser%2F

Comment 4

[Dão Gottwald [:dao]]

(In reply to Manish Kumar from comment #3)
> Hi. I'd like to be assigned to solve this bug. :)
> 
> 
> Do I need to do renaming in all these files right?
> 
> https://searchfox.org/mozilla-central/search?q=getTabValue&case=true&path=browser%2F
> 
> https://searchfox.org/mozilla-central/search?q=setTabValue&case=true&path=browser%2F

Yes, but not:

* browser/components/extensions/schemas/sessions.json
* https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/browser/components/extensions/ext-sessions.js#158,165
* browser/components/extensions/test/browser/browser_ext_sessions_window_tab_value.js

Comment 5

[Dão Gottwald [:dao]]

Manish, have you started working on this? Anything I can help with?

Comment 6

[Manish [:manishkk]]

Dao, 

I will work on this from today. Will submit the patch soon.
Thanks

Comment 7

[Manish [:manishkk]]

Attachment: Patch_Bug1446719

Please review.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8964116 [details] [diff] [review]
Patch_Bug1446719

You'll also need to update the lines where ext-sessions.js calls SessionStore.setTabValue and SessionStore.getTabValue.

Also, can you do the same for deleteTabValue? Sorry for not mentioning it earlier. https://searchfox.org/mozilla-central/search?q=deleteTabValue&case=true&path=

Comment 9

[Manish [:manishkk]]

Attachment: NewPatch_Bug1446719

Please review.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8964120 [details] [diff] [review]
NewPatch_Bug1446719

>--- a/browser/components/sessionstore/SessionStore.jsm
>+++ b/browser/components/sessionstore/SessionStore.jsm
>@@ -301,16 +301,16 @@ var SessionStore = {
>     SessionStoreInternal.deleteWindowValue(aWindow, aKey);
>   },
> 
>-  getTabValue: function ss_getTabValue(aTab, aKey) {
>-    return SessionStoreInternal.getTabValue(aTab, aKey);
>+  getTabValue: function ss_getCustomTabValue(aTab, aKey) {
>+    return SessionStoreInternal.getCustomTabValue(aTab, aKey);
>   },

You didn't actually change the method's name here. ;) You can just remove the "function ss_getTabValue" cruft, so this should become:

  getCustomTabValue(aTab, aKey) {
    ...


>-  setTabValue: function ss_setTabValue(aTab, aKey, aStringValue) {
>-    SessionStoreInternal.setTabValue(aTab, aKey, aStringValue);
>+  setCustomTabValue: function ss_setTabValue(aTab, aKey, aStringValue) {
>+    SessionStoreInternal.setCustomTabValue(aTab, aKey, aStringValue);
>   },
> 
>-  deleteTabValue: function ss_deleteTabValue(aTab, aKey) {
>-    SessionStoreInternal.deleteTabValue(aTab, aKey);
>+  deleteCustomTabValue: function ss_deleteCustomTabValue(aTab, aKey) {
>+    SessionStoreInternal.deleteCustomTabValue(aTab, aKey);
>   },

>-  getTabValue: function ssi_getTabValue(aTab, aKey) {
>+  getCustomTabValue: function ssi_getTabValue(aTab, aKey) {
>     return (aTab.__SS_extdata || {})[aKey] || "";
>   },
> 
>-  setTabValue: function ssi_setTabValue(aTab, aKey, aStringValue) {
>+  setCustomTabValue: function ssi_setTabValue(aTab, aKey, aStringValue) {

>-  deleteTabValue: function ssi_deleteTabValue(aTab, aKey) {
>+  deleteCustomTabValue: function ssi_deleteCustomTabValue(aTab, aKey) {

Ditto, please remove "function ss_..."

Comment 11

[Manish [:manishkk]]

Attachment: patch1446719

Please review

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8964142 [details] [diff] [review]
patch1446719

We're only interested in browser/ here, please remove all changes in mobile/.

Looks good otherwise. I sent it to the try server to see if it breaks any tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ef878b2ca5434ca3423876faf9ee22c36c13fcd

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 8964142 [details] [diff] [review]
patch1446719

>--- a/browser/components/sessionstore/SessionStore.jsm
>+++ b/browser/components/sessionstore/SessionStore.jsm
>@@ -301,16 +301,16 @@ var SessionStore = {
>     SessionStoreInternal.deleteWindowValue(aWindow, aKey);
>   },
> 
>-  getTabValue: function ss_getTabValue(aTab, aKey) {
>-    return SessionStoreInternal.getTabValue(aTab, aKey);
>+  getTabCustomValue(aTab, aKey) {

This should be getCustomTabValue, not getTabCustomValue.

Comment 14

[Manish [:manishkk]]

Attachment: NewPatch1446719

Please Review

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8964158 [details] [diff] [review]
NewPatch1446719

https://treeherder.mozilla.org/#/jobs?repo=try&revision=085769d99bbe23e9e7eafe08b08abffd2ce99ec1

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 8964158 [details] [diff] [review]
NewPatch1446719

>--- a/browser/base/content/tabbrowser.js
>+++ b/browser/base/content/tabbrowser.js
>@@ -3306,7 +3306,7 @@ window._gBrowser = {
>       event.initEvent("TabHide", true, false);
>       aTab.dispatchEvent(event);
>       if (aSource) {
>-        SessionStore.setTabValue(aTab, "hiddenBy", aSource);
>+        SessionStore.setCustomTabValue(aTab, "hiddenBy", aSource);
>       }
>     }
>   },

Looks like you forgot to update this line:
https://searchfox.org/mozilla-central/rev/42b34ba1961e37e0d2236deafdd126a0ba21b9ec/browser/base/content/tabbrowser.js#3291

Comment 17

[Manish [:manishkk]]

Attachment: NewPatch_Bug1446719

Any other changes required?

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8964160 [details] [diff] [review]
NewPatch_Bug1446719

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd02c05d8eaa185a26789be2db9cd29c97596da6

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8964160 [details] [diff] [review]
NewPatch_Bug1446719

Looks good, thank you!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f80a1b8f781b
Rename getTabValue / setTabValue / deleteTabValue to getCustomTabValue / setCustomTabValue / deleteCustomTabValue. r=dao

Comment 21

[Andreea Pavel [:apavel]]

Backed out for failing browser chrome at browser/components/sessionstore/test/browser_350525.js

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f80a1b8f781b359a16ea081e1a45ced7f40aa609

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=171373245&repo=mozilla-inbound&lineNumber=2182

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/3581cd72cde4998c0d7d19bc68b67c2b88b3d593

Comment 22

[Dão Gottwald [:dao]]

(In reply to Andreea Pavel [:apavel] from comment #21)
> Backed out for failing browser chrome at
> browser/components/sessionstore/test/browser_350525.js

Looks like you forgot to update deleteTabValue there...

Comment 23

[Manish [:manishkk]]

Attachment: Patchv2_Bug1446719

Please review

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a82b8f8426da
Rename getTabValue / setTabValue / deleteTabValue to getCustomTabValue / setCustomTabValue / deleteCustomTabValue. r=dao

Comment 25

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a82b8f8426da