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]
:
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 | Review
patch_2 (4.94 KB, patch)
2012-03-02 19:55 PST, Jiten [:deLta30]
no flags Details | Diff | Review
patch_3 (7.23 KB, patch)
2012-03-10 01:36 PST, Jiten [:deLta30]
mounir: review+
Details | Diff | Review
patch_4 (6.75 KB, patch)
2012-03-10 05:43 PST, Jiten [:deLta30]
no flags Details | Diff | Review
patch_5 (7.04 KB, patch)
2012-03-10 20:22 PST, Jiten [:deLta30]
mounir: checkin+
Details | Diff | Review

Description 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 Jiten [:deLta30] 2012-02-19 02:41:34 PST
Created attachment 598643 [details] [diff] [review]
patch_1
Comment 2 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 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 Jiten [:deLta30] 2012-03-02 19:55:57 PST
Created attachment 602565 [details] [diff] [review]
patch_2
Comment 5 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 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 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 Jiten [:deLta30] 2012-03-10 01:36:07 PST
Created attachment 604619 [details] [diff] [review]
patch_3
Comment 9 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 Jiten [:deLta30] 2012-03-10 05:43:44 PST
Created attachment 604639 [details] [diff] [review]
patch_4
Comment 11 Mounir Lamouri (:mounir) 2012-03-10 06:30:31 PST
Patch sent to try: https://tbpl.mozilla.org/?tree=Try&rev=2ad8a324e659
Comment 12 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 Jiten [:deLta30] 2012-03-10 09:20:59 PST
It compiles properly on my computer.May be I should update the repository.
Comment 14 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 Mounir Lamouri (:mounir) 2012-03-11 07:38:31 PDT
Re-pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=626d536764bc
Comment 16 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.