975542 - Set user icon from image taken with the webcam.

Status: RESOLVED FIXED

(Instantbird Graveyard :: Contacts window, enhancement)

Description

[Mayank Kumar [:mayanktg]]

Present Feature:
To change the user icon one has to choose image using the file picker.

To implement:
Apart from having the default option to change icon, user should also be able to change it by taking picture using the webcam, previewing it and if it is acceptable applying the change.

What needs to be done:
* If the device supports webcam, the context menu should display an option to change user icon using webcam.
* A window should pop-up to take picture, preview it and apply/reject the new user icon.
* In the preview pane we can add feature to crop/re-size the picture.

Comment 1

[Mayank Kumar [:mayanktg]]

Attachment: WIP for the bug

WIP for the bug,
-It can now capture picture but having trouble setting the picture to its size.
-have to improve the UI
-have to save captured image to the profile directory of user.

Comment 2

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture

Unable to set the width of the captured image. Even though the width is set to 128px it gets stretched when takePicture() is called.

Comment 3

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Trouble saving image to disc contained in image tag using the source url. setWebcamImage() is used for this purpose.

Comment 4

[aleth [:aleth]]

saveURL (not defined in your patch) seems to be defined here: http://dxr.mozilla.org/comm-central/source/mozilla/toolkit/content/contentAreaUtils.js

Including this file seems like massive overkill.

If you look at the code there, you will find it ultimately uses nsIWebBrowserPersist.saveURI to save the content to disk. https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWebBrowserPersist

Basically what you need is a way to get the data out of the canvas and into a file. One way to do this is to use toDataURL and then follow the route above, but other ways may be easier.

Start by investigating https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement to see the methods that are available.

For example you could make a stream and then copy the stream to disk. This is basically how the code in xmpp.jsm:_saveIcon works.

Or you could get a blob and then find a way to save that. 

Or get a File object (https://developer.mozilla.org/en-US/docs/Web/API/File) and then copy that file to disk, using OS.File methods.

I don't know the best way to do this either, but this should give you some avenues to explore!

Comment 5

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Please suggest the changes I need to make.

Comment 6

[Benedikt Pfeifer]

Comment on attachment 8417156 [details] [diff] [review]
Bug975542_takePicture.diff

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

I haven't reviewed the strings and the XUL code, nor have I run the code yet.
Here's some feedback so that you have something to work with.
The rest will follow hopefully this evening (UTC).

::: im/content/blist.js
@@ +21,5 @@
>                  "user-icon-changed",
>                  "prpl-quit"];
>  
>  const showOfflineBuddiesPref = "messenger.buddies.showOffline";
> +const kPrefUserIconFilename  =  "messenger.status.userIconFileName";

This constant is never used, so you won't need it at all :)

