Last Comment Bug 749930 - Replace uses of nsILocalFile with nsIFile (C++ bits)
: Replace uses of nsILocalFile with nsIFile (C++ bits)
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Geoff Lankow (:darktrojan)
:
Mentors:
Depends on: 682360 762079
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-28 03:44 PDT by Geoff Lankow (:darktrojan)
Modified: 2013-03-21 03:05 PDT (History)
11 users (show)
geoff: in‑testsuite-
geoff: in‑litmus-
geoff: in‑moztrap-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (583.16 KB, patch)
2012-04-28 04:39 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Review
WIP (585.03 KB, patch)
2012-05-08 15:55 PDT, Geoff Lankow (:darktrojan)
josh: feedback+
Details | Diff | Review
WIP (comm-central) (435.87 KB, patch)
2012-05-08 16:01 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Review
v3 (608.39 KB, patch)
2012-05-30 17:15 PDT, Geoff Lankow (:darktrojan)
benjamin: review+
Details | Diff | Review
changes between v2 and v3 (38.60 KB, patch)
2012-05-30 17:22 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Review
c-c v2 (438.18 KB, patch)
2012-05-30 17:51 PDT, Geoff Lankow (:darktrojan)
philipp: review+
neil: review+
standard8: superreview+
Details | Diff | Review
changes between v3 and v4 (31.05 KB, patch)
2012-06-05 05:53 PDT, Geoff Lankow (:darktrojan)
benjamin: review+
Details | Diff | Review
stray uses of QI (3.47 KB, patch)
2012-06-20 05:25 PDT, Geoff Lankow (:darktrojan)
neil: review-
Details | Diff | Review
stray uses of QI (3.50 KB, patch)
2012-06-20 05:45 PDT, Geoff Lankow (:darktrojan)
neil: review+
Details | Diff | Review

Description Geoff Lankow (:darktrojan) 2012-04-28 03:44:42 PDT
Erk, this is a pain as everything depends on everything else, except the JS stuff. Currently trying to figure out the easiest way to review, and how to keep the bitrot at bay.
Comment 1 Geoff Lankow (:darktrojan) 2012-04-28 04:39:50 PDT
Created attachment 619279 [details] [diff] [review]
WIP

Drive-by inspections welcome. There's bound to be some whitespace inconsistencies that I've missed, and probably other stuff too.
Comment 2 Geoff Lankow (:darktrojan) 2012-05-08 15:55:22 PDT
Created attachment 622183 [details] [diff] [review]
WIP
Comment 3 Geoff Lankow (:darktrojan) 2012-05-08 16:01:10 PDT
Created attachment 622186 [details] [diff] [review]
WIP (comm-central)
Comment 4 Geoff Lankow (:darktrojan) 2012-05-08 16:20:08 PDT
I think this is ready to be reviewed. I'm aiming to land it at the start of the next release cycle to give downstream projects as much time as possible.

To avoid reviewer death-by-boredom, as it's mostly straight find/replace changes, I could split it by module and assign to different reviewers. Alternatively I have the bits where I actually change code as a separate (much smaller) patch from the find/replace stuff.
Comment 5 Masatoshi Kimura [:emk] 2012-05-08 17:32:14 PDT
>-    nsCOMPtr<nsILocalFile> localFile;
>+    nsCOMPtr<nsIFile> localFile;
>     rv = NS_NewLocalFile(xpcomStr, false, getter_AddRefs(localFile));
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     file = do_QueryInterface(localFile);
Please remove localFile. nsIFile-to-nsIFile QI is meaningless.
Comment 6 Geoff Lankow (:darktrojan) 2012-05-23 18:49:11 PDT
I've created a diff without the chunks that are nothing but s/nsILocalFile\b/nsIFile/g (ie. just the 'interesting' bits) - it's about a fifth of the size of the full diff. Benjamin, would you be prepared to review that?
Comment 7 Josh Matthews [:jdm] 2012-05-24 10:26:13 PDT
Comment on attachment 622183 [details] [diff] [review]
WIP

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

I read through it all. Nothing looks broken, there are just some small cleanups that can be done.

::: content/base/src/nsDOMFile.cpp
@@ +562,1 @@
>      rv = NS_NewLocalFile(xpcomStr, false, getter_AddRefs(localFile));

