Make mInputData.mValue really used and usable

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Depends on 1 bug)

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

8 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
As said in bug 665612, this change should be implicitly tested by <input type='number'> tests but I will probably open a follow-up to make sure all value modes are tested.
Attachment #540555 - Flags: review?(bzbarsky)
Attachment #540555 - Flags: feedback?(ehsan)

Updated

8 years ago
Attachment #540555 - Flags: feedback?(ehsan) → feedback+
Assignee

Updated

8 years ago
Depends on: 665865
Assignee

Comment 1

8 years ago
Posted patch Patch v2Splinter Review
I forgot the type change case. I've updated the test (see bug 665865).
Attachment #540555 - Attachment is obsolete: true
Attachment #540715 - Flags: review?(bzbarsky)
Attachment #540555 - Flags: review?(bzbarsky)
Comment on attachment 540715 [details] [diff] [review]
Patch v2

s/UpdateStates/UpdateState/

Why is getting rid of the BF_PARSER_CREATING checks ok?
Assignee

Comment 3

8 years ago
IIRC, this check was here to prevent creating too much string during document parsing, thus reducing document loading speed. It could have been there to prevent value sanitizing but ::SetValueInternal is already preventing value sanitizing during parsing. Ideally, we should find a way to still reduce string creation but the problem is a bit harder than it was and except if it happens to be a real issue regarding speed, I think we could try to fix that in a follow-up, ideally after improving our test suite for this code (which isn't bad though).
Ok, I see.  You're right that as long as we don't sanitize during parsing this is probably safe, but it does mean that we'll end up doing the SetAttr thing for non-value value modes during parsing, right?  You're right that it's just a performance issue, but I would still prefer we didn't have it.

Is there a reason we actually need to run that code while parsing?
Assignee

Comment 5

8 years ago
(In reply to comment #4)
> Is there a reason we actually need to run that code while parsing?

It handles these situations:
<input type='foo' value='bar'>
and
<input value='bar' type='foo'>

In both situations, the type changes and the value is set before or after it. Though, we could probably run HandleTypeChange only when the parsing is done and set the value at this moment. I believe no one can call input.value in the middle of the parsing.

If you think this patch shouldn't be pushed, I can have a look at this in this bug. Otherwise, in a follow-up.
> It handles these situations:

Well, I understand that; I just don't see why those situations need a SetAttr call when going to a non-value type while parsing.

I guess let's get this landed and file a followup for optimizations...
Assignee

Comment 7

8 years ago
(In reply to comment #6)
> > It handles these situations:
> 
> Well, I understand that; I just don't see why those situations need a
> SetAttr call when going to a non-value type while parsing.

Indeed, we don't have to. I could probably keep the condition to |!parsing or mode=value|.
Comment on attachment 540715 [details] [diff] [review]
Patch v2

Meant to mark this too...
Attachment #540715 - Flags: review?(bzbarsky) → review+

Comment 9

8 years ago
Mounir, can this be landed?
Blocks: 836314
Mounir, the patch you uploaded to Try does not appear to work with the type=range patches you had applied. See this testcase for example, where the value alerted should be zero.
Assignee

Comment 11

6 years ago
(In reply to Jonathan Watt [:jwatt] from comment #10)
> Created attachment 713089 [details]
> testcase for type=range that fails
> 
> Mounir, the patch you uploaded to Try does not appear to work with the
> type=range patches you had applied. See this testcase for example, where the
> value alerted should be zero.

That's actually because of your patch. When you change the value because it is above max, you call SetValue(value) but you should call SetValueInternal(value, false, false), otherwise the value is considered to be set by the user, so, setAttr(max, 10) ends up being like setAttr(max, 10); .value = 10; which is not what you want.
Ah, thanks, Mounir.
Assignee

Comment 13

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/25ae6325380b
Flags: in-testsuite-
Whiteboard: [needs review]
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/25ae6325380b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 928224
You need to log in before you can comment on or make changes to this bug.