Closed Bug 1402485 Opened 2 years ago Closed 2 years ago

Option to remove all session cookies is shown for non-cookie storages

Categories

(DevTools :: Storage Inspector, enhancement, P3)

57 Branch
enhancement

Tracking

(firefox59 verified)

VERIFIED FIXED
Firefox 59
Tracking Status
firefox59 --- verified

People

(Reporter: sebo, Assigned: sagarbharadwaj50, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file)

The option "Delete All Session Cookies" is not only shown for cookie storages but also for other storages.

Test case:
1. Go to https://www.vanamco.com/2014/11/14/indexeddb-fundamentals-plus-a-indexeddb-example-tutorial/
2. Click the "add 6 predefined todos" button on the page
3. Reload the page
4. Expand the "https://www.vanamco.com" Indexed DB storage until you get to the "todo" item and select it.
5. Right-click the item list.

=> The option "Delete All Session Cookies" is shown.

Sebastian
To fix this, the code that was introduced in https://hg.mozilla.org/mozilla-central/rev/217f398fd59d needs to be adjusted so that the option is only shown for cookie storage lists.

Sebastian
Mentor: sebastianzartner
Keywords: good-first-bug
Just to be clear, the option "Delete All Session Cookies" should only be shown when you right-click the entry list (at the right side of the Storage Inspector) of a cookie storage. For all other storage types like e.g. local storages it must not appear.

Sebastian
Hey! I am new contributor looking to get started on my first couple bugs.Would it be okay if I got started on this bug?
(In reply to mithilan91 from comment #3)
> Hey! I am new contributor looking to get started on my first couple
> bugs.Would it be okay if I got started on this bug?
Sounds great to me!
Honza
Assignee: nobody → mithilan91
Status: NEW → ASSIGNED
Status: ASSIGNED → NEW
Hi @mithilan91,
Are you working on this issue? Is it fine if I can take this one if you aren't working.
Flags: needinfo?(mithilan91)
Hey, I am still working on it but I was trying to make a push to mozreview for another bug and it said I "cannot submit commits referencing multiple bugs" so thought maybe being assigned to two bugs was the issue. I'am not sure if this is the case but if its, feel free to to take it because I won't be able to push for mozreview.
Flags: needinfo?(mithilan91)
Hey @mithilan91,
No that's not the reason why you are seeing that message, I guess.
So this is my understanding.
I'm guessing this is how your tree looks like

-- your commit for bug1,bug2
--> commit from mozilla-central

It should ideally be:
-- your commit for bug1
--> commit from mozilla-central
push to mozreview should work now

and

-- your commit for bug2
--> commit from mozilla-central
push to mozreview should work now.

Just have commits related to a single bug on top of commits from mozilla-central and reference only that one bug in the mozreview commit.
Thank you for replying! Is there a way to reference a bug when making a commit for the future and is there a way to fix my previous commits so I can make a push to mozreview now?
So assuming your tree is right now in this state,
-- your commit for bug1,bug2
--> commit from mozilla-central

I would run the following
hg strip --keep -r -1
This would bring it to the state you were before creating the commit .i.e it would remove the commit from the log but still keep your changes. Run `hg log` and you can see that your commit is no longer there. Run `hg diff` and see that your changes are there.

Now you can store your changes of Bug2 to some diff file like this:
hg diff file1.js > ~/some/location/bug2.diff

Now you would need to revert your changes of Bug2 since those are not needed for Bug1's commit and since those are stored safely
hg revert --no-backup file1.js 

Now you only have Bug1 related changes
hg commit      (commit msg with Bug1 reference)
hg push review (should work now)

Next again strip one commit
hg strip --keep -r -1
Now you are at mozilla-central commit

import Bug2 changes
hg import ~/some/location/bug2.diff --no-commit
Run hg diff to verify the diff

Create a commit
hg commit      (commit msg with Bug2 reference)
hg push review (should work now)
Hello Mithilan,
Are you still working on this? Or would it be okay if I take this up?
Hello,
I have submitted a review request. This is my first time contributing and first time using mercurial as well. 
Apologies if I have not adhered to some workflow or convention.
Sorry about the wait... the patch looks good to me so I am running it through try (our automated testing system) to make sure it doesn't have any undesirable side effects.

If it runs through the testing system okay I will land your code for you.

You can see the try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4bd80d5d606
Has Regression Range: --- → irrelevant
Has STR: --- → yes
Attachment #8936129 - Flags: review?(sebastianzartner)
Attachment #8936129 - Flags: review?(odvarko)
Attachment #8936129 - Flags: review?(mratcliffe)
Thank you for you review Mike :)
Sorry Sagar that I didn't do the review. I am simply too busy lately. And thank you Mike for taking it over!

Sebastian
Hello,
I think the patch can be landed. I cannot add the checkin needed keyword because the bug is not assigned to me.
I've assigned the bug to you now, Sagar. Thank you for the patch!
Mithilan, feel free to pick another one when you have time again.

Sebastian
Assignee: mithilan91 → sagarbharadwaj50
Status: NEW → ASSIGNED
Keywords: checkin-needed
I don't see where this ever officially got an r+? Autoland can't push it because of that.
Flags: needinfo?(mratcliffe)
Keywords: checkin-needed
Comment on attachment 8936129 [details]
Bug 1402485 - Remove Delete all Session cookies option for non cookie staorage

https://reviewboard.mozilla.org/r/206904/#review215002
Attachment #8936129 - Flags: review?(mratcliffe) → review+
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa7c634fd552
Remove Delete all Session cookies option for non cookie staorage r=miker
https://hg.mozilla.org/mozilla-central/rev/aa7c634fd552
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Works fine for me in Firefox 59.0a1 2017-12-22. Thank you for the fix, Sagar!

Sebastian
Status: RESOLVED → VERIFIED
Flags: needinfo?(mratcliffe)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.