699860 - Remove the legacy deleteAllLike usage in ForgetAboutSite.jsm

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Marco Bonardo [:mak]]

PB is doing some cleanup using the synchronous API, this may be due to the related component being synchronous, should be fixed though, even if by refactoring that component.

Comment 1

[(no longer active)]

Which APIs are we talking about here?  The ones called from clearDomainData?

Comment 2

[Marco Bonardo [:mak]]

the ones in RemoveDataFromDomain. Why were those not added as APIs of the services? Doesn't look healthy to update the databases from the outside, if they'd be in the service the problem would be moved to making those services async.

Comment 3

[(no longer active)]

(In reply to Marco Bonardo [:mak] from comment #2)
> the ones in RemoveDataFromDomain. Why were those not added as APIs of the
> services?

Well, I didn't write that code, and I sort of objected to it living in the PB service, but we didn't have any place which was better.  ;-)

> Doesn't look healthy to update the databases from the outside, if
> they'd be in the service the problem would be moved to making those services
> async.

by "the service", do you mean the Places service?  If yes, I totally agree, the places service can just listen on the notification and do whatever it's need asynchronously.

Comment 4

[Marco Bonardo [:mak]]

These data are not related to Places at all, each service should have its own removeXXX methods that pb should be called into.

Comment 5

[Marco Bonardo [:mak]]

For example, clearing Completed downloads should be a method exposed by the downloads service, selecting and clearing content preferences should be a method exposed by ContentPreferences service.

Comment 6

[(no longer active)]

Why do any of these methods need to be exposed?  I would expect those services to each handle the notification and do whatever needs to be done themselves.

Comment 7

[(no longer active)]

Also, I removed the asyncPrivateBrowsing alias.  It takes way more than this bug to make the PB service asynchronous.

Comment 8

[Marco Bonardo [:mak]]

