The default bug view has changed. See this FAQ.

Don't expose WindowIdentifier in Hal.h

RESOLVED FIXED in mozilla11

Status

()

Core
Hardware Abstraction Layer (HAL)
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla11
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

15.26 KB, patch
Justin Lebar (not reading bugmail)
: review+
cjones
: superreview+
mounir
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 581667 [details] [diff] [review]
Patch v1

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*?
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 3

5 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?
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-
(Assignee)

Comment 6

5 years ago
Would it make sense to have WindowIdentifier living in dom/ instead of hal/?
(Assignee)

Comment 7

5 years ago
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.
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+
(Assignee)

Updated

5 years ago
Attachment #581689 - Flags: checkin+
(Assignee)

Updated

5 years ago
Flags: in-testsuite-
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/a1b13ab3909d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.