598244 - nsFileControlFrame should not parse the accept attribute but nsHTMLInputElement should do that

Status: RESOLVED FIXED

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

Description

[Mounir Lamouri (:mounir)]

nsFileControlFrame::ParseAcceptAttribute is parsing the accept attribute. This logic should be in the content.
In addition, this method has a complex callback mechanism which is going to be useless when patch from bug 598236 will be landed (IOW, when the second call of ParseAcceptAttribute will be removed).

Comment 1

[Jiten [:deLta30]]

Attachment: patch_1

Comment 2

[Jiten [:deLta30]]

> Created attachment 598643 [details] [diff] [review]
> patch_1

Added one method named nsHTMLInputElement::GetCapturePickerFromAccept in nsHTMLInputElement to parse the accept attribute and removed nsFileControlFrame::ParseAcceptAttribute.

Comment 3

[Mounir Lamouri (:mounir)]

Comment on attachment 598643 [details] [diff] [review]
patch_1

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

So, I think your approach could be improved a bit like this:
Instead of moving some of the Capture stuff in nsHTMLInputElement, you could keep all of it in the layout code and only get the filters from the content.
So, instead of calling |ParseAcceptAttribute(&CapturePickerAcceptCallback, (void*)&data);|, you could do:
data->mode = GetCaptureMode(data);

Where GetCaptureMode() would be a method like this:
PRUint32
nsFileControlFrame::GetCaptureMode(const CaptureCallbackData& aData)
{
  PRInt32 filters = nsHTMLInputElement::FromContent(mContent)->GetFilterFromAccept();

  if (filters == nsIFilePicker::filterImages) {
    // Call data.picker->ModeMayBeAvailable(nsICapturePicker::MODE_STILL, &captureEnabled);
    // and do like in CapturePickerAcceptCallback
    return mode;
  }

  if (filters == nsIFilePicker::filterAudio) {
    // Same thing for audio.
    return mode;
  }

  if (filters == nsIFilePicker::filterVideo) {
    // Same for video
    return mode;
  }

  return 0;
}

Does that make sense?

(and sorry for the late reply, I've been sick most of last week)

::: content/html/content/src/nsHTMLInputElement.h
@@ +86,5 @@
> +  nsICapturePicker* picker;
> +  PRUint32* mode;
> +};
> +
> +typedef struct CaptureCallbackData CaptureCallbackData;

I would prefer to keep that out of nsHTMLInputElement.

Comment 4

[Jiten [:deLta30]]

Attachment: patch_2

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 602565 [details] [diff] [review]
patch_2

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

Looks good generally but I would like to see the change in the GetCaptureMode parameters.

::: layout/forms/nsFileControlFrame.cpp
@@ +247,4 @@
>      CaptureCallbackData data;
>      data.picker = capturePicker;
>      data.mode = &mode;
> +    *data.mode = GetCaptureMode((void*)&data);

I think you can change:
*data.mode = GetCaptureMode((void*)&data);
to:
*data.mode = GetCaptureMode(data);

@@ +707,5 @@
>  }
>  #endif
>  
> +PRUint32
> +nsFileControlFrame::GetCaptureMode(const void* aData)

|const void*| should be |const CaptureCallbackData&|.

