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

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: dao, Assigned: manishkk, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 5 obsolete attachments)

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
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
(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).
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
Flags: needinfo?(dao+bmo)
(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
Assignee: nobody → 1991manish.kumar
Flags: needinfo?(dao+bmo)
Manish, have you started working on this? Anything I can help with?
Flags: needinfo?(1991manish.kumar)
Dao, 

I will work on this from today. Will submit the patch soon.
Thanks
Flags: needinfo?(1991manish.kumar)
Posted patch Patch_Bug1446719 (obsolete) — Splinter Review
Please review.
Attachment #8964116 - Flags: review?(dao+bmo)
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=
Attachment #8964116 - Flags: review?(dao+bmo)
Posted patch NewPatch_Bug1446719 (obsolete) — Splinter Review
Please review.
Attachment #8964116 - Attachment is obsolete: true
Attachment #8964120 - Flags: review?(dao+bmo)
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_..."
Attachment #8964120 - Flags: review?(dao+bmo)
Posted patch patch1446719 (obsolete) — Splinter Review
Please review
Attachment #8964120 - Attachment is obsolete: true
Attachment #8964142 - Flags: review?(dao+bmo)
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
Attachment #8964142 - Flags: review?(dao+bmo)
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.
Posted patch NewPatch1446719 (obsolete) — Splinter Review
Please Review
Attachment #8964142 - Attachment is obsolete: true
Attachment #8964158 - Flags: review?(dao+bmo)
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
Attachment #8964158 - Flags: review?(dao+bmo)
Posted patch NewPatch_Bug1446719 (obsolete) — Splinter Review
Any other changes required?
Attachment #8964158 - Attachment is obsolete: true
Attachment #8964160 - Flags: review?(dao+bmo)
Comment on attachment 8964160 [details] [diff] [review]
NewPatch_Bug1446719

Looks good, thank you!
Attachment #8964160 - Flags: review?(dao+bmo) → review+
Summary: Rename getTabValue / setTabValue to getCustomTabValue / setCustomTabValue → Rename getTabValue / setTabValue / deleteTabValue to getCustomTabValue / setCustomTabValue / deleteCustomTabValue
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f80a1b8f781b
Rename getTabValue / setTabValue / deleteTabValue to getCustomTabValue / setCustomTabValue / deleteCustomTabValue. r=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...
Please review
Attachment #8964160 - Attachment is obsolete: true
Flags: needinfo?(1991manish.kumar)
Attachment #8964174 - Flags: review?(dao+bmo)
Attachment #8964174 - Flags: review?(dao+bmo) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a82b8f8426da
Rename getTabValue / setTabValue / deleteTabValue to getCustomTabValue / setCustomTabValue / deleteCustomTabValue. r=dao
https://hg.mozilla.org/mozilla-central/rev/a82b8f8426da
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Blocks: 1451715
You need to log in before you can comment on or make changes to this bug.