1186750 - Clean up smart pointer usage of device storage events

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

When working on bug 1184729, I noticed that the device storage part has some smart pointer usages on event dispatch which need non-trivial fix, which I'm going to fix first.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 1 - Inlinize trivial constructors and destructors of events in DeviceStorageRequestParent.

Bug 1186750 part 1 - Inlinize trivial constructors and destructors of events in DeviceStorageRequestParent.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 2 - Remove some unused member fields in events in DeviceStorageRequestParent.

Bug 1186750 part 2 - Remove some unused member fields in events in DeviceStorageRequestParent.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 3 - Abstract CancelableFileEvent in DeviceStorageReqeustParent and use already_AddRefed&& for passing DeviceStorageFile parameter.

Bug 1186750 part 3 - Abstract CancelableFileEvent in DeviceStorageReqeustParent and use already_AddRefed&& for passing DeviceStorageFile parameter.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 4 - Clear runnable list in DeviceStorageRequestParent when being destroyed.

Bug 1186750 part 4 - Clear runnable list in DeviceStorageRequestParent when being destroyed.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 5 - Convert DeviceStorageUsedSpaceCache::Dispatch to use already_AddRef&&.

Bug 1186750 part 5 - Convert DeviceStorageUsedSpaceCache::Dispatch to use already_AddRef&&.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 6 - Use already_AddRefed&& to initialize most smart pointer fields of device storage events.

Bug 1186750 part 6 - Use already_AddRefed&& to initialize most smart pointer fields of device storage events.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 7 - Simplify code in DeviceStorageRequestParent::Dispatch.

Bug 1186750 part 7 - Simplify code in DeviceStorageRequestParent::Dispatch.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 8 - Convert all usage of Dispatch/NS_DispatchTo{Main,Current}Thread in dom/devicestorage to pass in either already_AddRefed or raw pointer.

Bug 1186750 part 8 - Convert all usage of Dispatch/NS_DispatchTo{Main,Current}Thread in dom/devicestorage to pass in either already_AddRefed or raw pointer.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Try push looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=247d0664351d

Comment 10

[Dave Hylands [:dhylands]]

Comment on attachment 8637840 [details]
MozReview Request: Bug 1186750 part 1 - Inlinize trivial constructors and destructors of events in DeviceStorageRequestParent.

https://reviewboard.mozilla.org/r/13981/#review12567

Ship It!

Comment 11

[Dave Hylands [:dhylands]]

Comment on attachment 8637841 [details]
MozReview Request: Bug 1186750 part 2 - Remove some unused member fields in events in DeviceStorageRequestParent.

https://reviewboard.mozilla.org/r/13983/#review12571

Ship It!

Comment 12

[Dave Hylands [:dhylands]]

Comment on attachment 8637842 [details]
MozReview Request: Bug 1186750 part 3 - Abstract CancelableFileEvent in DeviceStorageReqeustParent and use already_AddRefed&& for passing DeviceStorageFile parameter.

https://reviewboard.mozilla.org/r/13985/#review12577

Ship It!

Comment 13

[Dave Hylands [:dhylands]]

Comment on attachment 8637843 [details]
MozReview Request: Bug 1186750 part 4 - Clear runnable list in DeviceStorageRequestParent when being destroyed.

https://reviewboard.mozilla.org/r/13987/#review12579

Learning lots of C++11 features here :)

Comment 14

[Dave Hylands [:dhylands]]

Comment on attachment 8637844 [details]
MozReview Request: Bug 1186750 part 5 - Convert DeviceStorageUsedSpaceCache::Dispatch to use already_AddRef&&.

https://reviewboard.mozilla.org/r/13989/#review12581

Ship It!

Comment 15

[Dave Hylands [:dhylands]]

Comment on attachment 8637845 [details]
MozReview Request: Bug 1186750 part 6 - Use already_AddRefed&& to initialize most smart pointer fields of device storage events.

https://reviewboard.mozilla.org/r/13991/#review12583

Ship It!

Comment 16

[Dave Hylands [:dhylands]]

Comment on attachment 8637846 [details]
MozReview Request: Bug 1186750 part 7 - Simplify code in DeviceStorageRequestParent::Dispatch.

https://reviewboard.mozilla.org/r/13993/#review12585

Ship It!

Comment 17

[Dave Hylands [:dhylands]]

Comment on attachment 8637847 [details]
MozReview Request: Bug 1186750 part 8 - Convert all usage of Dispatch/NS_DispatchTo{Main,Current}Thread in dom/devicestorage to pass in either already_AddRefed or raw pointer.

