Carol Yang [:cyang] Reporter
	Description • 12 years ago

In GCF TC 27.22.4.10.3.1A, the test is ran as follows:

- Start GCF test which prompts for device power up
- After power up, hitting Continue from GCF test will send a proactive command to send short message with an icon display

At this point, the icon specified by the proactive command should be displayed, and not the alpha identifier "NO ICON".

Is there support for STK icon display?

	Joe Cheng [:jcheng] (please needinfo)
	Comment 1 • 12 years ago

Triage: BB-. STK display will show up in the bottom of Settings app (if your SIM has STK, goto Settings and scroll all the way down), you will see STK there.
In such case, is this still an issue? please renom if it is

	Michael Vines [:m1] [:evilmachines]
	Comment 2 • 12 years ago

Renoming, as this bug isnt about the location of the STK menu.   Thanks.

	Dylan Oliver [:doliver]
	Comment 3 • 12 years ago

Fernando, can you answer Carol's question re: icon support and provide a level of effort to fix if required?

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

Currently we don't support icon yet, because it's optional feature from spec, also this feature is not mandatory for the SIM apps from our partners.

	Fernando R. Sela (no CC, needinfo please) [:frsela]
	Comment 5 • 12 years ago

Yoshi answered before. Now it's not supported since it's optional

(In reply to Dylan Oliver [:doliver] from comment #3)
> Fernando, can you answer Carol's question re: icon support and provide a
> level of effort to fix if required?

	Ken Chang[:kenkai]
	Comment 6 • 11 years ago

Put this bug into backlog.

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

This is requirement from CAF for 2.1.
Set to 2.1? so that we can prioritize in 2.1

	Ken Chang[:kenkai]
	Comment 8 • 10 years ago

Hsinyi, you want to take this bug, right?

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 9 • 10 years ago

taken.

	Sandip Kamat
	Comment 10 • 10 years ago

Target 2.1

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 11 • 10 years ago

Some update about this bug... After some research, we realized that we may not need to add any Web API for this function, since the icon data is contained in the stk commands, and currently stk commands are sent using json data. We think the only thing we need to modify is the 'MozStkCommandEvent.webidl'.

From TS 101.267 Section 6.6, the proactive commands that may contain an icon are:
- DISPLAY TEXT
- GET INKEY
- GET INPUT
- PLAY TONE
- SET-UP MENU*
- SELECT ITEM*
- SEND SHORT MESSAGE/SS/USSD
- SET UP CALL
- SET UP IDEL MODE TEXT
- RUN AT COMMAND
- SEND DTMF COMMAND
- LAUNCH BROWSER (user confirmation phase)
- OPEN/CLOSE CHANNEL
- RECEIVE/SEND DATA

Among them, SET-UP MENU and SELECT ITEM may contain an item icon list for the menu items.

So, what I have in mind is to add a MozStkIcon dictionary in 'MozStkCommandEvent.webidl':

dictionary MozStkIcon
{
  /*
   * Width of the icon.
   */
  unsigned long width;

  /*
   * Height of the icon.
   */
  unsigned long height;

  /*
   * Array of pixels. Each pixel represents a color in the ARGB format made up
   * of four bytes, that is, the Alpha sample in the highest 8 bits, followed by
   * the Red sample, Green sample and the Blue sample in the lowest 8 bits. 
   */
  sequence<unsigned long> pixels;
}

And add the following for commands that support icon:

{
  // skipped...

  MozStkIcon icon;
  
  /*
   * to indicate whether the icon is self-explanatory, if so, it replaces the text.
   */
  boolean iconSelfExplanatory;
  
  // skipped...
}

I'd like to confirm with gaia if the MozStkIcon is okay for them to draw the icon.
Please let us know if there is any problem with this design, since I'm quite new at this. All suggestions are welcome!

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 12 • 10 years ago

May I have an early feedback from you about comment 11? Thanks.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 13 • 10 years ago

(In reply to Jessica Jong [:jjong] [:jessica] from comment #12)
> May I have an early feedback from you about comment 11? Thanks.

Hi Jessica,

The proposal looks good to me! As you, I am curious to know if MozStkIcon works for gaia though I think so.

	Fernando R. Sela (no CC, needinfo please) [:frsela]
	Comment 14 • 10 years ago

(In reply to Jessica Jong [:jjong] [:jessica] from comment #12)
> May I have an early feedback from you about comment 11? Thanks.

Is it specified the allowed formats for the pixel sequence? (PNG, JPEG, GIF, ...) or this is a raw image.

Theorically, we can display it it Gaia using [1] or if the blob image is not supported by this, using a canvas

Thank you !

[1] https://developer.mozilla.org/en-US/docs/Web/API/URL.createObjectURL

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 15 • 10 years ago

Please see my comments inline.

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #14)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #12)
> > May I have an early feedback from you about comment 11? Thanks.
> 
> Is it specified the allowed formats for the pixel sequence? (PNG, JPEG, GIF,
> ...) or this is a raw image.

This is raw data, without any compression, so that others can do anything with it.

> 
> Theorically, we can display it it Gaia using [1] or if the blob image is not
> supported by this, using a canvas
> 
> Thank you !
> 
> [1] https://developer.mozilla.org/en-US/docs/Web/API/URL.createObjectURL

I think you can use the pixels to create a canva's ImageData [1], something like this [2].
We can reorder the pixels into 'RGBA' order if it's more convenient for you.
What do you think? :)

[1] https://developer.mozilla.org/en-US/docs/Web/API/ImageData
[2] http://mxr.mozilla.org/gaia/source/apps/sms/js/wbmp.js#65

	Fernando R. Sela (no CC, needinfo please) [:frsela]
	Comment 16 • 10 years ago

Hi Jessica,

If it's raw data, the canvas solution is better.

Your proposal sounds great. And I agree to reorder pixels to reduce the overhead in system app.

Thank you !

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 17 • 10 years ago

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #16)
> Hi Jessica,
> 
> If it's raw data, the canvas solution is better.
> 
> Your proposal sounds great. And I agree to reorder pixels to reduce the
> overhead in system app.
> 
> Thank you !

Great! We will move forward with this proposal. Thanks for your feedback. :)

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 18 • 10 years ago

Hsinyi, may I have your feedback before asking dom peer for review? Thanks.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 19 • 10 years ago

Comment on attachment 8471446 [details] [diff] [review]
Part 1: webidl changes, v1.

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

Looks good, thank you.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 20 • 10 years ago

Change assignee as Jessica is working on this :)

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 22 • 10 years ago