(In reply to Ehsan Akhgari [:ehsan] from comment #7)
> Also, I removed the asyncPrivateBrowsing alias.  It takes way more than this
> bug to make the PB service asynchronous.

yep, the aliases are just a way to make the list in bug 699820 readable. Changingto NoPrivateBrowsingSQL

(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> Why do any of these methods need to be exposed?  I would expect those
> services to each handle the notification and do whatever needs to be done
> themselves.

Which notification? I think removeDataForDomain is the method invoked by the CRH dialog, and this method is just a proxy to the various RemoveXXXByTimeframe methods for each data provider. So my point is why adding pure queries to pb rather than adding proper RemoveXXXByTimeframe methods to those data providers.
But maybe I'm missing something?

Comment 9

[(no longer active)]

(In reply to Marco Bonardo [:mak] from comment #8)
> Which notification? I think removeDataForDomain is the method invoked by the
> CRH dialog, and this method is just a proxy to the various
> RemoveXXXByTimeframe methods for each data provider. So my point is why
> adding pure queries to pb rather than adding proper RemoveXXXByTimeframe
> methods to those data providers.
> But maybe I'm missing something?

The notification that I'm talking about is browser:purge-domain-data <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#759>.  I believe DOM storage and sessionstore already handle it.  If I were to design this code, I would just dispatch that notification, and then every component would handle it internally.

Comment 10

[(no longer active)]

Adjusting the summary to what this bug is really about.

Comment 11

[u428464]

There should also be a visual indicator when cleaning history take a bit of time. As experienced in Google Chrome this make the user feel the browser is more responsive.

Comment 12

[Marco Bonardo [:mak]]

this bug is not about slow clear history, it's about avoid avoiding synchronous Storoage API, I think the last title change is quite confusing.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

I agree that the morphing from comment 10 was a pretty drastic jump - there are other bugs on clear history performance in general (e.g. bug 985351). Morphing it back, and closing it because the private browsing service no longer exists.

Comment 14

[Marco Bonardo [:mak]]

I think all of this bug has been misunderstood.

The scope here was to not use mozStorage synchronous APIs, see

function deleteAllLike

http://mxr.mozilla.org/mozilla-central/source/toolkit/forgetaboutsite/ForgetAboutSite.jsm#128

my point is that it should invoke an API in the download manager that will do that work and not directly use mozStorage synchronous API.

Comment 15

[Marco Bonardo [:mak]]

I hope this makes more sense now.

note this bug is not very important for Firefox now cause we use jsdownloads component, though since we are trying to reduce use of the storage synchronous APIs might still be useful from a code point of view to move that code into a nsIDownloadManager API.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

That the code moved to forgetaboutsite.jsm is what confused me!

I imagine we will only fix this via bug 851471.

Comment 17

[Marco Bonardo [:mak]]

I'm inverting the dependency cause removing this code is part of decommissioning the old download manager API.
There are also other uses of useJSTransfer, I'm not sure if we have a bug tracking the removal of these:
http://mxr.mozilla.org/mozilla-central/search?string=useJSTransfer

Paolo?

Comment 18

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] from comment #17)
> I'm inverting the dependency cause removing this code is part of
> decommissioning the old download manager API.

That makes sense, I updated the bug summary as well.

> There are also other uses of useJSTransfer, I'm not sure if we have a bug
> tracking the removal of these:
> http://mxr.mozilla.org/mozilla-central/search?string=useJSTransfer

I filed bug 1152842 for the only actual use that would not be removed by removing the old Download Manager code entirely.

Comment 19

[:Paolo Amadini]

I think we can go ahead and assume useJSTransfer will be true here, and remove the code that sets it, as well as all the code that is called when it is false:

http://mxr.mozilla.org/mozilla-central/source/toolkit/forgetaboutsite/ForgetAboutSite.jsm#101

This will work for Firefox, while applications in the comm-central repository like Thunderbird and SeaMonkey might need to implement the removal at the front-end level, until they migrate to the new Downloads API.

Comment 20

[Daniel Miller]

I would like to work on this bug. If I understand you correctly, we just need to remove lines 102-110 and 117-162?

Comment 21

[:Paolo Amadini]

(In reply to Daniel Miller from comment #20)
> I would like to work on this bug. If I understand you correctly, we just
> need to remove lines 102-110 and 117-162?

That's the basic idea, and then there are the related test files. The oldDownloadManagerDisabled function and the "downloads.sqlite" reference are not required anymore:

http://mxr.mozilla.org/mozilla-central/source/toolkit/forgetaboutsite/test/unit/head_forgetaboutsite.js

Tests that use the function would have to be removed as well, you can find them here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/forgetaboutsite/test/unit/test_removeDataFromDomain.js

To verify this works, you can use the command:

./mach test toolkit/forgetaboutsite/test/unit/

Thanks for your interest! Looking forward to a patch, and if you have any questions feel free to ask.

Comment 22

[Daniel Miller]

Attachment: A patch for Bug 699860

Here's my first attempt at a patch. I can redo it if there are any errors.

Comment 23

[:Paolo Amadini]

Comment on attachment 8592631 [details] [diff] [review]
A patch for Bug 699860

This looks good to me, thanks! The only change needed is to re-indent by two spaces the code that is not in the conditional block anymore.

The procedure now is to upload a new patch, at which point you'll have the option to mark the old one as obsolete. In the same screen, look for the "review" flag and set it to "?", indicating my account (you can search for ":paolo") in the field that appears beside the flag.

I'll then run the new tests in our automated infrastructure to see whether everything still works as expected.

Comment 24

[Daniel Miller]

Attachment: An update to patch for Bug 699860

Fixed the indentation issue with the previous patch.

Comment 25

[:Paolo Amadini]

Comment on attachment 8592846 [details] [diff] [review]
An update to patch for Bug 699860

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

Sorry for the delay, and thanks for the updated patch, it looks good!

The next step is for me to start the tests on the tryserver, but I have to wait because we're having issues with our automation infrastructure at the moment. In general, it is to be expected that a bug may take a few days before it is integrated into Nightly and the bug is marked as resolved.

Comment 26

[:Paolo Amadini]

I've pushed this to the tryserver. I've also included the removal of two more support functions that are now unused, and I missed during my previous review pass.

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

We should wait until the xpcshell tests indicated by the "X" letter succeed on all platforms before pushing the change to the integration branches.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5a0267e655d0

Comment 28

[:Paolo Amadini]

I've pushed this to the integration branch.

If all tests pass, which is likely given the try build, this will be merged to mozilla-central.

Comment 29

[:Paolo Amadini]

As a note, this may require the test to be disabled or moved for applications that haven't migrated to the new Downloads API yet. I'm adding this to the tracking bug for the comm-central repository.

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5a0267e655d0

Comment 31

[:Paolo Amadini]

Congratulations Daniel for fixing your first bug!

Let me know if there are other bugs I can help you with, or if you have any area of the code you'd be interested in contributing to and you'd like to find a good next bug, now that you've seen how the process works.

Comment 32

[Daniel Miller]

(In reply to :Paolo Amadini from comment #31)
> Congratulations Daniel for fixing your first bug!
> 
> Let me know if there are other bugs I can help you with, or if you have any
> area of the code you'd be interested in contributing to and you'd like to
> find a good next bug, now that you've seen how the process works.

Thanks Paolo. And thanks a lot for guiding me through the process and handling all the work that came after I submitted my patch.

I would appreciate any help you can provide in identifying a good next bug to work on. I am contributing to Mozilla because:
- I would like to deepen my experience and understanding of JavaScript.
- I really believe in Mozilla's mission and just want to help in any way I can.

So I really am ready and willing to work on anything that requires work with JavaScript.

With this first bug I found that I was able to fix it without really understanding what was going on in the code. Perhaps you could help me identify a bug that would require a more in-depth reading of some section of the code base?

Thanks again,
Daniel

Comment 33

[:Paolo Amadini]

(In reply to Daniel Miller from comment #32)
> Perhaps you could help me
> identify a bug that would require a more in-depth reading of some section of
> the code base?

I think bug 1155272 fits your requirements well. It's about Telemetry, which is a self-contained area of the code base that implements the opt-in mechanism we use for understanding real-world performance of Mozilla software, and whose results you can see at <http://telemetry.mozilla.org/>.

The bug is about achieving a better understanding of the performance of Telemetry itself, using Telemetry. There is an existing probe that needs to be split - you need to understand how the system works, but you also have a starting point.

Vladan is mentoring the bug - I've CC'd myself there just in case. If you're interested, say so on the bug and ensure you tick "need more information from: reporter".

A (possibly) simpler but still interesting alternative is bug 1124185, the conversion from a synchronous API to an asynchronous one. This will require a technique we use a lot, that is searching for a function name globally, and then working up from there to its callers to see if something has to be changed as well.

Comment 34

[Daniel Miller]

(In reply to :Paolo Amadini from comment #33)
> (In reply to Daniel Miller from comment #32)
> > Perhaps you could help me
> > identify a bug that would require a more in-depth reading of some section of
> > the code base?
> 
> I think bug 1155272 fits your requirements well. It's about Telemetry, which
> is a self-contained area of the code base that implements the opt-in
> mechanism we use for understanding real-world performance of Mozilla
> software, and whose results you can see at <http://telemetry.mozilla.org/>.
> 
> The bug is about achieving a better understanding of the performance of
> Telemetry itself, using Telemetry. There is an existing probe that needs to
> be split - you need to understand how the system works, but you also have a
> starting point.
> 
> Vladan is mentoring the bug - I've CC'd myself there just in case. If you're
> interested, say so on the bug and ensure you tick "need more information
> from: reporter".
> 
> A (possibly) simpler but still interesting alternative is bug 1124185, the
> conversion from a synchronous API to an asynchronous one. This will require
> a technique we use a lot, that is searching for a function name globally,
> and then working up from there to its callers to see if something has to be
> changed as well.

Thanks Paolo, I'll take a look at both later today and request to be assigned to one.