959539 - Remove support for the old private browsing mode

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As you can see via bug 915554 we can finally remove all the code snippets which are related to the old private browsing mode. This mode was present until Firefox 17.0esr which is not supported anymore. 

So lets get all of those code removed.

This is a good mentored bug. I will set Andrei as mentor.

Comment 1

[k]

I would like to do this (it would be my first bug).

Any details on how to find the code that should be removed?

Comment 2

[Andrei Eftimie]

Hallo Kay,

Thanks for volunteering!

You can find the code for mozmill on github: https://github.com/mozilla/mozmill
Relevant code for this bug should have a comment referencing the bug mentioned above: 915554

You can find more information about Mozmill on MDN: https://developer.mozilla.org/en-US/docs/Mozmill

Once you have the changes done, generate a patch with `git format-patch`, upload it to this bug and ask a review from me.

If you need any information please ask. Either here or on IRC on #automation

Comment 3

[k]

Attachment: 0001-Bug-959539-Removed-support-for-the-old-private-brows.patch

I hope this is okay.

Comment 4

[Peter Retzer (:pretzer)]

Comment on attachment 8360300 [details] [diff] [review]
0001-Bug-959539-Removed-support-for-the-old-private-brows.patch

Kay, in the future you should always make sure to request review from an actual person, so it doesn't get lost ('Requestee' field). In this case you should request it from Andrei, the mentor of the bug. You can also find suggested reviewers when you click the 'suggested reviewers' link next to the 'Requestee' field.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Also the bug should be properly assigned by the mentor. I'm doing that now. Thanks Kay for working on this bug!

Comment 6

[Andrei Eftimie]

Comment on attachment 8360300 [details] [diff] [review]
0001-Bug-959539-Removed-support-for-the-old-private-brows.patch

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

Hi K,

A couple things to do here: 

This mutt test can also be safely removed:
> mutt/mutt/tests/js/testController/testEnterLeavePrivateBrowsing.js
(also remove the reference to it from the manifest.ini file)

Also in the commit message add the reviewer at the end, in this case: "Bug xxx - Description. r=aeftimie"

Please make these changes, amend the last commit or squash them together and upload a new patch.
Thanks

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andrei Eftimie from comment #6)
> This mutt test can also be safely removed:
> > mutt/mutt/tests/js/testController/testEnterLeavePrivateBrowsing.js
> (also remove the reference to it from the manifest.ini file)

Why? Do we have a replacement? We still have the private browsing mode and we need a test for the code.

> Also in the commit message add the reviewer at the end, in this case: "Bug
> xxx - Description. r=aeftimie"

No. Given that you are not an official reviewer of Mozmill code, you can't be listed. Instead it should be me or Dave Hunt.

Comment 8

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #7)
> (In reply to Andrei Eftimie from comment #6)
> > This mutt test can also be safely removed:
> > > mutt/mutt/tests/js/testController/testEnterLeavePrivateBrowsing.js
> > (also remove the reference to it from the manifest.ini file)
> 
> Why? Do we have a replacement? We still have the private browsing mode and
> we need a test for the code.

I don't think the old Private Browsing Mode exists anymore.
nsIPrivateBrowsingService appears to have been removed by now:
http://mxr.mozilla.org/mozilla-central/search?string=nsIPrivateBrowsingService

The mentioned test is skipped because of this.

> > Also in the commit message add the reviewer at the end, in this case: "Bug
> > xxx - Description. r=aeftimie"
> 
> No. Given that you are not an official reviewer of Mozmill code, you can't
> be listed. Instead it should be me or Dave Hunt.

Henrik, in this case I don't understand why you are adding me as a mentor in mozmill bugs.
This is confusing for everyone.

Comment 9

[k]

Well, yes.

Now I'm confused :D

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Well, I set Andrei as mentor because he was working on the workaround patch for Mozmill on the other mentioned bug. But yes, that might have been not the best choice as long as he is not an official reviewer for this component. So I will take over this bug.

(In reply to Andrei Eftimie from comment #8)
> > > > mutt/mutt/tests/js/testController/testEnterLeavePrivateBrowsing.js
> > > (also remove the reference to it from the manifest.ini file)
> > 
> > Why? Do we have a replacement? We still have the private browsing mode and
> > we need a test for the code.
> 
> I don't think the old Private Browsing Mode exists anymore.
> nsIPrivateBrowsingService appears to have been removed by now:
> http://mxr.mozilla.org/mozilla-central/
> search?string=nsIPrivateBrowsingService
> 
> The mentioned test is skipped because of this.

Ok, so I walked through the code and also checked our Firefox tests for private browsing. Entering this mode means that we simply open a new browser window in that mode. It would mean we could need a test to verify that, but such one we already have in the mozmill-tests repository. Given that no framework related code is around anymore, I also agree now that the test can go away. Sorry for not checking that in detail before.

Comment 11

[k]

Attachment: 0001-removed-private-browsing-test.patch

Another patch to remove testEnterLeavePrivateBrowsing.js
There wasn't a reference in the manifest.ini though...

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8361683 [details] [diff] [review]
0001-removed-private-browsing-test.patch

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

Kai, it looks like you missed to rebase to master before you created the patch with format-patch. That's necessary, because otherwise the former changes are not part of the patch. In this case I'm missing the observer changes within Mozmill itself.

Comment 13

[k]

Attachment: 0002-Bug-959539-Removed-support-for-the-old-private-brows.patch

I tried a rebase -i HEAD~2, renamed the first patch and squashed the second one into the first.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to k from comment #13)
> I tried a rebase -i HEAD~2, renamed the first patch and squashed the second
> one into the first.

Sorry, that my reply has been taken a bit longer. I will do the review in a bit. But just to comment on here first, and to help you creating patch files, it is better to rebase against the original branch instead of doing it against HEAD^x. With '-i master' it doesn't matter how many commits the integration branch has. All of them will appear and you can choose to fixup them. Then a format-patch with HEAD^ will work.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8363663 [details] [diff] [review]
0002-Bug-959539-Removed-support-for-the-old-private-brows.patch

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

Looks good! Thanks Kay.

::: mutt/mutt/tests/js/testController/testEnterLeavePrivateBrowsing.js
@@ -5,5 @@
> -"use strict";
> -
> -Cu.import("resource://gre/modules/Services.jsm");
> -
> -const PREF_PB_NO_PROMPT = 'browser.privatebrowsing.dont_prompt_on_enter';

Hm, it's strange that this test never has been added to the manifest file in this folder. So we actually never run it. :(

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://github.com/mozilla/mozmill/commit/5fca395aef87fcb09dc9c6c2e3e4171f518fe0d8 (master)

Kay, let me know if you want to help on another bug too. I'm happy to mentor.