Comment on attachment 8471460 [details]
emulator: add EFimg and EFiidf to emulator's sim (external/qemu) PR#112

Hi Vicamo, we are adding EFimgs and EFiidfs to support stk icon display. May I have your review? Thanks.

	Vicamo Yang [:vicamo][:vyang]
	Comment 23 • 10 years ago

Comment on attachment 8471460 [details]
emulator: add EFimg and EFiidf to emulator's sim (external/qemu) PR#112

A simple typo found. I will r+ this with one more entry with multiple image instance descriptors.

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 24 • 10 years ago

Comment on attachment 8471460 [details]
emulator: add EFimg and EFiidf to emulator's sim (external/qemu) PR#112

Thanks Vicamo. I've added a record 7 in EFimg, which contains two image instances, both referring to 4f02 but with different coding schemes. May I have your review again?

	Vicamo Yang [:vicamo][:vyang]
	Comment 25 • 10 years ago

Comment on attachment 8471460 [details]
emulator: add EFimg and EFiidf to emulator's sim (external/qemu) PR#112

Thank you :)

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 26 • 10 years ago

Per our discussion with Vicamo, we will change to return an array of icons, in case we need to return icons with multiple image instances. I have also exposed 'codingScheme' in MozStkIcon, so that users can select their preferred icon more easily. May I have your feedback again?

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 27 • 10 years ago

Comment on attachment 8472242 [details] [diff] [review]
Part 1: webidl changes, v2.

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

Looks good to me. Introducing codingScheme property is sweet!

	Vicamo Yang [:vicamo][:vyang]
	Comment 28 • 10 years ago