Scrap localFile, just use the file from previously and remove the extraneous QI below.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +1099,5 @@
>      }
>  
>      if (!file) {
>        // this is no "file://", try as local file
> +      nsCOMPtr<nsIFile> localFile;

Scrap this, use file below and skip the QI.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +321,5 @@
>  
>    rv = directory->Append(originSanitized);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  directory.forget(reinterpret_cast<nsIFile**>(aDirectory));

No cast necessary.

::: dom/plugins/base/nsPluginDirServiceProvider.cpp
@@ +518,5 @@
>        if (NS_SUCCEEDED(rv) && childKey) {
>          nsAutoString path;
>          rv = childKey->ReadStringValue(NS_LITERAL_STRING("Path"), path);
>          if (NS_SUCCEEDED(rv)) {
> +          nsCOMPtr<nsIFile> localFile;

This comment is about code outside of the context shown - there's a do_QueryInterface between localFile and temp that is now redundant.

::: modules/libjar/nsJAR.cpp
@@ +265,5 @@
>    //XXX If we guarantee that rv in the case of a non-empty directory
>    //XXX is always FILE_DIR_NOT_EMPTY, we can remove
>    //XXX |rv == NS_ERROR_FAILURE| - bug 322183 needs to be completely
>    //XXX fixed before that can happen
> +  nsresult rv;

Combine this with the next line.

::: modules/libjar/nsZipArchive.cpp
@@ +260,2 @@
>    nsresult rv;
> +  rv = nsZipHandle::Init(aFile, getter_AddRefs(handle));

Combine this with the previous line.

::: profile/dirserviceprovider/src/nsProfileDirServiceProvider.cpp
@@ +118,2 @@
>    if (mSharingEnabled)
>      dirToLock = do_QueryInterface(mNonSharedProfileDir);

No QI.

@@ +118,4 @@
>    if (mSharingEnabled)
>      dirToLock = do_QueryInterface(mNonSharedProfileDir);
>    else
>      dirToLock = do_QueryInterface(mProfileDir);

No QI.

::: profile/dirserviceprovider/src/nsProfileLock.cpp
@@ +498,1 @@
>      rv = aProfileDir->Clone((nsIFile **)((void **)getter_AddRefs(lockFile)));

Please remove this shameful relic of history.

@@ +569,1 @@
>      rv = aProfileDir->Clone((nsIFile **)((void **)getter_AddRefs(oldLockFile)));

Likewise.

::: xpcom/build/nsXPCOMCID.h
@@ +53,5 @@
>   * XPCOM File
>   *   The file abstraction provides ways to obtain and access files and 
>   *   directories located on the local system. 
>   *
> + *   This contract supports the nsIFile interface and the nsIFile interface.

But what if I want the nsIFile interface? There's too much choice!

::: xpcom/string/public/nsXPCOMStrings.h
@@ +771,5 @@
>  
>    /* Conversion from UTF-16 to the native filesystem charset may result in a
>     * loss of information.  No attempt is made to protect against data loss in
>     * this case.  The native filesystem charset applies to strings passed to
> +   * the "Native" method variants on nsIFile and nsIFile. */

Once again, with feeling!
Comment 8 Geoff Lankow (:darktrojan) 2012-05-30 17:15:13 PDT
Created attachment 628542 [details] [diff] [review]
v3
Comment 9 Geoff Lankow (:darktrojan) 2012-05-30 17:22:42 PDT
Created attachment 628546 [details] [diff] [review]
changes between v2 and v3

The license changes made an absolute mess of the interdiff, so here's a lovingly hand-crafted diff of the changes made since Josh's f+.
Comment 10 Geoff Lankow (:darktrojan) 2012-05-30 17:51:42 PDT
Created attachment 628551 [details] [diff] [review]
c-c v2
Comment 11 Geoff Lankow (:darktrojan) 2012-05-30 17:55:41 PDT
Comment on attachment 628542 [details] [diff] [review]
v3

Benjamin, Josh suggests that his f+ on the previous version (and someone reviewing the changes since) might be good enough. Is that okay with you?
Comment 12 Philipp Kewisch [:Fallen] 2012-05-30 22:25:47 PDT
Comment on attachment 628551 [details] [diff] [review]
c-c v2

What about the JS callers? There are 13 JS files in calendar/ that use nsILocalFile
Comment 13 Geoff Lankow (:darktrojan) 2012-05-31 01:08:44 PDT
(In reply to Philipp Kewisch [:Fallen] (away until May 28th) from comment #12)
> Comment on attachment 628551 [details] [diff] [review]
> c-c v2
> 
> What about the JS callers? There are 13 JS files in calendar/ that use
> nsILocalFile

Yes there are, I'll fix them in a new bug after this lands (was going to do it here but this bug's getting big enough already). They're not affected by the changes to interfaces. Sorry, I should've been more explicit about that.
Comment 14 Benjamin Smedberg [:bsmedberg] 2012-05-31 05:37:52 PDT
Comment on attachment 628542 [details] [diff] [review]
v3

Yes, I trust jdm's review on this.
Comment 15 Philipp Kewisch [:Fallen] 2012-05-31 06:18:42 PDT
Comment on attachment 628551 [details] [diff] [review]
c-c v2

r+ on the calendar parts.
Comment 16 neil@parkwaycc.co.uk 2012-05-31 16:09:49 PDT
Comment on attachment 628551 [details] [diff] [review]
c-c v2

Seems legit. Patch was mostly useless context though :-(

I assume you're going to reindent nsMsgSend.cpp etc. :-)

You fixed some of the silly code but I won't ask you to fix all of it ;-)
Comment 17 Mark Banner (:standard8) 2012-06-04 03:38:06 PDT
Comment on attachment 628551 [details] [diff] [review]
c-c v2

This looks fine to me, thanks for doing the work. I'll make this an sr since you technically need that for changing interfaces. I don't think you need Callek's review as you have got Neil's.
Comment 18 Geoff Lankow (:darktrojan) 2012-06-05 05:53:19 PDT
Created attachment 630138 [details] [diff] [review]
changes between v3 and v4
Comment 20 Geoff Lankow (:darktrojan) 2012-06-20 05:25:43 PDT
Created attachment 634860 [details] [diff] [review]
stray uses of QI
Comment 21 neil@parkwaycc.co.uk 2012-06-20 05:37:49 PDT
Comment on attachment 634860 [details] [diff] [review]
stray uses of QI

localDir is used later.
Comment 22 Geoff Lankow (:darktrojan) 2012-06-20 05:45:50 PDT
Created attachment 634864 [details] [diff] [review]
stray uses of QI

Er, oops.
Comment 23 Geoff Lankow (:darktrojan) 2012-06-21 00:48:44 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/52e51974da21
Comment 24 Ed Morley [:emorley] 2012-06-21 13:04:39 PDT
https://hg.mozilla.org/mozilla-central/rev/52e51974da21
Comment 25 :aceman 2013-03-21 00:56:39 PDT
What is the target milestone of this?

Also, there are still some uses of nsILocalFile in /mozilla cpp files...

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