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

RESOLVED FIXED in mozilla13

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: mounir, Assigned: deLta30)

Tracking

Trunk
mozilla13
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=mounir][lang=c++])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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).

Updated

5 years ago
Whiteboard: [mentor=jdm][lang=c++]
(Assignee)

Updated

5 years ago
Assignee: nobody → jitenmt
(Assignee)

Comment 1

5 years ago
Created attachment 598643 [details] [diff] [review]
patch_1
Attachment #598643 - Flags: review?(mounir)
Attachment #598643 - Flags: review?(josh)
(Assignee)

Comment 2

5 years ago
> 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.
(Reporter)

Comment 3

5 years ago
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.
Attachment #598643 - Flags: review?(mounir)
Attachment #598643 - Flags: review?(josh)
(Assignee)

Comment 4

5 years ago
Created attachment 602565 [details] [diff] [review]
patch_2
Attachment #598643 - Attachment is obsolete: true
Attachment #602565 - Flags: review?(mounir)
(Reporter)

Comment 5

5 years ago
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.
Attachment #602565 - Flags: review?(mounir)
(Assignee)

Comment 6

5 years ago
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
(Reporter)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
Created attachment 604619 [details] [diff] [review]
patch_3
Attachment #602565 - Attachment is obsolete: true
Attachment #604619 - Flags: review?(mounir)
(Reporter)

Comment 9

5 years ago
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.
Attachment #604619 - Flags: review?(mounir) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 604639 [details] [diff] [review]
patch_4
Attachment #604619 - Attachment is obsolete: true
(Reporter)

Comment 11

5 years ago
Patch sent to try: https://tbpl.mozilla.org/?tree=Try&rev=2ad8a324e659
Status: NEW → ASSIGNED
Whiteboard: [mentor=jdm][lang=c++] → [mentor=mounir][lang=c++]
(Reporter)

Comment 12

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

Comment 13

5 years ago
It compiles properly on my computer.May be I should update the repository.
(Assignee)

Comment 14

5 years ago
Created attachment 604715 [details] [diff] [review]
patch_5

Left one unavoidable mistake.I am so sorry for that.
Attachment #604639 - Attachment is obsolete: true
(Reporter)

Comment 15

5 years ago
Re-pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=626d536764bc
(Reporter)

Updated

5 years ago
Flags: in-testsuite-
Target Milestone: --- → mozilla13
(Reporter)

Updated

5 years ago
Attachment #604715 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/fead9a65565f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.