Comment on attachment 8472242 [details] [diff] [review]
Part 1: webidl changes, v2.

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

::: dom/webidl/MozStkCommandEvent.webidl
@@ +14,5 @@
> +   * @see TS 31.102, sub-clause 4.6.1.1, EFimg.
> +   */
> +  const unsigned short ICC_IMG_CODING_SCHEME_BASIC              = 0x11;
> +  const unsigned short ICC_IMG_CODING_SCHEME_COLOR              = 0x21;
> +  const unsigned short ICC_IMG_CODING_SCHEME_COLOR_TRANSPARENCY = 0x22;

enum IccImageCodingScheme { "basic", "color", "color-transparency" };

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 29 • 10 years ago

Address review comment 28:
- use enum instead of consts for image coding schemes.

	Vicamo Yang [:vicamo][:vyang]
	Comment 30 • 10 years ago

Comment on attachment 8472865 [details] [diff] [review]
Part 1: webidl changes, v3.

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

::: dom/webidl/MozStkCommandEvent.webidl
@@ +93,5 @@
> +   *
> +   * Array of icons, basically of a same image, that may differ in size,
> +   * resolution or coding scheme. The first icon should be the default one.
> +   */
> +  sequence<MozStkIcon> icons;

Shouldn't these new attributes be optional? From the implementation in bug 935843, they are not always assigned.

@@ +816,5 @@
>     */
>    unsigned short eventType;
>  };
> +
> +dictionary MozStkIcon

Please move it the line below IccImageCodingScheme definition.

@@ +838,5 @@
> +   * Array of pixels. Each pixel represents a color in the RGBA format made up
> +   * of four bytes, that is, the Red sample in the highest 8 bits, followed by
> +   * the Green sample, Blue sample and the Alpha sample in the lowest 8 bits.
> +   */
> +  sequence<unsigned long> pixels;

I find there is already a ImageData type [1] used by WebGL, but it doesn't quite fit our need. However, that still brings me another question -- should we use TypedArray, or Uint32Array specifically, instead?

[1]: http://mxr.mozilla.org/mozilla-central/source/dom/webidl/ImageData.webidl

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 31 • 10 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #30)
> Comment on attachment 8472865 [details] [diff] [review]
> Part 1: webidl changes, v3.
> 
> Review of attachment 8472865 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/MozStkCommandEvent.webidl
> @@ +93,5 @@
> > +   *
> > +   * Array of icons, basically of a same image, that may differ in size,
> > +   * resolution or coding scheme. The first icon should be the default one.
> > +   */
> > +  sequence<MozStkIcon> icons;
> 
> Shouldn't these new attributes be optional? From the implementation in bug
> 935843, they are not always assigned.

When icon was not an array, setting it to optional using '?' will fail to compile, the message was something like: "MozStkxxx has member with nullable dictionary type", but peeking at MozStkCommandEventBinding.h, mIcons was already surrounded by Optional<>, not sure why.
I can set it to optional now that it is a sequence, will change it in the next version.

> 
> @@ +816,5 @@
> >     */
> >    unsigned short eventType;
> >  };
> > +
> > +dictionary MozStkIcon
> 
> Please move it the line below IccImageCodingScheme definition.

Will do.

> 
> @@ +838,5 @@
> > +   * Array of pixels. Each pixel represents a color in the RGBA format made up
> > +   * of four bytes, that is, the Red sample in the highest 8 bits, followed by
> > +   * the Green sample, Blue sample and the Alpha sample in the lowest 8 bits.
> > +   */
> > +  sequence<unsigned long> pixels;
> 
> I find there is already a ImageData type [1] used by WebGL, but it doesn't
> quite fit our need. However, that still brings me another question -- should
> we use TypedArray, or Uint32Array specifically, instead?

I think both formats represents an array of unsigned 32-bit integers. Passing it as sequence to gaia, they can use it anyway they like. Mmm, I don't see any strong reason for changing into an Uint32Array.

> 
> [1]:
> http://mxr.mozilla.org/mozilla-central/source/dom/webidl/ImageData.webidl

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 32 • 10 years ago

