Created attachment 581667 [details] [diff] [review] Patch v1 This patch should make WindowIdentifier really private and clean up a bit Hal.h.
Does this compile? How does the DOM code (not currently in the tree) know that WindowIdentifier has an implicit cast from nsIDOMWindow*?
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.
Would it make sense to have WindowIdentifier living in dom/ instead of hal/?
Created attachment 581689 [details] [diff] [review] Patch v2 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.
>+ /** >+ * 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?