Closed Bug 1345767 Opened 7 years ago Closed 7 years ago

Factor validation related code out of HTMLInputElement

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jessica, Assigned: jessica)

References

Details

Attachments

(8 files, 3 obsolete files)

Bug 1345767 - Part 1: Create classes for each of the input types.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1345767 - Part 2: Factor IsTooLong/Short() out of HTMLInputElement.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1345767 - Part 3: Factor IsValueMissing() out of HTMLInputElement.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1345767 - Part 4: Factor HasTypeMismatch() out of HTMLInputElement.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1345767 - Part 5: Factor HasPatternMismatch() out of HTMLInputElement.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1345767 - Part 6: Factor IsRangeOverflow/Underflow() and HasStepMismatch() out of HTMLInputElement.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1345767 - Part 7: Factor HasBadInput() out of HTMLInputElement.
59 bytes, text/x-review-board-request
smaug
: review+
Details
a test
1.50 KB, text/html
Details 
      No description provided.
Summary: Factor out validation related code out of HTMLInputElement → Factor validation related code out of HTMLInputElement
Priority: -- → P3
I have grouped the input types as follow:
- SingleLineText
 * text
 * email
 * url
 * password
 * tel
 * search
- Numeric
 * number
 * range
- Checkable
 * radio
 * checkbox
- Buttons
 * image
 * reset
 * button
 * submit
- Date/Time
 * date
 * time
 * week
 * month
 * datetime-local
- Others
 * file
 * color
 * hidden
Comment on attachment 8846507 [details] [diff] [review]
Part 1: create classes for each of the input types.

This patch just creates classes for each input type. `InputType` holds a reference to HTMLInputElement, which is not ref-counted. We explicitly drop the reference when HTMLInputElement is destructed or when input type changes.
Attachment #8846507 - Flags: review?(bugs)
Comment on attachment 8846507 [details] [diff] [review]
Part 1: create classes for each of the input types.

Sorry, just asking for an early feedback. :)
Attachment #8846507 - Flags: review?(bugs) → feedback?(bugs)
Comment on attachment 8846510 [details] [diff] [review]
Part 2: factor our IsTooLong/Short.

Starting with simple functions: IsTooLong/IsTooShort.
Attachment #8846510 - Flags: feedback?(bugs)
Comment on attachment 8846510 [details] [diff] [review]
Part 2: factor our IsTooLong/Short.

