Split ArchiveReader and FileHandle implementation into separate dirs

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: janv, Assigned: janv)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

5 years ago
We now have ~60 subdirs under mozilla/dom, some of them contain just a couple of files. So I don't see a reason to share ArchiveReader and FileHandle impl in dom/file. FileSystem API implementation is going to create a new subir dom/filesystem. I propose to split dom/file into dom/archivereader and dom/filehandle.
Assignee

Comment 1

5 years ago
Guys, what do you think ?
Flags: needinfo?(jonas)
Flags: needinfo?(bent.mozilla)
Flags: needinfo?(amarchesini)
+1
Flags: needinfo?(amarchesini)
Sounds ok to me and jonas.
Flags: needinfo?(jonas)
Flags: needinfo?(bent.mozilla)
Assignee

Updated

5 years ago
Blocks: 942542
Assignee

Comment 4

5 years ago
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → Jan.Varga
Status: NEW → ASSIGNED
Attachment #8386729 - Flags: review?(bent.mozilla)
Attachment #8386729 - Flags: feedback?(amarchesini)
Comment on attachment 8386729 [details] [diff] [review]
patch

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

::: dom/archivereader/ArchiveReader.h
@@ +4,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef mozilla_dom_file_domarchivereader_h__

probably this should change as well... but I don't care too much.
Attachment #8386729 - Flags: feedback?(amarchesini) → feedback+
Comment on attachment 8386729 [details] [diff] [review]
patch

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

::: dom/archivereader/ArchiveReaderCommon.h
@@ +15,5 @@
> +#include "nsString.h"
> +#include "nsTArray.h"
> +
> +#define BEGIN_ARCHIVEREADER_NAMESPACE \
> +  namespace mozilla { namespace dom { namespace archivereader {

This sort of stuff has met with pretty strict resistance so I think it's better to move away from it. Let's just put archivereader and filehandle in the dom namespace? And no more FooCommon.h files since we're trying to forward-declare everything nowadays.
Attachment #8386729 - Flags: review?(bent.mozilla) → review-
Yeah, the namespace should be parallel to the directory structure.
Assignee

Updated

5 years ago
Status: ASSIGNED → NEW
Depends on: 984302
Assignee

Comment 8

5 years ago
Posted patch patch v2 (obsolete) — Splinter Review
Ok, I tried to do my best, but I don't have time and energy for doing the same to archivereader.

So besides moving filehandle into a separate dir, the patch does:
- move stuff from mozilla::dom::filehandle to mozilla::dom
- remove FileHandleCommon.h
- forward declare as much as possible
- always include all necessary .h files
- place all includes in one section
Attachment #8386729 - Attachment is obsolete: true
Attachment #8392734 - Flags: review?(bent.mozilla)
Assignee

Comment 9

5 years ago
Posted patch patch v2Splinter Review
rebased patch
Attachment #8392734 - Attachment is obsolete: true
Attachment #8392734 - Flags: review?(bent.mozilla)
Attachment #8410051 - Flags: review?(bent.mozilla)
Comment on attachment 8410051 [details] [diff] [review]
patch v2

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

Cool, thanks!
Attachment #8410051 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/03fdda1af1f4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.