Closed
Bug 909645
Opened 11 years ago
Closed 11 years ago
Try to avoid including ipdl-generated headers in DOM headers
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 1 obsolete file)
5.68 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
7.60 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
4.75 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
2.32 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
ipdl headers pull in all sorts of stuff...
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #795837 -
Flags: review?(bugs)
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #795838 -
Flags: review?(bugs)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #795839 -
Flags: review?(bugs)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #795840 -
Flags: review?(bugs)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #795873 -
Flags: review?(bugs)
Assignee | ||
Updated•11 years ago
|
Attachment #795838 -
Attachment is obsolete: true
Attachment #795838 -
Flags: review?(bugs)
Comment 6•11 years ago
|
||
Comment on attachment 795837 [details] [diff] [review] Don't include ipdl headers in nsGeolocation.h. Review of attachment 795837 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: dom/src/geolocation/nsGeolocation.cpp @@ +71,5 @@ > + NS_DECL_NSIGEOLOCATIONUPDATE > + > + NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(nsGeolocationRequest, nsIContentPermissionRequest) > + > + nsGeolocationRequest(mozilla::dom::Geolocation* locator, Nit: feel free to remove the namespace qualifications and/or make arguments use aFoo. @@ +74,5 @@ > + > + nsGeolocationRequest(mozilla::dom::Geolocation* locator, > + const mozilla::dom::GeoPositionCallback& callback, > + const mozilla::dom::GeoPositionErrorCallback& errorCallback, > + mozilla::idl::GeoPositionOptions* aOptions, I'd appreciate a followup to remove idl::GeoPositionOptions
Attachment #795837 -
Flags: review?(bugs) → review+
Comment 7•11 years ago
|
||
(In reply to :Ms2ger from comment #6) > Comment on attachment 795837 [details] [diff] [review] > Don't include ipdl headers in nsGeolocation.h. And please also add a 'Part 1' bit to the commit message
Comment 8•11 years ago
|
||
Comment on attachment 795873 [details] [diff] [review] part 2. Don't include ipdl headers in Hal.h. Review of attachment 795873 [details] [diff] [review]: ----------------------------------------------------------------- r=me
Attachment #795873 -
Flags: review?(bugs) → review+
Comment 9•11 years ago
|
||
Comment on attachment 795839 [details] [diff] [review] part 3. Make including SpeechRecognition.h and MediaManager.h not pull ipdl headers. Review of attachment 795839 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: content/media/webspeech/recognition/SpeechRecognition.cpp @@ +15,5 @@ > #include "endpointer.h" > > #include "GeneratedEvents.h" > #include "nsIDOMSpeechRecognitionEvent.h" > +#include "nsIObserverService.h" Nit: sort those @@ +18,5 @@ > #include "nsIDOMSpeechRecognitionEvent.h" > +#include "nsIObserverService.h" > +#include "mozilla/Services.h" > +#include "nsServiceManagerUtils.h" > +#include "MediaManager.h" mozilla/MediaManager.h ::: dom/media/MediaManager.cpp @@ +1625,5 @@ > + msg.get()); > + // Forward recording events to parent process. > + // The events are gathered in chrome process and used for recording indicator > + if (XRE_GetProcessType() != GeckoProcessType_Default) { > + unused << mozilla::dom::ContentChild::GetSingleton()->SendRecordingDeviceEvents(msg); Nit: drop the mozilla::dom::
Attachment #795839 -
Flags: review?(bugs) → review+
Comment 10•11 years ago
|
||
Comment on attachment 795840 [details] [diff] [review] part 4. Don't include ipdl headers in DesktopNotification.h. Review of attachment 795840 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: dom/src/notification/DesktopNotification.cpp @@ +23,5 @@ > + public nsRunnable, > + public PCOMContentPermissionRequestChild > + > +{ > + public: I don't think we usually half-indent these @@ +44,5 @@ > + ~DesktopNotificationRequest() > + { > + } > + > + virtual bool Recv__delete__(const bool& allow) MOZ_OVERRIDE Please do fix the one-space indentation @@ +47,5 @@ > + > + virtual bool Recv__delete__(const bool& allow) MOZ_OVERRIDE > + { > + if (allow) > + (void) Allow(); Nit: And feel free to fix aArgument / missing braces
Attachment #795840 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 11•11 years ago
|
||
> I'd appreciate a followup to remove idl::GeoPositionOptions Filed bug 909768. > Nit: drop the mozilla::dom:: Dropped the mozilla::, but the dom:: has to stay. Fixed the other nits.
Assignee | ||
Comment 12•11 years ago
|
||
Try run including these patches: https://tbpl.mozilla.org/?tree=Try&rev=9dc8acf1e1f7
Assignee | ||
Comment 13•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/042d50591265 https://hg.mozilla.org/integration/mozilla-inbound/rev/8bfc1b745203 https://hg.mozilla.org/integration/mozilla-inbound/rev/b00996f5cb3b https://hg.mozilla.org/integration/mozilla-inbound/rev/69f4af72765c
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla26
Assignee | ||
Comment 14•11 years ago
|
||
And backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/988f55f8196e because something someone else changed now causes Windows bustage in system headers(!).
Assignee | ||
Comment 15•11 years ago
|
||
Looks like the issue was DictionaryHelpers.h doing "#undef near" and the include order between that and windows.h changing for Navigator.cpp as a result of the DesktopNotification patch. Fixed that by removing the unnecessary undef, and pushed again: https://hg.mozilla.org/integration/mozilla-inbound/rev/4812540ac78a https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3d4e913ebb https://hg.mozilla.org/integration/mozilla-inbound/rev/a1951b745635 https://hg.mozilla.org/integration/mozilla-inbound/rev/e796234016ba
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4812540ac78a https://hg.mozilla.org/mozilla-central/rev/bb3d4e913ebb https://hg.mozilla.org/mozilla-central/rev/a1951b745635 https://hg.mozilla.org/mozilla-central/rev/e796234016ba
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•