Bug 49723 – Expose default value of maxLength in Chromium API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ilya Sherman]

Expose default value of maxLength in Chromium API

[Ilya Sherman]

Created attachment 74225 [details]
Patch

[Kent Tamura]

Comment on attachment 74225 [details]
Patch

ok

[Darin Fisher (:fishd, Google)]

Comment on attachment 74225 [details]
Patch

This breaks the multi-DLL build!  Exports need to be annotated with WEBKIT_API

[Ilya Sherman]

Created attachment 74284 [details]
Patch

[Ilya Sherman]

Comment on attachment 74284 [details]
Patch

WEBKIT_API added

[Darin Fisher (:fishd, Google)]

Comment on attachment 74284 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74284&action=review

> WebKit/chromium/public/WebInputElement.h:83
> +        WEBKIT_API static const int defaultMaximumLength;

since this is related to the maxLength attribute, it'd probably be nice to name
it defaultMaxLength.  also, please add a comment indicating this relationship.
the comment in your ChangeLog seems pretty good.  it would be nice to have it
in the code as well.

> WebKit/chromium/src/WebInputElement.cpp:163
> +const int WebInputElement::defaultMaximumLength =

nit: i would probably not insert a line break here as it is fine to exceed 80 chars in webkit code

[Ilya Sherman]

Created attachment 74286 [details]
Patch

[Ilya Sherman]

Comment on attachment 74286 [details]
Patch

(In reply to comment #6)
> (From update of attachment 74284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74284&action=review
> 
> > WebKit/chromium/public/WebInputElement.h:83
> > +        WEBKIT_API static const int defaultMaximumLength;
> 
> since this is related to the maxLength attribute, it'd probably be nice to name
> it defaultMaxLength.  also, please add a comment indicating this relationship.
> the comment in your ChangeLog seems pretty good.  it would be nice to have it
> in the code as well.

Done =)

> > WebKit/chromium/src/WebInputElement.cpp:163
> > +const int WebInputElement::defaultMaximumLength =
> 
> nit: i would probably not insert a line break here as it is fine to exceed 80 chars in webkit code

Doh, keep forgetting that WebKit isn't 80-col -- thanks :)

[Kent Tamura]

Comment on attachment 74286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74286&action=review

Darin, thank you for following up my wrong r+.

> WebKit/chromium/public/WebInputElement.h:84
> +        WEBKIT_API static const int defaultMaxLength;

It is better to make this a function.
The hard limit value might be platform-dependent in the future.

[Ilya Sherman]

Comment on attachment 74286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74286&action=review

>> WebKit/chromium/public/WebInputElement.h:84
>> +        WEBKIT_API static const int defaultMaxLength;
> 
> It is better to make this a function.
> The hard limit value might be platform-dependent in the future.

I'm confused -- why would a function be better than a constant in the face of platform dependencies?  I'm probably overlooking something, but it makes more sense to me to leave this as a constant and update it if/when the underlying WebKit implementation changes.

[Kent Tamura]

(In reply to comment #10)
> I'm confused -- why would a function be better than a constant in the face of platform dependencies?  I'm probably overlooking something, but it makes more sense to me to leave this as a constant and update it if/when the underlying WebKit implementation changes.

I was afraid the value would be calculated at runtime in the future.
I think It's ok to commit this change as is for now.

[WebKit Commit Bot]

Comment on attachment 74286 [details]
Patch

Clearing flags on attachment: 74286

Committed r72436: <http://trac.webkit.org/changeset/72436>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.