Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(188)

Issue 209933002: Remove custom FormData.append() bindings code. (Closed)

Created:
6 years, 9 months ago by sof
Modified:
6 years, 9 months ago
CC:
blink-reviews, kojih, arv+blink, jsbell+bindings_chromium.org, kouhei+bindings_chromium.org, abarth-chromium, marja+watch_chromium.org, dglazkov+blink, adamk+blink_chromium.org, Nate Chapin, watchdog-blink-watchlist_google.com, Inactive
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Remove custom FormData.append() bindings code. The overload resolution needed for the two FormData.append() methods can be handled directly by generated code, so drop the custom code we had in place for this API. Along with it, handle the conversion of append()'s DOMString arguments per WebIDL (as it is now), removing the special treatment of null and undefined arguments. This also aligns with how others (Gecko, Trident) handle these arguments. R=haraken@chromium.org BUG= Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169841

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -77 lines) Patch
M LayoutTests/http/tests/local/formdata/send-form-data-with-null-string-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M Source/bindings/bindings.gypi View 1 chunk +0 lines, -1 line 0 comments Download
D Source/bindings/v8/custom/V8FormDataCustom.cpp View 1 chunk +0 lines, -70 lines 0 comments Download
M Source/core/html/FormData.idl View 1 chunk +2 lines, -5 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
sof
Please take a look.
6 years, 9 months ago (2014-03-24 10:19:01 UTC) #1
haraken
LGTM. I'm super excited to see custom bindings removed :)
6 years, 9 months ago (2014-03-24 11:17:38 UTC) #2
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 9 months ago (2014-03-24 11:30:59 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/209933002/1
6 years, 9 months ago (2014-03-24 11:31:09 UTC) #4
commit-bot: I haz the power
Change committed as 169841
6 years, 9 months ago (2014-03-24 11:40:07 UTC) #5
Nils Barth (inactive)
On 2014/03/24 11:17:38, haraken wrote: > LGTM. I'm super excited to see custom bindings removed ...
6 years, 9 months ago (2014-03-25 02:37:42 UTC) #6
sof
6 years, 9 months ago (2014-03-25 06:23:20 UTC) #7
Message was sent while issue was closed.
On 2014/03/25 02:37:42, Nils Barth wrote:
> On 2014/03/24 11:17:38, haraken wrote:
> > LGTM. I'm super excited to see custom bindings removed :)
> 
> This CL is a good example of why implementing the correct
> overload resolution algorithm is worth doing:
> that's a big cause of custom bindings.
> Bug is:
> http://crbug.com/293561

+1. If needed, a useful stopping place on the way to a complete implementation
of that algorithm would be better support for union types in arguments.

(Remembering to update custom code when WebIDL type conversions change is
another issue with them; e.g. the DOMString handling here was from an older
edition of the spec, afaict.)

Powered by Google App Engine
This is Rietveld 408576698