https://reviewboard.mozilla.org/r/13995/#review12587

Ship It!

Comment 18

[Dave Hylands [:dhylands]]

P.S. - thanks for splitting the patches up the way you. This made it super easy to review.

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22950814675e

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

Sorry for the delay. It took a long time to land bug 1186745, and then I was stuck in some other things. It seems there was some significant change during this period, and patch 5~8 are no longer able to be applied cleanly. (And it also seems all NS_DispatchToCurrentThread uses were removed.)

I'm going to land patch 1~4 soon (which are mostly trivial changes) and request review for the rest of patches.

Sorry again for wasting your time reviewing patches which were not landed.

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 22

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03771f854ff0

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0a9cf2fb44acff1bdeb8852997de649546c0a1f
Bug 1186750 part 1 - Inlinize trivial constructors and destructors of events in DeviceStorageRequestParent. r=dhylands

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef5f9a2a41a47d4f61ee7c8c5fcce6ea5784ed39
Bug 1186750 part 2 - Remove some unused member fields in events in DeviceStorageRequestParent. r=dhylands

https://hg.mozilla.org/integration/mozilla-inbound/rev/5add786777f33f60445a5658ba9e3459e7eec84a
Bug 1186750 part 3 - Abstract CancelableFileEvent in DeviceStorageReqeustParent and use already_AddRefed&& for passing DeviceStorageFile parameter. r=dhylands

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa9922692623cea44fb2fb29064e24380372aafb
Bug 1186750 part 4 - Clear runnable list in DeviceStorageRequestParent when being destroyed. r=dhylands

Comment 24

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 5 - Convert nsDOMDeviceStorage::CheckPermission to take already_AddRefed&&.

Bug 1186750 part 5 - Convert nsDOMDeviceStorage::CheckPermission to take already_AddRefed&&.

Comment 25

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 6 - Remove unused and unimplemented method nsDOMDeviceStorage::StorePermission.

Bug 1186750 part 6 - Remove unused and unimplemented method nsDOMDeviceStorage::StorePermission.

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 7 - Convert DispatchToOwningThread and DispatchOrAbandon to take already_AddRefed&&.

Bug 1186750 part 7 - Convert DispatchToOwningThread and DispatchOrAbandon to take already_AddRefed&&.

Comment 27

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 8 - Convert DeviceStorageUsedSpaceCache::Dispatch to use already_AddRef&&.

Bug 1186750 part 8 - Convert DeviceStorageUsedSpaceCache::Dispatch to use already_AddRef&&.

Comment 28

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 9 - Use already_AddRefed&& to initialize mFile of device storage requests.

Bug 1186750 part 9 - Use already_AddRefed&& to initialize mFile of device storage requests.

Comment 29

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 10 - Simplify code in DeviceStorageRequestParent::Dispatch.

Bug 1186750 part 10 - Simplify code in DeviceStorageRequestParent::Dispatch.

Comment 30

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1186750 part 11 - Convert all usage of Dispatch/NS_DispatchToMainThread in dom/devicestorage to pass in either already_AddRefed or raw pointer.

Bug 1186750 part 11 - Convert all usage of Dispatch/NS_DispatchToMainThread in dom/devicestorage to pass in either already_AddRefed or raw pointer.

Comment 31

[Xidorn Quan [:xidorn] UTC+11]

Part 10 seems to be identical to the old part 7, so you probably can r+ directly.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0a9cf2fb44a
https://hg.mozilla.org/mozilla-central/rev/ef5f9a2a41a4
https://hg.mozilla.org/mozilla-central/rev/5add786777f3
https://hg.mozilla.org/mozilla-central/rev/fa9922692623

Comment 33

[Dave Hylands [:dhylands]]

Comment on attachment 8689441 [details]
MozReview Request: Bug 1186750 part 5 - Convert nsDOMDeviceStorage::CheckPermission to take already_AddRefed&&.

https://reviewboard.mozilla.org/r/25635/#review23155

Comment 34

[Dave Hylands [:dhylands]]

Comment on attachment 8689442 [details]
MozReview Request: Bug 1186750 part 6 - Remove unused and unimplemented method nsDOMDeviceStorage::StorePermission.

https://reviewboard.mozilla.org/r/25637/#review23157

Comment 35

[Dave Hylands [:dhylands]]

Comment on attachment 8689443 [details]
MozReview Request: Bug 1186750 part 7 - Convert DispatchToOwningThread and DispatchOrAbandon to take already_AddRefed&&.

https://reviewboard.mozilla.org/r/25639/#review23159

Comment 36

