Remove the legacy deleteAllLike usage in ForgetAboutSite.jsm

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: mak, Assigned: danjm.com, Mentored)

Tracking

(Blocks 1 bug)

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

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [good first bug][see comment 19], )

Attachments

(1 attachment, 1 obsolete attachment)

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.
Alias: asyncPrivateBrowsing
Which APIs are we talking about here?  The ones called from clearDomainData?
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.
(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.
These data are not related to Places at all, each service should have its own removeXXX methods that pb should be called into.
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.
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.
Also, I removed the asyncPrivateBrowsing alias.  It takes way more than this bug to make the PB service asynchronous.
Alias: asyncPrivateBrowsing
(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?
Alias: NoPrivateBrowsingSQL
(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.
Adjusting the summary to what this bug is really about.
Summary: nsPrivateBrowsingService.js should not use Storage synchronous API → Clear recent history asynchronously
Whiteboard: [Snappy]
Whiteboard: [Snappy] → [Snappy:P3]
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.
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.
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
Summary: Clear recent history asynchronously → nsPrivateBrowsingService.js should not use Storage synchronous API
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.
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.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: nsPrivateBrowsingService.js should not use Storage synchronous API → deleteAllLike in ForgetAboutSite.jsm should not directly use Storage synchronous API
Whiteboard: [Snappy:P3] → [see comment 14]
That the code moved to forgetaboutsite.jsm is what confused me!

I imagine we will only fix this via bug 851471.
Depends on: 851471
Alias: NoPrivateBrowsingSQL → ForgetSiteSQL
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?
Blocks: 851471
No longer depends on: 851471
Flags: needinfo?(paolo.mozmail)
(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.
Alias: ForgetSiteSQL
Flags: needinfo?(paolo.mozmail)
Summary: deleteAllLike in ForgetAboutSite.jsm should not directly use Storage synchronous API → Remove the legacy deleteAllLike usage in ForgetAboutSite.jsm
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.
Mentor: paolo.mozmail
Whiteboard: [see comment 14] → [good first bug][see comment 19]
I would like to work on this bug. If I understand you correctly, we just need to remove lines 102-110 and 117-162?
(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.
Posted patch A patch for Bug 699860 (obsolete) — Splinter Review
Here's my first attempt at a patch. I can redo it if there are any errors.
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.
Fixed the indentation issue with the previous patch.
Attachment #8592631 - Attachment is obsolete: true
Attachment #8592846 - Flags: review?(paolo.mozmail)
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.
Attachment #8592846 - Flags: review?(paolo.mozmail) → review+
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.
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.
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.
Blocks: cc-backlog
https://hg.mozilla.org/mozilla-central/rev/5a0267e655d0
Assignee: nobody → danjm.com
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
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.

(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
(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.
(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.
You need to log in before you can comment on or make changes to this bug.