@@ +631,4 @@
>    },
>  
> +  setWebcamImage: function bl_setWebcamImage() {
> +    Services.core.globalUserStatus.setUserIcon(null);

Why is this needed? chooseUserIcon() doesn't need to do that either, see https://mxr.mozilla.org/comm-central/source/im/content/blist.js#595, so it should work without it here too?

@@ +631,5 @@
>    },
>  
> +  setWebcamImage: function bl_setWebcamImage() {
> +    Services.core.globalUserStatus.setUserIcon(null);
> +    var x = document.getElementById("canvas");

Please use a more descriptive name than "x".
Also replace "var" by "let", please (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let).

@@ +635,5 @@
> +    var x = document.getElementById("canvas");
> +    x.toBlob(function(blob) {
> +      var read = new FileReader();
> +      read.addEventListener("loadend", function() {
> +        let view = new Int8Array(read.result);

Please add a comment here like "An ArrayBufferView is needed as input to OS.File.writeAtomic. Any other would have worked too."

@@ +636,5 @@
> +    x.toBlob(function(blob) {
> +      var read = new FileReader();
> +      read.addEventListener("loadend", function() {
> +        let view = new Int8Array(read.result);
> +        fileName = "userIcon.png";

If the file name isn't going to be more complicated than this (e.g. with random/unique parts) ...

@@ +637,5 @@
> +      var read = new FileReader();
> +      read.addEventListener("loadend", function() {
> +        let view = new Int8Array(read.result);
> +        fileName = "userIcon.png";
> +        let newName = OS.Path.join(OS.Constants.Path.tmpDir, fileName);

... you could inline (i.e. insert it directly) it here.

If you need to keep variable "fileName", you'll definitely need to declare it using "let" (that's the nicer version (because more local) of "var").

Maybe change that name to something that describes that this is not just a directory or file name but really a _path_ to you temporary file?

@@ +641,5 @@
> +        let newName = OS.Path.join(OS.Constants.Path.tmpDir, fileName);
> +        let promise = OS.File.writeAtomic(newName, view);
> +        promise.then(function() {
> +          let ui = Components.classes["@mozilla.org/file/local;1"]
> +                             .createInstance(Components.interfaces.nsILocalFile);

Components.classes are usually abbreviated by Cu, Components.interfaces by Ci.
These variables aren't existing in blist.js yet, so let's save this for later (as other occurences in this file would need to be updated and it would obscure your webcam related changes).

"ui" is also the commonn abbreviation for "user interface", so something different might help ("imageFile", "userIconFile", ... it's only used three times, so a londer name won't hurt a lot).

@@ +644,5 @@
> +          let ui = Components.classes["@mozilla.org/file/local;1"]
> +                             .createInstance(Components.interfaces.nsILocalFile);
> +          ui.initWithPath(newName);
> +          Services.core.globalUserStatus.setUserIcon(ui);
> +          document.getElementById("panel").hidePopup();

I think this should be moved out of the promise and callback and be run directly after toBlob() was called. The panel would go away immediately and everything else happens in the background. If there's a noticeable delay after closing the panel and before the icon appears in the contact list, we'd need to think about this again.

::: im/content/blist.xul
@@ +5,5 @@
>  
>  <?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
>  <?xml-stylesheet href="chrome://instantbird/skin/" type="text/css"?>
>  <?xml-stylesheet href="chrome://instantbird/content/blist.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://instantbird/content/takePicture.css" type="text/css"?>

Will go away after merging the stylesheet files...

@@ +126,5 @@
> +        <image id="userIcon" role="button" context="userIconContextMenu" popup="panel"
> +               aria-label="&userIcon.label;" tooltiptext="&userIcon.label;"/>
> +        <panel id="panel" type="arrow" height="210px" align="center"
> +               onpopupshowing="menus.updateUserIconContextMenuItems();">
> +          <deck id="userIconPanel">     

There are trailing spaces. You can set your editor to warn about them, or configure Hg to show colors on the console. They'll appear as bright red boxes then ;)

::: im/content/jar.mn
@@ +57,5 @@
>  	content/instantbird/proxy.xml
>  *	content/instantbird/tabbrowser.xml
>  	content/instantbird/tabbrowser.css
> +	content/instantbird/takePicture.js
> +	content/instantbird/takePicture.css

You won't need those after merging the files into blist.*

::: im/content/menus.js
@@ +129,5 @@
>  
> +  updateUserIconContextMenuItems: function menu_updateUserIconContextMenuItems() {
> +    document.getElementById("userIconPanel").
> +             setAttribute("selectedIndex", "0");
> +    document.getElementById("photo").

No need to wrap this line. It's not too long yet.

::: im/content/takePicture.css
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. */

You won't need a new file for this. Merging these rules into im/content/blist.css would be fine! :)

@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +video {

CSS rules should be as specific as possible. This rule would be applied to any video element that's in the document that this CSS file is applied on.
Set an id on the video element and use it for the rule and you're fine here.

@@ +4,5 @@
> +video {
> +	background-color: #000;
> +}
> +
> +canvas {

Same here.

@@ +8,5 @@
> +canvas {
> +	display: none;
> +}
> +
> +#photo {

Yep, ids like this! :)

::: im/content/takePicture.js
@@ +1,1 @@
> +navigator.getUserMedia = navigator.mozGetUserMedia;

You can merge this file into blist.js.

In my opinion you can just use mozGetUserMedia where you need it, we're going to unprefix things as soon as it's needed anyways.

@@ +1,3 @@
> +navigator.getUserMedia = navigator.mozGetUserMedia;
> +
> +var constraints = {audio: false, video: true};

We use "let" instead of "var"...

@@ +2,5 @@
> +
> +var constraints = {audio: false, video: true};
> +var video   = document.querySelector("video"),
> +	  canvas  = document.querySelector("#canvas"),
> +    photo   = document.querySelector('#photo');

Normally every line get it's own "let"/"var". We usually don't chain it using ",". Correct me if I'm wrong ;)
"canvas" needs its indentation corrected by the way.

If a variable is only used locally (like "photo"), please make it a variable only in that function (or inline it completely).

@@ +5,5 @@
> +	  canvas  = document.querySelector("#canvas"),
> +    photo   = document.querySelector('#photo');
> +
> +var ctx       = canvas.getContext('2d'),
> +    streaming = false,

What is this variable for?

@@ +7,5 @@
> +
> +var ctx       = canvas.getContext('2d'),
> +    streaming = false,
> +    width     = 640,
> +    height    = 480;

Please make these variable names more specific. Even now it is being imported to blist.xul and shares the scope with the other scripts imported from there. That means that if there's another variable with name "width"/... in any of those scripts, they might clash.

@@ +15,5 @@
> +  if (window.URL) {
> +    video.src = window.URL.createObjectURL(stream);
> +  } else {
> +    video.src = stream;
> +  }

## Need to look this up.

@@ +28,5 @@
> +function errorCallback(error){
> +  console.log("navigator.getUserMedia error: ", error);
> +}
> +
> +navigator.getUserMedia(constraints, successCallback, errorCallback);

That's the reason why the webcam starts as soon as the contact list loads. Please move this into the panel initialization code somewhere. How does one turn the camera off by the way?

Comment 7

[aleth [:aleth]]

Comment on attachment 8417156 [details] [diff] [review]
Bug975542_takePicture.diff

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

::: im/content/blist.js
@@ +650,5 @@
> +      });
> +      read.readAsArrayBuffer(blob);
> +    }, "image/png", 1.0);
> +
> +  },

It would be polite to delete the temporary file when it's not needed anymore.

Comment 8

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Please look into the following
* I'm unable to set resolution of cam less than 640X480.

+  stopWebcamStream: function menu_stopWebcamStream () {
+    //FIXME stream is undefined if camera remains closed.
+    stream.stop();
+  },
+
* 'stream' remains undefined if camera isn't started. stopWebcamStream is used as onpopuphiding.

Comment 9

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Note: Please enable WebRTC by adding MOZ_MEDIA_NAVIGATOR=1 to im/confvars.h and then performing a complete build of Ib.

*Fixed the above mentioned errors.
*Modified the panel menu closer to what Mic wanted it to be :) (https://wiki.instantbird.org/images/8/83/IbUserIconSelector.png)
*Added 4 new icons for the menu.

Comment 10

[Mayank Kumar [:mayanktg]]

Attachment: Change user Icon panel screenshot

Comment 11

[Patrick Cloke [:clokep]]

Attachment: Mockup from Mic originally at https://wiki.instantbird.org/images/8/83/IbUserIconSelector.png

(In reply to Mayank Kumar [:mayanktg] from comment #9)
> Created attachment 8421629 [details] [diff] [review]
> Bug975542_takePicture.diff
> 
> *Added 4 new icons for the menu.

Where are these icons from? (I.e. what's the license?)

Comment 12

[aleth [:aleth]]

(In reply to Mayank Kumar [:mayanktg] from comment #9)
> Note: Please enable WebRTC by adding MOZ_MEDIA_NAVIGATOR=1 to im/confvars.h
> and then performing a complete build of Ib.

This change should be part of your patch.

I don't think all of these icons look platform-native or mozilla-native. If you want to include icons, can you check if there is anything useable in m-c?

Comment 13

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #12)
> (In reply to Mayank Kumar [:mayanktg] from comment #9)
> > Note: Please enable WebRTC by adding MOZ_MEDIA_NAVIGATOR=1 to im/confvars.h
> > and then performing a complete build of Ib.
> 
> This change should be part of your patch.

Sorry, it's already there! I missed that.

Comment 14

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

*Updated icons
*Removed icon of "remove user icon" button

Comment 15

[Mayank Kumar [:mayanktg]]

Attachment: Change user Icon panel screenshot

Comment 16

[Florian Quèze [:florian]]

Attachment: user-icon-webcam.png

Here's a mockup of some idea I had to simplify the UI.

Comment 17

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> Here's a mockup of some idea I had to simplify the UI.

To clarify: the idea here is to style the "buttons" in a lightweight way, like at the bottom of the Australis hamburger menu.

Comment 18

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Modified as per Florian's mockup.
Changes:
* Video device detection: The "Take Image" button will only be enabled if a video camera is detected.
* simplified UI (remove button near the "userIconPanelImage" is enabled if a userIcon is set)

Comment 19

[Mayank Kumar [:mayanktg]]

Attachment: v2: Change user Icon panel screenshot

Comment 20

[Mayank Kumar [:mayanktg]]

Attachment: v3: Change user Icon panel screenshot

Comment 21

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Change-set:
* Video Display is in aspect ratio to that of icon.
* The aspect ratio of image taken is maintained with that of the icon.
* Flat buttons.

Comment 22

[Benedikt Pfeifer]

Comment on attachment 8426944 [details] [diff] [review]
Bug975542_takePicture.diff

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

Thanks!
This is a partial review only but I figured it would be nice to rather havesome feedback to work with than none at all. Will do the rest of it and the testing later today! Thanks for your patience.

::: im/content/blist.css
@@ +183,5 @@
> +.userIconPanelButton {
> +  -moz-appearance: none;
> +  -moz-box-orient: horizontal;
> +  font-size: 14px;
> +  border-top: 1px solid hsla(210,4%,10%,.14);

As far as I know we would format this rather like this: hsla(210, 4%, 10%, 0.14)

@@ +184,5 @@
> +  -moz-appearance: none;
> +  -moz-box-orient: horizontal;
> +  font-size: 14px;
> +  border-top: 1px solid hsla(210,4%,10%,.14);
> +  background-color: hsla(210,4%,10%,.07);

Formatting...

@@ +189,5 @@
> +  padding: 4px;
> +}
> +
> +.userIconPanelButton:hover  {
> +	background-color: hsla(210,4%,10%,.18);

Formatting, same for the rest of the file.

@@ +198,5 @@
> +  margin: 0px -20px 0px -10px;
> +}
> +
> +#takePictureMenuButton {
> +	margin-bottom: -10px;

Indentation, same for the rest of the file.

::: im/content/blist.js
@@ +670,5 @@
> +    let canvas = document.getElementById("userIconCanvas");
> +    canvas.toBlob(function(blob) {
> +      let read = new FileReader();
> +      read.addEventListener("loadend", function() {
> +        //An ArrayBufferView is needed as input to OS.File.WriteAtomic. Any

Please start with a space after the slashes.
"// An Array ..." instead of "//An Array ...".

@@ +671,5 @@
> +    canvas.toBlob(function(blob) {
> +      let read = new FileReader();
> +      read.addEventListener("loadend", function() {
> +        //An ArrayBufferView is needed as input to OS.File.WriteAtomic. Any
> +        //other would have worked too.

...

@@ +674,5 @@
> +        //An ArrayBufferView is needed as input to OS.File.WriteAtomic. Any
> +        //other would have worked too.
> +        let view    = new Int8Array(read.result),
> +            newName = OS.Path.join(OS.Constants.Path.tmpDir, "tmpUserIcon.png"),
> +            promise = OS.File.writeAtomic(newName, view);

A less generic name, please.

@@ +694,5 @@
> +             setAttribute("selectedIndex", "0");
> +    document.getElementById("webcamPhoto").removeAttribute("src");
> +    let icon         = Services.core.globalUserStatus.getUserIcon(),
> +        webcamButton = document.getElementById("takePictureMenuButton"),
> +        removeIcon   = document.getElementById("userIconPanelImageRemove");

Afaik our coding style is to have "let"s on every declaration. We usually don't chain them with commas afaik.

@@ +699,5 @@
> +    if (icon) {
> +      document.getElementById("userIconPanelImage").src = icon ? icon.spec : "";
> +      removeIcon.style.display = "block";
> +    }  else {
> +      document.getElementById("userIconPanelImage").

The dot goes to the next line.

@@ +700,5 @@
> +      document.getElementById("userIconPanelImage").src = icon ? icon.spec : "";
> +      removeIcon.style.display = "block";
> +    }  else {
> +      document.getElementById("userIconPanelImage").
> +               src = "chrome://instantbird/skin/userIcon.png";

i.e.:
    .src = "chrome://instantbird/skin/userIcon.png";

@@ +704,5 @@
> +               src = "chrome://instantbird/skin/userIcon.png";
> +      removeIcon.style.display = "none";
> +    }
> +    webcamButton.setAttribute("disabled", "true");
> +    //FIXME Bug 1011878. To call mozGetUserMediaDevices first call getUserMedia.

// FIXME: this is a workaroung for bug <...>: mozGetUserMediaDevices is currently only working after having called mozGetUserMedia at least once.

@@ +706,5 @@
> +    }
> +    webcamButton.setAttribute("disabled", "true");
> +    //FIXME Bug 1011878. To call mozGetUserMediaDevices first call getUserMedia.
> +    //There shouldn't be need to use getUserMedia.
> +    navigator.mozGetUserMedia(

Add a comment that calling getUserMedia with any parameters is good enough.

@@ +714,5 @@
> +    navigator.mozGetUserMediaDevices(
> +      { video: true,
> +        audio: false
> +      },
> +    devices => {

Most likely you won't even have to filter the list when you only requested video devices. Just checking the length should be fine! :)

::: im/content/blist.xul
@@ +128,2 @@
>          </box>
> +        <panel id="panel" type="arrow" align="center"

"panel" is a very general id, please choose something more specific.

@@ +134,5 @@
> +              <stack id="userIconPanelStack">
> +                <image id="userIconPanelImage"/>
> +                <image id="userIconPanelImageRemove" left="135" bottom="135"
> +                       tooltiptext="&removeIconCmd.label;"
> +                       onclick="buddyList.removeUserIcon();"/>

This should be a button because it gives us easy keyboard access. The handler need to be for "oncommand" (instead of mouse-only "onclick") if it should be called by the keyboard as well.

@@ +147,5 @@
> +	                            image="chrome://instantbird/skin/alert_camera.png"
> +	                            oncommand="buddyList.takePictureButton();"/>
> +	          </vbox>
> +	          <vbox>
> +	          	<stack id="webcamOverflow">

The indentation is wrong.

@@ +151,5 @@
> +	          	<stack id="webcamOverflow">
> +            	<html:video id="webcamVideo"></html:video>
> +            	</stack>
> +            	<html:canvas id="userIconCanvas"></html:canvas>
> +	            <hbox id="takePictureButtons">

Indentation...

::: im/locales/en-US/chrome/instantbird/instantbird.dtd
@@ +127,5 @@
>  <!ENTITY hideGroupTooltip              "Hide">
>  <!-- Context Menu to add/remove user icon -->
> +<!ENTITY chooseIconCmd.label           "Select file…">
> +<!ENTITY chooseIconCmd.accesskey       "S">
> +<!ENTITY takePictureCamCmd.label       "Take picture">

This label will need an ellipsis too because there are extra steps and the photo isn't taken immediately.

@@ +129,5 @@
> +<!ENTITY chooseIconCmd.label           "Select file…">
> +<!ENTITY chooseIconCmd.accesskey       "S">
> +<!ENTITY takePictureCamCmd.label       "Take picture">
> +<!ENTITY takePictureCamCmd.accesskey   "T">
> +<!ENTITY removeIconCmd.label           "Remove icon">

Do we want to call it 'icon'? Might be more technical than needed.

@@ +138,5 @@
> +<!ENTITY takePictureBackCmd.accesskey  "B">
> +<!ENTITY reshootCmd.label              "Reshoot">
> +<!ENTITY reshootCmd.accesskey          "R">
> +<!ENTITY setIconCmd.label              "Set Icon">
> +<!ENTITY setIconCmd.accesskey          "I">

Patrick had suggestions for the strings if I recall correctly. I think he said something about "Retake" instead of "Reshoot" to make the labels of the buttons use more similar terms.

::: im/themes/jar.mn
@@ +31,5 @@
>  	skin/classic/instantbird/newMessage.png
>  	skin/classic/instantbird/expand.png
>  	skin/classic/instantbird/collapse.png
>  	skin/classic/instantbird/userIcon.png
> +	skin/classic/instantbird/alert_camera.png

Where does this file come from? Are we sure it's licensed OK?

Comment 23

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

* Fixed indentations and other errors reviewd.
* added svg user icon in the panel area.
* The alert_camera.png image is taken from m-c drawable-mdpi icon-set. I guess it won't have the licensing problem.

Comment 24

[Florian Quèze [:florian]]

Comment on attachment 8427634 [details] [diff] [review]
Bug975542_takePicture.diff

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

I did only a quick coding style review. I'll definitely want to read the code at least once before we check it in :-).

::: im/content/blist.js
@@ +613,5 @@
> +    let width  = 213;
> +    let height = 160;
> +    window.stream = stream; // stream available to console
> +    if (window.URL) {
> +      video.src = window.URL.createObjectURL(stream);

You don't need the { } if there's only one line in the if body.

@@ +614,5 @@
> +    let height = 160;
> +    window.stream = stream; // stream available to console
> +    if (window.URL) {
> +      video.src = window.URL.createObjectURL(stream);
> +    } else {

\n before else.

@@ +618,5 @@
> +    } else {
> +      video.src = stream;
> +    }
> +    video.play();
> +    canvas.setAttribute('width', width);

Use double quotes everywhere unless there's a reason for using single quotes instead (a good reason is usually when the string you want to quote contains the " character).

@@ +623,5 @@
> +    canvas.setAttribute('height', height);
> +  },
> +
> +  webcamErrorCallback: function bl_webcamErrorCallback(error) {
> +    Cu.reportError("navigator.getUserMedia error: ", error);

I've never seen reportError called with 2 parameters, are you sure this works?

@@ +630,5 @@
> +  takePictureButton: function bl_takePictureButton() {
> +    let userIconPanel = document.getElementById("userIconPanel");
> +    userIconPanel.setAttribute("selectedIndex", "1");
> +    let constraints = {audio: false, video: true};
> +    navigator.mozGetUserMedia(constraints, this.webcamSuccessCallback, this.webcamErrorCallback);

This line is longer than 80 characters, and easy to wrap.

@@ +643,5 @@
> +    let ctx    = canvas.getContext("2d");
> +    ctx.save();
> +    canvas.setAttribute("width", "160px");
> +    ctx.drawImage(video, 80, 0, 480, 480, 0, 0, canvas.height, canvas.height);
> +    let data = canvas.toDataURL('image/png');

"

@@ +644,5 @@
> +    ctx.save();
> +    canvas.setAttribute("width", "160px");
> +    ctx.drawImage(video, 80, 0, 480, 480, 0, 0, canvas.height, canvas.height);
> +    let data = canvas.toDataURL('image/png');
> +    photo.setAttribute('src', data);

"

@@ +690,5 @@
> +    }, "image/png", 1.0);
> +  },
> +
> +  updateUserIconPanelItems: function bl_updateUserIconPanelItems() {
> +    document.getElementById("userIconPanel").

dot on the next line.

@@ +699,5 @@
> +    let removeIcon   = document.getElementById("userIconPanelImageRemove");
> +    if (icon) {
> +      document.getElementById("userIconPanelImage").src = icon ? icon.spec : "";
> +      removeIcon.style.display = "block";
> +    }  else {

Double space before "else" here.

Also, we always put a line break before "else" so that "else" is aligned with "if".

@@ +711,5 @@
> +    // mozGetUserMedia at least once.
> +    navigator.mozGetUserMedia(
> +      // Calling mozGetuserMedia with any parameters works.
> +      {audio: false, video: false},
> +      function() {},

No need for the line break here.

@@ +713,5 @@
> +      // Calling mozGetuserMedia with any parameters works.
> +      {audio: false, video: false},
> +      function() {},
> +      function() {});
> +    navigator.mozGetUserMediaDevices(

Add an empty line here, so that it's obvious where the end of the workaround is.

@@ +716,5 @@
> +      function() {});
> +    navigator.mozGetUserMediaDevices(
> +      { video: true,
> +        audio: false
> +      },

{audio: false, video: true},

Can be on a single line.

I wonder if {video: true}, wouldn't be enough.

@@ +723,5 @@
> +          webcamButton.removeAttribute("disabled");
> +      },
> +      error => {
> +        Components.utils.error("Error: " + error);
> +      } );

Remove space between } and )

@@ +726,5 @@
> +        Components.utils.error("Error: " + error);
> +      } );
> +  },
> +
> +  stopWebcamStream: function menu_stopWebcamStream () {

- No space between menu_stopWebcamStream and (
- Why "menu_" ? Shouldn't this be "bl_"?

@@ +728,5 @@
> +  },
> +
> +  stopWebcamStream: function menu_stopWebcamStream () {
> +    let video = document.getElementById("webcamVideo");
> +    if(video.src) stream.stop();

- space between if and (
- always a line break after if ()

::: im/content/blist.xul
@@ +144,5 @@
> +                             oncommand="buddyList.chooseUserIcon();"/>
> +              <toolbarbutton id="takePictureMenuButton" class="userIconPanelButton"
> +                             label="&takePictureCamCmd.label;"
> +                             accesskey="&takePictureCamCmd.accesskey;"
> +                             image="chrome://instantbird/skin/alert_camera.png"

If it's possible, setting this image should be done with CSS.

::: im/themes/jar.mn
@@ +31,5 @@
>  	skin/classic/instantbird/newMessage.png
>  	skin/classic/instantbird/expand.png
>  	skin/classic/instantbird/collapse.png
>  	skin/classic/instantbird/userIcon.png
> +	skin/classic/instantbird/alert_camera.png

Is there an @2x version of this file for Mac retina?

Comment 25

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

* added 2x alert_camera icons for retina display.

Comment 26

[Florian Quèze [:florian]]

Comment on attachment 8427670 [details] [diff] [review]
Bug975542_takePicture.diff

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

::: im/content/blist.js
@@ +610,5 @@
> +  webcamSuccessCallback: function bl_webcamSuccessCallback(stream) {
> +    let video  = document.getElementById("webcamVideo");
> +    let canvas = document.getElementById("userIconCanvas");
> +    let width  = 213;
> +    let height = 160;

These 2 variables are used only once, you can inline them.

@@ +612,5 @@
> +    let canvas = document.getElementById("userIconCanvas");
> +    let width  = 213;
> +    let height = 160;
> +    window.stream = stream; // stream available to console
> +    if (window.URL)

Put the "let video =" line just above this.

@@ +617,5 @@
> +      video.src = window.URL.createObjectURL(stream);
> +    else
> +      video.src = stream;
> +    video.play();
> +    canvas.setAttribute("width", width);

And the "let canvas =" line above this. And an empty line before.

@@ +637,5 @@
> +  takePicture: function bl_takePicture() {
> +    let userIconPanel = document.getElementById("userIconPanel");
> +    userIconPanel.setAttribute("selectedIndex", "2");
> +    let photo  = document.getElementById("webcamPhoto");
> +    let video  = document.getElementById("webcamVideo");

These 2 variables are used only once, you can inline.

@@ +710,5 @@
> +    // FIXME: This is a workaround for the Bug 1011878.
> +    // mozGetUserMediaDevices is currently only working after having called
> +    // mozGetUserMedia at least once.
> +    navigator.mozGetUserMedia(
> +      // Calling mozGetuserMedia with any parameters works.

I think it would be cleaner to put the comment about the function call, ie:

// Calling mozGetuserMedia with any parameters works.
navigator.mozGetUserMedia({audio: false, video: false},
                          function() {}, function() {});

@@ +714,5 @@
> +      // Calling mozGetuserMedia with any parameters works.
> +      {audio: false, video: false}, function() {}, function() {});
> +
> +    navigator.mozGetUserMediaDevices(
> +      { video: true },

{video: true}

@@ +725,5 @@
> +      });
> +  },
> +
> +  stopWebcamStream: function bl_stopWebcamStream() {
> +    let video = document.getElementById("webcamVideo");

You don't need this variable.

@@ +727,5 @@
> +
> +  stopWebcamStream: function bl_stopWebcamStream() {
> +    let video = document.getElementById("webcamVideo");
> +    if (video.src)
> +      stream.stop();

Where is this "stream" variable coming from?

Comment 27

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Thanks for correcting me up :)

@@ +725,5 @@
> +      });
> +  },
> +
> +  stopWebcamStream: function bl_stopWebcamStream() {
> +    let video = document.getElementById("webcamVideo");

Deleting the variable returns variable not defined error. Earlier in the function takePicture() we had to define "video" too. 

@@ +727,5 @@
> +
> +  stopWebcamStream: function bl_stopWebcamStream() {
> +    let video = document.getElementById("webcamVideo");
> +    if (video.src)
> +      stream.stop();

stream is the object passed with the webcamSuccesscallback(). If a stream is available then we can use stream.stop() to stop it.

Comment 28

[Florian Quèze [:florian]]

(In reply to Mayank Kumar [:mayanktg] from comment #27)

> > +  stopWebcamStream: function bl_stopWebcamStream() {
> > +    let video = document.getElementById("webcamVideo");
> 
> Deleting the variable returns variable not defined error. Earlier in the
> function takePicture() we had to define "video" too. 

I meant you can just use
    if (document.getElementById("webcamVideo").src)
instead of
    let video = document.getElementById("webcamVideo");
    if (video.src)

> > +  stopWebcamStream: function bl_stopWebcamStream() {
> > +    let video = document.getElementById("webcamVideo");
> > +    if (video.src)
> > +      stream.stop();
> 
> stream is the object passed with the webcamSuccesscallback(). If a stream is
> available then we can use stream.stop() to stop it.

I don't see it defined in the scope of "stopWebcamStream".

Comment 29

[Benedikt Pfeifer]

Comment on attachment 8427670 [details] [diff] [review]
Bug975542_takePicture.diff

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

Another partial review...

::: im/content/blist.css
@@ +189,5 @@
> +.userIconPanelButton {
> +  -moz-appearance: none;
> +  -moz-box-orient: horizontal;
> +  font-size: 14px;
> +  border-top: 1px solid hsla(210,4%,10%, 0.14);

The previous review comment hadn't just added the leading zero but also spaces after the commas. That might have been not obvious tbh. Please add them here and everywhere else where needed.

@@ +215,5 @@
> +  }
> +}
> +
> +#takePicture, #takePictureBackButton,
> +#reshoot, #setIcon {

I'd prefer consistent IDS for the buttons by the way (reshootButton

::: im/content/blist.js
@@ +611,5 @@
> +    let video  = document.getElementById("webcamVideo");
> +    let canvas = document.getElementById("userIconCanvas");
> +    let width  = 213;
> +    let height = 160;
> +    window.stream = stream; // stream available to console

What does this comment mean? It'd need to be a proper sentence with a leading capital and a dot at the end if it's going to stay.

@@ +612,5 @@
> +    let canvas = document.getElementById("userIconCanvas");
> +    let width  = 213;
> +    let height = 160;
> +    window.stream = stream; // stream available to console
> +    if (window.URL)

You won't need this afaik? It seems to come from a web example that needed to work with Chrome and Opera which handle the stream differently as it looks like.

Unconditional "video.src = window.URL.createObjectURL(stream);" should be enough for us?

@@ +627,5 @@
> +  },
> +
> +  takePictureButton: function bl_takePictureButton() {
> +    let userIconPanel = document.getElementById("userIconPanel");
> +    userIconPanel.setAttribute("selectedIndex", "1");

document.getElementById("userIconPanel")
	.setAttribute("selectedIndex", "1");

Please check the other functions if some of the variables there can easily be dropped too.

@@ +643,5 @@
> +    let ctx    = canvas.getContext("2d");
> +    ctx.save();
> +    canvas.setAttribute("width", "160px");
> +    ctx.drawImage(video, 80, 0, 480, 480, 0, 0, canvas.height, canvas.height);
> +    let data = canvas.toDataURL("image/png");

You can inline canvas.toDataURL() if you only use it once. Saves us creating a variable.

@@ +644,5 @@
> +    ctx.save();
> +    canvas.setAttribute("width", "160px");
> +    ctx.drawImage(video, 80, 0, 480, 480, 0, 0, canvas.height, canvas.height);
> +    let data = canvas.toDataURL("image/png");
> +    photo.setAttribute("src", data);

Same here: document.getElementById("webcamPhoto").setAttribute(...);

@@ +650,5 @@
> +  },
> +
> +  takePictureBackButton: function bl_takePictureBackButton() {
> +    let userIconPanel = document.getElementById("userIconPanel");
> +    userIconPanel.setAttribute("selectedIndex", "0");

Here too...

::: im/locales/en-US/chrome/instantbird/instantbird.dtd
@@ +128,5 @@
>  
>  <!ENTITY hideGroupTooltip              "Hide">
>  
>  <!-- Context Menu to add/remove user icon -->
> +<!ENTITY chooseIconCmd.label           "Select file…">

When changing a string we need to change its name, so that localizers can notice the change.

@@ +136,2 @@
>  <!ENTITY removeIconCmd.label           "Remove">
>  <!ENTITY removeIconCmd.accesskey       "R">

That's no longer used, is it? Let's remove it for now.

@@ +138,5 @@
> +<!ENTITY takePictureCmd.label          "Capture">
> +<!ENTITY takePictureCmd.accesskey      "C">
> +<!ENTITY takePictureBackCmd.label      "Back">
> +<!ENTITY takePictureBackCmd.accesskey  "B">
> +<!ENTITY reshootCmd.label              "Retake">

Maybe call that retakePicture now that the label has changed?

Comment 30

[Florian Quèze [:florian]]

(In reply to Benedikt Pfeifer [:Mic] from comment #29)

> > +    window.stream = stream; // stream available to console
> > +    if (window.URL)
> 
> You won't need this afaik? It seems to come from a web example that needed
> to work with Chrome and Opera which handle the stream differently as it
> looks like.
> 
> Unconditional "video.src = window.URL.createObjectURL(stream);" should be
> enough for us?

The correct solution for us is:
  video.mozSrcObject = stream;


> @@ +136,2 @@
> >  <!ENTITY removeIconCmd.label           "Remove">
> >  <!ENTITY removeIconCmd.accesskey       "R">
> 
> That's no longer used, is it? Let's remove it for now.

Isn't it still in the icon's context menu?

Comment 31

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Comment 32

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Change-set:
* added icons for "Select File…" and "Take Picture…"

Comment 33

[Mayank Kumar [:mayanktg]]

Attachment: Panel menu with icons for buttons.

Comment 34

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

* menu button icons aligned with userIcon of the panel

Comment 35

[Mayank Kumar [:mayanktg]]

Attachment: Panel menu with icons for buttons.

Comment 36

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Comment 37

[Mayank Kumar [:mayanktg]]

Attachment: Panel menu with icons for buttons.

Comment 38

[Florian Quèze [:florian]]

Comment on attachment 8428291 [details] [diff] [review]
Bug975542_takePicture.diff

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

Looks pretty good. I left several comments, but I think they are all easy to address. I'll probably want to actually try the next version of the patch :-).

::: im/content/blist.css
@@ +157,5 @@
>    max-width: 0;
>  }
> +
> +#userIconPanelStack {
> +  margin: 10px 0px 20px 10px;

nit: I think at some point someone replaced all the "0px" by "0" in our css files.

This css file is in content/, it should only contain the CSS code that determines _what_ is displayed (this is typically -moz-binding and display rules). Rules that determine _how_ things are displayed (eg. margin, font, background, ...) should go in the themes/ folder.

::: im/content/blist.js
@@ +4,5 @@
>  
> +const Cu = Components.utils;
> +Cu.import("resource:///modules/imStatusUtils.jsm");
> +Cu.import("resource:///modules/imXPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/osfile.jsm");

You use OS.File only if the user sets a new icon, so please import it with a lazy getter.

@@ +606,5 @@
>      if (fp.show() == nsIFilePicker.returnOK)
>        Services.core.globalUserStatus.setUserIcon(fp.file);
>    },
>  
> +  webcamSuccessCallback: function bl_webcamSuccessCallback(stream) {

aStream (a means "argument").

I think here you need to check that the panel hasn't been closed yet, and if it's been closed, stop the stream immediately and return.

@@ +607,5 @@
>        Services.core.globalUserStatus.setUserIcon(fp.file);
>    },
>  
> +  webcamSuccessCallback: function bl_webcamSuccessCallback(stream) {
> +    buddyList.stream = stream;

buddyList.stream is too generic. Please use a more descriptive name.

@@ +614,5 @@
> +    video.play();
> +
> +    let canvas = document.getElementById("userIconCanvas");
> +    canvas.setAttribute("width", 213);
> +    canvas.setAttribute("height", 160);

Can't you set these attributes in the .xul file?

@@ +617,5 @@
> +    canvas.setAttribute("width", 213);
> +    canvas.setAttribute("height", 160);
> +  },
> +
> +  webcamErrorCallback: function bl_webcamErrorCallback(error) {

aError

@@ +618,5 @@
> +    canvas.setAttribute("height", 160);
> +  },
> +
> +  webcamErrorCallback: function bl_webcamErrorCallback(error) {
> +    Cu.reportError(error);

If this is all you are doing, could you use Cu.reportError directly as the callback?

@@ +623,5 @@
> +  },
> +
> +  takePictureButton: function bl_takePictureButton() {
> +    document.getElementById("userIconPanel")
> +            .setAttribute("selectedIndex", "1");

replace .setAttribute("selectedIndex", "1"); with .selectedIndex = 1;

And this may now fit on one 80 char columns line instead of 2.

(applies in all the other places where you have similar code of course).

@@ +635,5 @@
> +            .setAttribute("selectedIndex", "2");
> +    let canvas = document.getElementById("userIconCanvas");
> +    let ctx    = canvas.getContext("2d");
> +    ctx.save();
> +    canvas.setAttribute("width", "160px");

Set this attribute in the xul file?

@@ +676,5 @@
> +          let userIconFile = Cc["@mozilla.org/file/local;1"]
> +                             .createInstance(Ci.nsILocalFile);
> +          userIconFile.initWithPath(newName);
> +          Services.core.globalUserStatus.setUserIcon(userIconFile);
> +          userIconFile.remove(newName);

Having to write to a temporary file and remove it is unfortunate, but I'm not going to ask you to change it here. Could you please file a bug to make user icon handling in chat/ be asynchronous with OS.File?

@@ +688,5 @@
> +    document.getElementById("userIconPanel")
> +            .setAttribute("selectedIndex", "0");
> +    document.getElementById("webcamPhoto").removeAttribute("src");
> +    let icon         = Services.core.globalUserStatus.getUserIcon();
> +    let webcamButton = document.getElementById("takePictureMenuButton");

Define this variable before its first use.

@@ +691,5 @@
> +    let icon         = Services.core.globalUserStatus.getUserIcon();
> +    let webcamButton = document.getElementById("takePictureMenuButton");
> +    let removeIcon   = document.getElementById("userIconPanelImageRemove");
> +    if (icon) {
> +      document.getElementById("userIconPanelImage").src = icon ? icon.spec : "";

This code is confused. You null-check icon twice.

@@ +696,5 @@
> +      removeIcon.style.display = "block";
> +    }
> +    else {
> +      document.getElementById("userIconPanelImage")
> +              .src = "chrome://instantbird/skin/userIcon.svg";

Can this be done as a CSS background applied if userIconPanelImage doesn't have an src attribute?

@@ +699,5 @@
> +      document.getElementById("userIconPanelImage")
> +              .src = "chrome://instantbird/skin/userIcon.svg";
> +      removeIcon.style.display = "none";
> +    }
> +    webcamButton.setAttribute("disabled", "true");

Move this line immediately before the gUMD call. Wouldn't webcamButton.disabled = true; work here?

@@ +700,5 @@
> +              .src = "chrome://instantbird/skin/userIcon.svg";
> +      removeIcon.style.display = "none";
> +    }
> +    webcamButton.setAttribute("disabled", "true");
> +    // FIXME: This is a workaround for the Bug 1011878.

Empty line before this comment.

@@ +705,5 @@
> +    // mozGetUserMediaDevices is currently only working after having called
> +    // mozGetUserMedia at least once.
> +    // Calling mozGetuserMedia with any parameters works.
> +    navigator.mozGetUserMedia(
> +      {audio: false, video: false}, function() {}, function() {});

This would be more readable this way:
navigator.mozGetUserMedia({audio: false, video: false},
                          function() {}, function() {});

@@ +715,5 @@
> +          webcamButton.removeAttribute("disabled");
> +      },
> +      error => {
> +        Cu.reportError(error);
> +      });

Can this be simplified to:
navigator.mozGetUserMediaDevices({video: true},
                                 devices => { webcamButton.disabled = !devices.length; },
                                 Cu.reportError);
?

@@ +718,5 @@
> +        Cu.reportError(error);
> +      });
> +  },
> +
> +  stopWebcamStream: function bl_stopWebcamStream() {

You need to remove webcamPhoto's src attribute when the panel is closed, otherwise that long data URL will stay in memory until the window is closed.

@@ +719,5 @@
> +      });
> +  },
> +
> +  stopWebcamStream: function bl_stopWebcamStream() {
> +    if (document.getElementById("webcamVideo").mozSrcObject)

check buddyList.stream instead.

@@ +720,5 @@
> +  },
> +
> +  stopWebcamStream: function bl_stopWebcamStream() {
> +    if (document.getElementById("webcamVideo").mozSrcObject)
> +      buddyList.stream.stop();

You also need to set buddyList.stream to null, otherwise a reference to the stream would be kept until the window is closed.

::: im/content/blist.xul
@@ +123,5 @@
>      <toolbar id="statusArea">
>        <stack id="statusImageStack">
>          <!-- The box around the user icon is a workaround for bug 955673. -->
>          <box id="userIconHolder">
> +          <image id="userIcon" role="button" context="userIconContextMenu" popup="changeUserIconPanel"

So are you keeping or removing the context menu?
(If you keep it, wrap this long line. If you don't, remove the context attribute and the unused entities in the .dtd file)

@@ +137,5 @@
> +                <button id="userIconPanelImageRemove" left="135" bottom="135"
> +                        tooltiptext="&removeIconCmd.label;"
> +                        oncommand="buddyList.removeUserIcon();"/>
> +              </stack>
> +              <toolbarbutton id="selectFileMenuButton" class="userIconPanelButton"

What's the meaning of "Menu" in this id?

@@ +141,5 @@
> +              <toolbarbutton id="selectFileMenuButton" class="userIconPanelButton"
> +                             label="&selectFileCmd.label;"
> +                             accesskey="&selectFileCmd.accesskey;"
> +                             oncommand="buddyList.chooseUserIcon();"/>
> +              <toolbarbutton id="takePictureMenuButton" class="userIconPanelButton"

Same question here.

@@ +148,5 @@
> +                             oncommand="buddyList.takePictureButton();"/>
> +            </vbox>
> +            <vbox>
> +              <stack id="webcamOverflow">
> +                <html:video id="webcamVideo"></html:video>

Is <html:video id="webcamVideo"/> not working?

@@ +150,5 @@
> +            <vbox>
> +              <stack id="webcamOverflow">
> +                <html:video id="webcamVideo"></html:video>
> +              </stack>
> +              <html:canvas id="userIconCanvas"></html:canvas>

Same question here.

::: im/themes/jar.mn
@@ +37,5 @@
>  #ifdef XP_UNIX
>  #ifdef XP_MACOSX
> +	skin/classic/instantbird/tag.png                (tag-mac.png)
> +	skin/classic/instantbird/[e-mail]             ([e-mail])
> +	skin/classic/instantbird/actionicon-tab.png     (pscactionicon-tab-mac.png)

Is "psc" here a typo?

::: im/themes/userIcon.svg
@@ +8,5 @@
> +   xmlns:svg="http://www.w3.org/2000/svg"
> +   xmlns="http://www.w3.org/2000/svg"
> +   xmlns:xlink="http://www.w3.org/1999/xlink"
> +   xmlns:sodipodi="http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd"
> +   xmlns:inkscape="http://www.inkscape.org/namespaces/inkscape"

I wonder if there's an automated way to cleanup svg files. I assume all the stuff in the sodipodi or inskape namespaces isn't used by Gecko.

Comment 39

[Mayank Kumar [:mayanktg]]

Thanks a lot for taking a look into the patch and reviewing it. I did the changes you mentioned but had a few queries about some changes.

> @@ +606,5 @@
> >      if (fp.show() == nsIFilePicker.returnOK)
> >        Services.core.globalUserStatus.setUserIcon(fp.file);
> >    },
> >  
> > +  webcamSuccessCallback: function bl_webcamSuccessCallback(stream) {
> 
> aStream (a means "argument").
> 
> I think here you need to check that the panel hasn't been closed yet, and if
> it's been closed, stop the stream immediately and return.
The webcamSuccesscallback isn't called until we click on "Take Picture..." button. Why do we need to check then? If we have some reason for it, please tell how to check if a panel is closed? idk :(

> @@ +635,5 @@
> > +            .setAttribute("selectedIndex", "2");
> > +    let canvas = document.getElementById("userIconCanvas");
> > +    let ctx    = canvas.getContext("2d");
> > +    ctx.save();
> > +    canvas.setAttribute("width", "160px");
> 
> Set this attribute in the xul file?
canvas "width" attr is being reassigned to 160px here. Earlier it was "213px" for the video stream. We are using overflow to hide the video and then set image with width 160px.
> 
> @@ +676,5 @@
> > +          let userIconFile = Cc["@mozilla.org/file/local;1"]
> > +                             .createInstance(Ci.nsILocalFile);
> > +          userIconFile.initWithPath(newName);
> > +          Services.core.globalUserStatus.setUserIcon(userIconFile);
> > +          userIconFile.remove(newName);
> 
> Having to write to a temporary file and remove it is unfortunate, but I'm
> not going to ask you to change it here. Could you please file a bug to make
> user icon handling in chat/ be asynchronous with OS.File?
Filed Bug 1015678 for this. :)
> 
> @@ +696,5 @@
> > +      removeIcon.style.display = "block";
> > +    }
> > +    else {
> > +      document.getElementById("userIconPanelImage")
> > +              .src = "chrome://instantbird/skin/userIcon.svg";
> 
> Can this be done as a CSS background applied if userIconPanelImage doesn't
> have an src attribute?
I tried using image.style.background = "url('chrome://instantbird/skin/userIcon.svg')"; to set the image as background but the image doesn't fit correctly. Why we have to set background inside a <image/> ? 
> 
> @@ +718,5 @@
> > +        Cu.reportError(error);
> > +      });
> > +  },
> > +
> > +  stopWebcamStream: function bl_stopWebcamStream() {
> 
> You need to remove webcamPhoto's src attribute when the panel is closed,
> otherwise that long data URL will stay in memory until the window is closed.
I would have never thought about it. Thank you so much :)
> 
> ::: im/themes/userIcon.svg
> @@ +8,5 @@
> > +   xmlns:svg="http://www.w3.org/2000/svg"
> > +   xmlns="http://www.w3.org/2000/svg"
> > +   xmlns:xlink="http://www.w3.org/1999/xlink"
> > +   xmlns:sodipodi="http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd"
> > +   xmlns:inkscape="http://www.inkscape.org/namespaces/inkscape"
> 
> I wonder if there's an automated way to cleanup svg files. I assume all the
> stuff in the sodipodi or inskape namespaces isn't used by Gecko.
The sodipodi and inkscape namespaces seemed to be used by Gecko. Though there was an error caused by an undefined css expression "enable-background:accumulate;" I removed it.

Comment 40

[aleth [:aleth]]

(In reply to Mayank Kumar [:mayanktg] from comment #39)
> > > +      document.getElementById("userIconPanelImage")
> > > +              .src = "chrome://instantbird/skin/userIcon.svg";
> > 
> > Can this be done as a CSS background applied if userIconPanelImage doesn't
> > have an src attribute?
> I tried using image.style.background =
> "url('chrome://instantbird/skin/userIcon.svg')"; to set the image as
> background but the image doesn't fit correctly. Why we have to set
> background inside a <image/> ? 

flo is referring to something like http://mxr.mozilla.org/comm-central/source/im/themes/blist.css#343.

Comment 41

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Comment 42

[Florian Quèze [:florian]]

Comment on attachment 8428387 [details] [diff] [review]
Bug975542_takePicture.diff

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

::: im/content/blist.css
@@ +157,5 @@
>    max-width: 0;
>  }
> +
> +#userIconPanelImageRemove {
> +  -moz-appearance: none;

-moz-apperance is theming too.

::: im/content/blist.js
@@ +5,5 @@
> +const Cu = Components.utils;
> +Cu.import("resource:///modules/imStatusUtils.jsm");
> +Cu.import("resource:///modules/imXPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyGetter(this, "OS", function() {

XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");

@@ +610,5 @@
>      if (fp.show() == nsIFilePicker.returnOK)
>        Services.core.globalUserStatus.setUserIcon(fp.file);
>    },
>  
> +  webcamSuccessCallback: function bl_webcamSuccessCallback(aStream) {

You wrote this method this way:

if (ok) {
  // do stuff
}
else {
  // handle error condition
  return; <- note that the return here isn't actually useful.
}


We tend to prefer handling errors this way:

if (not ok) {
  // handle error
  return;
}

// Do stuff


This avoids increasing the indent level of the code actually doing stuff.

@@ +611,5 @@
>        Services.core.globalUserStatus.setUserIcon(fp.file);
>    },
>  
> +  webcamSuccessCallback: function bl_webcamSuccessCallback(aStream) {
> +    if(document.getElementById("changeUserIconPanel").state == "open") {

Space between "if" and "("

@@ +613,5 @@
>  
> +  webcamSuccessCallback: function bl_webcamSuccessCallback(aStream) {
> +    if(document.getElementById("changeUserIconPanel").state == "open") {
> +      buddyList.webcamStream = aStream;
> +      let video  = document.getElementById("webcamVideo");

Why the double space here?

@@ +637,5 @@
> +    let canvas = document.getElementById("userIconCanvas");
> +    let ctx    = canvas.getContext("2d");
> +    ctx.save();
> +    canvas.setAttribute("width", "160px");
> +    let video  = document.getElementById("webcamVideo");

video = (avoid the double space)

@@ +647,5 @@
> +
> +  captureBackButton: function bl_captureBackButton() {
> +    document.getElementById("userIconPanel").selectedIndex = 0;
> +    document.getElementById("webcamPhoto").removeAttribute("src");
> +    buddyList.webcamStream.stop();

Are you sure buddyList.webcamStream isn't null here?

@@ +664,5 @@
> +  setWebcamImage: function bl_setWebcamImage() {
> +    let canvas = document.getElementById("userIconCanvas");
> +    canvas.toBlob(function(blob) {
> +      let read = new FileReader();
> +      read.addEventListener("loadend", function() {

Add a comment somewhere around here saying that writing the new icon to a temporary file to then create an nsIFile to pass it to Service.core is temporary, and indicate the bug number.

@@ +692,5 @@
> +    let image        = document.getElementById("userIconPanelImage");
> +    image.src = icon ? icon.spec : "";
> +    removeIcon.style.display = "block";
> +    if (!image.src) {
> +      removeIcon.style.display = "none";

Would something like this in im/content/blist.css work?
#userIconPanelImage:not([src]) + #userIconPanelImageRemove {
  display: none;
}

@@ +712,5 @@
> +  stopWebcamStream: function bl_stopWebcamStream() {
> +    if (buddyList.webcamStream) {
> +      buddyList.webcamStream.stop();
> +      buddyList.webcamStream = null;
> +    }

I think you have this code in at least 3 places already; what about calling this method in the other places?

@@ +716,5 @@
> +    }
> +
> +    let webcamPhoto = document.getElementById("webcamPhoto");
> +    if (webcamPhoto.src)
> +      webcamPhoto.removeAttribute("src");

removeAttribute doesn't fail if you remove an attribute that doesn't exist, so I think you can just do:
document.getElementById("webcamPhoto").removeAttribute("src");

::: im/locales/en-US/chrome/instantbird/instantbird.dtd
@@ +136,1 @@
>  <!ENTITY removeIconCmd.label           "Remove">

Is removeIconCmd.label still used?

::: im/themes/userIcon.svg
@@ +15,5 @@
> +   id="svg2"
> +   version="1.1"
> +   inkscape:version="0.48.1 r9760"
> +   sodipodi:docname="placeholder.svg"
> +   inkscape:export-filename="placeholder.png"

Still plenty of useless lines in this file...

Comment 43

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Comment 44

[Mayank Kumar [:mayanktg]]

(In reply to Florian Quèze [:florian] [:flo] from comment #42)

> ::: im/content/blist.js
> @@ +5,5 @@
> > +const Cu = Components.utils;
> > +Cu.import("resource:///modules/imStatusUtils.jsm");
> > +Cu.import("resource:///modules/imXPCOMUtils.jsm");
> > +
> > +XPCOMUtils.defineLazyGetter(this, "OS", function() {
> 
> XPCOMUtils.defineLazyModuleGetter(this, "OS",
> "resource://gre/modules/osfile.jsm");
I tried using this way. I guess we have to "return OS".
 
> Would something like this in im/content/blist.css work?
> #userIconPanelImage:not([src]) + #userIconPanelImageRemove {
>   display: none;
> }
This worked :)
#userIconPanelImage[src=""] + #userIconPanelImageRemove {
  display: none;
}

> ::: im/locales/en-US/chrome/instantbird/instantbird.dtd
> @@ +136,1 @@
> >  <!ENTITY removeIconCmd.label           "Remove">
> 
> Is removeIconCmd.label still used?
Yes we use this as a tooltiptext now in the "userPanelImageRemove" button

> ::: im/themes/userIcon.svg
> @@ +15,5 @@
> > +   id="svg2"
> > +   version="1.1"
> > +   inkscape:version="0.48.1 r9760"
> > +   sodipodi:docname="placeholder.svg"
> > +   inkscape:export-filename="placeholder.png"
> 
> Still plenty of useless lines in this file...
Yes you're right. I removed most of the useless svg items in that file.

Comment 45

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Comment 46

[Mayank Kumar [:mayanktg]]

(In reply to Mayank Kumar [:mayanktg] from comment #44)
> (In reply to Florian Quèze [:florian] [:flo] from comment #42)
> 
> > ::: im/content/blist.js
> > @@ +5,5 @@
> > > +const Cu = Components.utils;
> > > +Cu.import("resource:///modules/imStatusUtils.jsm");
> > > +Cu.import("resource:///modules/imXPCOMUtils.jsm");
> > > +
> > > +XPCOMUtils.defineLazyGetter(this, "OS", function() {
> > 
> > XPCOMUtils.defineLazyModuleGetter(this, "OS",
> > "resource://gre/modules/osfile.jsm");
> I tried using this way. I guess we have to "return OS".
Sorry. Corrected it with your version.
> > ::: im/themes/userIcon.svg
> > @@ +15,5 @@
> > > +   id="svg2"
> > > +   version="1.1"
> > > +   inkscape:version="0.48.1 r9760"
> > > +   sodipodi:docname="placeholder.svg"
> > > +   inkscape:export-filename="placeholder.png"
> > 
> > Still plenty of useless lines in this file...
> Yes you're right. I removed most of the useless svg items in that file.
Updated it with your version of simplified svg.

Comment 47

[Florian Quèze [:florian]]

Comment on attachment 8428864 [details] [diff] [review]
Bug975542_takePicture.diff

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

::: im/content/blist.js
@@ +4,5 @@
> +const Cu = Components.utils;
> +Cu.import("resource:///modules/imStatusUtils.jsm");
> +Cu.import("resource:///modules/imXPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");

nit: Group this with the other Cu.import line, and keep an empty line before |const events = ...|
And keep an empty line after the license header.

@@ +606,5 @@
>      fp.appendFilters(nsIFilePicker.filterImages);
>      if (fp.show() == nsIFilePicker.returnOK)
>        Services.core.globalUserStatus.setUserIcon(fp.file);
>    },
> +  webcamSuccessCallback: function bl_webcamSuccessCallback(aStream) {

Empty line before this method.

@@ +607,5 @@
>      if (fp.show() == nsIFilePicker.returnOK)
>        Services.core.globalUserStatus.setUserIcon(fp.file);
>    },
> +  webcamSuccessCallback: function bl_webcamSuccessCallback(aStream) {
> +    if (!document.getElementById("changeUserIconPanel").state == "open") {

I thought we discussed on IRC that this check also needs to verify that the deck's selected index is still the one showing the <video> tag (ie. 1)?

@@ +610,5 @@
> +  webcamSuccessCallback: function bl_webcamSuccessCallback(aStream) {
> +    if (!document.getElementById("changeUserIconPanel").state == "open") {
> +      this.stopWebcamStream();
> +      return;
> +    }

Empty line here.

@@ +629,5 @@
> +    document.getElementById("userIconPanel").selectedIndex = 2;
> +    let canvas = document.getElementById("userIconCanvas");
> +    let ctx    = canvas.getContext("2d");
> +    ctx.save();
> +    canvas.setAttribute("width", "160px");

I still don't understand why this is done here rather than in the xul file. You said that 2 different widths are used, but you never set it to what it was before, so something may be broken the second time the panel is used.

@@ +675,5 @@
> +          userIconFile.remove(newName);
> +        });
> +      });
> +      read.readAsArrayBuffer(blob);
> +    }, "image/png", 1.0);

Shouldn't you also call stopWebcamStream here? Or maybe just close the panel automatically after the image has been set?

::: im/themes/blist.css
@@ +477,5 @@
> +}
> +
> +#userIconPanelImage[src=""] + #userIconPanelImageRemove {
> +  display: none;
> +}

This goes in the content css file.

@@ +486,5 @@
> +  max-width: 16px;
> +  min-width: 16px;
> +  background-color: transparent;
> +  border: none;
> +  list-style-image: url("chrome://global/skin/icons/close.png");

Do we have an @2x version of this image for mac retina screens?

::: im/themes/userIcon.svg
@@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?><svg xmlns:svg="http://www.w3.org/2000/svg" xmlns="http://www.w3.org/2000/svg" width="200" height="200"><g transform="translate(-593.0434,-353.33237)"><path style="color:#000000;fill:#000000;fill-opacity:1;fill-rule:nonzero;stroke:none;marker:none;visibility:visible;display:inline;overflow:visible;opacity:0.25" d="m 695.71945,383.01987 c 12.99568,-0.18543 27.54264,9.28722 32.68749,23.15625 1.10135,2.96893 0.35572,9.33095 0.48842,17.94559 0.0163,1.05579 1.15948,0.9485 1.13658,3.36189 0.0726,11.11488 -0.38857,10.67578 -2.65625,16.19252 -1.21066,2.94527 -3.44869,5.675 -4.125,6.5 -0.90544,1.1045 -1.33784,5.6612 -2.48252,10.03848 -0.74519,2.8496 -2.37021,5.39197 -2.64248,7.64902 -0.46861,3.88467 -0.27865,4.26356 2.65625,5.78125 1.73497,0.89719 5.99391,6.64639 8.06742,8.51969 4.95607,4.47751 12.18524,6.97286 24.28939,9.66364 21.38687,4.75436 23.17433,13.29621 24.4043,18.94868 3.29386,15.13729 4.39991,31.26088 4.14514,42.55549 l -175.84375,0 c 0.24325,-11.68664 1.09066,-23.37944 2.70257,-34.82756 1.51802,-7.09096 2.85602,-14.00598 10.07869,-19.57869 2.0217,-1.32557 10.6625,-4.99668 19.1875,-8.15625 13.82906,-5.12537 19.77455,-5.63791 23.875,-9.8125 2.52774,-2.57346 5.79024,-5.86662 7.25,-7.0625 2.19453,-1.79784 2.875,-3.38459 2.875,-7.3125 0,-2.54681 -2.0481,-5.06245 -3.29704,-9.10439 -0.69649,-2.25403 -1.15597,-5.51703 -1.94451,-7.72496 -0.87616,-2.45327 -3.51067,-4.49829 -4.21319,-6.10815 -2.39128,-5.47973 -2.77289,-9.87187 -2.82149,-15.24191 -0.0274,-3.02488 1.23321,-2.96242 1.40123,-4.75809 0.31551,-3.37176 0.39419,-7.34167 0.6875,-11.46875 0.44951,-6.325 1.35424,-12.37426 1.90625,-13.5 3.38086,-6.89469 8.97806,-9.80777 15.0625,-10.5 2.17085,-0.24698 4.11891,-1.16156 6.8125,-2.59375 3.15812,-1.67919 6.67371,-2.51058 10.3125,-2.5625 z"/></g></svg>
\ No newline at end of file

This is not the last version I pastebined.

Comment 48

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Comment 49

[Mayank Kumar [:mayanktg]]

(In reply to Florian Quèze [:florian] [:flo] from comment #47)
> Comment on attachment 8428864 [details] [diff] [review]
> Bug975542_takePicture.diff

> @@ +675,5 @@
> > +          userIconFile.remove(newName);
> > +        });
> > +      });
> > +      read.readAsArrayBuffer(blob);
> > +    }, "image/png", 1.0);
> 
> Shouldn't you also call stopWebcamStream here? Or maybe just close the panel
> automatically after the image has been set?
We have called hidePopup() above which hides the popup and thus stopWebcamStream is called onpopuphiding. Is calling hidePopup enough?  
> @@ +486,5 @@
> > +  max-width: 16px;
> > +  min-width: 16px;
> > +  background-color: transparent;
> > +  border: none;
> > +  list-style-image: url("chrome://global/skin/icons/close.png");
> 
> Do we have an @2x version of this image for mac retina screens?
We don't have similar red colored @2x icons for retina screens, but there are grey close icons
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/icons/close.png
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/icons/[e-mail]

Comment 50

[aleth [:aleth]]

Comment on attachment 8429034 [details] [diff] [review]
Bug975542_takePicture.diff

Jamie, could I ask you to give mayanktg (one of our gsoc students) some feedback on whether this new UI is sufficiently accessible?

Comment 51

[James Teh [:Jamie]]

(In reply to aleth [:aleth] from comment #50)
> Jamie, could I ask you to give mayanktg (one of our gsoc students) some
> feedback on whether this new UI is sufficiently accessible?
Unfortunately, I don't have a build environment set up and am not likely to have time to do this in the near future. Do you have the ability to make try builds?

Comment 52

[aleth [:aleth]]

(In reply to James Teh [:Jamie] from comment #51)
> (In reply to aleth [:aleth] from comment #50)
> > Jamie, could I ask you to give mayanktg (one of our gsoc students) some
> > feedback on whether this new UI is sufficiently accessible?
> Unfortunately, I don't have a build environment set up and am not likely to
> have time to do this in the near future. Do you have the ability to make try
> builds?

Thanks -- we don't have try builds unfortunately. The best way forward is probably for this to land in nightlies, and to handle any issues in followups.

Comment 53

[Benedikt Pfeifer]

I'll have a look with DOMi at the accessibility properties and see if there's something that obviously needs some attention.

Comment 54

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Button icons added for Aero.

Comment 55

[aleth [:aleth]]

http://log.bezut.info/instantbird/140609/#m728 "other OS dependent files don't have their own folders but contain the OS name in their files names and are renamed while being packaged."

I think this is a bad idea in this case. There are two conventions for this, one is the older one used by (most of) IB and one is the newer one with OS-specific folders (now) used by TB and FX. We should move towards the newer one, because it is much less confusing. But especially here, where we are using files copied from m-c and it is much less confusing for the future if we retain the filenames and the folder structure.

Comment 56

[Benedikt Pfeifer]

Comment on attachment 8437247 [details] [diff] [review]
Bug975542_takePicture.diff

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

::: im/content/blist.js
@@ +614,5 @@
> +      this.stopWebcamStream();
> +      return;
> +    }
> +
> +    buddyList.webcamStream = aStream;

this.webcamStream

@@ +659,5 @@
> +    let canvas = document.getElementById("userIconCanvas");
> +    canvas.toBlob(function(blob) {
> +      let read = new FileReader();
> +      read.addEventListener("loadend", function() {
> +        // FIXME: This is a workaround for Bug 1015678.

Side note: normally we follow the style to enclose multi-line comments in /* <...> */ . This file seems to do that differently. So keep it like you have it already for consistency.

@@ +701,5 @@
> +                                     Cu.reportError);
> +  },
> +
> +  stopWebcamStream: function bl_stopWebcamStream() {
> +    if (buddyList.webcamStream) {

"this.webcamStream"

@@ +702,5 @@
> +  },
> +
> +  stopWebcamStream: function bl_stopWebcamStream() {
> +    if (buddyList.webcamStream) {
> +      buddyList.webcamStream.stop();

...

@@ +703,5 @@
> +
> +  stopWebcamStream: function bl_stopWebcamStream() {
> +    if (buddyList.webcamStream) {
> +      buddyList.webcamStream.stop();
> +      buddyList.webcamStream = null;

"delete this.webcamStream;"

::: im/themes/blist.css
@@ +483,5 @@
> +  min-width: 16px;
> +  background-color: transparent;
> +  border: none;
> +  list-style-image: url("chrome://global/skin/icons/close.png");
> +  -moz-image-region: rect(0, 16px, 16px, 0);

So there are only grey icons for 'close' in high resolution as you say? If that's what's used for retina displays then maybe we should do that too.

::: im/themes/jar.mn
@@ +39,5 @@
> +	skin/classic/instantbird/tag.png                (tag-mac.png)
> +	skin/classic/instantbird/[e-mail]             ([e-mail])
> +	skin/classic/instantbird/actionicon-tab.png     (actionicon-tab-mac.png)
> +	skin/classic/instantbird/[e-mail]  ([e-mail])
> +	skin/classic/instantbird/webcam.png             (webRTC-shareDevice-16.png)

This is the same file for all three OS'. We can package it unconditionally instead of duplicating the same line three times.

@@ +40,5 @@
> +	skin/classic/instantbird/[e-mail]             ([e-mail])
> +	skin/classic/instantbird/actionicon-tab.png     (actionicon-tab-mac.png)
> +	skin/classic/instantbird/[e-mail]  ([e-mail])
> +	skin/classic/instantbird/webcam.png             (webRTC-shareDevice-16.png)
> +	skin/classic/instantbird/[e-mail]          (webRTC-shareDevice-16-mac.png)

Is this an image with high/retina resolution? If yes then the original name should reflect that, i.e. contain an "@2x" and not just a "-mac".

@@ +41,5 @@
> +	skin/classic/instantbird/actionicon-tab.png     (actionicon-tab-mac.png)
> +	skin/classic/instantbird/[e-mail]  ([e-mail])
> +	skin/classic/instantbird/webcam.png             (webRTC-shareDevice-16.png)
> +	skin/classic/instantbird/[e-mail]          (webRTC-shareDevice-16-mac.png)
> +	skin/classic/instantbird/select-file.png        (select-file.png)

This is also OS independent -> unconditional packaging.

::: im/themes/userIcon.svg
@@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="no"?><svg xmlns="http://www.w3.org/2000/svg" width="200" height="200"><path transform="translate(-593.0434,-353.33237)" style="opacity:0.25" d="m 695.71945,383.01987 c 12.99568,-0.18543 27.54264,9.28722 32.68749,23.15625 1.10135,2.96893 0.35572,9.33095 0.48842,17.94559 0.0163,1.05579 1.15948,0.9485 1.13658,3.36189 0.0726,11.11488 -0.38857,10.67578 -2.65625,16.19252 -1.21066,2.94527 -3.44869,5.675 -4.125,6.5 -0.90544,1.1045 -1.33784,5.6612 -2.48252,10.03848 -0.74519,2.8496 -2.37021,5.39197 -2.64248,7.64902 -0.46861,3.88467 -0.27865,4.26356 2.65625,5.78125 1.73497,0.89719 5.99391,6.64639 8.06742,8.51969 4.95607,4.47751 12.18524,6.97286 24.28939,9.66364 21.38687,4.75436 23.17433,13.29621 24.4043,18.94868 3.29386,15.13729 4.39991,31.26088 4.14514,42.55549 l -175.84375,0 c 0.24325,-11.68664 1.09066,-23.37944 2.70257,-34.82756 1.51802,-7.09096 2.85602,-14.00598 10.07869,-19.57869 2.0217,-1.32557 10.6625,-4.99668 19.1875,-8.15625 13.82906,-5.12537 19.77455,-5.63791 23.875,-9.8125 2.52774,-2.57346 5.79024,-5.86662 7.25,-7.0625 2.19453,-1.79784 2.875,-3.38459 2.875,-7.3125 0,-2.54681 -2.0481,-5.06245 -3.29704,-9.10439 -0.69649,-2.25403 -1.15597,-5.51703 -1.94451,-7.72496 -0.87616,-2.45327 -3.51067,-4.49829 -4.21319,-6.10815 -2.39128,-5.47973 -2.77289,-9.87187 -2.82149,-15.24191 -0.0274,-3.02488 1.23321,-2.96242 1.40123,-4.75809 0.31551,-3.37176 0.39419,-7.34167 0.6875,-11.46875 0.44951,-6.325 1.35424,-12.37426 1.90625,-13.5 3.38086,-6.89469 8.97806,-9.80777 15.0625,-10.5 2.17085,-0.24698 4.11891,-1.16156 6.8125,-2.59375 3.15812,-1.67919 6.67371,-2.51058 10.3125,-2.5625 z"/></svg>
\ No newline at end of file

It might be possible to remove the 'transform="translate(...)"' as well by using a tool that applies the transform once and for all on the coordinates / the path. I don't know if it's worth finding out how to do that though.

Comment 57

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

*Added grey close@2x icons for remove icon button.
*The panel buttons are now keyboard accessible.
*Further compressed the userIcon.svg by removing transforms and reducing the decimal precision.

Comment 58

[Florian Quèze [:florian]]

Comment on attachment 8439128 [details] [diff] [review]
Bug975542_takePicture.diff

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

* Clicking "Set Icon" or "Back" should close the webcam immediately. It's OK if you keep it running until "Set Icon" is clicked, to have the webcam working immediately if the user clicks "Retake". I'm really surprised (and disappointed) to find such an issue in a patch I thought was (almost) ready.

* Theming on Mac needs work. Screenshots: http://imgur.com/goWUXMB,M62waFi,Z7pcUaS
The first one is on a retina screen. The second one is on an external screen. Notice that on the third one, the margin on the right side of the two button is smaller than on the second screenshot.

* The select file and take picture icons don't seem to be of the same color. Could you take both icons from the Firefox toolbar to have something consistent?

* No 'hand' pointer over the [X] button please.

* Clicking the "Capture" button before the webcam is ready causes this error:
Timestamp: 15/06/14 18:34:18
Error: NS_ERROR_NOT_AVAILABLE: 
Source File: chrome://instantbird/content/blist.js
Line: 637

That button should be disabled until the webcam is ready.

::: im/content/blist.js
@@ +609,5 @@
>    },
>  
> +  webcamSuccessCallback: function bl_webcamSuccessCallback(aStream) {
> +    if (!(document.getElementById("changeUserIconPanel").state == "open") ||
> +        !(document.getElementById("userIconPanel").selectedIndex == 1)) {

!(blah == foo) should be simplified to blah != foo

Comment 59

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

For the UI changes in Mac and Windows, Mic has agreed to try to fix it up for Windows and nhnt11 for Mac. :)

Comment 60

[Mayank Kumar [:mayanktg]]

Attachment: v4: Change user icon panel with buttons

Comment 61

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Changes to fix Mac issues

Here's a diff of the changes I made to get the following things working on Mac:
- setting width and height to 16px makes the icon sizes work properly (both retina and normal)
- The other changes fix the margins for the buttons on the initial view ("Select file" and "Take picture").

Issues: Margins on the buttons for the other views are still off. A couple of icons are not cropped correctly (retina select-file icon and non-retina webcam icon), please check.

For the other views, I tried and failed to get the margins to look correct. At this point my suggestion is to put the buttons in separate boxes (with zero padding), and put the image/preview in its own box (with appropriate padding). This should help to avoid using arbitrary negative margins everywhere (to an extent). I hope this isn't unreasonable. Please ask for the others' opinions on this as well.

Comment 62

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Changesets:
* Changed icons for Select File and Take Picture buttons. They have a hover state now.
* The "remove icon" button now doesn't focus when hovered over the user icon.
* The buttons in the panel are now in a separate box "userIconButtonBox". It can be used to fix the margin error occurring on retina screens(L484 of the patch).

Comment 63

[Mayank Kumar [:mayanktg]]

Attachment: v5: Change user icon panel with buttons

Comment 64

[Mayank Kumar [:mayanktg]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #61)
> Created attachment 8441326 [details] [diff] [review]
> Changes to fix Mac issues
Thanks a lot Nihanth for taking a look into my issue, your suggestions helped me a lot. I applied the changes you mentioned and the icons for the "Select File" and "Take Picture"  buttons must be fixed for Mac, thanks again! I have also changed the icons and made sure they are cropped correctly :)
Moving the buttons to a box certainly helped and almost all the negative margins have been removed (except for removing the default margins set in the panel).

Comment 65

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Comment 66

[Florian Quèze [:florian]]

Comment on attachment 8441735 [details] [diff] [review]
Bug975542_takePicture.diff

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

Here are pictures of the appearance on Mac: http://i.imgur.com/EpCi3it.png http://i.imgur.com/rjUKGM2.png

::: im/content/blist.js
@@ +610,5 @@
>  
> +  webcamSuccessCallback: function bl_webcamSuccessCallback(aStream) {
> +    if ((document.getElementById("changeUserIconPanel").state != "open") ||
> +        (document.getElementById("userIconPanel").selectedIndex != 1)) {
> +      this.stopWebcamStream();

Are you sure this works? See comment later.

@@ +614,5 @@
> +      this.stopWebcamStream();
> +      return;
> +    }
> +
> +    this.webcamStream = aStream;

I don't think you actually need this, as you can access the stream later from the .mozSrcObject property.

@@ +624,5 @@
> +
> +  takePictureButton: function bl_takePictureButton() {
> +    document.getElementById("userIconPanel").selectedIndex = 1;
> +    navigator.mozGetUserMedia({audio: false, video: true},
> +                              this.webcamSuccessCallback,

You probably need a .bind(this) here.

@@ +685,5 @@
> +  updateUserIconPanelItems: function bl_updateUserIconPanelItems() {
> +    document.getElementById("userIconPanel").selectedIndex = 0;
> +    let icon         = Services.core.globalUserStatus.getUserIcon();
> +    let webcamButton = document.getElementById("takePictureButton");
> +    let image        = document.getElementById("userIconPanelImage");

This 'image' variable is used only once, you don't really need it.

@@ +695,5 @@
> +    // Calling mozGetuserMedia with any parameters works.
> +    navigator.mozGetUserMedia({audio: false, video: false},
> +                              function() {}, function() {});
> +
> +    webcamButton.disabled = true;

Move the |let webcamButton = ...| line to right before this one.

@@ +702,5 @@
> +                                     Cu.reportError);
> +  },
> +
> +  stopWebcamStream: function bl_stopWebcamStream() {
> +    let webcamStream = this.webcamSuccessCallback.webcamStream;

It's very strange that this works, as you did this.webcamStream = aStream;

If the this value during that function's execution is the function itself, the |this.stopWebcamStream();| call is likely broken.

@@ +705,5 @@
> +  stopWebcamStream: function bl_stopWebcamStream() {
> +    let webcamStream = this.webcamSuccessCallback.webcamStream;
> +    if (webcamStream) {
> +      webcamStream.stop();
> +      delete webcamStream;

The delete call here won't do much, as you are just deleting a local variable.

I think you want something like:
let video = document.getElementById("webcamVideo");
let stream = video.mozSrcObject;
if (stream) {
  stream.stop();
  video.mozSrcObject = null;
}

::: im/content/blist.xul
@@ +137,5 @@
> +                <button id="userIconPanelImageRemove" left="135" bottom="135"
> +                        tooltiptext="&removeIconCmd.label;"
> +                        oncommand="buddyList.removeUserIcon();"/>
> +              </stack>
> +              <vbox id="userIconButtonBox">

Can you remove this vbox?

@@ +154,5 @@
> +              <stack id="webcamOverflow">
> +                <html:video id="webcamVideo"/>
> +              </stack>
> +              <html:canvas id="userIconCanvas" width="160" height="160"/>
> +              <vbox id="userIconButtonBox">

What's this vbox for? It doesn't seem needed.

@@ +170,5 @@
> +              </vbox>
> +            </vbox>
> +            <vbox>
> +              <image id="webcamPhoto"/>
> +              <vbox id="userIconButtonBox">

same here.

::: im/themes/blist.css
@@ +512,5 @@
> +  background-color: hsla(210, 4%, 10%, 0.18);
> +}
> +
> +#userIconButtonBox {
> +  position: absolute;

What are you achieving with this? It seems strange in a xul markup.

@@ +598,5 @@
> +#webcamVideo {
> +  height: 156px;
> +  min-width: 213px;
> +  max-width: 213px;
> +  margin-left: -30px;

What is this negative margin for?

::: im/themes/jar.mn
@@ +156,5 @@
>  	skin/classic/aero/instantbird/tabbrowser/mainwindow-dropdown-arrow.png	(tabbrowser-winstripe/mainwindow-dropdown-arrow.png)
> +	skin/classic/aero/instantbird/webcam.png            (webRTC-shareDevice-16.png)
> +	skin/classic/aero/instantbird/select-file.png       (select-file.png)
> +	skin/classic/instantbird/[e-mail]          ([e-mail])
> +	skin/classic/instantbird/[e-mail]     ([e-mail])

Why have you copied these 2 mac retina files at the end of the Windows aero section?

Comment 67

[James Teh [:Jamie]]

Removing myself from cc for now, but please feel free to cc me again once this is in a build and you want for a11y feedback.

Comment 68

[Mayank Kumar [:mayanktg]]

Attachment: Take Picture using webcam patch

Comment 69

[(inactive) Nihanth Subramanya [:nhnt11]]

This latest patch fixes the issues with the buttons on OS X (retina and non-retina screens) with one change - Make the toolbarbuttons on the second and third panels just buttons. I don't think you need toolbarbuttons here (no icons), and buttons intrinsically occupy as much space as possible instead of trying to best-fit their text labels. This change fixes a few pixels of whitespace at the right edge, and ensures that the left and right buttons are equally sized. I can confirm that the active states of the buttons work fine.

However, this works only when no user icon is set. When a user icon is set, there is still a small whitespace at the right edge. Also, on a retina display, the user icon is stretched vertically so that it's no longer square. The size of the "X" button is also messed up on retina screens. Looking into the issues now. I suspect that the image size change is due to the "X" size - the X overflows the image on the right edge, so I think the combined size of the X + image is still 160x160.

Comment 70

[(inactive) Nihanth Subramanya [:nhnt11]]

The "x" button sizing is fixed by...
>#userIconPanelImageRemove > .toolbarbutton-icon {
>  max-width: 16px;
>}
... as discussed on IRC but I figured I should document it here.

Comment 71

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Fix styling on OS X (retina and non-retina screens)

This diff includes all the changes I had to make for everything to look nice.
The "x" button is changed to a toolbarbutton simply because I got it to work easily that way. Maybe it works with some other tweaks as a button.
Changing the toolbarbuttons on panels 2 and 3 to buttons is necessary to make them align properly.

Comment 72

[Mayank Kumar [:mayanktg]]

Attachment: Bug975542_takePicture.diff

Thanks a lot for fixing the style for Mac Nihanth. I have made the changes you've suggested, please check one more time if everything works correctly.

Mic, could you try this patch on Windows build and tell the changes what needs to be done?

Comment 73

[Florian Quèze [:florian]]

Comment on attachment 8444388 [details] [diff] [review]
Bug975542_takePicture.diff

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

In the future, please use a different name for each of your patches attached to the same bug. Using bugzilla's interdiff feature is really difficult when all the attachments in the list have the same name.

::: im/content/blist.js
@@ +608,5 @@
>        Services.core.globalUserStatus.setUserIcon(fp.file);
>    },
>  
> +  webcamSuccessCallback: function bl_webcamSuccessCallback(aStream) {
> +    if ((document.getElementById("changeUserIconPanel").state != "open") ||

You don't need all these ().

@@ +704,5 @@
> +  stopWebcamStream: function bl_stopWebcamStream() {
> +    let webcamStream = document.getElementById("webcamVideo").mozSrcObject;
> +    if (webcamStream) {
> +      webcamStream.stop();
> +      webcamStream = null;

This is as ineffective as the delete you had before.

::: im/themes/blist.css
@@ +460,5 @@
>    border: dotted 1px -moz-dialogtext;
>  %endif
>  }
> +
> +.resetMargin {

Use .takePictureButtons here instead of .resetMargin, and get rid of resetMargin.

@@ +467,5 @@
> +}
> +
> +#changeUserIconPanel > .panel-arrowcontainer > .panel-arrowcontent {
> +  padding: 0;
> +  margin: 0;

This is duplicated...

@@ +499,5 @@
> +#userIconPanelImageRemove > .toolbarbutton-icon {
> +  max-width: 16px;
> +}
> +
> +#userIconPanelImageRemove:hover:not([disabled = "true"]) {

We usually don't have spaces around the '=' when checking an attribute in CSS.

@@ +539,5 @@
> +  -moz-image-region: rect(16px, 16px, 32px, 0);
> +}
> +
> +#selectFileButton > .toolbarbutton-text,
> +#takePictureButton > .toolbarbutton-text {

Isn't there a class shared by both #selectFileButton and #takePictureButton? If not, I guess you could add one.

::: im/themes/jar.mn
@@ +138,1 @@
>  	skin/classic/aero/instantbird/userIcon.png

Your alphabetical sort isn't correct here.

Comment 74

[Mayank Kumar [:mayanktg]]

Attachment: New: Take picture using webcam patch.

Comment 75

[Patrick Cloke [:clokep]]

Attachment: webcam-win8.png

This had a few images of this patch in action on Windows 8.1. The third image has the "Set icon" button hovered.

Comment 76

[Mayank Kumar [:mayanktg]]

Attachment: v6: Change user icon panel with buttons

Thanks Cloke for trying the patch. I have added a background for the Remove icon, set min-height for takePictureButtons and added rounded bottom corners for buttons when seen in Windows (as mentioned by Mic).

Hope this works now.

Comment 77

[Mayank Kumar [:mayanktg]]

Attachment: v7: Updated buttons CSS for Windows

Comment 78

[Patrick Cloke [:clokep]]

Attachment: win8-webcam.png

Not positive if this is the look you're going for, but there's no longer any weird gaps.

Comment 79

[Patrick Cloke [:clokep]]

From IRC discussions...it looks like this looks fine on Windows now, expect that Windows specific border. We should remove that and land this ASAP. If something besides Windows 8 wants a border there (i.e. non-aero?) we can handle that in a follow-up.

Comment 80

[Mayank Kumar [:mayanktg]]

Attachment: v8: Removed windows specific bottom border radius

Changeset:
* Removed Windows specific bottom radius for buttons.
* Corrected mentioning of Bug 1011878 in the setWebcamImage().

Comment 81

[Florian Quèze [:florian]]

Comment on attachment 8456078 [details] [diff] [review]
v8: Removed windows specific bottom border radius

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

I didn't do a detailed review this time, but I think the comments I had before have been addressed, let's try this in nightlies.

::: im/themes/jar.mn
@@ +32,5 @@
>  	skin/classic/instantbird/expand.png
>  	skin/classic/instantbird/collapse.png
>  	skin/classic/instantbird/userIcon.png
>  	skin/classic/instantbird/multiUserIcon.png
> +	skin/classic/instantbird/userIcon.svg

This line should be right after the "userIcon.png" line.

@@ +137,5 @@
>  	skin/classic/aero/instantbird/multiUserIcon.png
>  	skin/classic/aero/instantbird/newConversation.png
>  	skin/classic/aero/instantbird/tag.png		(tag-aero.png)
> +	skin/classic/aero/instantbird/userIcon.png
> +	skin/classic/aero/instantbird/userIcon.svg

It doesn't really make sense to move this away from "multiUserIcon".

More generally, it would be better if the order in the aero and non aero sections were similar.

Comment 82

[aleth [:aleth]]

Please, in the future, do a "hg qref -e" before attaching the patch and enter the commit message there (look at the log for examples). This will also help you while working on patches as it shows up when you do hg qser -v -s to see the state of your mq.

I fixed the order in jar.mn before checking in. 

Looking forward to trying this in nightlies!

https://hg.mozilla.org/comm-central/rev/74718714c5fd

Comment 83

[Mayank Kumar [:mayanktg]]

The patch has been fixed now. Jamie could you please check the accessibility of the panel?

Comment 84

[James Teh [:Jamie]]

I don't use Instantbird any longer and I probably won't have any spare cycles to look at this. Sorry. :(