897913 - Turn on Promise for b2g

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Comment 2

[Gene Lian (I already quit Mozilla)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=5711fe760e18

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 780897 [details] [diff] [review]
Patch

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

lgtm

Comment 4

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V1.1

Add the following fix with a proper default:

Promise::PrefEnabled()
{
  return Preferences::GetBool("dom.promise.enabled", true);
}

Try: https://tbpl.mozilla.org/?tree=Try&rev=c639ed2bf886

Comment 5

[Gene Lian (I already quit Mozilla)]

Comment on attachment 780909 [details] [diff] [review]
Patch, V1.1

Per discussion with Baku, we don't need comment #4. Set it to false if the entry is not available.

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 780897 [details] [diff] [review]
Patch

Promises are not enabled on purpose. The specification is not finalised yet so we should not ship it for the moment. It has landed in mozilla-central for testing purposes but not for usage by real content.

Gene, why do you want to enable this by default?

Comment 7

[Gene Lian (I already quit Mozilla)]

(In reply to Mounir Lamouri (:mounir) from comment #6)
> Comment on attachment 780897 [details] [diff] [review]
> Patch
> 
> Promises are not enabled on purpose. The specification is not finalised yet
> so we should not ship it for the moment. It has landed in mozilla-central
> for testing purposes but not for usage by real content.
> 
> Gene, why do you want to enable this by default?

Because Inter-App Communication and Data Store API need it. Or we should just enable it for B2G only for this moment?

Comment 8

[Gene Lian (I already quit Mozilla)]

Or we should still use DOMRequest for this moment?

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we can enable Promises on B2G. Especially since what we have implemented is a pretty safe subset.

If we wanted to be extra conservative we could disable the Promise constructors (including the static Promise.resolve and Promise.reject) functions. Those aren't needed for API implementations anyway.

Comment 10

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V2

Comment 11

[Gene Lian (I already quit Mozilla)]

Oops... Try server doesn't pass test_promise.html:

SpecialPowers.setBoolPref("dom.promise.enabled", false);
ok(!("Promise" in window), "Promise object should not exist if disabled by pref");

Weird... Will figure it out next Monday.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Weird... Will figure it out next Monday.

Changing the pref will affect window objects created after that point, but not existing window objects, of course.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 781612 [details] [diff] [review]
Patch, V2

I think we should in fact disable the static methods....

Comment 14

[Gene Lian (I already quit Mozilla)]

OK. I see. Thank you guys for all the suggestions. I'll come back with a new patch.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So the plan is to turn off just the static functions, but leave the constructor and the .then/.catch functions?

Comment 16

[Andrea Marchesini [:baku]]

(In reply to Jonas Sicking (:sicking) from comment #15)
> So the plan is to turn off just the static functions, but leave the
> constructor and the .then/.catch functions?

Currently our Promise implementation supports Promise.resolve(), Promise.reject() as static methods. We don't support Promise.some/any/every yet.

Comment 17

[Boris Zbarsky [:bzbarsky]]

We can make window.Promise hidden too.

Comment 18

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #15)
> So the plan is to turn off just the static functions, but leave the
> constructor and the .then/.catch functions?

I think we should turn-off the ctor unless we have a strong reason to not do so.

Comment 19

[Andrea Marchesini [:baku]]

it seems strange to me that we have API that are able to create promise, but content code cannot. If we turn-off the ctor we don't allow to create chain of promises, except with then/catch. What is the reason to turn-off the ctor?

Comment 20

[Jan Jongboom [:janjongboom] (inactive)]

Does anyone know how I could reference a Promise in my interface definition in b2g.idl ?

Comment 21

[Boris Zbarsky [:bzbarsky]]

1) Don't write .idl
2) If you have to, use nsISupports for a promise, I guess.

Comment 22

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3

This is just a WIP. It seems the final decision is:

* For B2G
  1. window.Promise is available.
  2. Turn on Promise(), then() and catch().
  3. Turn off other static methods.

* For non-B2G
  1. window.Promise is NOT available, thus we cannot use Promise in any way, including the constructor.

Is that correct?

Comment 23

[Gene Lian (I already quit Mozilla)]

(In reply to Andrea Marchesini (:baku) from comment #19)
> it seems strange to me that we have API that are able to create promise, but
> content code cannot. If we turn-off the ctor we don't allow to create chain
> of promises, except with then/catch. What is the reason to turn-off the ctor?

Agree. If we turn off the Promise constructor, how can we write something like:

function API() {
  return new this._window.Promise(...);
}

in the Gecko codes of content process (note that I don't mean content codes)?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 783151 [details] [diff] [review]
Patch, V3

>  1. window.Promise is available.

Why?  I would prefer we not expose it to web pages even on b2g.  Certified apps or chrome or whatnot is a separate issue.

And "not available" should mean "not defined", not "will mess up you object detection but throw if you call it", for both window.promise and the static methods.  See https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#.5BFunc.3D.22funcname.22.5D and https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#PrefControlled

Note that given the IDL and the way PrefControlled works the changes to Constructor here make no sense.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> how can we write something like:

Is the code in question running as chrome?  Then you can do it pretty easily by making the ctor [ChromeOnly].  I can make [Func] work on interfaces too, if we want to enable if either chrome or pref is set.

Comment 26

[Gene Lian (I already quit Mozilla)]

Understand! Thanks! Will come back with new patch.

Comment 28

[Boris Zbarsky [:bzbarsky]]

In fact, I suspect that making Func work on interfaces is the right thing here. Please let me know if you want me to do that.

Comment 29

[Gene Lian (I already quit Mozilla)]

(In reply to Boris Zbarsky (:bz) from comment #28)
> In fact, I suspect that making Func work on interfaces is the right thing
> here. Please let me know if you want me to do that.

Yeap, thank you very much! I think I should assign this to the expert to save time without too many discussions back and forth. The objectives for the B2G API purposes are:

1. For non-B2G build:
   a. Web pages cannot see window.Promise and cannot use constructor.
   b. Web pages cannot use Promise.resolve() and Promise.reject().
   c. Web pages cannot use promiseObj.then() and promiseObj.catch().

2. For B2G build:
   a. Chrome codes can see window.Promise and can use the constructor.
   b. Web pages cannot see window.Promise and cannot use constructor.
   c. Web pages cannot use Promise.resolve() and Promise.reject().
   d. Web pages can use promiseObj.then() and promiseObj.catch().

Note: #2-a implies the API implementation can return a Promise object to the web page and #2-d implies web page can use then() to run the callback based on the Promise object returned from API.

Please feel free to let me know if you want me to finish it or further fix the test cases. Note that there could be a tricky behaviour stopping B2G passing the tests [1]:

  "Note that on b2g pref setting happens asynchronously. Use PushPrefEnv instead."

[1] https://developer.mozilla.org/en-US/docs/Mochitest#What_if_I_need_to_change_a_preference_to_run_my_test.3F

Comment 30

[Boris Zbarsky [:bzbarsky]]

Hmm.  So here's an interesting question: do you need contentWindow.Promise to exist when touching the window from chrome code?  Or is it enough if chromeWindow.Promise exists?

Also, I see no reason not to use the "For B2G build" setup described in comment 29 for non-b2g builds as well, right?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Oh, hmm.  And I guess if we need to create the Promise objects in the content scope that would also immediately resolve window.Promise.  Hrm...

Comment 32

[Gene Lian (I already quit Mozilla)]

(In reply to Boris Zbarsky (:bz) from comment #30)
> Hmm.  So here's an interesting question: do you need contentWindow.Promise
> to exist when touching the window from chrome code?  Or is it enough if
> chromeWindow.Promise exists?

Oh, I mean content window. For your reference, please search |Window.Promise| for its usage context. Is that feasible to new window.Promise only for chrome codes but not for web pages?

[1] DataStore API: https://bugzilla.mozilla.org/attachment.cgi?id=776987&action=diff
[2] Inter-App Comm API: https://bugzilla.mozilla.org/attachment.cgi?id=783131&action=diff

> 
> Also, I see no reason not to use the "For B2G build" setup described in
> comment 29 for non-b2g builds as well, right?

One difference is between #1-c and #2-d. It depends on whether the web pages can use .then()/.catch() on the non-b2g builds. Honestly, I don't have strong opinion about that but it seems OK to expose them because these private methods look relatively safe according to previous discussions.

Also, we don't allow chrome code to use window.Promise in non-b2g build (#2-a). Right?

Comment 33

[Boris Zbarsky [:bzbarsky]]

> Is that feasible to new window.Promise only for chrome codes but not for web pages?

That's actually pretty difficult.  Getting contentWindow.Promise on an Xray would normally set up window.Promise in the content window and then give chrome an Xray to the constructor object.

So we can make |new window.Promise| throw in content, but we can't easily make |"Promise" in window| false in content while allowing contentWindow.Promise from chrome.

Let me think about whether there's a way to do this sanely.

> One difference is between #1-c and #2-d. 

I see no reason non-b2g builds can't have the #2-d behavior.  It'll only matter if some chrome code gives content a Promise object.

> Also, we don't allow chrome code to use window.Promise in non-b2g build (#2-a). Right?

I want to allow non-b2g chrome code to use Promise; I see no reason to disallow it.

Comment 34

[[:fabrice] Fabrice Desré]

Why don't we want content code to create promises?

Comment 35

[Boris Zbarsky [:bzbarsky]]

See comment 6.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not really convinced that it's worth hiding the Promise constructor. My understanding is that the behavior of the constructor is not one of the controversial issues that still remain.

Comment 37

[Boris Zbarsky [:bzbarsky]]

The big problem from my point of view is that it's reasonable for pages to check for window.Promise and then assume the DOM Promises spec is implemented.  So exposing the constructor without exposing most of the rest of the spec is not so great.  :(

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That is indeed a good point

Comment 39

[Jan Jongboom [:janjongboom] (inactive)]

Can't we add the whole implementation with a prefix?

Comment 40

[Masatoshi Kimura [:emk]]

We will not add any new prefixed interfaces. (Except on b2g?)

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Given that constructable promises is strictly a convenience function, I think we can do without it for now. For 1.3 the spec should have stabilized enough that we can ship it as well as use it in our production code.

Comment 42

[Boris Zbarsky [:bzbarsky]]

So I think I have an idea for how we can expose contentWindow.Promise to chrome without exposing it to content.  Will update later today.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Don't assert that app id exists when asked for app status; just claim not installed if there is no app id.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Allow touching interface objects via an Xray even if the page they're in can't touch them.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Enable Promise in chrome and certified apps, even when preffed off.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Gene, I'd appreciate if it if you have time to put together some tests.....

Comment 47

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 785912 [details] [diff] [review]
part 1.  Don't assert that app id exists when asked for app status; just claim not installed if there is no app id.

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

I'd rather not do this change as part of this bug. It's certainly something that we can discuss, but I'd rather not hold up this bug for it.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 785912 [details] [diff] [review]
part 1.  Don't assert that app id exists when asked for app status; just claim not installed if there is no app id.

Well, it's either this or write much more complicated code for other parts of this bug, as far as I can tell.  If you insist, I can do the latter, but this seems like the quickest way to get things done to me.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Note that if you're just not convinced the change should be made, I can write the more complicated code, of course...

Comment 50

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 785912 [details] [diff] [review]
part 1.  Don't assert that app id exists when asked for app status; just claim not installed if there is no app id.

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

Ok, I'd like Mounir's review then. Mounir, given that so many callsites are working around these asserts by checking the app-id first. It's questionable if they are gaining us anything. In particular, it seems that we still end up with Documents that have UNKNONW_APP_ID principals, and as long as that's the case I don't think that asserts are actually getting us anything.

Comment 51

[Boris Zbarsky [:bzbarsky]]

It's entirely possible that the callsites checking the app-id are just cargo-culting... as in, the app-id checks could just be removed anyway.

Comment 52

[Gene Lian (I already quit Mozilla)]

(In reply to Boris Zbarsky (:bz) from comment #46)
> Gene, I'd appreciate if you have time to put together some tests.....

Sure. No problem!

Comment 53

[Bobby Holley (:bholley)]

Comment on attachment 785915 [details] [diff] [review]
part 3.  Enable Promise in chrome and certified apps, even when preffed off.

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

::: dom/promise/Promise.cpp
@@ +126,5 @@
> +  // aCx here will be in the Xray compartment but the object we're passed is in
> +  // the content compartment!  We don't want to ever use the object in this
> +  // method, because we want to base everything on the Xray compartment.
> +  bool isChrome = NS_IsMainThread() ?
> +    xpc::AccessCheck::isChrome(js::GetContextCompartment(aCx)) :

I'm not happy about the AccessCheck include here. Do you have any reason to believe that aCx will not be stack-top here? If so, that's a serious bug in our bindings. Please just use nsContentUtils::GetSubjectPrincipal here and remove the AccessCheck stuff.

@@ +133,5 @@
> +    return true;
> +  }
> +
> +  // XXXbz Would be nice to have soemthing like GetObjectPrincipal but for
> +  // JSContext.

Well, in general there should never be a reason to inspect the context principal if it's not stack-top outside of XPConnect...

::: dom/webidl/Promise.webidl
@@ +24,5 @@
> +  // Belt-and-suspenders: disable the static methods when the interface object
> +  // is supposed to be disabled, just in case someone screws up and manages to
> +  // create a Promise object in this scope without having resolved the interface
> +  // object first... or in case some code decides to walk over to .constructor
> +  // from the proto of a promise object.

The latter seems pretty clearly non-belt-and-suspenders.

Comment 54

[Bobby Holley (:bholley)]

Comment on attachment 785913 [details] [diff] [review]
part 2.  Allow touching interface objects via an Xray even if the page they're in can't touch them.

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

As usual, I didn't review the Codegen.py changes very closely.

::: dom/base/nsDOMClassInfo.cpp
@@ +3539,5 @@
>        Maybe<JSAutoCompartment> ac;
>        JS::Rooted<JSObject*> global(cx);
>        bool defineOnXray = xpc::WrapperFactory::IsXrayWrapper(obj);
>        if (defineOnXray) {
> +        // Check whether to define this property on the Xray first

Nit - period. And elaborate a bit about how this callback is special.

@@ +3554,5 @@
>        } else {
>          global = obj;
>        }
>  
> +      // Check whether to define on the global too.

Note in the comments how we've (quite un-idomatically) entered a different compartment by this point.

::: dom/bindings/BindingUtils.h
@@ +379,5 @@
>                         unsigned ctorNargs, const NamedConstructor* namedConstructors,
>                         JS::Heap<JSObject*>* constructorCache, const DOMClass* domClass,
>                         const NativeProperties* regularProperties,
>                         const NativeProperties* chromeOnlyProperties,
> +                       const char* name, bool defineOnGlobal);

I think we need some comments somewhere explaining why we might not want to define it on the global.

Comment 55

[Boris Zbarsky [:bzbarsky]]

> Do you have any reason to believe that aCx will not be stack-top here? 

Hmm.  Not offhand, no.

> Please just use nsContentUtils::GetSubjectPrincipal here

Does that work correctly on workers?

> As usual, I didn't review the Codegen.py changes very closely.

OK.  In this case those are pretty finicky, so I'll try another reviewer for those.

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 785913 [details] [diff] [review]
part 2.  Allow touching interface objects via an Xray even if the page they're in can't touch them.

Olli, I know you hate it, but I need review on the codegen bits here.  :(

Comment 57

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #55)
> > Please just use nsContentUtils::GetSubjectPrincipal here
> 
> Does that work correctly on workers?

No, it will abort - I meant in the NS_IsMainThread branch.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3 updated to comments

Comment 59

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 785912 [details] [diff] [review]
part 1.  Don't assert that app id exists when asked for app status; just claim not installed if there is no app id.

Mounir's review should be enough.

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 785913 [details] [diff] [review]
part 2.  Allow touching interface objects via an Xray even if the page they're in can't touch them.

Somewhat odd to define the property occasionally in bindings and in some cases
in nsDOMClassInfo. But this patch doesn't really change that, just moves code.

Comment 61

[Bobby Holley (:bholley)]

Comment on attachment 786450 [details] [diff] [review]
Part 3 updated to comments

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

r=bholley

::: dom/promise/Promise.cpp
@@ +124,5 @@
> +  // Note that we have no concept of a certified app in workers.
> +  // XXXbz well, why not?
> +  if (!NS_IsMainThread()) {
> +    return
> +      mozilla::dom::workers::GetWorkerPrivateFromContext(aCx)->IsChromeWorker();

This spacing/indentation decision looks ugly to me.

::: dom/webidl/Promise.webidl
@@ +24,5 @@
> +  // Disable the static methods when the interface object is supposed
> +  // to be disabled, just in case someone screws up and manages to
> +  // create a Promise object in this scope without having resolved the
> +  // interface object first... or in case some code decides to walk
> +  // over to .constructor from the proto of a promise object.

My beef with this comment earlier is that the second reason is much more compelling and universally relevant than the first, which is why it seems wrong to frame it as an afterthought.

Comment 62

[Boris Zbarsky [:bzbarsky]]

> This spacing/indentation decision looks ugly to me.

Ok, how about:

    return workers::GetWorkerPrivateFromContext(aCx)->IsChromeWorker();

since we're in the mozilla::dom namespace anyway?

> the second reason is much more compelling and universally relevant than the first

Fair.  Flipped the comment around.

Comment 63

[Bobby Holley (:bholley)]

sounds great.

Comment 64

[Gene Lian (I already quit Mozilla)]

(In reply to Boris Zbarsky (:bz) (vacation Aug 10-19) from comment #46)
> Gene, I'd appreciate if it if you have time to put together some tests.....

It seems we don't need to fix any test cases [1].

[1] https://tbpl.mozilla.org/?tree=Try&rev=1a129c1f81b0

Comment 65

[Boris Zbarsky [:bzbarsky]]

Well, yes, but we need to add tests for stuff like "web page does not see Promise constructor if pref is off" (if we don't have one already) and "chrome sees Promise constructor even if pref is off" and "certified app sees Promise constructor even if pref is off".

Comment 66

[Mounir Lamouri (:mounir)]

Comment on attachment 785912 [details] [diff] [review]
part 1.  Don't assert that app id exists when asked for app status; just claim not installed if there is no app id.

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

I would prefer if the MOZ_ASSERT could be changed for something else. NS_ASSERTION might break DEBUG tests nowadays so maybe NS_WARNING? I think being able to track UNKNOWMN_APP_ID wouldn't hurt given that we do not expect them to exist when those methods are called and even if that proves to be incorrect, keeping track of it wouldn't hurt.

::: content/base/src/nsDocument.cpp
@@ +2549,5 @@
>  
>    nsIPrincipal* principal = NodePrincipal();
>  
> +  uint16_t appStatus = principal->GetAppStatus();
> +  applyAppDefaultCSP = appStatus == nsIPrincipal::APP_STATUS_PRIVILEGED ||

bool applyDefaultCSP = ...;
and remove the init above.

You can as well move the other init below this one.

Comment 67

[Boris Zbarsky [:bzbarsky]]

> I would prefer if the MOZ_ASSERT could be changed for something else.

I can even leave the MOZ_ASSERT if it's ok to leave it and call into this code for a document's principal without checking the app id.... I just don't know whether that's an OK thing to do.  Is it?

Comment 68

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) (vacation Aug 10-19) from comment #67)
> > I would prefer if the MOZ_ASSERT could be changed for something else.
> 
> I can even leave the MOZ_ASSERT if it's ok to leave it and call into this
> code for a document's principal without checking the app id.... I just don't
> know whether that's an OK thing to do.  Is it?

It is okay but I believe that code was added because it was crashing...

Comment 69

[Ed Morley [:emorley]]

Push backed out for Windows build failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=26277854&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/11e2c4170bed
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4b50c8c081fc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/84010c0ebe0c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5d8cf15c3b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eae52d24d046

Comment 70

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It *should* be ok yes. But we've in the past hit rare buggy edge-cases which triggered the assertion. To be clear, in those cases the assertion did indicate that something was wrong. However edge cases only lived in Firefox Desktop code, and these errors don't affect Firefox Desktop. So that's why it's been worked around in various cases.

It's unclear if we'd actually hit the assertion here though. Putting a comment above the assertion saying that if we keep hitting it in hard-to-fix/low-value cases, then changing the assertion to a warning is ok with me. Or simply changing to a warning right now is also ok with me.

Comment 71

[(no longer active)]

Boris, Andrea is currently on vacation, and will be back on Aug 19th.

Comment 72

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment 70 was in response to comment 67.

I think going with a warning sounds is the way to go for now. Our current APIs are just not good enough to iron-proof enough to be sure that the assert won't happen.

Comment 73

[Boris Zbarsky [:bzbarsky]]

The failure was from bug 902485.

Comment 74

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d89c4889007
https://hg.mozilla.org/integration/mozilla-inbound/rev/0835f3ad6ad4
https://hg.mozilla.org/integration/mozilla-inbound/rev/61c90eecd72a

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d89c4889007
https://hg.mozilla.org/mozilla-central/rev/0835f3ad6ad4
https://hg.mozilla.org/mozilla-central/rev/61c90eecd72a

Comment 76

[Andrea Marchesini [:baku]]

Comment on attachment 786450 [details] [diff] [review]
Part 3 updated to comments

lgtm