Closed
Bug 710745
Opened 12 years ago
Closed 12 years ago
Don't expose WindowIdentifier in Hal.h
Categories
(Core :: Hardware Abstraction Layer (HAL), defect)
Core
Hardware Abstraction Layer (HAL)
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: mounir, Assigned: mounir)
References
Details
Attachments
(1 file, 1 obsolete file)
15.26 KB,
patch
|
justin.lebar+bug
:
review+
cjones
:
superreview+
mounir
:
checkin+
|
Details | Diff | Splinter Review |
This patch should make WindowIdentifier really private and clean up a bit Hal.h.
Attachment #581667 -
Flags: superreview?(jones.chris.g)
Attachment #581667 -
Flags: review?(justin.lebar+bug)
Comment 1•12 years ago
|
||
Does this compile? How does the DOM code (not currently in the tree) know that WindowIdentifier has an implicit cast from nsIDOMWindow*?
Comment 2•12 years ago
|
||
I should say: This will not compile when DOM code tries to call Vibrate. In fact, this will make it impossible for the DOM to call Vibrate without including WindowIdentifier.h.
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #2) > I should say: This will not compile when DOM code tries to call Vibrate. In > fact, this will make it impossible for the DOM to call Vibrate without > including WindowIdentifier.h. The DOM patch doing that has landed?
Comment 4•12 years ago
|
||
I really don't like this WindowIdentifier hack, so I'm in favor of getting rid of it somehow. You could make Hal.h define both Vibrate(nsIDOMWindow* window) and Vibrate(WindowIdentifier identifier) and only define the first one inside hal::. But that's still a hack, and now you're pushing errors into the linking phase, which is a bummer.
Comment 5•12 years ago
|
||
> The DOM patch doing that has landed? No; it's gated on the tree reopening. Bug 679966.
Updated•12 years ago
|
Attachment #581667 -
Flags: review?(justin.lebar+bug) → review-
Assignee | ||
Comment 6•12 years ago
|
||
Would it make sense to have WindowIdentifier living in dom/ instead of hal/?
Assignee | ||
Comment 7•12 years ago
|
||
Outside of hal/ code will be able to call Vibrate/CancelVibrate with nsIDOMWindow* and hal/ code will be able to use nsIDOMWindow* or a WindowIdentifier object. WindowIdentifier object version of the method will not be usable outside of hal/, that will produce a compilation error.
Attachment #581667 -
Attachment is obsolete: true
Attachment #581667 -
Flags: superreview?(jones.chris.g)
Attachment #581689 -
Flags: superreview?(jones.chris.g)
Attachment #581689 -
Flags: review?(justin.lebar+bug)
Comment 8•12 years ago
|
||
>+ /**
>+ * Wrap the given window in a WindowIdentifier. These two
>+ * constructors automatically grab the window's ID and append it to
>+ * the array of IDs.
>+ *
>+ * Note that these constructors allow an implicit conversion to a
>+ * WindowIdentifier.
>+ */
>+ WindowIdentifier(nsIDOMWindow* window);
>+ WindowIdentifier(nsCOMPtr<nsIDOMWindow> &window);
Can you please make the nsIDOMWindow* constructor explicit, and get rid of the nsCOMPtr<nsIDOMWindow> call?
Updated•12 years ago
|
Attachment #581689 -
Flags: review?(justin.lebar+bug) → review+
Updated•12 years ago
|
Attachment #581689 -
Flags: superreview?(jones.chris.g) → superreview+
Assignee | ||
Updated•12 years ago
|
Attachment #581689 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Flags: in-testsuite-
Target Milestone: --- → mozilla11
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a1b13ab3909d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•