963294 - [e10s] Implement a proxy for the color picker

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Tom S. (please needinfo tschuster)]

Attachment: color-picker

So I decided, because bug 910384 is blocked on that FileHandle stuff to instead implement <input type=color>. We have a lot of the same life-time issues as the other case, which I need help figuring out. Inheriting from both nsIColorPicker and PColorPickerChild seems to be hard. It seems like we should deref nsColorPickerProxy when IPDL isn't going to touch it again. Not sure what the right point for that is though: Proxy::Recv__delete__ or TabChild::DeallocPColorPickerChild (this looks kinda good?)

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8364656 [details] [diff] [review]
color-picker

Thanks, I'll take a look soon. I've been thinking of how we can generalize some of the nsWidgetFactory stuff, so maybe we can work this in.

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: color-picker v2

Although probably violating all kinds of best practices, I think I figured out the lifetime management in the child.

Comment 3

[Tom S. (please needinfo tschuster)]

Attachment: color-picker v3

Comment 4

[Josh Matthews [:jdm]]

Comment on attachment 8377819 [details] [diff] [review]
color-picker v3

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

::: dom/ipc/ColorPickerParent.cpp
@@ +43,5 @@
> +  mColorPickerParent = nullptr;
> +}
> +
> +bool
> +ColorPickerParent::CreatColorPicker()

Create

::: dom/ipc/PBrowser.ipdl
@@ +222,5 @@
>       */
>      ShowTooltip(uint32_t x, uint32_t y, nsString tooltip);
>      HideTooltip();
>  
> +    PColorPicker(nsString title, nsString initialColor);

Can't hurt to document this.

::: dom/ipc/moz.build
@@ +17,5 @@
>      'nsIRemoteBlob.h',
>  ]
>  
>  EXPORTS.mozilla.dom += [
> +    'ColorPickerParent.h',

Does this need to be exported?

::: widget/xpwidgets/nsColorPickerProxy.h
@@ +3,5 @@
> + * 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/. */
> +
> +#ifndef nsColorPickerProxy_h__
> +#define nsColorPickerProxy_h__

Scrap the __ on these lines.

@@ +17,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSICOLORPICKER
> +
> +  nsColorPickerProxy() {};
> +  ~nsColorPickerProxy() {};

No need for ; on these lines

Comment 5

[Tom S. (please needinfo tschuster)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e4cee2fcbc6

Comment 6

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3fedf7d4f967 for https://tbpl.mozilla.org/php/getParsedLog.php?id=35122778&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=35122784&tree=Mozilla-Inbound

Comment 7

[Tom S. (please needinfo tschuster)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02959a932fb7
try: https://tbpl.mozilla.org/?tree=Try&rev=90a387c24af6

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/02959a932fb7