(In reply to Jessica Jong [:jjong] [:jessica] from comment #31)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #30)
> > Comment on attachment 8472865 [details] [diff] [review]
> > Part 1: webidl changes, v3.
> > 
> > Review of attachment 8472865 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/webidl/MozStkCommandEvent.webidl
> > @@ +93,5 @@
> > > +   *
> > > +   * Array of icons, basically of a same image, that may differ in size,
> > > +   * resolution or coding scheme. The first icon should be the default one.
> > > +   */
> > > +  sequence<MozStkIcon> icons;
> > 
> > Shouldn't these new attributes be optional? From the implementation in bug
> > 935843, they are not always assigned.
> 
> When icon was not an array, setting it to optional using '?' will fail to
> compile, the message was something like: "MozStkxxx has member with nullable
> dictionary type", but peeking at MozStkCommandEventBinding.h, mIcons was
> already surrounded by Optional<>, not sure why.
> I can set it to optional now that it is a sequence, will change it in the
> next version.

Sorry, need to correct this:
1. MozStkIcon icon -> MozStkIcon mIcons (in MozStkCommandEventBinding.h)
2. sequence<MozStkIcon> icons -> Optional<Sequence<MozStkIcon>> mIcons
3. sequence<MozStkIcon>? icons -> Optional<Nullable<Sequence<MozStkIcon>>> mIcons

I think option 2 is enough, right?
And 'bool selfExplanatory' is already 'Optional<bool> mIconSelfExplanatory' in MozStkCommandEventBinding.h.

> 
> > 
> > @@ +816,5 @@
> > >     */
> > >    unsigned short eventType;
> > >  };
> > > +
> > > +dictionary MozStkIcon
> > 
> > Please move it the line below IccImageCodingScheme definition.
> 
> Will do.
> 
> > 
> > @@ +838,5 @@
> > > +   * Array of pixels. Each pixel represents a color in the RGBA format made up
> > > +   * of four bytes, that is, the Red sample in the highest 8 bits, followed by
> > > +   * the Green sample, Blue sample and the Alpha sample in the lowest 8 bits.
> > > +   */
> > > +  sequence<unsigned long> pixels;
> > 
> > I find there is already a ImageData type [1] used by WebGL, but it doesn't
> > quite fit our need. However, that still brings me another question -- should
> > we use TypedArray, or Uint32Array specifically, instead?
> 
> I think both formats represents an array of unsigned 32-bit integers.
> Passing it as sequence to gaia, they can use it anyway they like. Mmm, I
> don't see any strong reason for changing into an Uint32Array.
> 
> > 
> > [1]:
> > http://mxr.mozilla.org/mozilla-central/source/dom/webidl/ImageData.webidl

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 33 • 10 years ago

Hi smaug, we are adding some new attributes to the stk commands to support stk icon display.

We have some doubts about the webidl design. Since the icon is optional, these new attributes are supposed to be optional. However, peeking at the generated MozStkCommandEventBinding.h, we found that 'bool' and 'sequence' properties are already surrounded in Optional<>. Is that enough to represent that the property is optional? Is there need to use '?'?

Another question is, why using 'MozStkIcon? icon' results in compile error? How to represent an optional dictionary property?

One last question is, in our case, what is the preferable type for pixels?

So many questions, really appreciate any help you can provide. :)

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 34 • 10 years ago

Marionette tests for stk icon, based on the latest webidl (v4).

	Eric Chang [:ericcc] [:echang]
	Comment 35 • 10 years ago

1. Original STK test cases.
https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-status=active&filter-productversion=196&filter-suite=214
2. Test with emulator.
3. Currently we have no SIM with icon display support.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 36 • 10 years ago