[Dave Hylands [:dhylands]]

Comment on attachment 8689444 [details]
MozReview Request: Bug 1186750 part 8 - Convert DeviceStorageUsedSpaceCache::Dispatch to use already_AddRef&&.

https://reviewboard.mozilla.org/r/25641/#review23161

Comment 37

[Dave Hylands [:dhylands]]

Comment on attachment 8689445 [details]
MozReview Request: Bug 1186750 part 9 - Use already_AddRefed&& to initialize mFile of device storage requests.

https://reviewboard.mozilla.org/r/25643/#review23163

Comment 38

[Dave Hylands [:dhylands]]

Comment on attachment 8689446 [details]
MozReview Request: Bug 1186750 part 10 - Simplify code in DeviceStorageRequestParent::Dispatch.

https://reviewboard.mozilla.org/r/25645/#review23165

Comment 39

[Dave Hylands [:dhylands]]

Comment on attachment 8689447 [details]
MozReview Request: Bug 1186750 part 11 - Convert all usage of Dispatch/NS_DispatchToMainThread in dom/devicestorage to pass in either already_AddRefed or raw pointer.

https://reviewboard.mozilla.org/r/25647/#review23153

r=me if you address the XPCOM_NO_SMARTPOINTER_DISPATCH define. I couldn't see any reason for it to be defined.

::: dom/devicestorage/moz.build:52
(Diff revision 1)
> +CXXFLAGS += ['-DXPCOM_NO_SMARTPOINTER_DISPATCH']

What does this do? I couldn't find any references to this in the codebase.

Comment 40

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Dave Hylands [:dhylands] from comment #39)
> Comment on attachment 8689447 [details]
> MozReview Request: Bug 1186750 part 11 - Convert all usage of
> Dispatch/NS_DispatchToMainThread in dom/devicestorage to pass in either
> already_AddRefed or raw pointer.
> 
> https://reviewboard.mozilla.org/r/25647/#review23153
> 
> r=me if you address the XPCOM_NO_SMARTPOINTER_DISPATCH define. I couldn't
> see any reason for it to be defined.
> 
> ::: dom/devicestorage/moz.build:52
> (Diff revision 1)
> > +CXXFLAGS += ['-DXPCOM_NO_SMARTPOINTER_DISPATCH']
> 
> What does this do? I couldn't find any references to this in the codebase.

It is something I want to introduce in bug 1226073. Since that bug gets stuck, I'm going to land this patchset without this line. Thanks for the fast reviewing.

Comment 41

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd192633b5e49db4b88e31ffd11157be6d446ae8
Bug 1186750 part 5 - Convert nsDOMDeviceStorage::CheckPermission to take already_AddRefed&&. r=dhylands

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc63519e3cae242606a1009f707af3d0f6943344
Bug 1186750 part 6 - Remove unused and unimplemented method nsDOMDeviceStorage::StorePermission. r=dhylands

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbd3f6d49362e867b8e9e9bdfea3a18c970d3a6b
Bug 1186750 part 7 - Convert DispatchToOwningThread and DispatchOrAbandon to take already_AddRefed&&. r=dhylands

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb8a91b830ab636683df1aa684f002caef9438e
Bug 1186750 part 8 - Convert DeviceStorageUsedSpaceCache::Dispatch to use already_AddRef&&. r=dhylands

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5aa32cc038f734fe2b41caced968fb15a692405
Bug 1186750 part 9 - Use already_AddRefed&& to initialize mFile of device storage requests. r=dhylands

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fb7145a9eecf6e26a33217fa665ad5b6b330258
Bug 1186750 part 10 - Simplify code in DeviceStorageRequestParent::Dispatch. r=dhylands

https://hg.mozilla.org/integration/mozilla-inbound/rev/41db68696ad0434455ac585e522be98e43a119d3
Bug 1186750 part 11 - Convert all usage of Dispatch/NS_DispatchToMainThread in dom/devicestorage to pass in either already_AddRefed or raw pointer. r=dhylands

Comment 42

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

https://hg.mozilla.org/mozilla-central/rev/bd192633b5e4
https://hg.mozilla.org/mozilla-central/rev/dc63519e3cae
https://hg.mozilla.org/mozilla-central/rev/cbd3f6d49362
https://hg.mozilla.org/mozilla-central/rev/ebb8a91b830a
https://hg.mozilla.org/mozilla-central/rev/c5aa32cc038f
https://hg.mozilla.org/mozilla-central/rev/0fb7145a9eec
https://hg.mozilla.org/mozilla-central/rev/41db68696ad0