Closed Bug 710745 Opened 8 years ago Closed 8 years ago

Don't expose WindowIdentifier in Hal.h

Categories

(Core :: Hardware Abstraction Layer (HAL), defect)

defect
Not set

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
Attached patch Patch v1 (obsolete) — 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)
Does this compile?

How does the DOM code (not currently in the tree) know that WindowIdentifier has an implicit cast from nsIDOMWindow*?
Blocks: 710748
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.
(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?
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.
> The DOM patch doing that has landed?

No; it's gated on the tree reopening.  Bug 679966.
Attachment #581667 - Flags: review?(justin.lebar+bug) → review-
Would it make sense to have WindowIdentifier living in dom/ instead of hal/?
Attached patch Patch v2Splinter Review
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)
>+  /**
>+   * 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?
Attachment #581689 - Flags: review?(justin.lebar+bug) → review+
Attachment #581689 - Flags: superreview?(jones.chris.g) → superreview+
Attachment #581689 - Flags: checkin+
Flags: in-testsuite-
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/a1b13ab3909d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.