Last Comment Bug 865188 - [Storage] Remove "profile" database from OpenSpecialDatabase
: [Storage] Remove "profile" database from OpenSpecialDatabase
Status: RESOLVED FIXED
[good-first-bug][mentor=mak][lang=cpp]
: addon-compat, dev-doc-needed
Product: Toolkit
Classification: Components
Component: Storage (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla25
Assigned To: Roberta
:
Mentors:
Depends on: 702559
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-24 04:58 PDT by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2013-07-11 19:10 PDT (History)
3 users (show)
mak77: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
All references to "profile" and NS_APP_STORAGE_50_FILE are removed. (5.31 KB, patch)
2013-07-09 12:03 PDT, Roberta
mak77: feedback+
Details | Diff | Review
All references to "profile", NS_APP_STORAGE_50_FILE and STORAGE_FILE_50_NAME are removed. (6.39 KB, patch)
2013-07-10 14:07 PDT, Roberta
mak77: review+
Details | Diff | Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-04-24 04:58:13 PDT

    
Comment 1 Roberta 2013-07-08 13:47:23 PDT
Hi,
I'd like to be assigned to this bug.
Comment 2 Marco Bonardo [::mak] 2013-07-08 13:49:35 PDT
Hi, welcome to bugs fixing :)
Comment 3 Roberta 2013-07-09 12:03:26 PDT
Created attachment 772800 [details] [diff] [review]
All references to "profile" and NS_APP_STORAGE_50_FILE are removed.
Comment 4 Marco Bonardo [::mak] 2013-07-10 12:59:00 PDT
Comment on attachment 772800 [details] [diff] [review]
All references to "profile" and NS_APP_STORAGE_50_FILE are removed.

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

You should fix the patch format as explained here:
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Basically, you need the proper annotations for author and commit message.
You can set the author with hg qref -U (for yourself) or hg qref -u "name <mail>" (for someone else). and you can set the commit message with hg qref -m "message" or, if you have setup an editor in the hgrc file you can just use hg qref -e. The message should be like "Bug NNNNNNN - Description of the changes. r=reviewer"
So in this case could be something like "Bug 865188 - Remove Storage support for "profile" special database. r=mak"
The mdn above explains how to setup defaults so that all patches are already annotated with you as the author.

Then you can export the patch with this small trick:
hg log -r "$REV" --template '# HG changeset patch\n# User {author}\n{desc}\n\n' && hg qdiff > /path-to-exported-patch/865188.diff
or you can install the bzexport extension that allows to attach patches from your queue to bugzilla.

There is still something to remove though:

::: profile/dirserviceprovider/src/nsProfileDirServiceProvider.cpp
@@ -228,5 @@
>    }
> -  else if (strcmp(prop, NS_APP_STORAGE_50_FILE) == 0) {
> -    rv = domainDir->Clone(getter_AddRefs(localFile));
> -    if (NS_SUCCEEDED(rv))
> -      rv = localFile->AppendNative(STORAGE_FILE_50_NAME);

you should also remove all of the instances of STORAGE_FILE_50_NAME, since now it would be unused.

::: storage/public/mozIStorageService.idl
@@ +75,5 @@
>    /**
>     * Get a connection to a named special database storage.
>     *
>     * @param aStorageKey a string key identifying the type of storage
> +   * requested.  Valid values include: "memory".

please also remove MOZ_STORAGE_PROFILE_STORAGE_KEY from this file, it looks completely unused already.
Comment 5 Marco Bonardo [::mak] 2013-07-10 12:59:31 PDT
ehr, I actually meant:
hg log -r tip --template '# HG changeset patch\n# User {author}\n{desc}\n\n' && hg qdiff > /path-to-exported-patch/865188.diff
Comment 6 Marco Bonardo [::mak] 2013-07-10 13:49:33 PDT
(In reply to Marco Bonardo [:mak] from comment #4)
> ::: storage/public/mozIStorageService.idl
> @@ +75,5 @@
> >    /**
> >     * Get a connection to a named special database storage.
> >     *
> >     * @param aStorageKey a string key identifying the type of storage
> > +   * requested.  Valid values include: "memory".
> 
> please also remove MOZ_STORAGE_PROFILE_STORAGE_KEY from this file, it looks
> completely unused already.

sorry, this was correct already, I missed it.
Comment 7 Roberta 2013-07-10 14:07:27 PDT
Created attachment 773550 [details] [diff] [review]
All references to "profile", NS_APP_STORAGE_50_FILE and STORAGE_FILE_50_NAME are removed.
Comment 8 Marco Bonardo [::mak] 2013-07-10 14:35:50 PDT
Comment on attachment 773550 [details] [diff] [review]
All references to "profile", NS_APP_STORAGE_50_FILE and STORAGE_FILE_50_NAME are removed.

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

I verified on addons code we track the usage of this feature, I could only find an add-on for Thunderbird 3.0, so I think we are in a very good shape to kill this monster.
The patch looks good, based on my research on mxr, there are no internal consumers nor tests for it, you already ran other tests locally, so I'd say we are good to go.

You can set the checkin-needed keyword to ask for someone to push the patch to the tree, after some contributions you may request access to the Try server, and later full access to all of the repository, to push by yourself.

Thank you and congrats for your first contribution.
Comment 9 Marco Bonardo [::mak] 2013-07-10 14:36:56 PDT
Setting addon-compat, even if the addons impact is very very limited, it's unlikely someone ever thought to use this hidden feature.
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-07-11 19:10:23 PDT
https://hg.mozilla.org/mozilla-central/rev/aeab42c59423

Note You need to log in before you can comment on or make changes to this bug.