Bug 84009

Summary: Introduce an internal feature for a fixed placeholder
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: haraken, morrita, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 83872    
Attachments:
Description Flags
Patch
none
Patch 2
none
Patch 3
none
Patch 4 none

Description Kent Tamura 2012-04-15 23:29:49 PDT
Introduce an internal feature for a fixed placeholder
Comment 1 Kent Tamura 2012-04-15 23:38:52 PDT
Created attachment 137282 [details]
Patch
Comment 2 Kentaro Hara 2012-04-16 01:22:36 PDT
Comment on attachment 137282 [details]
Patch

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

> Source/WebCore/html/InputType.h:243
> +    // If supportsPlaceholder() && defaultPlaceholder().isEmpty(), it means a
> +    // type supports the 'placeholder' attribute.
> +    // If supportsPlaceholder() && !defaultPlaceholder().isEmpty(), it means a
> +    // type doesn't support the 'placeholder' attribute, but shows
> +    // defaultPlaceholder() string as a placeholder.

Nit: It might be better to put this comment above supportsPlaceholder().

> Source/WebCore/html/TextFieldInputType.cpp:397
> +    if (placeholderText.isEmpty())

Shouldn't this be 'if (!placeholderText.isEmpty())'?
Comment 3 Kent Tamura 2012-04-16 01:37:15 PDT
Comment on attachment 137282 [details]
Patch

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

>> Source/WebCore/html/InputType.h:243
>> +    // defaultPlaceholder() string as a placeholder.
> 
> Nit: It might be better to put this comment above supportsPlaceholder().

It's reasonable.  I'll do.

>> Source/WebCore/html/TextFieldInputType.cpp:397
>> +    if (placeholderText.isEmpty())
> 
> Shouldn't this be 'if (!placeholderText.isEmpty())'?

No.  If defaultPlaceholder() is empty, we'll use the 'placeholder' attribute value.
Comment 4 Kent Tamura 2012-04-16 01:38:27 PDT
Created attachment 137302 [details]
Patch 2

Fix a comment
Comment 5 Kent Tamura 2012-04-16 01:39:55 PDT
Created attachment 137303 [details]
Patch 3
Comment 6 Kentaro Hara 2012-04-16 01:40:41 PDT
Comment on attachment 137282 [details]
Patch

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

>>> Source/WebCore/html/TextFieldInputType.cpp:397
>>> +    if (placeholderText.isEmpty())
>> 
>> Shouldn't this be 'if (!placeholderText.isEmpty())'?
> 
> No.  If defaultPlaceholder() is empty, we'll use the 'placeholder' attribute value.

Ah, got it. I'd been confused.

> Source/WebCore/html/TextFieldInputType.cpp:398
> +        placeholderText = element()->strippedPlaceholder();

Nit: You can move this statement inside the below if statement.
Comment 7 Kentaro Hara 2012-04-16 01:41:41 PDT
(In reply to comment #6)
> > Source/WebCore/html/TextFieldInputType.cpp:398
> > +        placeholderText = element()->strippedPlaceholder();
> 
> Nit: You can move this statement inside the below if statement.

Sorry, ignore this comment. You code is right.
Comment 8 Kent Tamura 2012-04-17 00:09:47 PDT
Created attachment 137486 [details]
Patch 4

Add InputType::usesFixedPlaceholder()
Comment 9 WebKit Review Bot 2012-04-17 01:36:57 PDT
Comment on attachment 137486 [details]
Patch 4

Clearing flags on attachment: 137486

Committed r114360: <http://trac.webkit.org/changeset/114360>
Comment 10 WebKit Review Bot 2012-04-17 01:37:02 PDT
All reviewed patches have been landed.  Closing bug.