> We have some doubts about the webidl design. Since the icon is optional,
> these new attributes are supposed to be optional. However, peeking at the
> generated MozStkCommandEventBinding.h, we found that 'bool' and 'sequence'
> properties are already surrounded in Optional<>. Is that enough to represent
> that the property is optional? Is there need to use '?'?
'?' means nullable. In a dictionary all the properties a optional by default, and that is why
when an event is codegen'ed, one must provide default values for the properties of the
*EventInit.


 
> Another question is, why using 'MozStkIcon? icon' results in compile error?
> How to represent an optional dictionary property?
Dictionary properties are optional by default, 
http://heycam.github.io/webidl/#idl-dictionaries
"On a given dictionary value, the presence of each dictionary member is optional. "
What is the compile error you get?

 
> One last question is, in our case, what is the preferable type for pixels?
I guess it depends on how the API will be used. sequence<unsigned long> pixels could be fine, 
other option is perhaps ArrayBuffer.

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 37 • 10 years ago

Thanks Olli for the information provided. :)

(In reply to Olli Pettay [:smaug] from comment #36)
> > We have some doubts about the webidl design. Since the icon is optional,
> > these new attributes are supposed to be optional. However, peeking at the
> > generated MozStkCommandEventBinding.h, we found that 'bool' and 'sequence'
> > properties are already surrounded in Optional<>. Is that enough to represent
> > that the property is optional? Is there need to use '?'?
> '?' means nullable. In a dictionary all the properties a optional by
> default, and that is why
> when an event is codegen'ed, one must provide default values for the
> properties of the
> *EventInit.

I see, then I think it's okay the way it is now.

> 
> 
>  
> > Another question is, why using 'MozStkIcon? icon' results in compile error?
> > How to represent an optional dictionary property?
> Dictionary properties are optional by default, 
> http://heycam.github.io/webidl/#idl-dictionaries
> "On a given dictionary value, the presence of each dictionary member is
> optional. "
> What is the compile error you get?

Oh, thanks for the info. The message I got is something like: "MozStkxxx has member with nullable dictionary type"

> 
>  
> > One last question is, in our case, what is the preferable type for pixels?
> I guess it depends on how the API will be used. sequence<unsigned long>
> pixels could be fine, 
> other option is perhaps ArrayBuffer.

ArrayBuffer is a good option, however, since the stkcommand message is json-stringified before sending to DOM, the data in the ArrayBuffer gets lost. So, I think sequence<unsigned long> is fine for now. 


From the above comments, the webidl seems okay the way it is. May I have your review? Thanks.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 38 • 10 years ago

Comment on attachment 8473470 [details] [diff] [review]
Part 1: webidl changes, v4.

Hmm, we have 
boolean iconSelfExplanatory;
and 
sequence<MozStkIcon> icons;

Couldn't we have some base dictionary which has those, and other dictionaries then extend that.
Though, oddly, MozStkItem gets only icons (and itemIconSelfExplanatory is in MozStkMenu)


But anyhow, some mix of inheritance of dictionaries would make this a bit simpler I think.

If you disagree, re-ask review.

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 39 • 10 years ago

(In reply to Olli Pettay [:smaug] from comment #38)
> Comment on attachment 8473470 [details] [diff] [review]
> Part 1: webidl changes, v4.
> 
> Hmm, we have 
> boolean iconSelfExplanatory;
> and 
> sequence<MozStkIcon> icons;
> 
> Couldn't we have some base dictionary which has those, and other
> dictionaries then extend that.
> Though, oddly, MozStkItem gets only icons (and itemIconSelfExplanatory is in
> MozStkMenu)
> 
> 
> But anyhow, some mix of inheritance of dictionaries would make this a bit
> simpler I think.
> 
> If you disagree, re-ask review.

Thanks for the review, just wanted to make sure I got your comments correctly.
Do you mean to have something like this:

dictionary MozStkIcon {
  boolean iconSelfExplanatory;
  sequence<MozStkIconData> icons; // MozStkIconData is a dict that contains width, height and pixels
};

and let MozStkTextMessage, MozStkMenu, etc, extend this interface?

Mmm, we could wrap iconSelfExplanatory and icons into a dictionary, or even simpler, just move iconSelfExplanatory into MozStkMenu, even though it is the same value for all icons. But I am not sure about the inheritance part, it makes the code simpler, but I think composition describes the relationship better between stk messages and stk icon. Does it make sense?

Let me know if I got you wrong and thanks again!

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 40 • 10 years ago

disctionary isn't an interface. It is just a dictionary.

I would perhaps call the dictionary MozStkIconContainer or some such.
Then MozStkMenu for example could be
dictionary MozStkMenu : MozStkIconContainer
{
  ...
}

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 41 • 10 years ago

(In reply to Olli Pettay [:smaug] from comment #40)
> disctionary isn't an interface. It is just a dictionary.
> 
> I would perhaps call the dictionary MozStkIconContainer or some such.
> Then MozStkMenu for example could be
> dictionary MozStkMenu : MozStkIconContainer
> {
>   ...
> }

Sounds good to me. I'll upload a new patch. Thanks!

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 42 • 10 years ago

itemIconSelfExplanatory is not needed, we'll use iconSelfExplanatory in each MozStkItem instead.

Olli, may I have your review again? Thanks.

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 43 • 10 years ago

This is based on bug 935843 part 1 - readIMG and readIIDF (v11).
I've added codingScheme property and moved iconSelfExplanatory into each menu item.

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 44 • 10 years ago

Sorry that I am going to be away until September 1st, so I'll leave my current WIPs based on Bug 935843 to hsinyi. I've tested the whole function with marionette tests and everything works well! Just need to pass through reviewer's review. Thanks!

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 45 • 10 years ago

rebased only.

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 46 • 10 years ago

Added some tests for icon with multiple image instances.

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 47 • 10 years ago

Comment on attachment 8474407 [details] [diff] [review]
Part 2: implementation, v2.

Hi Edgar, may I have your review? Thanks.

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 48 • 10 years ago

Comment on attachment 8474408 [details] [diff] [review]
Part 3: marionette tests, v2.

Hi Edgar, may I have your review? Thanks.

	Edgar Chen [:edgar]
	Comment 49 • 10 years ago

Comment on attachment 8474407 [details] [diff] [review]
Part 2: implementation, v2.

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

::: dom/system/gonk/ril_consts.js
@@ +678,5 @@
>  this.ICC_IMG_CODING_SCHEME_BASIC              = 0x11;
>  this.ICC_IMG_CODING_SCHEME_COLOR              = 0x21;
>  this.ICC_IMG_CODING_SCHEME_COLOR_TRANSPARENCY = 0x22;
>  
> +this.GECKO_IMG_CODING_SCHEME_BASIC              = "basic";

Please help add comment about the value of GECKO_IMG_CODING_SCHEME_* should be in sync with webidl.

@@ +681,5 @@
>  
> +this.GECKO_IMG_CODING_SCHEME_BASIC              = "basic";
> +this.GECKO_IMG_CODING_SCHEME_COLOR              = "color";
> +this.GECKO_IMG_CODING_SCHEME_COLOR_TRANSPARENCY = "color-transparency";
> +

Please add following consts for the coding scheme mapping,

this.ICC_IMG_CODING_SCHEME_TO_GECKO = {};
this.ICC_IMG_CODING_SCHEME_TO_GECKO[this.ICC_IMG_CODING_SCHEME_BASIC] = this.GECKO_IMG_CODING_SCHEME_BASIC;
...

::: dom/system/gonk/ril_worker.js
@@ +15804,5 @@
> +                                     rawData.width, rawData.height,
> +                                     rawData.bitsPerImgPoint,
> +                                     rawData.numOfClutEntries,
> +                                     rawData.clut, rawData.body);
> +    }

return null;

@@ +15810,5 @@
> +
> +  _convertCodingScheme: function(codingScheme) {
> +    switch(codingScheme) {
> +      case ICC_IMG_CODING_SCHEME_BASIC:
> +        return GECKO_IMG_CODING_SCHEME_BASIC;

Don't need this convert function.

@@ +15820,5 @@
> +        return GECKO_IMG_CODING_SCHEME_BASIC;
> +    }
> +  },
> +
> +  decodeBasicImage: function(width, height, body) {

Add a "_" prefix.

@@ +15837,5 @@
> +        if (pixelIndex % 8 == 0) {
> +          currentByte = body[currentByteIndex++];
> +          bitIndex = 7;
> +        }
> +        let bit = (currentByte >> bitIndex-- ) & 0x01;

nit: |bitIndex| can be calculated

let bit = (currentByte >> (7 - (pixelIndex % 8))) & 0x01;

@@ +15839,5 @@
> +          bitIndex = 7;
> +        }
> +        let bit = (currentByte >> bitIndex-- ) & 0x01;
> +        let color = bit == 1 ? WHITE : BLACK;
> +        pixels[pixelIndex++] = color;

nit: pixels[pixelIndex++] = bit ? WHITE : BLACK;

@@ +15848,5 @@
> +              width: width,
> +              height: height};
> +  },
> +
> +  decodeColorImage: function(codingScheme, width, height, bitsPerImgPoint,

Add a "_" prefix.

@@ +15879,5 @@
> +                  clutEntry == numOfClutEntries - 1;
> +      pixels[pixelIndex++] = convertToRGBA(clut[clutIndex],
> +                                           clut[clutIndex + 1],
> +                                           clut[clutIndex + 2],
> +                                           alpha ? 0x00 : 0xff);

Don't need |convertToRGBA|.

pixels[pixelIndex++] = alpha ? 0x00
                             : (clut[clutIndex] << 24 |
                                clut[clutIndex + 1] << 16 |
                                clut[clutIndex + 2] << 8 |
                                0xFF) >>> 0;

@@ +15885,5 @@
> +      bitIndex -= bitsPerImgPoint;
> +    }
> +
> +    return {pixels: pixels,
> +            codingScheme: this._convertCodingScheme(codingScheme),

Use |ICC_IMG_CODING_SCHEME_TO_GECKO|.

	Edgar Chen [:edgar]
	Comment 50 • 10 years ago

Comment on attachment 8474408 [details] [diff] [review]
Part 3: marionette tests, v2.

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

Looks good, thank you.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 51 • 10 years ago

Thank you Edgar. Let me address your comments and provide a revision for review. :)

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 52 • 10 years ago

