Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Description • 11 years ago

      No description provided.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 3 • 11 years ago

memory diff between without/with nfc_worker

Main Process (pid NNN)
Explicit Allocations

-0.72 MB (100.0%) -- explicit
├──-1.04 MB (143.77%) -- workers/workers()
│  ├──-1.01 MB (139.86%) -- worker(resource://gre/modules/nfc_worker.js, 0xNNN)
│  │  ├──-0.32 MB (44.94%) -- runtime
│  │  │  ├──-0.11 MB (15.03%) ── script-data [-]
│  │  │  ├──-0.06 MB (08.65%) ── atoms-table [-]
│  │  │  ├──-0.06 MB (08.65%) -- code
│  │  │  │  ├──-0.04 MB (06.20%) ── other [-]
│  │  │  │  ├──-0.01 MB (01.73%) ── unused [-]
│  │  │  │  └──-0.01 MB (00.72%) ── baseline [-]
│  │  │  ├──-0.04 MB (05.41%) ── runtime-object [-]
│  │  │  ├──-0.02 MB (03.32%) ── script-sources [-]
│  │  │  ├──-0.02 MB (02.16%) ── gc/marker
│  │  │  └──-0.01 MB (01.73%) ++ (4 tiny)
│  │  ├──-0.26 MB (36.00%) -- zone(0xb0188800)
│  │  │  ├──-0.19 MB (26.40%) -- compartment(web-worker)
│  │  │  │  ├──-0.08 MB (11.34%) -- shapes
│  │  │  │  │  ├──-0.05 MB (07.55%) -- gc-heap
│  │  │  │  │  │  ├──-0.04 MB (05.17%) -- tree
│  │  │  │  │  │  │  ├──-0.03 MB (04.05%) ── global-parented [-]
│  │  │  │  │  │  │  └──-0.01 MB (01.11%) ── non-global-parented [-]
│  │  │  │  │  │  └──-0.02 MB (02.39%) ── base [-]
│  │  │  │  │  └──-0.03 MB (03.78%) ── malloc-heap/compartment-tables
│  │  │  │  ├──-0.08 MB (10.56%) -- objects
│  │  │  │  │  ├──-0.05 MB (07.35%) -- gc-heap
│  │  │  │  │  │  ├──-0.03 MB (04.67%) ── function [-]
│  │  │  │  │  │  └──-0.02 MB (02.68%) ── ordinary [-]
│  │  │  │  │  └──-0.02 MB (03.21%) ── malloc-heap/slots
│  │  │  │  ├──-0.02 MB (02.96%) -- sundries
│  │  │  │  │  ├──-0.02 MB (02.38%) ── malloc-heap [-]
│  │  │  │  │  └──-0.00 MB (00.58%) ── gc-heap [-]
│  │  │  │  └──-0.01 MB (01.54%) ── scripts/gc-heap
│  │  │  ├──-0.06 MB (08.68%) ── unused-gc-things [-]
│  │  │  └──-0.01 MB (00.92%) ++ sundries
│  │  ├──-0.20 MB (28.13%) -- zone(0xb159cc00)
│  │  │  ├──-0.15 MB (21.43%) -- compartment(web-worker)
│  │  │  │  ├──-0.06 MB (07.84%) -- objects
│  │  │  │  │  ├──-0.04 MB (05.75%) -- gc-heap
│  │  │  │  │  │  ├──-0.03 MB (03.71%) ── function [-]
│  │  │  │  │  │  └──-0.01 MB (02.03%) ── ordinary [-]
│  │  │  │  │  └──-0.02 MB (02.09%) ── malloc-heap/slots
│  │  │  │  ├──-0.05 MB (06.45%) -- shapes
│  │  │  │  │  ├──-0.03 MB (04.55%) -- gc-heap
│  │  │  │  │  │  ├──-0.02 MB (03.27%) ── tree/global-parented
│  │  │  │  │  │  └──-0.01 MB (01.28%) ── dict [-]
│  │  │  │  │  └──-0.01 MB (01.89%) ── malloc-heap/compartment-tables
│  │  │  │  ├──-0.03 MB (03.75%) ── scripts/gc-heap
│  │  │  │  └──-0.02 MB (03.40%) -- sundries
│  │  │  │     ├──-0.01 MB (01.99%) ── malloc-heap [-]
│  │  │  │     └──-0.01 MB (01.41%) ── gc-heap [-]
│  │  │  ├──-0.05 MB (06.37%) ── unused-gc-things [-]
│  │  │  └──-0.00 MB (00.33%) ── sundries/gc-heap
│  │  ├──-0.19 MB (26.46%) -- zone(0xb159b800)
│  │  │  ├──-0.17 MB (23.55%) -- strings
│  │  │  │  ├──-0.14 MB (19.10%) -- normal
│  │  │  │  │  ├──-0.09 MB (12.28%) ── gc-heap [-]
│  │  │  │  │  └──-0.05 MB (06.82%) ── malloc-heap [-]
│  │  │  │  └──-0.03 MB (04.45%) ── short-gc-heap [-]
│  │  │  ├──-0.01 MB (01.50%) ── unused-gc-things [-]
│  │  │  ├──-0.01 MB (01.13%) ── jit-codes-gc-heap [-]
│  │  │  └──-0.00 MB (00.28%) ++ (2 tiny)
│  │  └──-0.03 MB (04.32%) ── gc-heap/chunk-admin
│  └──-0.03 MB (03.91%) -- worker(resource://gre/modules/ril_worker.js, 0xNNN)
│     ├──-0.59 MB (82.01%) -- zone(0xb1542000)
│     │  ├──-0.52 MB (72.10%) -- compartment(web-worker)
│     │  │  ├──-0.20 MB (27.75%) -- objects
│     │  │  │  ├──-0.11 MB (14.63%) -- gc-heap
│     │  │  │  │  ├──-0.07 MB (09.26%) ── function [-]
│     │  │  │  │  └──-0.04 MB (05.37%) ── ordinary [-]
│     │  │  │  └──-0.09 MB (13.12%) -- malloc-heap
│     │  │  │     ├──-0.05 MB (07.54%) ── elements/non-asm.js
│     │  │  │     └──-0.04 MB (05.57%) ── slots [-]
│     │  │  ├──-0.18 MB (25.03%) -- shapes
│     │  │  │  ├──-0.11 MB (15.82%) -- gc-heap
│     │  │  │  │  ├──-0.06 MB (08.44%) -- tree
│     │  │  │  │  │  ├──-0.05 MB (06.67%) ── global-parented [-]
│     │  │  │  │  │  └──-0.01 MB (01.77%) ── non-global-parented [-]
│     │  │  │  │  ├──-0.04 MB (04.88%) ── dict [-]
│     │  │  │  │  └──-0.02 MB (02.50%) ── base [-]
│     │  │  │  └──-0.07 MB (09.21%) -- malloc-heap
│     │  │  │     ├──-0.03 MB (03.78%) ── compartment-tables [-]
│     │  │  │     ├──-0.02 MB (02.49%) ── tree-tables [-]
│     │  │  │     ├──-0.01 MB (01.74%) ── tree-shape-kids [-]
│     │  │  │     └──-0.01 MB (01.20%) ── dict-tables [-]
│     │  │  ├──-0.11 MB (14.92%) -- scripts
│     │  │  │  ├──-0.09 MB (11.88%) ── gc-heap [-]
│     │  │  │  └──-0.02 MB (03.04%) ── malloc-heap/data
│     │  │  ├──-0.02 MB (02.30%) ── baseline/stubs/fallback
│     │  │  └──-0.02 MB (02.11%) -- sundries
│     │  │     ├──-0.01 MB (01.77%) ── malloc-heap [-]
│     │  │     └──-0.00 MB (00.34%) ── gc-heap [-]
│     │  ├──-0.06 MB (08.54%) ── unused-gc-things [-]
│     │  └──-0.01 MB (01.37%) -- sundries
│     │     ├──-0.01 MB (01.23%) ── gc-heap [-]
│     │     └──-0.00 MB (00.13%) ── malloc-heap [-]
│     └───0.56 MB (-78.09%) ++ (4 tiny)
└───0.32 MB (-43.77%) -- (7 tiny)
    ├──0.43 MB (-59.29%) ── heap-unclassified
    ├──-0.11 MB (15.12%) -- heap-overhead
    │  ├──-0.15 MB (20.54%) ── page-cache
    │  └───0.04 MB (-5.42%) ── waste
    ├──-0.00 MB (00.54%) ++ window-objects/top(app://system.gaiamobile.org/index.html, id=3)
    ├──0.00 MB (-0.12%) ++ js-non-window
    ├──0.00 MB (-0.03%) ++ xpconnect
    ├──-0.00 MB (00.01%) ── freetype
    └──0.00 MB (-0.01%) ── xpcom/component-manager

Other Measurements

0.00 MB (100.0%) -- blob-urls
└──0.00 MB (100.0%) -- owner(app://system.gaiamobile.org/index.html)
   ├──-0.12 MB (100.0%) ++ (5 tiny)
   └──0.12 MB (100.0%) ── blob:0fa63164-bc26-4529-8d15-3ed21457416e [+]

-1.47 MB (100.0%) -- decommitted
├──-1.48 MB (100.27%) ── workers/workers()/worker(resource://gre/modules/nfc_worker.js, 0xNNN)/gc-heap/decommitted-arenas
└───0.00 MB (-0.27%) ── js-non-window/gc-heap/decommitted-arenas

-0.00 MB (100.0%) -- js-main-runtime
├──-0.00 MB (115.63%) -- zones
│  ├──-0.00 MB (126.05%) ── unused-gc-things
│  └───0.00 MB (-10.42%) ++ (2 tiny)
└───0.00 MB (-15.63%) ++ (2 tiny)

0 (100.0%) -- js-main-runtime-compartments
└──0 (100.0%) ++ user

-0.00 MB (100.0%) -- js-main-runtime-gc-heap-committed
├──-0.00 MB (99.22%) ── unused/gc-things
└──-0.00 MB (00.78%) ── used/arena-admin

0.00 MB (100.0%) -- window-objects
└──0.00 MB (100.0%) ++ layout

-0.02 MB ── heap-allocated
-0.13 MB ── heap-committed
  -0.39% ── heap-overhead-ratio
     -10 ── page-faults-hard
  -3,481 ── page-faults-soft
-1.57 MB ── resident
-0.81 MB ── resident-unique
-1.17 MB ── vsize

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 6 • 11 years ago

Comment on attachment 8375405 [details] [diff] [review]
Part 2: NFC Message Handler

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

::: dom/system/gonk/nfc_consts.js
@@ -23,4 @@
>  this.DEBUG_CONTENT_HELPER = false || DEBUG_ALL;
>  this.DEBUG_NFC = false || DEBUG_ALL;
>  
>  // Current version

Below consts are used by nfc_worker.js,
I'll also remove these.

::: dom/system/gonk/nfc_worker.js
@@ -357,5 @@
> -  let status       = Buf.readInt32();
> -  let majorVersion = Buf.readInt32();
> -  let minorVersion = Buf.readInt32();
> -  debug("NFC_NOTIFICATION_INITIALIZED status:" + status);
> -  if ((majorVersion != NFC_MAJOR_VERSION) || (minorVersion != NFC_MINOR_VERSION)) {

I'll also check the versions in NfcMessageHandler.

	Dimi Lee [:dimi]
	Comment 7 • 11 years ago

Comment on attachment 8375405 [details] [diff] [review]
Part 2: NFC Message Handler

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

::: dom/system/gonk/NfcMessageHandler.cpp
@@ +58,5 @@
> +  }
> +
> +  return result;
> +}
> +

mPendingReq do not have to be a queue ?

@@ +139,5 @@
> +  aOptions.mType = NS_ConvertUTF8toUTF16(kConfigResponse);
> +  aOptions.mStatus = aParcel.readInt32();
> +  return true;
> +}
> +

Suggest for request functions, use const reference for CommandOptions,
for response functions, use const reference for Parcel.

::: dom/system/gonk/NfcOptions.h
@@ +49,5 @@
> +
> +  if (aOther.mRecords.WasPassed()) {
> +    mozilla::dom::Sequence<mozilla::dom::NDEFRecord> const & currentValue = aOther.mRecords.InternalValue();
> +    uint32_t length = currentValue.Length();
> +    mRecordCount = currentValue.Length();

You can use length directly here

::: dom/webidl/NfcOptions.webidl
@@ +40,5 @@
> +
> +  boolean isReadOnly;
> +  boolean canBeMadeReadOnly;
> +  long maxSupportedLength;
> +};

Maybe we will need to add those into DummyBinding.webidl ?

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 8 • 11 years ago

Addressed Dimi's comments/

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 9 • 11 years ago

(In reply to Dimi Lee[:dimi][:dlee] from comment #7)
> ::: dom/webidl/NfcOptions.webidl
> @@ +40,5 @@
> > +
> > +  boolean isReadOnly;
> > +  boolean canBeMadeReadOnly;
> > +  long maxSupportedLength;
> > +};
> 
> Maybe we will need to add those into DummyBinding.webidl ?

DummyBindings is not neccesary anymore, see Bug 968665.

	[:fabrice] Fabrice Desré
	Comment 10 • 11 years ago

Comment on attachment 8375404 [details] [diff] [review]
Part 1: NFC Service

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

::: dom/system/gonk/Nfc.js
@@ +434,5 @@
>     *        A text message type.
>     * @param message [optional]
>     *        An optional message object to send.
>     */
>    sendToWorker: function sendToWorker(nfcMessageType, message) {

nit: this could be renamed since we have no real worker anymore.

::: dom/system/gonk/NfcService.cpp
@@ +146,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsCOMPtr<nsIRunnable> runnable = new WorkerRunnable();
> +  mWorkerThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL);

I'm probably missing something, but WorkerRunnable() does nothing in it's Run() method so how are we sending the aOptions around? (we're not using this parameter anywhere).

	[:fabrice] Fabrice Desré
	Comment 11 • 11 years ago

Comment on attachment 8376969 [details] [diff] [review]
Part 2: NFC Message Handler v2

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

Yoshi, I don't know the nfc protocols and api well enough to review that.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 12 • 11 years ago

(In reply to Fabrice Desré [:fabrice] from comment #10)
> ::: dom/system/gonk/NfcService.cpp
> @@ +146,5 @@
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  nsCOMPtr<nsIRunnable> runnable = new WorkerRunnable();
> > +  mWorkerThread->Dispatch(runnable, nsIEventTarget::DISPATCH_NORMAL);
> 
> I'm probably missing something, but WorkerRunnable() does nothing in it's
> Run() method so how are we sending the aOptions around? (we're not using
> this parameter anywhere).

Hi Fabrice:
aOptions passing is actually done in Part 2, because it involves marshalling/unmarshalling NFC binary data.

My original Part 1 is focus on how the data is transfered between Nfc.js (Main Thread on Chrome Process) to nfcd.

I'll reformat these two patches to make them more clearer,
also I will split marshalling/unmarshalling part into another part and let Dimi to review it.

Thanks for your comment.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 13 • 11 years ago

rename sendToWorker to sendToNfcService by fabrice's comment.

Also move some code from Part2 to Part 1.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 14 • 11 years ago

Comment on attachment 8378113 [details] [diff] [review]
Part 1: NFC Service v2.

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

Hi, Fabrice

This patch is adjusted by your last comment.

Also this code uses the same pattern used in "rewrite wifi_worker in C++" and "rewrite net_worker in C++"
And I've moved the NFCD protocol things to Part 2.
I hope the patch here won't be too complicated for you to review.

Can you kindly help to review it?
If you need me to split into more parts please let me know.

Thanks

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 15 • 11 years ago

This is the same patch with Part 2. v2.

Just I leave only NFCD protcol specific code in this part, and move NfcService related code to Part 1 as Fabrice commented.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 16 • 11 years ago

fabrice, review ping?

	[:fabrice] Fabrice Desré
	Comment 17 • 11 years ago

(In reply to Yoshi Huang[:allstars.chh] from comment #16)
> fabrice, review ping?

I'm really sorry Yoshi, I had no time to look at that and I should have notified you. I'll try to do it this week, but if you have other reviewers don't block on me.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 18 • 11 years ago

Comment on attachment 8378113 [details] [diff] [review]
Part 1: NFC Service v2.

No worries, Fabrice.

Hi, Kyle
Could you help to review this for me?
Or if you don't have time, please let me know, I'll find another reviewer. :)

Thanks

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 19 • 11 years ago

I might be able to review this on my long flight this week.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 20 • 11 years ago

Comment on attachment 8378113 [details] [diff] [review]
Part 1: NFC Service v2.

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

::: dom/system/gonk/NfcMessageHandler.cpp
@@ +11,5 @@
> +bool
> +NfcMessageHandler::Marshall(Parcel& aParcel, const CommandOptions& aOptions)
> +{
> +  bool result;
> +  return result;

When you land this you should squash these two patches together.  Obviously this doesn't work by itself ;-)

::: dom/system/gonk/NfcOptions.h
@@ +1,1 @@
> +#ifndef NfcOptions_h

No license header?

@@ +9,5 @@
> +struct NDEFRecordStruct
> +{
> +  uint8_t mTnf;
> +  uint32_t mTypeLength;
> +  nsAutoArrayPtr<uint8_t> mType;

This might be cleaner if you used nsTArray<uint8_t> to replace mFoo and mFooLength.

@@ +27,5 @@
> +  if (aOther.prop.WasPassed()) {                      \
> +    prop = aOther.prop.Value();                       \
> +  } else {                                            \
> +    prop = defaultValue;                              \
> +  }

I think we should add a method to Optional<T> that returns Value() if it was passed or else a provided default value.  Perhaps const T& ValueOrDefault(const T&)?

@@ +38,5 @@
> +      prop = aOther.prop.Value();                     \
> +    }                                                 \
> +  } else {                                            \
> +    prop = defaultValue;                              \
> +  }

Why are you handling null like this?  This seems like something that should either happen at the WebIDL layer or not at all.

@@ +46,5 @@
> +  COPY_OPT_STRING_FIELD(mRequestId, EmptyString())
> +  COPY_OPT_FIELD(mPowerLevel, 0)
> +  COPY_OPT_FIELD(mTechType, 0)
> +
> +  if (aOther.mRecords.WasPassed()) {

Seems like we should be writing an operator= for something instead of all this code.

@@ +105,5 @@
> +  int32_t mMinorVersion;
> +  int32_t mTechListLength;
> +  nsAutoArrayPtr<uint8_t> mTechList;
> +  int32_t mRecordCount;
> +  nsAutoArrayPtr<NDEFRecordStruct> mRecords;

Same things here about using nsTArray.

::: dom/system/gonk/NfcService.cpp
@@ +37,5 @@
> +
> +namespace mozilla {
> +namespace nfc {
> +
> +static StaticRefPtr<NfcService> gNfcService;

You can just use a raw pointer here.  The XPCOM service manager will hold a strong reference to your service until XPCOM shutdown.

@@ +42,5 @@
> +
> +NS_IMPL_ISUPPORTS1(NfcService, nsINfcService)
> +
> +// Runnable used to call SendCommand on the worker thread.
> +class WorkerRunnable : public nsRunnable

Please give this a more descriptive name.  Maybe NfcCommandRunnable?

@@ +85,5 @@
> +    JS::Rooted<JS::Value> value(cx);
> +    RootedDictionary<NfcEventOptions> event(cx);
> +    if (!event.Init(cx, value)) {
> +      return NS_ERROR_TYPE_ERR;
> +    }

Why do you need to call Init at all?  IIRC that is only for constructing a dictionary from a JS value or JSON string.

@@ +112,5 @@
> +      }
> +
> +      for (int i = 0; i < mEvent.mTechListLength; i++) {
> +        nsString& elem = *event.mTechList.Value().AppendElement();
> +        elem = NS_ConvertUTF8toUTF16(NfcTechString[mEvent.mTechList[i]]);

Where are these parameters validated?  What stops mEvent.mTechList[i] from being larger than the length of NfcTechString?  I can't find the code that does that, but even if it does exist, you should add an assertion here for the length.

Rather than converting them to wide strings you should just store them as wide strings.

@@ +224,5 @@
> +NS_IMETHODIMP
> +NfcService::Start(nsINfcEventListener* aListener)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aListener);

You should also assert that we have not been started already.

@@ +226,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aListener);
> +
> +  nsresult rv = NS_NewThread(getter_AddRefs(mWorkerThread));

Please use NS_NewNamedThread and give this a useful name.

@@ +240,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +NfcService::Shutdown()

What calls this?

@@ +279,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  mozilla::AutoSafeJSContext cx;
> +  JS::RootedValue val(cx);

You need to enter a compartment here.  But it's not actually clear how to get the right compartment (of mListener) here :(

I think maybe you should make the listener a WebIDL callback instead of an XPIDL one, and then you could just let the WebIDL binding layer deal with compartments for you.

::: dom/system/gonk/NfcService.h
@@ +4,5 @@
> +
> +#ifndef NfcService_h
> +#define NfcService_h
> +
> +#include "mozilla/dom/NfcOptionsBinding.h"

Forward declare mozilla::dom::NfcEventOptions.

@@ +9,5 @@
> +#include "mozilla/ipc/Nfc.h"
> +#include "mozilla/ipc/UnixSocket.h"
> +#include "nsCOMPtr.h"
> +#include "nsINfcService.h"
> +#include "nsThread.h"

and nsIThread

@@ +13,5 @@
> +#include "nsThread.h"
> +#include "NfcMessageHandler.h"
> +
> +namespace mozilla {
> +namespace nfc {

Drop the nfc namespace.  You're already prefixed the classes with Nfc.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 22 • 11 years ago

Modified after Part 1 changed.
I'll ask r? to Dimi again when Part 1 is r+.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 23 • 11 years ago

Comment on attachment 8423605 [details] [diff] [review]
Part 1: NFC Service v3.

Hi, Kyle
Thanks for the previous review, and sorry for send another r? so late,
I was interrupted by other NFC bugs :( 

I have addresed your comments, see my comments below.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #20)
> ::: dom/system/gonk/NfcMessageHandler.cpp
> @@ +11,5 @@
> > +bool
> > +NfcMessageHandler::Marshall(Parcel& aParcel, const CommandOptions& aOptions)
> > +{
> > +  bool result;
> > +  return result;
> 
> When you land this you should squash these two patches together.  Obviously
> this doesn't work by itself ;-)
> 

Yeah I would merge the teo patches when they are r+.

> ::: dom/system/gonk/NfcOptions.h
> @@ +1,1 @@
> > +#ifndef NfcOptions_h
> 
> No license header?
> 
Fixed.

> @@ +9,5 @@
> > +struct NDEFRecordStruct
> > +{
> > +  uint8_t mTnf;
> > +  uint32_t mTypeLength;
> > +  nsAutoArrayPtr<uint8_t> mType;
> 
> This might be cleaner if you used nsTArray<uint8_t> to replace mFoo and
> mFooLength.
> 
Fixed, use nsTArray now.

> @@ +27,5 @@
> > +  if (aOther.prop.WasPassed()) {                      \
> > +    prop = aOther.prop.Value();                       \
> > +  } else {                                            \
> > +    prop = defaultValue;                              \
> > +  }
> 
> I think we should add a method to Optional<T> that returns Value() if it was
> passed or else a provided default value.  Perhaps const T&
> ValueOrDefault(const T&)?
> 
This pattern is also used by Wifi, network when they are rewritten by C++.
I'd file a follow up bug for this, is this okay for you?


> @@ +38,5 @@
> > +      prop = aOther.prop.Value();                     \
> > +    }                                                 \
> > +  } else {                                            \
> > +    prop = defaultValue;                              \
> > +  }
> 
> Why are you handling null like this?  This seems like something that should
> either happen at the WebIDL layer or not at all.
> 

This was used by older NFC code, now we don't have to handle this.
(The older patch was used by 'config' command, which is sent from Gaia by CustomEvent, now after Bug 970251 is fixed, all commands sent from Gecko have requestId now)

> @@ +46,5 @@
> > +  COPY_OPT_STRING_FIELD(mRequestId, EmptyString())
> > +  COPY_OPT_FIELD(mPowerLevel, 0)
> > +  COPY_OPT_FIELD(mTechType, 0)
> > +
> > +  if (aOther.mRecords.WasPassed()) {
> 
> Seems like we should be writing an operator= for something instead of all
> this code.
> 

Sorry I don't understand this comment, can you check my new patch and see if I have addressed it?


> @@ +105,5 @@
> > +  int32_t mMinorVersion;
> > +  int32_t mTechListLength;
> > +  nsAutoArrayPtr<uint8_t> mTechList;
> > +  int32_t mRecordCount;
> > +  nsAutoArrayPtr<NDEFRecordStruct> mRecords;
> 
> Same things here about using nsTArray.
> 
Fixed.


> ::: dom/system/gonk/NfcService.cpp
> @@ +37,5 @@
> > +
> > +namespace mozilla {
> > +namespace nfc {
> > +
> > +static StaticRefPtr<NfcService> gNfcService;
> 
> You can just use a raw pointer here.  The XPCOM service manager will hold a
> strong reference to your service until XPCOM shutdown.
> 

Fixed.

> @@ +42,5 @@
> > +
> > +NS_IMPL_ISUPPORTS1(NfcService, nsINfcService)
> > +
> > +// Runnable used to call SendCommand on the worker thread.
> > +class WorkerRunnable : public nsRunnable
> 
> Please give this a more descriptive name.  Maybe NfcCommandRunnable?
> 

Fixed.

> @@ +85,5 @@
> > +    JS::Rooted<JS::Value> value(cx);
> > +    RootedDictionary<NfcEventOptions> event(cx);
> > +    if (!event.Init(cx, value)) {
> > +      return NS_ERROR_TYPE_ERR;
> > +    }
> 
> Why do you need to call Init at all?  IIRC that is only for constructing a
> dictionary from a JS value or JSON string.
> 

Fixed. removed Init(..)

> @@ +112,5 @@
> > +      }
> > +
> > +      for (int i = 0; i < mEvent.mTechListLength; i++) {
> > +        nsString& elem = *event.mTechList.Value().AppendElement();
> > +        elem = NS_ConvertUTF8toUTF16(NfcTechString[mEvent.mTechList[i]]);
> 
> Where are these parameters validated?  What stops mEvent.mTechList[i] from
> being larger than the length of NfcTechString?  I can't find the code that
> does that, but even if it does exist, you should add an assertion here for
> the length.
> 

These parameters will be set in Part 2.
I also add an asserting for check the length.

> Rather than converting them to wide strings you should just store them as
> wide strings.
> 

Fixed, store in wide strings.

> @@ +224,5 @@
> > +NS_IMETHODIMP
> > +NfcService::Start(nsINfcEventListener* aListener)
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(aListener);
> 
> You should also assert that we have not been started already.
> 

Fixed.

> @@ +226,5 @@
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  MOZ_ASSERT(aListener);
> > +
> > +  nsresult rv = NS_NewThread(getter_AddRefs(mWorkerThread));
> 
> Please use NS_NewNamedThread and give this a useful name.
> 

Fixed. call it NfcThread (I tried to name it NfcMarshallerThread but it exceeds to max length 16 chars).

> @@ +240,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +NS_IMETHODIMP
> > +NfcService::Shutdown()
> 
> What calls this?
> 

Fixed, Nfc.js will call nfcService.shutdown() now.

> @@ +279,5 @@
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  mozilla::AutoSafeJSContext cx;
> > +  JS::RootedValue val(cx);
> 
> You need to enter a compartment here.  But it's not actually clear how to
> get the right compartment (of mListener) here :(
> 
> I think maybe you should make the listener a WebIDL callback instead of an
> XPIDL one, and then you could just let the WebIDL binding layer deal with
> compartments for you.
> 
I am not sure about this, 
this pattern is also used by Wifi (Bug 864932) and Network worker (Bug 864931),
if it is still not correct, should I file a followup bug and fix this for NFC, Wifi and Network?


> ::: dom/system/gonk/NfcService.h
> @@ +4,5 @@
> > +
> > +#ifndef NfcService_h
> > +#define NfcService_h
> > +
> > +#include "mozilla/dom/NfcOptionsBinding.h"
> 
> Forward declare mozilla::dom::NfcEventOptions.
> 
Fixed.

> @@ +9,5 @@
> > +#include "mozilla/ipc/Nfc.h"
> > +#include "mozilla/ipc/UnixSocket.h"
> > +#include "nsCOMPtr.h"
> > +#include "nsINfcService.h"
> > +#include "nsThread.h"
> 
> and nsIThread
> 

Fixed.

> @@ +13,5 @@
> > +#include "nsThread.h"
> > +#include "NfcMessageHandler.h"
> > +
> > +namespace mozilla {
> > +namespace nfc {
> 
> Drop the nfc namespace.  You're already prefixed the classes with Nfc.

Removed all nfc namespace.

Thanks again for your prompt review.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 24 • 10 years ago

rebase

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 25 • 10 years ago

rebase

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 26 • 10 years ago

Now the patches will cause linking error on m-c now, 

B2G_Nexus4/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.7/bin/../lib/gcc/arm-linux-androideabi/4.7/../../../../arm-linux-androideabi/bin/ld: error: hidden symbol '_ZNK7android6Parcel9readInt32Ev' is not defined locally

Now I found out it's caused by Bug 1001320 Part 5, which uses '#pragma GCC visibility push(hidden)', and from GCC Wiki [1], '#pragma GCC visibility is stronger than -fvisibility; it affects extern declarations as well. -fvisibility only affects definitions, so that existing code can be recompiled with minimal changes'.

I'll upload another Part 3 patch for fixing this linking error.

[1]: https://gcc.gnu.org/wiki/Visibility

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 27 • 10 years ago

Comment on attachment 8439813 [details] [diff] [review]
Part 1: NFC Service v4

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

::: dom/system/gonk/NfcMessageHandler.h
@@ +5,5 @@
> +#ifndef NfcMessageHandler_h
> +#define NfcMessageHandler_h
> +
> +namespace android {
> +class Parcel;

adding MOZ_EXPORT here could fix the linking error mentioned in Comment 26.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 30 • 10 years ago

Comment on attachment 8442705 [details] [diff] [review]
Part 1: NFC Service v5

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

Hi, Kyle
This version just rebases to latest m-c and adds 'MOZ_EXPORT' as explained in Comment 27,
sorry I didn't make it before you leave Taipei,
can you help to review this ?

Thank you ~

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 31 • 10 years ago

update target milestone to current one.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 32 • 10 years ago

Comment on attachment 8442705 [details] [diff] [review]
Part 1: NFC Service v5

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

Some questions, but this looks reasonable.

I assume you've tested this in a debug build to make sure the assertions are correct?

::: dom/system/gonk/NfcMessageHandler.cpp
@@ +10,5 @@
> +bool
> +NfcMessageHandler::Marshall(Parcel& aParcel, const CommandOptions& aOptions)
> +{
> +  bool result;
> +  return result;

I hope there's another patch that does something here ...

::: dom/system/gonk/NfcOptions.h
@@ +8,5 @@
> +#include "mozilla/dom/NfcOptionsBinding.h"
> +
> +namespace mozilla {
> +
> +struct NDEFRecordStruct

Why can't we just use the generated DOM dictionaries here?  Is it because of the JS values?

@@ +18,5 @@
> +};
> +
> +struct CommandOptions
> +{
> +  CommandOptions(const mozilla::dom::NfcCommandOptions& aOther) {

Everything inside this function should be indented another 2 spaces.

::: dom/system/gonk/NfcService.cpp
@@ +54,5 @@
> +  }
> +
> +  NS_IMETHOD Run()
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());

You should add a function that asserts that do_GetCurrentThread == NfcService::mThread, and use that in all of these runnables.

@@ +87,5 @@
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    mozilla::AutoSafeJSContext cx;
> +    JS::Rooted<JS::Value> value(cx);

Unless I'm missing something 'value' is unused.

@@ +90,5 @@
> +    mozilla::AutoSafeJSContext cx;
> +    JS::Rooted<JS::Value> value(cx);
> +    RootedDictionary<NfcEventOptions> event(cx);
> +
> +    // the copy constructor is private.

Which is quite unfortunate because this code is ugly :/

@@ +190,5 @@
> +      const uint8_t* data = mData->mData.get();
> +      uint32_t parcelSize = ((data[offset + 0] & 0xff) << 24) |
> +                            ((data[offset + 1] & 0xff) << 16) |
> +                            ((data[offset + 2] & 0xff) <<  8) |
> +                             (data[offset + 3] & 0xff);

This worries me a lot.  What stops a malicious NFC peer from constructing a packet with a parcelSize that is much larger than mData->mData.Length() and causing us to read off the end of the buffer?  I think there needs to be some bound checking here.

::: dom/system/gonk/NfcService.h
@@ +28,5 @@
> +  static already_AddRefed<NfcService> FactoryCreate();
> +
> +  void DispatchNfcEvent(const mozilla::dom::NfcEventOptions& aOptions);
> +
> +  void ReceiveSocketData(nsAutoPtr<mozilla::ipc::UnixSocketRawData>& aData) MOZ_OVERRIDE;

nit: mark this as virtual

@@ +29,5 @@
> +
> +  void DispatchNfcEvent(const mozilla::dom::NfcEventOptions& aOptions);
> +
> +  void ReceiveSocketData(nsAutoPtr<mozilla::ipc::UnixSocketRawData>& aData) MOZ_OVERRIDE;
> +private:

nit: \n above private

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 33 • 10 years ago

Thanks for the review, Kyle :)

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #32)
> Some questions, but this looks reasonable.
> 
> I assume you've tested this in a debug build to make sure the assertions are
> correct?
>
Yes

> ::: dom/system/gonk/NfcMessageHandler.cpp
> @@ +10,5 @@
> > +bool
> > +NfcMessageHandler::Marshall(Parcel& aParcel, const CommandOptions& aOptions)
> > +{
> > +  bool result;
> > +  return result;
> 
> I hope there's another patch that does something here ...
> 
Yes, Part 2 will add more impl in this funciton, I tried to seperate them to make them easier to review, however if it's actually making you confused, next version I'll only leave the declaration in the header, and move the definition to Part 2.

> ::: dom/system/gonk/NfcOptions.h
> @@ +8,5 @@
> > +#include "mozilla/dom/NfcOptionsBinding.h"
> > +
> > +namespace mozilla {
> > +
> > +struct NDEFRecordStruct
> 
> Why can't we just use the generated DOM dictionaries here?  Is it because of
> the JS values?
> 
As I remember when I implemented this, dictionary seems to have to be used on main thread, at that time I remembered I met some crashes when trying to parse the dictionary on NfcService thread.
Not sure if dictionary can be manipulated on other thread now, but I will try to not to use the NDEFRecordStruct.

And I'll address your other comments.

Thank you.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 34 • 10 years ago

Right now I met 
"JavaScript Error: "Accessing TypedArray data over Xrays is slow, and forbidden in order to encourage performant code. To copy TypedArrays across origin boundaries, consider using Components.utils.cloneInto()."

I guess it's from Bug 987163, I'll add another (Part 3) patch to address this.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 35 • 10 years ago

Looks like this is being actively worked on. Let's defer the target milestone.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 36 • 10 years ago

Comment on attachment 8442705 [details] [diff] [review]
Part 1: NFC Service v5

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

::: dom/system/gonk/Nfc.js
@@ +491,3 @@
>     */
>    onmessage: function onmessage(event) {
> +    let message = event;

let message = Cu.cloneInto(event, this);
Could fix the error from Comment 34.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 37 • 10 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #32)
> ::: dom/system/gonk/NfcOptions.h
> @@ +8,5 @@
> > +#include "mozilla/dom/NfcOptionsBinding.h"
> > +
> > +namespace mozilla {
> > +
> > +struct NDEFRecordStruct
> 
> Why can't we just use the generated DOM dictionaries here?  Is it because of
> the JS values?
> 

Hi Kyle
I find now I can use the generator DOM on a non-main thread, however for the generated DOM dict, its copy constructor is private, so I have to construct those memebers manually when I pass it to a nsRunnable, that seems not have advantage over using plain struct, and even in the nsRunnable, I still have to call member.WasPassed(), member.Value(), ....

Can you help to explain again why we should use the generated DOM dict directly on a I/O thread?

Also can you help to check if the Comment 36 is the correct usage for cloneInfo ? I am not sure if I can use the 'this' object as the 2nd parameter.

Thank you

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 38 • 10 years ago

(In reply to Yoshi Huang[:allstars.chh] from comment #37)
> Can you help to explain again why we should use the generated DOM dict
> directly on a I/O thread?

I am asking if you can, not saying that you should.  It sounds like the answer is no.

> Also can you help to check if the Comment 36 is the correct usage for
> cloneInfo ? I am not sure if I can use the 'this' object as the 2nd
> parameter.

The problem there is that DispatchNfcEvent is using the safe JS context and not the correct compartment.  Bobby, what's the right way to enter the compartment of a JS implemented service?

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 39 • 10 years ago

addressed Kyle's comments.

Given we are still waiting for bholley to reply for the Cu.cloneInto problem, I didn't include the fix in Comment 36 here.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 41 • 10 years ago

Comment on attachment 8455227 [details] [diff] [review]
Part 1: NFC Service v6

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

Kyle,
I have addressed your previous comments and tested this on device and passed the marionette-webapi tests on my local build.
Can you help to review this again for me ?

Thank you.

	Bobby Holley (:bholley)
	Comment 43 • 10 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #38)
> The problem there is that DispatchNfcEvent is using the safe JS context and
> not the correct compartment.  Bobby, what's the right way to enter the
> compartment of a JS implemented service?

If I'm understanding your question correctly, probably something like:

nsCOMPtr<nsIFoo> svc = do_GetService(contractID);
nsCOMPtr<nsIXPConnectWrappedJS> wjs = do_QueryInterface(svc);
MOZ_ASSERT(wjs);

nsCOMPtr<nsIGlobalObject> global = xpc::GetNativeForGlobal(js::GetGlobalForObjectCrossCompartment(wjs->GetJSObject()));
if (NS_WARN_IF(!global))
  return false;

AutoJSAPI jsapi;
if (NS_WARN_IF(!jsapi.Init(global)))
  return false;
JSContext *cx = jsapi.cx();

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 44 • 10 years ago

Comment on attachment 8455227 [details] [diff] [review]
Part 1: NFC Service v6

Try bholley's suggestion to remove the cloneInto please.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 45 • 10 years ago

Hi, Kyle and Bholley
This is the patch as suggested by bholley, however with this patch I'll still get the error in Comment 34.

Can you help to check where I did it wrong ?

Thank you.

	Bobby Holley (:bholley)
	Comment 46 • 10 years ago

Comment on attachment 8456006 [details] [diff] [review]
WIP. Part 4: TypedArray cross compartment problem

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

This looks like what I suggested in general. Is this a common pattern? If so, we should add a helper for it.

::: dom/system/gonk/NfcService.cpp
@@ +325,3 @@
>    JS::RootedValue val(cx);
>  
>    if (!ToJSValue(cx, aOptions, &val)) {

This only puts the options in the scope of the service, not the event itself. Is that the problem?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 47 • 10 years ago

(In reply to Bobby Holley (:bholley) from comment #46)
> This only puts the options in the scope of the service, not the event
> itself. Is that the problem?

Almost certainly.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 48 • 10 years ago

Comment on attachment 8456006 [details] [diff] [review]
WIP. Part 4: TypedArray cross compartment problem

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

Try using the listener instead of the service here.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 49 • 10 years ago

Nfc also implements nsINfcEventListener.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 50 • 10 years ago

rebase

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 52 • 10 years ago

forgot to add nsINfcEventListener in the Nfc.classInfo

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 53 • 10 years ago

Comment on attachment 8456810 [details] [diff] [review]
WIP. Part 4: TypedArray cross compartment problem v2

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

Kyle,
I have updated Part 1, to make Nfc implements nsINfcEventListener now.

However I still have the same problem. (Comment 34).

Also I don't quite understand what Bholley and you meant in Comment 46,
how do I put the val into the scope of Nfc?

Sorry for asking this again :(

Can you help to shed me some light here?

Thank you

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 54 • 10 years ago

No, that's not what I meant.  Find me on IRC today.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 55 • 10 years ago

using mListener, however still has the same problem

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 56 • 10 years ago

Move Cu.cloneInto back.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 58 • 10 years ago

Comment on attachment 8457916 [details] [diff] [review]
Part 1: NFC Service v8

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

r=me

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 59 • 10 years ago

Sorry for the delay.  Got caught up in 2.0 homescreen regressions :/

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 60 • 10 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #59)
> Sorry for the delay.  Got caught up in 2.0 homescreen regressions :/
No worries, Kyle. :)

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 61 • 10 years ago

Comment on attachment 8457917 [details] [diff] [review]
Part 2: NFC Message Handler v8

Mostly are the same, except some type changes after Part 1 changed.

	Dimi Lee [:dimi]
	Comment 62 • 10 years ago

Comment on attachment 8457917 [details] [diff] [review]
Part 2: NFC Message Handler v8

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

::: dom/system/gonk/NfcMessageHandler.cpp
@@ +58,5 @@
> +  } else if (!strcmp(type, kCloseRequest)) {
> +    result = CloseRequest(aParcel, aOptions);
> +    mPendingReqQueue.AppendElement(eNfcRequest_Close);
> +  }
> +

handle else here or give result a default value to prevent uninitiailized return value
same below

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 63 • 10 years ago

Add else part.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 64 • 10 years ago

Try looks green
https://tbpl.mozilla.org/?tree=Try&rev=8b127addae5d

Also marionette-webapi on emulator-JB on local have been passed.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] Assignee
	Comment 65 • 10 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/fc3b90212225
https://hg.mozilla.org/integration/b2g-inbound/rev/85744a1b9708
https://hg.mozilla.org/integration/b2g-inbound/rev/127ca52e4cce

	Ryan VanderMeulen [:RyanVM]
	Comment 66 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/fc3b90212225
https://hg.mozilla.org/mozilla-central/rev/85744a1b9708
https://hg.mozilla.org/mozilla-central/rev/127ca52e4cce