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

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: dao, Assigned: manishkk, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

a year ago
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

a year ago
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
(Reporter)

Comment 2

a year ago
(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).
(Assignee)

Comment 3

a year ago
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)
(Reporter)

Comment 4

a year ago
(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)
(Reporter)

Comment 5

a year ago
Manish, have you started working on this? Anything I can help with?
Flags: needinfo?(1991manish.kumar)
(Assignee)

Comment 6

a year ago
Dao, 

I will work on this from today. Will submit the patch soon.
Thanks
Flags: needinfo?(1991manish.kumar)
(Assignee)

Comment 7

a year ago
Posted patch Patch_Bug1446719 (obsolete) — Splinter Review
Please review.
Attachment #8964116 - Flags: review?(dao+bmo)
(Reporter)

Comment 8

a year ago
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)
(Assignee)

Comment 9

a year ago
Posted patch NewPatch_Bug1446719 (obsolete) — Splinter Review
Please review.
Attachment #8964116 - Attachment is obsolete: true
Attachment #8964120 - Flags: review?(dao+bmo)
(Reporter)

Comment 10

a year ago
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)
(Assignee)

Comment 11

a year ago
Posted patch patch1446719 (obsolete) — Splinter Review
Please review
Attachment #8964120 - Attachment is obsolete: true
Attachment #8964142 - Flags: review?(dao+bmo)
(Reporter)

Comment 12

a year ago
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)
(Reporter)

Comment 13

a year ago
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.
(Assignee)

Comment 14

a year ago
Posted patch NewPatch1446719 (obsolete) — Splinter Review
Please Review
Attachment #8964142 - Attachment is obsolete: true
Attachment #8964158 - Flags: review?(dao+bmo)
(Reporter)

Comment 16

a year ago
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)
(Assignee)

Comment 17

a year ago
Posted patch NewPatch_Bug1446719 (obsolete) — Splinter Review
Any other changes required?
Attachment #8964158 - Attachment is obsolete: true
Attachment #8964160 - Flags: review?(dao+bmo)
(Reporter)

Comment 19

a year ago
Comment on attachment 8964160 [details] [diff] [review]
NewPatch_Bug1446719

Looks good, thank you!
Attachment #8964160 - Flags: review?(dao+bmo) → review+
(Reporter)

Updated

a year ago
Summary: Rename getTabValue / setTabValue to getCustomTabValue / setCustomTabValue → Rename getTabValue / setTabValue / deleteTabValue to getCustomTabValue / setCustomTabValue / deleteCustomTabValue

Comment 20

a year ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f80a1b8f781b
Rename getTabValue / setTabValue / deleteTabValue to getCustomTabValue / setCustomTabValue / deleteCustomTabValue. r=dao
(Reporter)

Comment 22

a year ago
(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...
(Assignee)

Comment 23

a year ago
Please review
Attachment #8964160 - Attachment is obsolete: true
Flags: needinfo?(1991manish.kumar)
Attachment #8964174 - Flags: review?(dao+bmo)
(Reporter)

Updated

a year ago
Attachment #8964174 - Flags: review?(dao+bmo) → review+

Comment 24

a year ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a82b8f8426da
Rename getTabValue / setTabValue / deleteTabValue to getCustomTabValue / setCustomTabValue / deleteCustomTabValue. r=dao

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a82b8f8426da
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
(Reporter)

Updated

a year ago
Blocks: 1451715
You need to log in before you can comment on or make changes to this bug.