comment 49 addressed.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 53 • 10 years ago

xpcshell test

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 54 • 10 years ago

Comment on attachment 8475756 [details] [diff] [review]
Part 2: implementation, v3.

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

Hi Edgar,

Your comments are addressed. Please help review again :)

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 55 • 10 years ago

Comment on attachment 8475757 [details] [diff] [review]
Part 4: xpcshell test, v1

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

Data format returned from iconLoader.loadIcons() is changed. This patch handles that.

	Edgar Chen [:edgar]
	Comment 56 • 10 years ago

Comment on attachment 8475756 [details] [diff] [review]
Part 2: implementation, v3.

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

Thank you. :)

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 57 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=5a1a02efce57 try green with emulator revision.

Hi Vicamo,
Would you please help merge emulator PR so that we can land following patches? Thank you.

	Vicamo Yang [:vicamo][:vyang]
	Comment 58 • 10 years ago

https://github.com/mozilla-b2g/platform_external_qemu/commit/a567a787b5ac3e0cb663aa6464b18a24ec764409

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 59 • 10 years ago

   https://hg.mozilla.org/integration/b2g-inbound/rev/9cf5b28c0fd3
   https://hg.mozilla.org/integration/b2g-inbound/rev/8e541d381d86
   https://hg.mozilla.org/integration/b2g-inbound/rev/d7bb9616831c
   https://hg.mozilla.org/integration/b2g-inbound/rev/b7bf0fb51d44

Thanks Jessica :)

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

https://hg.mozilla.org/mozilla-central/rev/9cf5b28c0fd3
https://hg.mozilla.org/mozilla-central/rev/8e541d381d86
https://hg.mozilla.org/mozilla-central/rev/d7bb9616831c
https://hg.mozilla.org/mozilla-central/rev/b7bf0fb51d44

	Eric Chang [:ericcc] [:echang]
	Comment 61 • 10 years ago

Verified landed, emulator okay.
Gaia      fbb297c39aab5f17b179533d2a9a6c5166b2c197
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/fb5e796da813
BuildID   20140902160204
Version   34.0a2
ro.build.version.incremental=110
ro.build.date=Fri Jun 27 15:57:58 CST 2014
B1TC00011230