Convert Navigator to WebIDL

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
7 years ago
4 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

Trunk
mozilla25
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 9 obsolete attachments)

38.17 KB, patch
smaug
: review+
sicking
: superreview+
Details | Diff | Splinter Review
27.01 KB, patch
smaug
: review+
peterv
: superreview+
Details | Diff | Splinter Review
3.93 KB, patch
khuey
: review+
Details | Diff | Splinter Review
12.43 KB, patch
bent.mozilla
: review+
sicking
: superreview+
Details | Diff | Splinter Review
13.87 KB, patch
smaug
: review+
sicking
: superreview+
Details | Diff | Splinter Review
15.77 KB, patch
smaug
: review+
sicking
: superreview+
Details | Diff | Splinter Review
11.65 KB, patch
smaug
: review+
sicking
: superreview+
Details | Diff | Splinter Review
10.49 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.49 KB, patch
smaug
: review+
Details | Diff | Splinter Review
18.50 KB, patch
bholley
: review+
Details | Diff | Splinter Review
I can't think of a reason not to, and it would make a lot of the conditional stuff it does right now simpler, I think.
How about the LookupNavigatorName stuff?
Hmm.  Could those not become prefcontrolled normal properties on navigator?
I'm not sure what you guys are referring to; perhaps you meant to cc someone else?
I'm not sure who the right person to cc is.

What we're talking about are all the things in http://mxr.mozilla.org/mozilla-central/search?string=JavaScript-navigator-property&find=manifest and how we determine whether they're exposed to web content or not.
Ah, I see.  Thanks for clarifying.

I'm still not sure I have something interesting to contribute here; I don't see why we couldn't control those properties by prefs.
That counts as an interesting contribution in my book.  If there's no obvious reason it can't work, then we should just try it.  ;)
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
I'm going to poke at this....
Assignee: nobody → bzbarsky
Depends on: 855611
Depends on: 885171
Comment on attachment 765171 [details] [diff] [review]
Make Navigator wrappercached and cycle-collected, and have it hold a strong reference to its window always.

This is for bug 885171.
Attachment #765171 - Attachment is obsolete: true
Attachment #765171 - Flags: review?(bugs)
Blocks: 884935
Blocks: 884921
Blocks: 884925
Attachment #765763 - Attachment is obsolete: true
Attachment #770595 - Attachment is obsolete: true
Attachment #770595 - Flags: review?(bugs)
Attachment #771036 - Flags: review?(khuey)
Attachment #771019 - Attachment is obsolete: true
Attachment #771019 - Flags: review?(khuey)
Comment on attachment 770593 [details] [diff] [review]
part 1.  Implement WebIDL API on Navigator for the parts that are specified or are in nsIDOMNavigator.


>@@ -714,53 +756,79 @@ Navigator::AddIdleObserver(nsIIdleObserv
>   }
> 
>   NS_ENSURE_ARG_POINTER(aIdleObserver);
> 
>   if (!CheckPermission("idle")) {
>     return NS_ERROR_DOM_SECURITY_ERR;
>   }
> 
>-  if (NS_FAILED(mWindow->RegisterIdleObserver(aIdleObserver))) {
>+  AddIdleObserver(*aIdleObserver);
>+  return NS_OK;
>+}
>+
>+void
>+Navigator::AddIdleObserver(nsIIdleObserver& aIdleObserver)
>+{
>+  if (NS_FAILED(mWindow->RegisterIdleObserver(&aIdleObserver))) {
>     NS_WARNING("Failed to add idle observer.");
>   }
>+}
> 
>-  return NS_OK;
>+void
>+Navigator::AddIdleObserver(MozIdleObserver& aIdleObserver, ErrorResult& aRv)
>+{
>+  if (!mWindow) {
>+    aRv.Throw(NS_ERROR_UNEXPECTED);
>+    return;
>+  }
>+  CallbackObjectHolder<MozIdleObserver, nsIIdleObserver> holder(&aIdleObserver);
>+  nsCOMPtr<nsIIdleObserver> obs = holder.ToXPCOMCallback();
>+  return AddIdleObserver(*obs);
> }

I think HasIdleSupport should be somewhere close here. Same thing with other Has*
And perhaps also some comment that it is used for the webidl case.
The setup is a bit error prone. C++ caller gets different results depending on the
which version of the method it is calling. Could we at least add
MOZ_ASSERT to the methods for webidl that permission is enabled.

Also, some b2g dev should comment whether the permission may change



>+  if (aPattern.Length() > sMaxVibrateListLen) {
>+    // XXXbz this should be returning false instead
>+    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
>+    return;
>+  }
>+
>+  for (size_t i = 0; i < aPattern.Length(); ++i) {
>+    if (aPattern[i] > sMaxVibrateMS) {
>+      // XXXbz this should be returning false instead
>+      aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
>+      return;
>+    }
>+  }
Could you say in XXX something like: per spec this should be returning false instead.
Perhaps add even the bug number 884935