Nit, { after method name/params go to its own line.


Looks reasonable.

One thing to keep in mind is to try to avoid too many virtual calls.
Like, perhaps we don't need MinOrMaxLengthApplies() but it could be part of IsTooLong()/IsTooShort()? (not sure, but something to think)
Attachment #8846510 - Flags: feedback?(bugs) → feedback+
Comment on attachment 8846507 [details] [diff] [review]
Part 1: create classes for each of the input types.

>@@ -5134,16 +5144,17 @@ HTMLInputElement::HandleTypeChange(uint8
> 
>   if (GetEditorState()) {
>     sp = mInputData.mState->GetSelectionProperties();
>   }
> 
>   // We already have a copy of the value, lets free it and changes the type.
>   FreeData();
>   mType = aNewType;
>+  mInputType = InputType::Create(this, mType);
Hmm, I wonder, could the type be stored in the InputType object?

>+  InputType* mInputType;
This should be either UniquePtr or if there is some reason to keep the object alive a bit longer, maybe it could be
refcounted (possibly not cycle collectable) and then RefPtr here.
But UniquePtr might be enough.

>+class ButtonInputType : public ButtonInputTypeBase {
nit, { to its own line. Same also elsewhere.

Depending on the amount of the code, I might not object putting all the button type classes to same .h/.cpp files etc.
Same with other types, one .h/.cpp per InputType kind? The code might be easier to follow that way (or might not, not sure)


>+class InputType {
So I would have expected the base class to have some mType or such member variable.

>+  mozilla::dom::HTMLInputElement* mInputElement;
>+};
Ok, the setup will increase total size of some elements 4 or 8 bytes, but the, I guess in generic memory will be used less since
most common types will have rather simple InputTypes

I like
Attachment #8846507 - Flags: feedback?(bugs) → feedback+
(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8846510 [details] [diff] [review]
> Part 2: factor our IsTooLong/Short.
> 
> Nit, { after method name/params go to its own line.
> 
> 
> Looks reasonable.
> 
> One thing to keep in mind is to try to avoid too many virtual calls.
> Like, perhaps we don't need MinOrMaxLengthApplies() but it could be part of
> IsTooLong()/IsTooShort()? (not sure, but something to think)

Yeah, makes sense, since we are calling a virtual call already, we'll just let each InputType decide whether it applies or not and act accordingly.

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8846507 [details] [diff] [review]
> Part 1: create classes for each of the input types.
> 
> >@@ -5134,16 +5144,17 @@ HTMLInputElement::HandleTypeChange(uint8
> > 
> >   if (GetEditorState()) {
> >     sp = mInputData.mState->GetSelectionProperties();
> >   }
> > 
> >   // We already have a copy of the value, lets free it and changes the type.
> >   FreeData();
> >   mType = aNewType;
> >+  mInputType = InputType::Create(this, mType);
> Hmm, I wonder, could the type be stored in the InputType object?

We could, but I'm not sure why we need it? Doesn't each derived InputType know what it is?

> 
> >+  InputType* mInputType;
> This should be either UniquePtr or if there is some reason to keep the
> object alive a bit longer, maybe it could be
> refcounted (possibly not cycle collectable) and then RefPtr here.
> But UniquePtr might be enough.

I'll go with UniquePtr, thanks.

> 
> >+class ButtonInputType : public ButtonInputTypeBase {
> nit, { to its own line. Same also elsewhere.
> 
> Depending on the amount of the code, I might not object putting all the
> button type classes to same .h/.cpp files etc.
> Same with other types, one .h/.cpp per InputType kind? The code might be
> easier to follow that way (or might not, not sure)

Sounds goot to me, I will put input types of the same kind in one .h/.cpp.

> 
> 
> >+class InputType {
> So I would have expected the base class to have some mType or such member
> variable.
> 
> >+  mozilla::dom::HTMLInputElement* mInputElement;
> >+};
> Ok, the setup will increase total size of some elements 4 or 8 bytes, but
> the, I guess in generic memory will be used less since
> most common types will have rather simple InputTypes
> 
> I like
(In reply to Jessica Jong [:jessica] from comment #8)
> > >   mType = aNewType;
> > >+  mInputType = InputType::Create(this, mType);
> > Hmm, I wonder, could the type be stored in the InputType object?
> 
> We could, but I'm not sure why we need it? Doesn't each derived InputType
> know what it is?
> 
Do they? Like password vs text handling may be a tad different and use the same InputType class?
Of course if InputType classes don't need the type at all, then no need to move it there
(In reply to Olli Pettay [:smaug] from comment #9)
> (In reply to Jessica Jong [:jessica] from comment #8)
> > > >   mType = aNewType;
> > > >+  mInputType = InputType::Create(this, mType);
> > > Hmm, I wonder, could the type be stored in the InputType object?
> > 
> > We could, but I'm not sure why we need it? Doesn't each derived InputType
> > know what it is?
> > 
> Do they? Like password vs text handling may be a tad different and use the
> same InputType class?
> Of course if InputType classes don't need the type at all, then no need to
> move it there

Hmm, assuming that different input types are going to share a same InputType class, then yes, we'll need the mType.
I've uploaded the first version of the patches on mozreview board. We may need to think how to improve the performance for this.
Some thought about this:
- Override operator new in InputType and allocate memory for the largest derived class. That memory can be reused when HTMLInputElement type is changed. But I'm not sure how to know the size of the largest derived class object in runtime, when derived objects are not created yet.
- Create InputType (and maybe nsTextEditorState as well) lazily. That is, if HTMLInputElement is created from parser, we can defer creation of InputType and nsTextEditorState when type attribute is set or when done creating element.
Assignee: nobody → jjong
Allocation is still allocation, which is slow.

We need to know the size of the largest derived class on compile time I think, so that we can reserve
right amount extra memory at the end of HTMLInputElement. That way we wouldn't need to do any extra malloc/free.
Or, could we get similar effect by having union of all the derived classes?
(In reply to Olli Pettay [:smaug] from comment #19)
> Allocation is still allocation, which is slow.
> 
> We need to know the size of the largest derived class on compile time I
> think, so that we can reserve
> right amount extra memory at the end of HTMLInputElement. That way we
> wouldn't need to do any extra malloc/free.

That means we need to override HTMLInputElement's operator new and allocate an extra space for InputType. And when creating InputType, we can use placement new to create the object at the specific memory address. Sounds feasible, we'll just need to handle memory very carefully. :/

> Or, could we get similar effect by having union of all the derived classes?

We can't use union as union does not allow non-static data member with a non-trivial default constructor. Maybe we can use mozilla::Variant instead. But that means we'll need to know the type to get the corresponding variant each time we need the InputType, I think it'd be something like:

mozilla::Variant<TextInputType, SearchInputType, EmailInpuType, ...> mInputType;

InputType* GetInputType()
{
  switch(mType) {
    case NS_FORM_INPUT_TEXT:
      return &mInputType.as<TextInputType>();
    case NS_FORM_INPUT_SEARCH:
      return &mInputType.as<SearchInputType>();
      ....
  }
}

Is this something you have in mind?
(In reply to Jessica Jong [:jessica] from comment #20)
> (In reply to Olli Pettay [:smaug] from comment #19)
> > Allocation is still allocation, which is slow.
> > 
> > We need to know the size of the largest derived class on compile time I
> > think, so that we can reserve
> > right amount extra memory at the end of HTMLInputElement. That way we
> > wouldn't need to do any extra malloc/free.
> 
> That means we need to override HTMLInputElement's operator new
Why?
We can just have some inline buffer (large enough to be used for any InputType) in HTMLInputElement.


> and allocate
> an extra space for InputType. And when creating InputType, we can use
> placement new to create the object at the specific memory address. Sounds
> feasible, we'll just need to handle memory very carefully. :/
Yes, that is what one needs to do when trying to avoid slow new/delete.



> 
> We can't use union as union does not allow non-static data member with a
> non-trivial default constructor. Maybe we can use mozilla::Variant instead.
> But that means we'll need to know the type to get the corresponding variant
> each time we need the InputType, I think it'd be something like:
Ah, Variant doesn't have a public method to get the raw pointer which could be just casted to InputType.
Either we need that, or not use Variant.
Since as() does a cast from ptr() perhaps ptr() could be made public?
(Even better would be if Variant had some method to return value as a base class type)
(In reply to Olli Pettay [:smaug] from comment #21)
> (In reply to Jessica Jong [:jessica] from comment #20)
> > (In reply to Olli Pettay [:smaug] from comment #19)
> > > Allocation is still allocation, which is slow.
> > > 
> > > We need to know the size of the largest derived class on compile time I
> > > think, so that we can reserve
> > > right amount extra memory at the end of HTMLInputElement. That way we
> > > wouldn't need to do any extra malloc/free.
> > 
> > That means we need to override HTMLInputElement's operator new
> Why?
> We can just have some inline buffer (large enough to be used for any
> InputType) in HTMLInputElement.

Right, that'll do too. Will upload new patches based on this approach.
I've uploaded the new patches.
So, I am using mozilla::Variant<> to know the largest InputType size at compile time, and create the memory buffer using the obtained size. Then, pass the allocated memory address to each input type, so that it can be created on that space.

I ran gecko profile for the following scenario:
for (let i = 0; i < 10000; i++) {
  document.createElement("input");
}

Without patches: https://perfht.ml/2p4kDjf
With patches: https://perfht.ml/2p4EzSW

Actually, I'm not sure how to do a good profiling, I noticed the time spent varies a lot, even with the same build.
And InputType::Create() rarely shows up on the profile, is it because it takes less than 1ms?
yeah, the actual time may vary a lot. Occasionally I use testcases similar to 
https://bug934607.bmoattachments.org/attachment.cgi?id=8855002 (not my code originally but can't recall where I took it)

Also, I don't trust the actual times profiler tell. It is more interesting to observe how % between different areas change.
In fact the profiler I use doesn't even show times.
Looks like the loop is so fast that either creating more elements or calling that loop several time might make profiles easier to compare.
Thanks Olli for the test!
I ran the test page at least five times.

Without patch:
Running 20 times Ignoring warm-up run (129) 125 128 179 310 183 178 177 307 172 172 115 214 112 128 168 117 218 120 172 173 avg 173.4 stdev 54.77992332962871 
Running 20 times Ignoring warm-up run (129) 127 131 155 301 206 181 180 293 172 179 113 211 122 147 169 107 249 170 174 173 avg 178 stdev 52.060541679855774 
Running 20 times Ignoring warm-up run (132) 130 139 210 322 181 174 179 286 171 181 172 226 113 119 122 116 232 167 166 168 avg 178.7 stdev 53.67969821077611 
Running 20 times Ignoring warm-up run (236) 121 126 162 313 170 181 104 221 169 178 159 115 186 107 121 155 178 134 180 109 avg 159.45 stdev 47.40514212614493 
Running 20 times Ignoring warm-up run (136) 126 135 198 312 180 175 176 289 168 177 113 204 111 131 111 119 289 180 164 112 avg 173.5 stdev 59.7448742571277 

With patches:
Running 20 times Ignoring warm-up run (143) 132 138 231 342 208 187 194 268 184 190 120 123 126 185 208 130 298 222 205 122 avg 190.65 stdev 60.11844558868766 
Running 20 times Ignoring warm-up run (298) 143 168 274 301 221 229 212 122 214 198 216 207 208 118 168 203 201 121 151 206 avg 194.05 stdev 46.774432118412726 
Running 20 times Ignoring warm-up run (146) 141 150 253 209 218 227 221 318 230 211 123 201 119 181 208 197 130 189 209 128 avg 193.15 stdev 48.77630059772881 
Running 20 times Ignoring warm-up run (248) 126 138 232 308 193 193 194 185 107 189 107 113 194 172 186 190 183 124 192 108 avg 171.7 stdev 48.50061855275662 
Running 20 times Ignoring warm-up run (149) 139 157 274 356 214 218 215 335 212 214 123 121 126 196 207 168 317 225 208 122 avg 207.35 stdev 68.12068334947911 

So, with the patches, it's around 10% slower... :(
Since refactoring will certainly introduce some delay, I wonder what is the tolerance limit for this?
Do you have profiles for those runs? Where is the slowing down coming from?
(In reply to Olli Pettay [:smaug] from comment #35)
> Do you have profiles for those runs? Where is the slowing down coming from?

Yes, I took profiles on some extreme cases.

Without patch:
Running 20 times Ignoring warm-up run (236) 140 189 197 164 249 173 160 103 200 158 156 105 145 165 169 164 108 200 151 153 avg 162.45 stdev 34.110812068902725 
https://perfht.ml/2pcG6qd

With patch:
Running 20 times Ignoring warm-up run (148) 165 259 259 326 203 218 214 310 206 232 123 127 330 118 118 118 205 196 213 215 avg 207.75 stdev 65.31299640959676 
https://perfht.ml/2pcOlTk

Delay comes from the sum of different parts. Some significant parts I noticed are:
- HTMLInputElement calling base class constructor nsGenericHTMLFormElementWithState::nsGenericHTMLFormElementWithState() takes longer
- When doing GC, ~HTMLInputElement() takes longer when calling HTMLInputElement::ReleaseTextEditorState(nsTextEditorState*) (this shouldn't be related) and when deallocating

Can you notice something else?
Note that, size of the largest InputType is 32 bytes, and the original size of HTMLInputElement was 512 bytes.
So before the patches sizeof HTMLInputElement is 512 (that is a lot), and after patches what?
Hmm, creating the InputType doesn't seem to take much time.

I wonder what happens to performance if nsTextEditorState member variable is moved to the TextInputType class and not allocated explicitly from heap but similar member variable there as what HTMLTextArea has.
(Then we could get rid of the nsTextEditorState cache I just added :) )


But the sizeof element might cause some difference here, falling to different bucket.
http://searchfox.org/mozilla-central/rev/d4eaa9c2fa54d553349ac88f0c312155a4c6e89e/memory/mozjemalloc/jemalloc.c#53-83
If we want to shrink the size of HTMLInputElement, some of the type=file related member variables could be combined and allocated only when needed.
(In reply to Olli Pettay [:smaug] from comment #37)
> So before the patches sizeof HTMLInputElement is 512 (that is a lot), and
> after patches what?

Sorry, let me correct myself. The original size of HTMLInputElement is 504, and after the patches it's 544.

(In reply to Olli Pettay [:smaug] from comment #39)
> If we want to shrink the size of HTMLInputElement, some of the type=file
> related member variables could be combined and allocated only when needed.

Just wanted to confirm which member variables specifically, cause mFilesOrDirectories, mGetFilesRecursiveHelper, mFileList, etc. are 8 bytes each. What takes more memory is inner struct/class, like nsFilePickerShownCallback and nsFilePickerFilter.

I can also try moving nsTextEditorState to the SingleLineTextInputTypeBase class, maybe it's less complicated than moving type=file relate member variables. Let's see :)
> (In reply to Olli Pettay [:smaug] from comment #39)
> > If we want to shrink the size of HTMLInputElement, some of the type=file
> > related member variables could be combined and allocated only when needed.
> 
> Just wanted to confirm which member variables specifically, cause
> mFilesOrDirectories, mGetFilesRecursiveHelper, mFileList, etc. are 8 bytes
> each. What takes more memory is inner struct/class, like
> nsFilePickerShownCallback and nsFilePickerFilter.

Moving nsFilePickerShownCallback and nsFilePickerFilter out of HTMLInputElement does not reduce its size, I thought inner struct/class would take its parent class memory space. :/

> 
> I can also try moving nsTextEditorState to the SingleLineTextInputTypeBase
> class, maybe it's less complicated than moving type=file relate member
> variables. Let's see :)

So moving nsTextEditorState to SingleLineTextInputTypeBase and put it into the stack does not help much, since the entire size of HTMLInputElement becomes 704, and although we initialize it in the constructor initialization list, like HTMLTextArea, it still takes some time.
When I was talking about type=file stuff I meant something like
struct FileData {
  // all the type=file specific member variables,
  // totally 56 bytes if I count right.
};
UniquePtr<FileData> mFileData; // this is 8 bytes on 64 bit

That should mean 48 decrease in size (down to 456) and your patch should increase it by 40 so we should be at 496, which maps nicely to a mozjemalloc bucket size and definitely under 512.
(after 512 mozjemalloc allocates 1kB)
(In reply to Jessica Jong [:jessica] from comment #41)
> Moving nsFilePickerShownCallback and nsFilePickerFilter out of
> HTMLInputElement does not reduce its size, I thought inner struct/class
> would take its parent class memory space. :/

Hmm, what was I thinking, no member variable that is of that type of struct/class, so no memory space allocated.

(In reply to Olli Pettay [:smaug] from comment #42)
> When I was talking about type=file stuff I meant something like
> struct FileData {
>   // all the type=file specific member variables,
>   // totally 56 bytes if I count right.
> };
> UniquePtr<FileData> mFileData; // this is 8 bytes on 64 bit
> 
> That should mean 48 decrease in size (down to 456) and your patch should
> increase it by 40 so we should be at 496, which maps nicely to a mozjemalloc
> bucket size and definitely under 512.
> (after 512 mozjemalloc allocates 1kB)

Yes, this will definitely help. I will work on this. Thanks.
So with this patch on top of the other patches in this bug, we get a good result. The size of HTMLInputElement becomes 480.

With all patches:
Running 20 times Ignoring warm-up run (120) 117 133 184 311 154 164 166 247 157 160 103 172 160 98 119 144 100 102 109 132 avg 151.6 stdev 50.85902083210018 
Running 20 times Ignoring warm-up run (126) 111 126 139 270 160 166 164 281 158 164 182 104 119 106 116 133 165 179 170 170 avg 159.15 stdev 45.969854252542504 
Running 20 times Ignoring warm-up run (122) 112 142 205 321 159 164 164 280 158 167 155 174 106 103 111 102 190 152 154 103 avg 161.1 stdev 55.42373137925667 
Running 20 times Ignoring warm-up run (228) 136 131 194 330 159 98 116 238 160 159 104 101 204 121 156 97 205 132 154 153 avg 157.4 stdev 54.84377813389592 
Running 20 times Ignoring warm-up run (126) 236 133 168 173 283 162 156 106 105 141 173 161 106 236 106 107 108 218 136 165 avg 158.95 stdev 49.71063769456192 

Without patches:
Running 20 times Ignoring warm-up run (134) 153 164 204 169 162 170 160 168 170 163 162 166 163 326 159 172 107 105 192 115 avg 167.5 stdev 43.51264184119369 
Running 20 times Ignoring warm-up run (135) 135 153 203 303 179 172 116 219 112 125 132 209 115 127 116 114 220 119 135 170 avg 158.7 stdev 49.369119903032505 
Running 20 times Ignoring warm-up run (240) 118 129 155 156 174 181 167 117 171 161 109 114 119 159 162 176 402 169 166 173 avg 163.9 stdev 59.425499577201705 
Running 20 times Ignoring warm-up run (130) 119 133 182 308 182 160 167 261 163 159 107 178 112 130 165 106 193 118 171 104 avg 160.9 stdev 50.454831285021655 
Running 20 times Ignoring warm-up run (130) 126 125 174 311 161 170 168 284 165 164 112 206 113 139 101 133 277 177 158 112 avg 168.8 stdev 57.869335575933476 


If this is ok, I'll file a separate bug for this and land that bug first.
Attachment #8859498 - Flags: feedback?(bugs)
Comment on attachment 8859498 [details] [diff] [review]
Allocate type=file related member variables dynamically

>     case VALUE_MODE_FILENAME:
>       if (it->OwnerDoc()->IsStaticDocument()) {
>         // We're going to be used in print preview.  Since the doc is static
>         // we can just grab the pretty string and use it as wallpaper
>-        GetDisplayFileName(it->mStaticDocFileList);
>+        GetDisplayFileName(it->mFileData->mStaticDocFileList);
>       } else {
>-        it->ClearGetFilesHelpers();
>-        it->mFilesOrDirectories.Clear();
>-        it->mFilesOrDirectories.AppendElements(mFilesOrDirectories);
>+        it->mFileData->ClearGetFilesHelpers();
>+        it->mFileData->mFilesOrDirectories.Clear();
>+        it->mFileData->mFilesOrDirectories.AppendElements(
>+          mFileData->mFilesOrDirectories);
It is guaranteed we have mFileData on both elements? 'this' and it

> HTMLInputElement::GetDisplayFileName(nsAString& aValue) const
> {
>   if (OwnerDoc()->IsStaticDocument()) {
>-    aValue = mStaticDocFileList;
>+    aValue = mFileData->mStaticDocFileList;
Is it guaranteed that we have mFileData here?

> HTMLInputElement::SetFilesOrDirectories(const nsTArray<OwningFileOrDirectory>& aFilesOrDirectories,
>                                         bool aSetValueChanged)
> {
>-  ClearGetFilesHelpers();
>+  mFileData->ClearGetFilesHelpers();
Is it guaranteed we have mFileData here.

> HTMLInputElement::SetFiles(nsIDOMFileList* aFiles,
>                            bool aSetValueChanged)
> {
>   RefPtr<FileList> files = static_cast<FileList*>(aFiles);
>-  mFilesOrDirectories.Clear();
>-  ClearGetFilesHelpers();
>+  mFileData->mFilesOrDirectories.Clear();
>+  mFileData->ClearGetFilesHelpers();
ditto


So, could you still ensure we don't crash when accessing null mFileData.
Is is guaranteed that various type=file related methods are only called when type is actually file?
Attachment #8859498 - Flags: feedback?(bugs) → feedback+
(In reply to Olli Pettay [:smaug] from comment #45)
> Comment on attachment 8859498 [details] [diff] [review]
> Allocate type=file related member variables dynamically
> 
> >     case VALUE_MODE_FILENAME:
> >       if (it->OwnerDoc()->IsStaticDocument()) {
> >         // We're going to be used in print preview.  Since the doc is static
> >         // we can just grab the pretty string and use it as wallpaper
> >-        GetDisplayFileName(it->mStaticDocFileList);
> >+        GetDisplayFileName(it->mFileData->mStaticDocFileList);
> >       } else {
> >-        it->ClearGetFilesHelpers();
> >-        it->mFilesOrDirectories.Clear();
> >-        it->mFilesOrDirectories.AppendElements(mFilesOrDirectories);
> >+        it->mFileData->ClearGetFilesHelpers();
> >+        it->mFileData->mFilesOrDirectories.Clear();
> >+        it->mFileData->mFilesOrDirectories.AppendElements(
> >+          mFileData->mFilesOrDirectories);
> It is guaranteed we have mFileData on both elements? 'this' and it

Yes, 'this' value mode is VALUE_MODE_FILENAME, so it's a input type=file, and 'it' will inherits 'this'' attributes, so  'it'' mFileData will be created when AfterSetAttr() gets called.

> 
> > HTMLInputElement::GetDisplayFileName(nsAString& aValue) const
> > {
> >   if (OwnerDoc()->IsStaticDocument()) {
> >-    aValue = mStaticDocFileList;
> >+    aValue = mFileData->mStaticDocFileList;
> Is it guaranteed that we have mFileData here?

Called from HTMLInputElement::Clone, HTMLInputElement::AfterSetFilesOrDirectories and nsFileControlFrame::CreateAnonymousContent. So yes, we should have a mFileData here. But I will add an assertion to make sure we do.

> 
> > HTMLInputElement::SetFilesOrDirectories(const nsTArray<OwningFileOrDirectory>& aFilesOrDirectories,
> >                                         bool aSetValueChanged)
> > {
> >-  ClearGetFilesHelpers();
> >+  mFileData->ClearGetFilesHelpers();
> Is it guaranteed we have mFileData here.

Supposely, yes. This is called from file picker callback and other ChromeOnly methods.

> 
> > HTMLInputElement::SetFiles(nsIDOMFileList* aFiles,
> >                            bool aSetValueChanged)
> > {
> >   RefPtr<FileList> files = static_cast<FileList*>(aFiles);
> >-  mFilesOrDirectories.Clear();
> >-  ClearGetFilesHelpers();
> >+  mFileData->mFilesOrDirectories.Clear();
> >+  mFileData->ClearGetFilesHelpers();
> ditto

This is called from nsFileControlFrame.

> 
> 
> So, could you still ensure we don't crash when accessing null mFileData.
> Is is guaranteed that various type=file related methods are only called when
> type is actually file?

I'll add assertions to make sure we do have a mFileData.
From the spec, it seems there is only:
  readonly attribute FileList? files;
that is type=file related, which we return null when type is not "file". We can add assertions for the other ChromeOnly methods for type=file.
[ChromeOnly] methods don't ensure right type. Need to be sure that type is actually right before calling, or do some check in the method implementation. I'd prefer the latter to prevent crashes.
(In reply to Olli Pettay [:smaug] from comment #47)
> [ChromeOnly] methods don't ensure right type. Need to be sure that type is
> actually right before calling, or do some check in the method
> implementation. I'd prefer the latter to prevent crashes.

Right, I'm using NS_WARN_IF for the ChromeOnly methods in the latest patch (attachment 8859882 [details] [diff] [review]).
Bug 1357981 has landed so I guess this is ready for review now!
Comment on attachment 8855712 [details]
Bug 1345767 - Part 1: Create classes for each of the input types.

https://reviewboard.mozilla.org/r/127596/#review138840

r+, but since I haven't looked at the other patches yet, it isn't clear whether we need mType. So, this may need some minor tweaking.

::: dom/html/input/ButtonInputTypes.h:32
(Diff revision 3)
> +public:
> +  static InputType*
> +  Create(mozilla::dom::HTMLInputElement* aInputElement, uint8_t aType,
> +         void* aMemory)
> +  {
> +    return new(aMemory) ButtonInputType(aInputElement, aType);

Nit, missing space after new, or I think that is the usual style with replacement new. Not sure it is documented anywhere.

::: dom/html/input/InputType.h:47
(Diff revision 3)
> +    , mType(aType)
> +
> +  {}
> +
> +  mozilla::dom::HTMLInputElement* mInputElement;
> +  uint8_t mType;

Hmm, since I moved mType to nsIFormControl, do we need mType here?

::: dom/html/input/moz.build:7
(Diff revision 3)
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +EXPORTS += [

Do we need to export these? dom/html/moz.build has local include for dom/html/input.
Attachment #8855712 - Flags: review?(bugs) → review+
Comment on attachment 8855713 [details]
Bug 1345767 - Part 2: Factor IsTooLong/Short() out of HTMLInputElement.

https://reviewboard.mozilla.org/r/127598/#review138846

::: dom/html/input/SingleLineTextInputTypes.cpp:12
(Diff revision 3)
> +#include "SingleLineTextInputTypes.h"
> +
> +#include "mozilla/dom/HTMLInputElement.h"
> +
> +int32_t
> +SingleLineTextInputTypeBase::GetTextLength() const

Why we need this? Why can't we use mInputElement->InputTextLength ?
Or will that method be removed in some other patch?
Attachment #8855713 - Flags: review?(bugs) → review+
Comment on attachment 8855714 [details]
Bug 1345767 - Part 3: Factor IsValueMissing() out of HTMLInputElement.

https://reviewboard.mozilla.org/r/127600/#review138848

Less code duplication would be good.

::: dom/html/input/CheckableInputTypes.cpp:18
(Diff revision 3)
> +{
> +  if (!mInputElement->HasAttr(kNameSpaceID_None, nsGkAtoms::required)) {
> +    return false;
> +  }
> +
> +  if (mInputElement->IsDisabled()) {

Aha, ok, IsDisabled() is enough for checkbox

::: dom/html/input/SingleLineTextInputTypes.cpp:54
(Diff revision 3)
> +{
> +  if (!mInputElement->HasAttr(kNameSpaceID_None, nsGkAtoms::required)) {
> +    return false;
> +  }
> +
> +  if (mInputElement->IsDisabled() ||

I'm not sure I like having all these HasAttr(required) and IsDisabled() and readonly checks everywhere. The existing IsMutable() is simpler.
Attachment #8855714 - Flags: review?(bugs) → review-
Comment on attachment 8855715 [details]
Bug 1345767 - Part 4: Factor HasTypeMismatch() out of HTMLInputElement.

https://reviewboard.mozilla.org/r/127602/#review138852

::: dom/html/input/SingleLineTextInputTypes.cpp:196
(Diff revision 3)
> +
> +  return true;
> +}
> +
> +/* static */ bool
> +EmailInputType::PunycodeEncodeEmailAddress(const nsAString& aEmail,

You just copy PunycodeEncodeEmailAddress here, but don't remove the original implementation.
I assume removing the old one will happen in another patch.
Attachment #8855715 - Flags: review?(bugs) → review+
Comment on attachment 8855716 [details]
Bug 1345767 - Part 5: Factor HasPatternMismatch() out of HTMLInputElement.

https://reviewboard.mozilla.org/r/127604/#review138854
Attachment #8855716 - Flags: review?(bugs) → review+
Comment on attachment 8855717 [details]
Bug 1345767 - Part 6: Factor IsRangeOverflow/Underflow() and HasStepMismatch() out of HTMLInputElement.

https://reviewboard.mozilla.org/r/127606/#review138858

::: dom/html/input/InputType.h:16
(Diff revision 3)
> +#include "mozilla/Decimal.h"
>  #include "mozilla/UniquePtr.h"
>  #include "nsString.h"
> +#include "nsError.h"
> +
> +inline mozilla::Decimal

Please don't drop the comment about NS_floorModulo

::: dom/html/input/InputType.h:59
(Diff revision 3)
>    virtual bool HasPatternMismatch() const;
> +  virtual bool IsRangeOverflow() const;
> +  virtual bool IsRangeUnderflow() const;
> +  virtual bool HasStepMismatch(bool aUseZeroIfValueNaN) const;
> +
> +  virtual nsresult UpdateValueDueToAttrChange();

This is super generic method name, and not all attribute changes cause this to be called.

Maybe call this MinMaxStepAttrChanged ?
Attachment #8855717 - Flags: review?(bugs) → review+
Comment on attachment 8855718 [details]
Bug 1345767 - Part 7: Factor HasBadInput() out of HTMLInputElement.

https://reviewboard.mozilla.org/r/127608/#review138866
Attachment #8855718 - Flags: review?(bugs) → review+
Comment on attachment 8855712 [details]
Bug 1345767 - Part 1: Create classes for each of the input types.

https://reviewboard.mozilla.org/r/127596/#review138840

> Nit, missing space after new, or I think that is the usual style with replacement new. Not sure it is documented anywhere.

Ok, in our codebase, I see there is a space after new when using placement new.

> Hmm, since I moved mType to nsIFormControl, do we need mType here?

Hmm, this was done after comment 9, but it looks like we don't need mType in InputType, at least for now.

> Do we need to export these? dom/html/moz.build has local include for dom/html/input.

Looks like we do, otherwise I get a `obj-firefox/dist/include/mozilla/dom/HTMLInputElement.h:32:38: fatal error: SingleLineTextInputTypes.h: No such file or directory` error.
Comment on attachment 8855713 [details]
Bug 1345767 - Part 2: Factor IsTooLong/Short() out of HTMLInputElement.

https://reviewboard.mozilla.org/r/127598/#review138846

> Why we need this? Why can't we use mInputElement->InputTextLength ?
> Or will that method be removed in some other patch?

Hmm, good question, I'm not sure why I added this. mInputElement->InputTextLength will be removed after we factor out GetValidationMessage() from HTMLInputElement, but that will be in another bug, so I think we should use mInputElement->InputTextLength here.
Comment on attachment 8855714 [details]
Bug 1345767 - Part 3: Factor IsValueMissing() out of HTMLInputElement.

https://reviewboard.mozilla.org/r/127600/#review138848

> I'm not sure I like having all these HasAttr(required) and IsDisabled() and readonly checks everywhere. The existing IsMutable() is simpler.

Well, because `required` and `readonly` attribute do not apply to all input types, so some input types need to check for `IsDisabled()` only and some may need to check for `readonly` attr. We could override `IsMutable()` in each InputType, that'd be clearer I think...
Comment on attachment 8855715 [details]
Bug 1345767 - Part 4: Factor HasTypeMismatch() out of HTMLInputElement.

https://reviewboard.mozilla.org/r/127602/#review138852

> You just copy PunycodeEncodeEmailAddress here, but don't remove the original implementation.
> I assume removing the old one will happen in another patch.

Yes, PunycodeEncodeEmailAddress in HTMLInputElement will be removed in Part 7.
Comment on attachment 8855717 [details]
Bug 1345767 - Part 6: Factor IsRangeOverflow/Underflow() and HasStepMismatch() out of HTMLInputElement.

https://reviewboard.mozilla.org/r/127606/#review138858

> Please don't drop the comment about NS_floorModulo

Sure, I'll add it back.

> This is super generic method name, and not all attribute changes cause this to be called.
> 
> Maybe call this MinMaxStepAttrChanged ?

MinMaxStepAttrChanged sounds good, thanks.
Comment on attachment 8855714 [details]
Bug 1345767 - Part 3: Factor IsValueMissing() out of HTMLInputElement.

https://reviewboard.mozilla.org/r/127600/#review139136

Could you still before landing this ensure that we don't regress in that microbenchmark.
Attachment #8855714 - Flags: review?(bugs) → review+
oh, one thing I'd like to have somewhere is a check that sizeof HTMLInputElement doesn't go above 512.
static_assert(sizeof(HTMLInputElement) <= 512); or some such should be enough.
(In reply to Olli Pettay [:smaug] from comment #76)
> Comment on attachment 8855714 [details]
> Bug 1345767 - Part 3: Factor IsValueMissing() out of HTMLInputElement.
> 
> https://reviewboard.mozilla.org/r/127600/#review139136
> 
> Could you still before landing this ensure that we don't regress in that
> microbenchmark.

Sure, will do.

(In reply to Olli Pettay [:smaug] from comment #77)
> oh, one thing I'd like to have somewhere is a check that sizeof
> HTMLInputElement doesn't go above 512.
> static_assert(sizeof(HTMLInputElement) <= 512); or some such should be
> enough.

We have that assertion already [1], I added in bug 1357981. :)

[1] http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/dom/html/HTMLInputElement.cpp#1177-1179
Attachment #8859498 - Attachment is obsolete: true
Attachment #8846507 - Attachment is obsolete: true
Attachment #8846510 - Attachment is obsolete: true
With patches:

Running 20 times Ignoring warm-up run (126) 148 182 193 398 105 106 110 144 274 104 106 111 261 105 108 109 246 109 106 109 avg 156.7 stdev 78.2093984122113 
Running 20 times Ignoring warm-up run (134) 144 181 181 327 111 105 104 104 250 117 103 112 115 156 172 171 163 422 116 108 avg 163.1 stdev 81.12761552024071 
Running 20 times Ignoring warm-up run (138) 152 168 178 326 112 110 118 215 311 110 107 114 271 100 98 104 240 100 100 99 avg 156.65 stdev 73.42497871977902 
Running 20 times Ignoring warm-up run (124) 107 122 146 358 104 113 111 185 286 113 144 175 334 107 110 106 304 105 116 108 avg 162.7 stdev 82.8692343394097 
Running 20 times Ignoring warm-up run (151) 192 197 179 312 113 111 108 157 274 113 103 123 245 117 106 110 248 107 109 105 avg 156.45 stdev 64.58364731106474 
Running 20 times Ignoring warm-up run (126) 158 178 176 323 114 115 105 146 274 105 105 105 280 106 105 104 291 107 106 106 avg 155.45 stdev 72.61230956249774 

Without patches (after bug 1357981):

Running 20 times Ignoring warm-up run (123) 147 159 163 301 105 97 100 176 258 96 97 102 318 102 94 107 268 94 99 98 avg 149.05 stdev 73.69631944676749 
Running 20 times Ignoring warm-up run (133) 146 169 202 309 102 95 110 160 255 94 100 102 317 92 99 99 175 234 106 99 avg 153.25 stdev 71.69998256624613 
Running 20 times Ignoring warm-up run (123) 138 169 167 317 103 106 103 141 260 102 107 104 274 102 95 105 234 107 98 100 avg 146.6 stdev 67.11735990040133 
Running 20 times Ignoring warm-up run (126) 126 159 170 299 97 91 98 179 258 101 93 97 153 152 151 158 216 380 95 92 avg 158.25 stdev 76.01899433694187 
Running 20 times Ignoring warm-up run (115) 134 165 163 298 107 94 97 192 260 102 101 98 278 98 99 93 280 92 93 99 avg 147.15 stdev 71.59278944139557 
Running 20 times Ignoring warm-up run (116) 126 166 167 306 95 91 102 115 254 93 96 96 109 128 146 155 160 399 97 90 avg 149.55 stdev 79.40432922706418 

After bug 1357981, the size of HTMLInputElement went down to 440 bytes, and with the patches in this bug, increased to 472. So, it's slightly slower, but it's still faster than before bug 1357981 (see comment 44).
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/443b66118588
Part 1: Create classes for each of the input types. r=smaug
https://hg.mozilla.org/integration/autoland/rev/34bacb8599bd
Part 2: Factor IsTooLong/Short() out of HTMLInputElement. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d5880ff564eb
Part 3: Factor IsValueMissing() out of HTMLInputElement. r=smaug
https://hg.mozilla.org/integration/autoland/rev/1b4c181052c7
Part 4: Factor HasTypeMismatch() out of HTMLInputElement. r=smaug
https://hg.mozilla.org/integration/autoland/rev/05dc4f814acc
Part 5: Factor HasPatternMismatch() out of HTMLInputElement. r=smaug
https://hg.mozilla.org/integration/autoland/rev/c8deb87d938b
Part 6: Factor IsRangeOverflow/Underflow() and HasStepMismatch() out of HTMLInputElement. r=smaug
https://hg.mozilla.org/integration/autoland/rev/b532001b1116
Part 7: Factor HasBadInput() out of HTMLInputElement. r=smaug
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: