Last Comment Bug 598244 - nsFileControlFrame should not parse the accept attribute but nsHTMLInputElement should do that
: nsFileControlFrame should not parse the accept attribute but nsHTMLInputEleme...
Status: RESOLVED FIXED
[mentor=mounir][lang=c++]
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Jiten [:deLta30]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-20 20:00 PDT by Mounir Lamouri (:mounir)
Modified: 2012-03-12 13:41 PDT (History)
5 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch_1 (6.99 KB, patch)
2012-02-19 02:41 PST, Jiten [:deLta30]
no flags Details | Diff | Splinter Review
patch_2 (4.94 KB, patch)
2012-03-02 19:55 PST, Jiten [:deLta30]
no flags Details | Diff | Splinter Review
patch_3 (7.23 KB, patch)
2012-03-10 01:36 PST, Jiten [:deLta30]
mounir: review+
Details | Diff | Splinter Review
patch_4 (6.75 KB, patch)
2012-03-10 05:43 PST, Jiten [:deLta30]
no flags Details | Diff | Splinter Review
patch_5 (7.04 KB, patch)
2012-03-10 20:22 PST, Jiten [:deLta30]
mounir: checkin+
Details | Diff | Splinter Review

Description User image Mounir Lamouri (:mounir) 2010-09-20 20:00:58 PDT
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 User image Jiten [:deLta30] 2012-02-19 02:41:34 PST
Created attachment 598643 [details] [diff] [review]
patch_1
Comment 2 User image Jiten [:deLta30] 2012-02-19 02:44:14 PST
> 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 User image Mounir Lamouri (:mounir) 2012-02-28 06:51:08 PST
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 User image Jiten [:deLta30] 2012-03-02 19:55:57 PST
Created attachment 602565 [details] [diff] [review]
patch_2
Comment 5 User image Mounir Lamouri (:mounir) 2012-03-06 08:40:57 PST
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 User image Jiten [:deLta30] 2012-03-08 23:10:45 PST
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 User image Mounir Lamouri (:mounir) 2012-03-09 05:56:17 PST
(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 User image Jiten [:deLta30] 2012-03-10 01:36:07 PST
Created attachment 604619 [details] [diff] [review]
patch_3
Comment 9 User image Mounir Lamouri (:mounir) 2012-03-10 03:26:40 PST
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 User image Jiten [:deLta30] 2012-03-10 05:43:44 PST
Created attachment 604639 [details] [diff] [review]
patch_4
Comment 11 User image Mounir Lamouri (:mounir) 2012-03-10 06:30:31 PST
Patch sent to try: https://tbpl.mozilla.org/?tree=Try&rev=2ad8a324e659
Comment 12 User image Mounir Lamouri (:mounir) 2012-03-10 09:11:52 PST
Jiten, seems like your patch doesn't build (see link in comment 11). Could you attach a fixed patch?
Comment 13 User image Jiten [:deLta30] 2012-03-10 09:20:59 PST
It compiles properly on my computer.May be I should update the repository.
Comment 14 User image Jiten [:deLta30] 2012-03-10 20:22:07 PST
Created attachment 604715 [details] [diff] [review]
patch_5

Left one unavoidable mistake.I am so sorry for that.
Comment 15 User image Mounir Lamouri (:mounir) 2012-03-11 07:38:31 PDT
Re-pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=626d536764bc
Comment 16 User image Matt Brubeck (:mbrubeck) 2012-03-12 13:41:49 PDT
https://hg.mozilla.org/mozilla-central/rev/fead9a65565f

Note You need to log in before you can comment on or make changes to this bug.