>+'Navigator': {
>+    'register': False,
>+},
Er, why false? What is still missing? ...or perhaps some other patch enables it.

>+callback interface MozIdleObserver {
>+  // Time is in seconds and is read only when idle observers are added
>+  // and removed.
>+  readonly attribute unsigned long time;
>+  void onidle();
>+  void onactive();
>+};
ugh. UGH.
Attachment #770593 - Flags: review?(bugs) → review+
Comment on attachment 771020 [details] [diff] [review]
Part 2 updated to remove some cruft

In this patch too, the methods for WebIDL should have MOZ_ASSERT for
permission. Or at least NS_WARNING


>+Navigator::MozSetMessageHandler(const nsAString& aType,
>+                                systemMessageCallback* aCallback,
oddly named type. Should be capital S


>+// nsIDOMNavigatorDeviceStorage
>+partial interface Navigator {
>+  // XXXbz can this be hidden outside "certified apps"?
>+  // Or just controlled by "device.storage.enabled"?  sicking says yes.
Sicking says yes to what? to the first question? Perhaps ask dougt too, and if the answer to the
first one is yes, this could be changed. But I guess Pref is ok for now.
Attachment #771020 - Flags: review?(bugs) → review+
> Could we at least add MOZ_ASSERT to the methods for webidl that permission is enabled.

That would actually not be a valid assert.  In the WebIDL setup if you grab the method from a window where the permission _is_ enabled and .call() it on an object that comes from a window where the permsission is _not_ enabled, it will actually work. I think this is perfectly ok, frankly.

> Also, some b2g dev should comment whether the permission may change

I was told no.

> Er, why false? What is still missing?

Well, part 2, and all the ifdeffed stuff and the resolve hook and actually using SetIsDOMBinding().  ;)  The register:False will go away in the same patch that adds the SetIsDOMBinding() call, when all the rest of this stuff is done.

> ugh. UGH.

Yeah, indeed.  Maybe we should file a bug on that API.  :(

> oddly named type.

That's what the spec, such as it is, calls it.  I agree the spec is a bit odd; take that up with Mounir?

> Sicking says yes to what?

Er, sicking says yes to just having the property not be there based on the conditions that would cause it to fail in today's world (which is when the pref is unset).  I removed that comment; it was a note to myself to make sure to check how stuff should work here....
Blocks: 859616
Attachment #771036 - Attachment is obsolete: true
Attachment #771036 - Flags: review?(khuey)
> I think HasIdleSupport should be somewhere close here. Same thing with other Has*

Hmm.  Just in the .cpp or in the header too?  I was trying to put actual implementations of WebIDL stuff together and the miscellaneous helpers separately from those....
Flags: needinfo?(bugs)
Or just add a comment to the methods which webidl uses that they rely on the pref check in webidl.
Flags: needinfo?(bugs)
Blocks: 874923
Attachment #773276 - Attachment is obsolete: true
Attachment #773276 - Flags: review?(bugs)
Attachment #773358 - Attachment is obsolete: true
Attachment #773358 - Flags: review?(bugs)
Attachment #773272 - Flags: review?(bugs) → review+
Attachment #773273 - Flags: review?(bugs) → review+
Attachment #773274 - Flags: review?(bugs) → review+
Attachment #773272 - Flags: superreview?(jonas)
Attachment #773273 - Flags: superreview?(jonas)
Attachment #773274 - Flags: superreview?(jonas)
Attachment #771020 - Flags: superreview?(jonas)
Attachment #770593 - Flags: superreview?(jonas)
Attachment #772243 - Flags: superreview?(jonas)
Comment on attachment 773275 [details] [diff] [review]
part 8.  Switch the Navigator resolve hook over to a more WebIDL-like API.

>+Navigator::DoNewResolve(JSContext* aCx, JS::Handle<JSObject*> aObject,
>+                        JS::Handle<jsid> aId, unsigned aFlags,
>+                        JS::MutableHandle<JSObject*> aObjp)
>+{
>+  if (!JSID_IS_STRING(aId)) {
>+    return true;
>+  }
>+
>+  nsScriptNameSpaceManager *nameSpaceManager =
nsScriptNameSpaceManager* nameSpaceManager


So this was mostly just moving code.
Attachment #773275 - Flags: review?(bugs) → review+
> So this was mostly just moving code.

Yes, exactly.
Comment on attachment 770593 [details] [diff] [review]
part 1.  Implement WebIDL API on Navigator for the parts that are specified or are in nsIDOMNavigator.

Review of attachment 770593 [details] [diff] [review]:
-----------------------------------------------------------------

sr=me on Navigator.webidl
Attachment #770593 - Flags: superreview?(jonas) → superreview+
Comment on attachment 773472 [details] [diff] [review]
Part 9 with more test changes

>+JSObject*
>+Navigator::WrapObject(JSContext* cx, JS::Handle<JSObject*> scope)
aCx, aScope

>+  virtual JSObject* WrapObject(JSContext* cx,
>+                               JS::Handle<JSObject*> scope) MOZ_OVERRIDE;
ditto



r+ assuming jonas is ok with the permission handling changes (which are in the most of the patches).
Attachment #773472 - Flags: review?(bugs) → review+
Attachment #772243 - Flags: superreview?(jonas) → superreview+
Comment on attachment 773272 [details] [diff] [review]
part 5.  Implement remaining MOZ_B2G_RIL-conditional WebIDL APIs on Navigator.

Review of attachment 773272 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Navigator.cpp
@@ +2058,5 @@
> +                                      JSObject* aGlobal)
> +{
> +  nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
> +  return win && CheckPermission(win, "mobileconnection") &&
> +    CheckPermission(win, "mobilenetwork");

This should be

win && (CheckPermission("mobileconnection") || CheckPermission("mobilenetwork))
Attachment #773272 - Flags: superreview?(jonas) → superreview+
> win && (CheckPermission("mobileconnection") || CheckPermission("mobilenetwork))

Oh, good catch!  It worries me a bit that this passed tests.  ;)
Attachment #773273 - Flags: superreview?(jonas) → superreview+
Attachment #773274 - Flags: superreview?(jonas) → superreview+
> Oh, good catch!  It worries me a bit that this passed tests.  ;)

Good news: this does in fact cause one test failure on B2G, in test_mobileconnection.html.
No longer blocks: 859616
Duplicate of this bug: 859616
Comment on attachment 771020 [details] [diff] [review]
Part 2 updated to remove some cruft

Review of attachment 771020 [details] [diff] [review]:
-----------------------------------------------------------------

Getting deviceStorage is definitely a privileged thing. Ping me on irc if you need help with finding the security checks.
Attachment #771020 - Flags: superreview?(jonas) → superreview-
> Getting deviceStorage is definitely a privileged thing.

In a vanilla trunk build on Mac, if I set the "device.storage.enabled" preference to true, navigator.getDeviceStorage("videos") in a web page returns a DeviceStorage object.
Comment on attachment 771020 [details] [diff] [review]
Part 2 updated to remove some cruft

Per comment 39....
Attachment #771020 - Flags: superreview- → superreview?(jonas)
Comment on attachment 772243 [details] [diff] [review]
part 4.  Implement WebIDL API on Navigator for the WebTelephony.

Review of attachment 772243 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine but I think it should change now that we decided not to do the tri-state (present, null, or valid value):

::: dom/base/Navigator.cpp
@@ +1443,2 @@
>      NS_ENSURE_STATE(mWindow);
> +    if (!telephony::Telephony::CheckPermission(mWindow)) {

Now that we don't define the property unless permission is granted is there any reason not to just assert this?

@@ +1452,5 @@
> +  return rv.ErrorCode();
> +}
> +
> +nsIDOMTelephony*
> +Navigator::GetMozTelephony(ErrorResult& aRv)

Hm, I don't see the benefit of this second method. Can't it just be combined into the other one?

@@ +1462,5 @@
> +      aRv.Throw(NS_ERROR_UNEXPECTED);
> +      return nullptr;
> +    }
> +    mTelephony = telephony::Telephony::Create(mWindow, aRv);
> +    // Note: mTelephony can be null here, even if aRv is success

This shouldn't be true any longer.

::: dom/telephony/Telephony.cpp
@@ +142,4 @@
>    telephony->mListener = new Listener(telephony);
>  
> +  // XXXbz Should failures in EnumerateCalls and RegisterTelephonyMsg
> +  // become exceptions instead of a null return?

Yes, definitely, and same for the other things that can fail above and below. We should communicate the error result if we can.

::: dom/telephony/Telephony.h
@@ +9,5 @@
>  
>  #include "TelephonyCommon.h"
> +// Need to include TelephonyCall.h because we have inline methods that
> +// assume they see the definition of TelephonyCall.
> +#include "TelephonyCall.h"

Wait, really? How is this not failing to compile right now?

@@ +58,5 @@
>                                                     Telephony,
>                                                     nsDOMEventTargetHelper)
>  
>    static already_AddRefed<Telephony>
> +    Create(nsPIDOMWindow* aOwner, ErrorResult& aRv);

Nit, no indent here. And add a newline after :)
Attachment #772243 - Flags: review?(bent.mozilla) → review+
> Now that we don't define the property unless permission is granted is there any reason
> not to just assert this?

Yes.  We don't define the _webidl_ property, but the check you're looking at is in the _xpidl_ method.  I'll remove all this xpidl stuff in a followup, but wanted to make it easier to revert this change for now as needed.

> Hm, I don't see the benefit of this second method.

This is the one the WebIDL bindings call.

> This shouldn't be true any longer.

Given your comments about Create(), agreed.

> Yes, definitely, and same for the other things that can fail above and below.

Done.

> Wait, really? How is this not failing to compile right now?

The only places that include Telephony.h right now are TelephonyCall.cpp, Telephony.cpp, and nsDOMClassInfo.cpp.  All three also include TelephonyCall.h.

> Nit, no indent here. And add a newline after :)

Done.

Thank you for the review!
Comment on attachment 771020 [details] [diff] [review]
Part 2 updated to remove some cruft

sr+ing to unblock this. The patch maintains the status quo, we can come up with a better solution in a followup which bz will file.
Attachment #771020 - Flags: superreview?(jonas) → superreview+
Flags: in-testsuite+
Target Milestone: --- → mozilla25
Ugh.  There are orange Mn on my try push now too that weren't there when I last looked at it.  :(

I guess I need to fix those tests as well, because they do dynamic permissions stuff.
(In reply to Boris Zbarsky (:bz) from comment #46)
> Ugh.  There are orange Mn on my try push now too that weren't there when I
> last looked at it.  :(

As a quick aside, can you please post a link to your last Try run when you push to inbound? It probably would have saved us a lot of time trying to track this down yesterday. Especially since B2G full-stack builds + tests have a roughly 2.5 hour turnaround time.
Blocks: 888592
Attachment #775871 - Flags: review?(bobbyholley+bmo)
Attachment #775932 - Flags: review?(bobbyholley+bmo)
Attachment #775871 - Attachment is obsolete: true
Attachment #775871 - Flags: review?(bobbyholley+bmo)
Attachment #775932 - Attachment is obsolete: true
Attachment #775932 - Flags: review?(bobbyholley+bmo)
Comment on attachment 775951 [details] [diff] [review]
Xray+newresolve, without a boolean outparam

Review of attachment 775951 [details] [diff] [review]:
-----------------------------------------------------------------

I only superficial follow some of the mechanical Codegen changes, but it all looks reasonable and you know what you're doing. r=bholley

::: dom/base/nsDOMClassInfo.cpp
@@ +4660,5 @@
>  {
>    JS::Rooted<JSObject*> obj(cx, aObj);
>    JS::Rooted<jsid> id(cx, aId);
>    nsCOMPtr<nsIDOMNavigator> navigator = do_QueryWrappedNative(wrapper);
> +  JS::Rooted<JS::Value> value(cx);

Given the calling convention here, I think that per-EIBTI we should explicitly pass UndefinedValue() here, though I could be convinced otherwise.

::: dom/bindings/Codegen.py
@@ +85,5 @@
>          if self.descriptor.concrete and self.descriptor.proxy:
>              resolveOwnProperty = "ResolveOwnProperty"
>              enumerateOwnProperties = "EnumerateOwnProperties"
> +        elif self.descriptor.interface.getExtendedAttribute("NeedNewResolve"):
> +            resolveOwnProperty = "ResolveOwnPropertyViaNewresolve"

Would we ever have both a NewResolve hook _and_ other own properties that might be resolved some other way? Or are they orthogonal (proxies vs non-proxies)? If they might overlap, this should probably be called 'ResolveOwnPropertyWithNewResolve' or something.

@@ +6678,5 @@
> +                "if (!self->DoNewResolve(cx, obj, id, &value)) {\n"
> +                "  return false;\n"
> +                "}\n"
> +                "if (!value.isUndefined()) {\n"
> +                "  FillPropertyDescriptor(desc, wrapper, value, false);\n"

Please add /* readOnly = */ before the |false|.
Attachment #775951 - Flags: review?(bobbyholley+bmo) → review+
I can't think of any case where something would both have a named getter and a resolve hook except for Window, and Window is all sorts of special snowflake anyway.

Will fix the other bits.  Thanks for the review!
https://hg.mozilla.org/mozilla-central/rev/7c7ef42d1fe1 caused a regression.  general.appname.override no longer is honored due to the removal of the return:

   1.714 -nsresult
   1.715 +void
   1.716  NS_GetNavigatorAppName(nsAString& aAppName)
   1.717  {
   1.718    if (!nsContentUtils::IsCallerChrome()) {
   1.719      const nsAdoptingString& override =
   1.720        mozilla::Preferences::GetString("general.appname.override");
   1.721  
   1.722      if (override) {
   1.723        aAppName = override;
   1.724 -      return NS_OK;
   1.725      }
   1.726    }
   1.727  
   1.728    aAppName.AssignLiteral("Netscape");
   1.729 -  return NS_OK;
   1.730  }
Adam, please file a new bug and make it block this one.
Depends on: 939445
Duplicate of this bug: 815129
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.