@@ +712,3 @@
>  {
> +  PRInt32 filters = nsHTMLInputElement::FromContent(mContent)->GetFilterFromAccept();
> +  CaptureCallbackData* data = (CaptureCallbackData*)aData;

You shouldn't need that line with the type change for |aData|.

@@ +717,5 @@
>  
> +  if (filters == nsIFilePicker::filterImages) {
> +    rv = data->picker->ModeMayBeAvailable(nsICapturePicker::MODE_STILL,
> +                                             &captureEnabled);
> +    NS_ENSURE_SUCCESS(rv, true);

NS_ENSURE_SUCCESS(a, b) is equivalent to:
if (a != NS_OK) {
  return b;
}
So, here, you don't want to return true but 0.

@@ +727,5 @@
> +
> +  if (filters == nsIFilePicker::filterAudio) {
> +    rv = data->picker->ModeMayBeAvailable(nsICapturePicker::MODE_AUDIO_CLIP,
> +                                             &captureEnabled);
> +    NS_ENSURE_SUCCESS(rv, true);

ditto

@@ +737,5 @@
> +
> +  if (filters == nsIFilePicker::filterVideo) {
> +    rv = data->picker->ModeMayBeAvailable(nsICapturePicker::MODE_VIDEO_CLIP,
> +                                             &captureEnabled);
> +    NS_ENSURE_SUCCESS(rv, true);

ditto

@@ +743,5 @@
> +      return nsICapturePicker::MODE_VIDEO_CLIP;
> +    }
> +    rv = data->picker->ModeMayBeAvailable(nsICapturePicker::MODE_VIDEO_NO_SOUND_CLIP,
> +                                             &captureEnabled);
> +    NS_ENSURE_SUCCESS(rv, true);

ditto

@@ +749,5 @@
> +      return nsICapturePicker::MODE_VIDEO_NO_SOUND_CLIP;
> +    }
> +    return 0;
> +  }
> +  return 0;

nit: can you leave an empty line before that return statement.

::: layout/forms/nsFileControlFrame.h
@@ +97,4 @@
>  #endif
>  
>    typedef bool (*AcceptAttrCallback)(const nsAString&, void*);
> +  PRUint32 GetCaptureMode(const void* aData);

I'm pretty sure you could pass CaptureCallbackData instead of a void. A const reference instead of a const pointer too.

Comment 6

[Jiten [:deLta30]]

If I change the parameter from const void* to CaptureCallbackData then I will have to move structure from nsFileControlFrame.cpp to nsFileControlFrame.h even if it is never used out side of nsFileControlFrame.cpp

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to Jiten [:deLta30] from comment #6)
> If I change the parameter from const void* to CaptureCallbackData then I
> will have to move structure from nsFileControlFrame.cpp to
> nsFileControlFrame.h even if it is never used out side of
> nsFileControlFrame.cpp

You can make the struct private in nsFileControlFrame. That would be okay for me.

Comment 8

[Jiten [:deLta30]]

Attachment: patch_3

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 604619 [details] [diff] [review]
patch_3

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

r=me with the requested changes

Please, re-attach here the patch with the changes.

::: layout/forms/nsFileControlFrame.cpp
@@ +239,5 @@
>  
>      CaptureCallbackData data;
>      data.picker = capturePicker;
>      data.mode = &mode;
> +    *data.mode = GetCaptureMode(data);

I'm re-reading this code and I don't understand why |data.mode| is a pointer. Seems like it could be a plain int.
Could you change that? So you could do |data.mode = GetCaptureMode(data);| without calling |data.mode = &mode;| before and you could use |data.mode| instead of |mode| below.

@@ +700,5 @@
>  }
>  #endif
>  
> +PRUint32
> +nsFileControlFrame::GetCaptureMode(const CaptureCallbackData& aData)

I realize you could have passed nsICapturePicker insead of CaptureCallbackData but it doesn't really matter. You can change that or not, as you prefer.

::: layout/forms/nsFileControlFrame.h
@@ +55,5 @@
> +private:
> +  struct CaptureCallbackData {
> +    nsICapturePicker* picker;
> +    PRUint32* mode;
> +  };

I think you can add that below in the protected section (I don't mind if it's protected instead of private).

@@ +57,5 @@
> +    nsICapturePicker* picker;
> +    PRUint32* mode;
> +  };
> +
> +  typedef struct CaptureCallbackData CaptureCallbackData;

I don't think you need that in C++.

@@ +102,5 @@
>  
>  #ifdef ACCESSIBILITY
>    virtual already_AddRefed<nsAccessible> CreateAccessible();
>  #endif
> +  

nit: you are adding a new line here, you don't need that.

@@ +107,3 @@
>  
>    typedef bool (*AcceptAttrCallback)(const nsAString&, void*);
> +  PRUint32 GetCaptureMode(const CaptureCallbackData& aData);

That method should be protected too.

Comment 10

[Jiten [:deLta30]]

Attachment: patch_4

Comment 11

[Mounir Lamouri (:mounir)]

Patch sent to try: https://tbpl.mozilla.org/?tree=Try&rev=2ad8a324e659

Comment 12

[Mounir Lamouri (:mounir)]

Jiten, seems like your patch doesn't build (see link in comment 11). Could you attach a fixed patch?

Comment 13

[Jiten [:deLta30]]

It compiles properly on my computer.May be I should update the repository.

Comment 14

[Jiten [:deLta30]]

Attachment: patch_5

Left one unavoidable mistake.I am so sorry for that.

Comment 15

[Mounir Lamouri (:mounir)]

Re-pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=626d536764bc

Comment 16

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/fead9a65565f