Last Comment Bug 710745 - Don't expose WindowIdentifier in Hal.h
: Don't expose WindowIdentifier in Hal.h
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Hardware Abstraction Layer (HAL) (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks: 710748
  Show dependency treegraph
 
Reported: 2011-12-14 09:15 PST by Mounir Lamouri (:mounir)
Modified: 2011-12-18 15:37 PST (History)
3 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (9.86 KB, patch)
2011-12-14 09:15 PST, Mounir Lamouri (:mounir)
justin.lebar+bug: review-
Details | Diff | Splinter Review
Patch v2 (15.26 KB, patch)
2011-12-14 09:55 PST, Mounir Lamouri (:mounir)
justin.lebar+bug: review+
cjones.bugs: superreview+
mounir: checkin+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-12-14 09:15:24 PST
Created attachment 581667 [details] [diff] [review]
Patch v1

This patch should make WindowIdentifier really private and clean up a bit Hal.h.
Comment 1 Justin Lebar (not reading bugmail) 2011-12-14 09:24:28 PST
Does this compile?

How does the DOM code (not currently in the tree) know that WindowIdentifier has an implicit cast from nsIDOMWindow*?
Comment 2 Justin Lebar (not reading bugmail) 2011-12-14 09:25:15 PST
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.
Comment 3 Mounir Lamouri (:mounir) 2011-12-14 09:26:46 PST
(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 Justin Lebar (not reading bugmail) 2011-12-14 09:27:19 PST
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 Justin Lebar (not reading bugmail) 2011-12-14 09:27:48 PST
> The DOM patch doing that has landed?

No; it's gated on the tree reopening.  Bug 679966.
Comment 6 Mounir Lamouri (:mounir) 2011-12-14 09:29:25 PST
Would it make sense to have WindowIdentifier living in dom/ instead of hal/?
Comment 7 Mounir Lamouri (:mounir) 2011-12-14 09:55:36 PST
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.
Comment 8 Justin Lebar (not reading bugmail) 2011-12-14 10:16:55 PST
>+  /**
>+   * 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?
Comment 9 Matt Brubeck (:mbrubeck) 2011-12-18 15:37:26 PST
https://hg.mozilla.org/mozilla-central/rev/a1b13ab3909d

Note You need to log in before you can comment on or make changes to this bug.