Closed Bug 598244 Opened 9 years ago Closed 8 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: mounir, Assigned: deLta30)

Details

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

Attachments

(1 file, 4 obsolete files)

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).
Whiteboard: [mentor=jdm][lang=c++]
Assignee: nobody → jitenmt
Attached patch patch_1 (obsolete) — Splinter Review
Attachment #598643 - Flags: review?(mounir)
Attachment #598643 - Flags: review?(josh)
> 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 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)
Attached patch patch_2 (obsolete) — Splinter Review
Attachment #598643 - Attachment is obsolete: true
Attachment #602565 - Flags: review?(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.
Attachment #602565 - Flags: review?(mounir)
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
(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.
Attached patch patch_3 (obsolete) — Splinter Review
Attachment #602565 - Attachment is obsolete: true
Attachment #604619 - Flags: review?(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.
Attachment #604619 - Flags: review?(mounir) → review+
Attached patch patch_4 (obsolete) — Splinter Review
Attachment #604619 - Attachment is obsolete: true
Patch sent to try: https://tbpl.mozilla.org/?tree=Try&rev=2ad8a324e659
Status: NEW → ASSIGNED
Whiteboard: [mentor=jdm][lang=c++] → [mentor=mounir][lang=c++]
Jiten, seems like your patch doesn't build (see link in comment 11). Could you attach a fixed patch?
It compiles properly on my computer.May be I should update the repository.
Attached patch patch_5Splinter Review
Left one unavoidable mistake.I am so sorry for that.
Attachment #604639 - Attachment is obsolete: true
Flags: in-testsuite-
Target Milestone: --- → mozilla13
Attachment #604715 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/fead9a65565f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.